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

[sashiko-bot@kernel.org]

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