Re: [PATCH v2 2/2] hwmon: (lm90) Add lock protection to lm90_alert

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Race condition between sysfs reads and devres teardown (`lm90_stop_work`) leaves delayed work enqueued after the data structure is freed.
--

commit a26cbd27d33d410e7909a5d68c680507e329f297
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (lm90) Add lock protection to lm90_alert

This commit adds hwmon_lock protection to lm90_alert() to prevent
concurrent sysfs writes from overwriting hardware register modifications
made during alert handling.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 08a568f7b56f9..9d0c23420842a 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -2944,6 +2944,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
>  		 */
>  		struct lm90_data *data = i2c_get_clientdata(client);
>  
> +		hwmon_lock(data->hwmon_dev);
>  		if (!data->shutdown && (data->flags & LM90_HAVE_BROKEN_ALERT) &&
>  		    (data->current_alarms & data->alert_alarms)) {
>  			if (!(data->config & 0x80)) {
> @@ -2953,6 +2954,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
>  			schedule_delayed_work(&data->alert_work,
>  				max_t(int, HZ, msecs_to_jiffies(data->update_interval)));
>  		}
> +		hwmon_unlock(data->hwmon_dev);

While this isn't a regression introduced by this patch, could a similar race
condition cause a use-after-free during driver unbind?

In lm90_probe(), the hwmon device is registered via
devm_hwmon_device_register_with_info() before registering the teardown action
lm90_stop_work(). Since devres actions execute in LIFO order,
lm90_stop_work() runs while the hwmon device and sysfs attributes are still
accessible.

lm90_stop_work() sets data->shutdown = true and completes
cancel_delayed_work_sync() without acquiring hwmon_lock(data->hwmon_dev).

Could this race with a concurrent sysfs read operation in
lm90_update_alarms_locked()?

CPU1 (sysfs read)
hwmon_lock(data->hwmon_dev);
// checks data->shutdown, sees false
// preempted before scheduling work...

CPU2 (unbind)
lm90_stop_work()
    data->shutdown = true;
    cancel_delayed_work_sync(&data->alert_work);
    // proceeds to unregister device, blocking on CPU1's sysfs read

CPU1 (resumes)
schedule_delayed_work(&data->alert_work, ...);
hwmon_unlock(data->hwmon_dev);

After CPU1 releases the lock, CPU2 unblocks, finishes unregistering the
device, and devres frees the data structure. The incorrectly enqueued work
timer will then fire and access the freed memory.

Should lm90_stop_work() also acquire hwmon_lock() when setting
data->shutdown = true to serialize against concurrent sysfs operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514235626.1403695-1-linux@roeck-us.net?part=2