[PATCH v1 0/1] Fix sysfs device unregistration deadlock

[PATCH v1 0/1] Fix sysfs device unregistration deadlock

[Ninad Palsule]

Hello,

Please review the changes to fix deadlock in the occ-hwmon driver. This
avoids kernel crash due to hung application process.

Ninad Palsule (1):
  hwmon: (occ) Fix sysfs device unreg deadlock

 drivers/hwmon/occ/common.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.51.0

[PATCH v1 1/1] hwmon: (occ) Fix sysfs device unreg deadlock

[Ninad Palsule]

Release the driver lock before unregistering the hwmon device to prevent
a deadlock. The device_unregister() call can block waiting for sysfs
operations to complete, but those operations may be blocked waiting for
the same lock held during unregistration.

The deadlock occurs when:
1. Thread A holds the driver lock and calls device_unregister()
2. device_unregister() waits for sysfs operations to drain
3. Thread B is blocked in a sysfs read operation waiting for the driver
   lock
4. Neither thread can proceed, causing a hung task

This was observed during power system remote restart operations when
concurrent sysfs accesses occurred during OCC active state transitions.

Kernel log excerpt showing the deadlock:
  INFO: task openpower-occ-c:1430 blocked for more than 122 seconds.
  Call trace:
   schedule+0x28/0xfc
   kernfs_drain+0xc8/0x174
   __kernfs_remove.part.0+0x138/0x21c
   kernfs_remove_by_name_ns+0x7c/0xcc
   [...]
   device_unregister+0x1c/0x5c
   hwmon_device_unregister+0x58/0xb0
   occ_active+0x6c/0xe74
   occ_active_store+0x54/0x80

Reproducer:
  Run these two scripts in parallel:

  Script 1 (continuous sysfs read):
    while true; do
        cat /sys/class/hwmon/hwmon14/power15_cap_user
    done

  Script 2 (OCC active state toggle):
    while true; do
        echo 0 > /sys/bus/platform/drivers/occ-hwmon/occ-hwmon.1/occ_active
        echo 1 > /sys/bus/platform/drivers/occ-hwmon/occ-hwmon.1/occ_active
        sleep 0.2
    done

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 drivers/hwmon/occ/common.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 42cc6068bb08..a5641dbbba2f 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -1149,15 +1149,22 @@ int occ_active(struct occ *occ, bool active)
 			goto unlock;
 		}
 	} else {
+		struct device *hwmon_dev = occ->hwmon;
+
 		if (!occ->active) {
 			rc = -EALREADY;
 			goto unlock;
 		}
 
-		if (occ->hwmon)
-			hwmon_device_unregister(occ->hwmon);
 		occ->active = false;
 		occ->hwmon = NULL;
+
+		mutex_unlock(&occ->lock);
+
+		if (hwmon_dev)
+			hwmon_device_unregister(hwmon_dev);
+
+		return 0;
 	}
 
 unlock:
-- 
2.51.0

Re: [PATCH v1 1/1] hwmon: (occ) Fix sysfs device unreg deadlock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Releasing the driver lock before unregistering the hwmon device introduces a data race on the shared `occ->group` structures.
- [High] Pending sysfs reads execute hardware access via `occ_poll()` after the device has been explicitly deactivated due to a missing `occ->active` check.

Pre-existing issues:
- [High] The exact same sysfs unregistration deadlock fixed in `occ_active()` remains unfixed in `occ_shutdown()`.
- [High] Repeatedly toggling the OCC active state via sysfs causes an unbounded memory leak due to `devm_kcalloc` allocations that are never explicitly freed.
--

commit 834d342f5072edda5a28c9ccd24f73161c2356f5
Author: Ninad Palsule <ninad@linux.ibm.com>

hwmon: (occ) Fix sysfs device unreg deadlock

This commit addresses a deadlock when unregistering the hwmon device by
releasing the driver lock before calling hwmon_device_unregister(). This
allows pending sysfs operations to complete without blocking unregistration.

> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -1149,15 +1149,22 @@ int occ_active(struct occ *occ, bool active)
>  			goto unlock;
>  		}
>  	} else {
> +		struct device *hwmon_dev = occ->hwmon;
> +
>  		if (!occ->active) {
>  			rc = -EALREADY;
>  			goto unlock;
>  		}
>  
> -		if (occ->hwmon)
> -			hwmon_device_unregister(occ->hwmon);
>  		occ->active = false;
>  		occ->hwmon = NULL;
> +
> +		mutex_unlock(&occ->lock);

[Severity: High]
Could releasing occ->lock here expose the shared occ->group structures to
concurrent modification?

If one thread unregisters the device while another thread concurrently
activates it, the activating thread calls occ_setup_sensor_attrs() which
dynamically reallocates the attributes:

occ_setup_sensor_attrs()
    ...
    occ->group.attrs = devm_kcalloc(...)

Since the unregistering thread is simultaneously iterating over occ->groups,
does this risk a NULL pointer dereference in sysfs_remove_file() due to
reading an uninitialized pointer?

> +
> +		if (hwmon_dev)
> +			hwmon_device_unregister(hwmon_dev);

[Severity: High]
Since occ->lock is dropped before unregistering, could pending sysfs accesses
now communicate with deactivated hardware?

If a sysfs read or write is waiting for occ->lock in occ_update_response() or
occ_set_user_power_cap(), it will wake up once the lock is released here.
Because those functions do not verify if occ->active is still true after
acquiring the lock, will they proceed to call occ_poll() or send_cmd() on a
device that has just been deactivated?

> +
> +		return 0;
>  	}
>  
>  unlock:

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the exact same deadlock
still exist in occ_shutdown()?

Looking at occ_shutdown():

