[PATCH v2 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[PATCH v2 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[Guenter Roeck]

Sashiko reports:

In lm90_probe(), the devm action to cancel the alert_work and report_work
(lm90_restore_conf) is registered in lm90_init_client() before
devm_hwmon_device_register_with_info() is called.

Because devm executes cleanup actions in reverse order during module
unbind or probe failure, the hwmon device is unregistered and freed first.

If lm90_alert_work() or lm90_report_alarms() runs in the window between
the hwmon device being freed and the delayed works being cancelled,
lm90_update_alarms() will dereference the freed data->hwmon_dev here.

Fix the problem by canceling the workers separately after registering
the hwmon device and before registering the interrupt handler. This ensures
that the workers are canceled after interrupts are disabled and before
the hwmon device is released. Add "shutdown" flag to indicate that device
shutdown is in progress to prevent workers from being re-armed.

Fixes: f6d0775119fb9 ("hwmon: (lm90) Rework alarm/status handling")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Add 'shutdown' flag to prevent workers from being re-armed

 drivers/hwmon/lm90.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 3c10a5066b53..08a568f7b56f 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -736,6 +736,7 @@ struct lm90_data {
 	struct hwmon_chip_info chip;
 	struct delayed_work alert_work;
 	struct work_struct report_work;
+	bool shutdown;		/* true if shutting down */
 	bool valid;		/* true if register values are valid */
 	bool alarms_valid;	/* true if status register values are valid */
 	unsigned long last_updated; /* in jiffies */
@@ -1154,6 +1155,9 @@ static void lm90_report_alarms(struct work_struct *work)
 
 static int lm90_update_alarms_locked(struct lm90_data *data, bool force)
 {
+	if (data->shutdown)
+		return 0;
+
 	if (force || !data->alarms_valid ||
 	    time_after(jiffies, data->alarms_updated + msecs_to_jiffies(data->update_interval))) {
 		struct i2c_client *client = data->client;
@@ -2584,15 +2588,21 @@ static void lm90_restore_conf(void *_data)
 	struct lm90_data *data = _data;
 	struct i2c_client *client = data->client;
 
-	cancel_delayed_work_sync(&data->alert_work);
-	cancel_work_sync(&data->report_work);
-
 	/* Restore initial configuration */
 	if (data->flags & LM90_HAVE_CONVRATE)
 		lm90_write_convrate(data, data->convrate_orig);
 	lm90_write_reg(client, LM90_REG_CONFIG1, data->config_orig);
 }
 
+static void lm90_stop_work(void *_data)
+{
+	struct lm90_data *data = _data;
+
+	data->shutdown = true;
+	cancel_delayed_work_sync(&data->alert_work);
+	cancel_work_sync(&data->report_work);
+}
+
 static int lm90_init_client(struct i2c_client *client, struct lm90_data *data)
 {
 	struct device_node *np = client->dev.of_node;
@@ -2902,6 +2912,10 @@ static int lm90_probe(struct i2c_client *client)
 
 	data->hwmon_dev = hwmon_dev;
 
+	err = devm_add_action_or_reset(&client->dev, lm90_stop_work, data);
+	if (err)
+		return err;
+
 	if (client->irq) {
 		dev_dbg(dev, "IRQ: %d\n", client->irq);
 		err = devm_request_threaded_irq(dev, client->irq,
@@ -2930,7 +2944,7 @@ static void lm90_alert(struct i2c_client *client, enum i2c_alert_protocol type,
 		 */
 		struct lm90_data *data = i2c_get_clientdata(client);
 
-		if ((data->flags & LM90_HAVE_BROKEN_ALERT) &&
+		if (!data->shutdown && (data->flags & LM90_HAVE_BROKEN_ALERT) &&
 		    (data->current_alarms & data->alert_alarms)) {
 			if (!(data->config & 0x80)) {
 				dev_dbg(&client->dev, "Disabling ALERT#\n");
-- 
2.45.2

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

[Guenter Roeck]

Sashiko reports:

lm90_alert() executes in the smbus alert context and calls
lm90_update_confreg() to disable the hardware alert line, without
acquiring hwmon_lock.

Concurrently, sysfs write operations (such as lm90_write_convrate) hold
the hwmon_lock, temporarily modify data->config, and then restore it.

If an alert interrupt occurs concurrently with a sysfs write, the sysfs
path will overwrite the alert handler's modifications to data->config
and the hardware register.

This unintentionally re-enables the hardware alert line while the alarm is
still active, causing an interrupt storm.

Add the missing lock to lm90_alert() to solve the problem.

Fixes: 7a1d220ccb0cc ("hwmon: (lm90) Introduce function to update configuration register")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No changes

 drivers/hwmon/lm90.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 08a568f7b56f..9d0c23420842 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);
 	} else {
 		dev_dbg(&client->dev, "Everything OK\n");
 	}
-- 
2.45.2

Re: [PATCH v2 1/2] hwmon: (lm90) Stop work before releasing hwmon device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in `lm90_stop_work()` allows canceled workers to be re-armed via concurrent sysfs accesses, leading to a Use-After-Free.
--

commit e9b97015cb5267aa1cbf5c4dd89535d551b6b503
Author: Guenter Roeck <linux@roeck-us.net>
hwmon: (lm90) Stop work before releasing hwmon device

This commit fixes a use-after-free issue by canceling the workers after
registering the hwmon device and before registering the interrupt handler.
It introduces a shutdown flag to prevent workers from being re-armed during
device teardown.

> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 3c10a5066b53d..08a568f7b56f9 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
[ ... ]
> @@ -2584,15 +2588,21 @@ static void lm90_restore_conf(void *_data)
>  	struct lm90_data *data = _data;
>  	struct i2c_client *client = data->client;
>  
> -	cancel_delayed_work_sync(&data->alert_work);
> -	cancel_work_sync(&data->report_work);
> -
>  	/* Restore initial configuration */
>  	if (data->flags & LM90_HAVE_CONVRATE)
>  		lm90_write_convrate(data, data->convrate_orig);
>  	lm90_write_reg(client, LM90_REG_CONFIG1, data->config_orig);
>  }
>  
> +static void lm90_stop_work(void *_data)
> +{
> +	struct lm90_data *data = _data;
> +
> +	data->shutdown = true;

Can this race with a concurrent sysfs read, allowing the workers to be
re-armed after they are canceled?

Since devres unwinds in reverse order, lm90_stop_work() executes before the
hwmon device is unregistered. This leaves the sysfs attributes fully
accessible.

If a sysfs read happens concurrently, the hwmon core will acquire the hwmon
lock, and the driver could call lm90_update_alarms_locked(). Because
lm90_stop_work() sets data->shutdown without acquiring hwmon_lock(), the
concurrent sysfs read might read data->shutdown as false before it is updated.

If the sysfs thread blocks on I2C reads, lm90_stop_work() could set
data->shutdown to true and call cancel_delayed_work_sync(). Since the work is
not yet queued, the cancellation returns immediately.

When the sysfs thread finishes its I2C transfers, it could call
schedule_work() or schedule_delayed_work(), successfully re-arming the workers
after they were supposedly canceled.

As the devres unbind continues, it unregisters the hwmon device and frees the
data structure. When the newly scheduled work executes, it would dereference
the freed memory, causing a use-after-free.

Should lm90_stop_work() hold hwmon_lock(data->hwmon_dev) while setting
data->shutdown to synchronize with sysfs reads, releasing it before calling
the sync cancellation functions to avoid deadlocks?

> +	cancel_delayed_work_sync(&data->alert_work);
> +	cancel_work_sync(&data->report_work);
> +}

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

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

[sashiko-bot]

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