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
| ||
|
Date: Thu, 30 May 2019 10:21:20 -0700 From: Guenter Roeck <linux@...ck-us.net> To: "Adamski, Krzysztof (Nokia - PL/Wroclaw)" <krzysztof.adamski@...ia.com> Cc: Jean Delvare <jdelvare@...e.com>, "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] hwmon: pmbus: protect read-modify-write with lock Hi, On Thu, May 30, 2019 at 06:45:48AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote: > The operation done in the pmbus_update_fan() function is a > read-modify-write operation but it lacks any kind of lock protection > which may cause problems if run more than once simultaneously. This > patch uses an existing update_lock mutex to fix this problem. > > Signed-off-by: Krzysztof Adamski <krzysztof.adamski@...ia.com> > --- > > I'm resending this patch to proper recipients this time. Sorry if the > previous submission confused anybody. > > drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index ef7ee90ee785..94adbede7912 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -268,6 +268,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id, > int rv; > u8 to; > > + mutex_lock(&data->update_lock); > from = pmbus_read_byte_data(client, page, > pmbus_fan_config_registers[id]); > if (from < 0) > @@ -278,11 +279,15 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id, > rv = pmbus_write_byte_data(client, page, > pmbus_fan_config_registers[id], to); > if (rv < 0) > - return rv; > + goto out; > } > > - return _pmbus_write_word_data(client, page, > - pmbus_fan_command_registers[id], command); > + rv = _pmbus_write_word_data(client, page, > + pmbus_fan_command_registers[id], command); > + > +out: > + mutex_lock(&data->update_lock); Should be mutex_unlock(), meaning you have not tested this ;-). Either case, I think this is unnecessary. The function is (or should be) always called with the lock already taken (ie with pmbus_set_sensor() in the call path). If not, we would need a locked and an unlocked version of this function to avoid lock recursion. Thanks, Guenter > + return rv; > } > EXPORT_SYMBOL_GPL(pmbus_update_fan); > > -- > 2.20.1 >
Powered by blists - more mailing lists