linux-kernel - Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and i8k_ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211122160122.gf6i3qj6dnwi6wla@pali>
Date:   Mon, 22 Nov 2021 17:01:22 +0100
From:   Pali Rohár <pali@...nel.org>
To:     Armin Wolf <W_Armin@....de>
Cc:     jdelvare@...e.com, linux@...ck-us.net, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: (dell-smm) Unify i8k_ioctl() and
 i8k_ioctl_unlocked()

On Saturday 20 November 2021 18:03:19 Armin Wolf wrote:
> The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked()
> with i8k_mutex held. Judging from the hwmon code, this mutex
> only needs to be held when setting the fan speed/mode.

Really? I think that there is no difference between setting and getting
fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do
not need it. Some more explanation is needed...

I'm not sure here but SMM call consist of two 'out' instructions and we
probably need to ensure that interrupt does not happen between them.

> Unify both functions and reduce the locking of i8k_mutex
> to I8K_SET_FAN.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@....de>
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index b5d1703faa62..e2d388f7360e 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -449,12 +449,12 @@ static int i8k_get_power_status(void)
>   * Procfs interface
>   */
> 
> -static int
> -i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd, unsigned long arg)
> +static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  {
> -	int val = 0;
> -	int speed, err;
> +	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
>  	int __user *argp = (int __user *)arg;
> +	int speed, err;
> +	int val = 0;
> 
>  	if (!argp)
>  		return -EINVAL;
> @@ -516,11 +516,14 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  		if (copy_from_user(&speed, argp + 1, sizeof(int)))
>  			return -EFAULT;
> 
> +		mutex_lock(&data->i8k_mutex);
>  		err = i8k_set_fan(data, val, speed);
> -		if (err < 0)
> +		if (err < 0) {
> +			mutex_unlock(&data->i8k_mutex);
>  			return err;
> -
> +		}
>  		val = i8k_get_fan_status(data, val);
> +		mutex_unlock(&data->i8k_mutex);
>  		break;
> 
>  	default:
> @@ -536,18 +539,6 @@ i8k_ioctl_unlocked(struct file *fp, struct dell_smm_data *data, unsigned int cmd
>  	return 0;
>  }
> 
> -static long i8k_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> -{
> -	struct dell_smm_data *data = PDE_DATA(file_inode(fp));
> -	long ret;
> -
> -	mutex_lock(&data->i8k_mutex);
> -	ret = i8k_ioctl_unlocked(fp, data, cmd, arg);
> -	mutex_unlock(&data->i8k_mutex);
> -
> -	return ret;
> -}
> -
>  /*
>   * Print the information for /proc/i8k.
>   */
> --
> 2.30.2
>