linux-kernel - Re: [PATCH] hwmon (ina3221) Add mutex lock to shunt nodes

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: <20181113043122.GA11205@roeck-us.net>
Date:   Mon, 12 Nov 2018 20:31:22 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Nicolin Chen <nicoleotsuka@...il.com>
Cc:     jdelvare@...e.com, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon (ina3221) Add mutex lock to shunt nodes

On Mon, Nov 12, 2018 at 08:23:24PM -0800, Nicolin Chen wrote:
> The shunt resistor values are used to calculate shunt voltages
> and currents. As a part of sysfs nodes, it would be better to
> get protected with the same mutex too as other sysfs ABI nodes,
> although this is not very critical because the mutex was added
> to mainly protect register access.
> 
> So this patch adds the mutex lock to protect the shunt node.
> 

I am missing something here. I don't see the point of this mutex.
It just reads a variable and reports the result. When setting,
it just writes a value. Protecting the conversion of the passed
value with a mutex is pointless. Protecting writing the value
against reading it is just as pointless.

Guenter

> Signed-off-by: Nicolin Chen <nicoleotsuka@...il.com>
> ---
>  drivers/hwmon/ina3221.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 86281afd2619..f7a09ab6c440 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -532,8 +532,15 @@ static ssize_t ina3221_show_shunt(struct device *dev,
>  	struct ina3221_data *ina = dev_get_drvdata(dev);
>  	unsigned int channel = sd_attr->index;
>  	struct ina3221_input *input = &ina->inputs[channel];
> +	ssize_t ret;
>  
> -	return snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
> +	mutex_lock(&ina->lock);
> +
> +	ret = snprintf(buf, PAGE_SIZE, "%d\n", input->shunt_resistor);
> +
> +	mutex_unlock(&ina->lock);
> +
> +	return ret;
>  }
>  
>  static ssize_t ina3221_set_shunt(struct device *dev,
> @@ -547,14 +554,20 @@ static ssize_t ina3221_set_shunt(struct device *dev,
>  	int val;
>  	int ret;
>  
> +	mutex_lock(&ina->lock);
> +
>  	ret = kstrtoint(buf, 0, &val);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&ina->lock);
>  		return ret;
> +	}
>  
>  	val = clamp_val(val, 1, INT_MAX);
>  
>  	input->shunt_resistor = val;
>  
> +	mutex_unlock(&ina->lock);
> +
>  	return count;
>  }
>  
> -- 
> 2.17.1
>