occ_shutdown() {
    mutex_lock(&occ->lock);
    occ_shutdown_sysfs(occ);
    if (occ->hwmon)
        hwmon_device_unregister(occ->hwmon);
    ...
}

Since this continues to hold occ->lock while unregistering the hwmon device,
wouldn't a concurrent sysfs read waiting for occ->lock in
occ_update_response() cause an AB-BA deadlock during driver unbind?

[Severity: High]
This is a pre-existing issue, but does repeatedly toggling the active state
via sysfs cause an unbounded memory leak of occ->attrs?

When the device is activated, occ_setup_sensor_attrs() allocates arrays:

occ_setup_sensor_attrs() {
    occ->attrs = devm_kcalloc(dev, num_attrs, sizeof(*occ->attrs),
                              GFP_KERNEL);
    ...
}

Since these are devres allocations, they are only cleaned up when the platform
device unbinds. When occ_active is toggled to false, they are not explicitly
freed. If a management script repeatedly writes to the occ_active sysfs
attribute, will this continuously leak occ->attrs and occ->group.attrs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625164700.2797362-2-ninad@linux.ibm.com?part=1

Re: [PATCH v1 1/1] hwmon: (occ) Fix sysfs device unreg deadlock

[Guenter Roeck]

Hi,

On 6/25/26 09:59, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Releasing the driver lock before unregistering the hwmon device introduces a data race on the shared `occ->group` structures.
> - [High] Pending sysfs reads execute hardware access via `occ_poll()` after the device has been explicitly deactivated due to a missing `occ->active` check.
> 
> Pre-existing issues:
> - [High] The exact same sysfs unregistration deadlock fixed in `occ_active()` remains unfixed in `occ_shutdown()`.
> - [High] Repeatedly toggling the OCC active state via sysfs causes an unbounded memory leak due to `devm_kcalloc` allocations that are never explicitly freed.

The driver apparently has a number of issues. I understand that not all of them
can be fixed immediately, but it looks like this patch introduces new problems.
Please have a look.

Thanks,
Guenter

> --
> 
> commit 834d342f5072edda5a28c9ccd24f73161c2356f5
> Author: Ninad Palsule <ninad@linux.ibm.com>
> 
> hwmon: (occ) Fix sysfs device unreg deadlock
> 
> This commit addresses a deadlock when unregistering the hwmon device by
> releasing the driver lock before calling hwmon_device_unregister(). This
> allows pending sysfs operations to complete without blocking unregistration.
> 
>> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
>> --- a/drivers/hwmon/occ/common.c
>> +++ b/drivers/hwmon/occ/common.c
>> @@ -1149,15 +1149,22 @@ int occ_active(struct occ *occ, bool active)
>>   			goto unlock;
>>   		}
>>   	} else {
>> +		struct device *hwmon_dev = occ->hwmon;
>> +
>>   		if (!occ->active) {
>>   			rc = -EALREADY;
>>   			goto unlock;
>>   		}
>>   
>> -		if (occ->hwmon)
>> -			hwmon_device_unregister(occ->hwmon);
>>   		occ->active = false;
>>   		occ->hwmon = NULL;
>> +
>> +		mutex_unlock(&occ->lock);
> 
> [Severity: High]
> Could releasing occ->lock here expose the shared occ->group structures to
> concurrent modification?
> 
> If one thread unregisters the device while another thread concurrently
> activates it, the activating thread calls occ_setup_sensor_attrs() which
> dynamically reallocates the attributes:
> 
> occ_setup_sensor_attrs()
>      ...
>      occ->group.attrs = devm_kcalloc(...)
> 
> Since the unregistering thread is simultaneously iterating over occ->groups,
> does this risk a NULL pointer dereference in sysfs_remove_file() due to
> reading an uninitialized pointer?
> 
>> +
>> +		if (hwmon_dev)
>> +			hwmon_device_unregister(hwmon_dev);
> 
> [Severity: High]
> Since occ->lock is dropped before unregistering, could pending sysfs accesses
> now communicate with deactivated hardware?
> 
> If a sysfs read or write is waiting for occ->lock in occ_update_response() or
> occ_set_user_power_cap(), it will wake up once the lock is released here.
> Because those functions do not verify if occ->active is still true after
> acquiring the lock, will they proceed to call occ_poll() or send_cmd() on a
> device that has just been deactivated?
> 
>> +
>> +		return 0;
>>   	}
>>   
>>   unlock:
> 
> [ ... ]
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the exact same deadlock
> still exist in occ_shutdown()?
> 
> Looking at occ_shutdown():
> 
> occ_shutdown() {
>      mutex_lock(&occ->lock);
>      occ_shutdown_sysfs(occ);
>      if (occ->hwmon)
>          hwmon_device_unregister(occ->hwmon);
>      ...
> }
> 
> Since this continues to hold occ->lock while unregistering the hwmon device,
> wouldn't a concurrent sysfs read waiting for occ->lock in
> occ_update_response() cause an AB-BA deadlock during driver unbind?
> 
> [Severity: High]
> This is a pre-existing issue, but does repeatedly toggling the active state
> via sysfs cause an unbounded memory leak of occ->attrs?
> 
> When the device is activated, occ_setup_sensor_attrs() allocates arrays:
> 
> occ_setup_sensor_attrs() {
>      occ->attrs = devm_kcalloc(dev, num_attrs, sizeof(*occ->attrs),
>                                GFP_KERNEL);
>      ...
> }
> 
> Since these are devres allocations, they are only cleaned up when the platform
> device unbinds. When occ_active is toggled to false, they are not explicitly
> freed. If a management script repeatedly writes to the occ_active sysfs
> attribute, will this continuously leak occ->attrs and occ->group.attrs?
>