Re: [PATCH v8 3/4] HID: hid-msi: Add RGB control interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Explicitly unregistering a devm-managed LED class device causes a double unregister and list corruption upon device teardown.
- [High] Calling sleepable functions that allocate memory with GFP_KERNEL or take sleeping locks while holding a spinlock.
- [High] Race condition between sysfs/LED attributes and `claw_remove()` allows `rgb_queue` to be scheduled after cancellation.
- [High] Missing cancellation of the `rgb_queue` delayed work during system suspend can cause USB I/O to a suspended device.
--

commit 34d0916b058e394f024a63889da5c0fbe434c516
Author: Derek J. Clark <derekjohn.clark@gmail.com>

    HID: hid-msi: Add RGB control interface

    Adds RGB control interface for MSI Claw devices. The MSI Claw uses a
    fairly unique RGB interface with 9 total zones, supporting up to 8
    sequential frames of RGB zone data. The driver mirrors the effects
    available in Windows while keeping effect names consistent with Lenovo
    drivers.

> diff --git a/drivers/hid/hid-msi.c b/drivers/hid/hid-msi.c
> index b9901869075f6..0b875603dbb32 100644
> --- a/drivers/hid/hid-msi.c
> +++ b/drivers/hid/hid-msi.c

[ ... ]

> +static ssize_t effect_store(struct device *dev,
> +			    struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct led_classdev_mc *led_mc = container_of(led_cdev, struct led_classdev_mc, led_cdev);
> +	struct claw_drvdata *drvdata = container_of(led_mc, struct claw_drvdata, led_mc);
> +	int ret;
> +
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from cfg_setup_fn in system_wq context */
> +		if (!smp_load_acquire(&drvdata->rgb_registered))
> +			return -ENODEV;
> +	}
> +
> +	ret = sysfs_match_string(claw_rgb_effect_text, buf);
> +	if (ret < 0)
> +		return ret;
> +
> +	drvdata->rgb_effect = ret;
> +	mod_delayed_work(system_wq, &drvdata->rgb_queue, msecs_to_jiffies(50));

[Severity: High]
Can mod_delayed_work() here race with claw_remove()?

If claw_remove() runs concurrently, it acquires the registration_lock, sets
rgb_registered to false, drops the lock, and calls
cancel_delayed_work_sync(&drvdata->rgb_queue).

It then calls device_remove_group(), which blocks until this sysfs store
completes. Once device_remove_group() returns, devres frees drvdata.

However, since this sysfs store function already checked rgb_registered and
dropped the lock, it will continue executing and call mod_delayed_work(),
re-arming the workqueue. Will this lead to the delayed work executing on
freed memory?

> +
> +	return count;
> +}

[ ... ]

> +static void cfg_setup_fn(struct work_struct *work)
> +{
[ ... ]
> +try_gamepad:
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from below */
> +		if (!smp_load_acquire(&drvdata->gamepad_registered) && gamepad_ready) {
> +			ret = devm_device_add_group(&drvdata->hdev->dev, &claw_gamepad_attr_group);

[Severity: High]
Does devm_device_add_group() sleep?

It allocates memory using GFP_KERNEL inside devres_alloc() and acquires
the sleeping kernfs_mutex during sysfs group creation. Calling this inside a
spinlock block with interrupts disabled will trigger a scheduling while atomic
panic. Can this be called outside the spinlock?

> +			if (ret) {
> +				dev_err(&drvdata->hdev->dev,
> +					"Failed to create gamepad attrs: %d\n", ret);
> +				goto try_rgb;
> +			}
> +
> +			/* Pairs with smp_load_acquire in attribute show/store functions */
> +			smp_store_release(&drvdata->gamepad_registered, true);
> +		}
>  	}
> -	scoped_guard(spinlock_irqsave, &drvdata->registration_lock)
> -		/* Pairs with smp_load_acquire in attribute show/store functions */
> -		smp_store_release(&drvdata->gamepad_registered, true);
>  
> -	kobject_uevent(&drvdata->hdev->dev.kobj, KOBJ_CHANGE);
> +try_rgb:
> +	/* Add and enable RGB interface once we have the device state */
> +	scoped_guard(spinlock_irqsave, &drvdata->registration_lock) {
> +		/* Pairs with smp_store_release from below */
> +		if (!smp_load_acquire(&drvdata->rgb_registered) && rgb_ready) {
> +			ret = devm_led_classdev_multicolor_register(&drvdata->hdev->dev,
> +								    &drvdata->led_mc);

[Severity: High]
Does devm_led_classdev_multicolor_register() also sleep?

Similar to devm_device_add_group(), this registers a device which takes
sleeping locks and allocates memory with GFP_KERNEL. Should this be called
outside of the spinlock block?

> +			if (ret) {
> +				dev_err(&drvdata->hdev->dev,
> +					"Failed to create led device: %d\n", ret);
> +				goto update_kobjects;
> +			}
> +
> +			ret = device_add_group(drvdata->led_mc.led_cdev.dev, &claw_rgb_attr_group);
> +			if (ret) {
> +				dev_err(&drvdata->hdev->dev,
> +					"Failed to create RGB attrs: %d\n", ret);
> +				led_classdev_unregister(&drvdata->led_mc.led_cdev);

[Severity: High]
Does this manual unregister cause a double-free on device teardown?

Since devm_led_classdev_multicolor_register() attaches a devres cleanup
action, manually calling led_classdev_unregister() here leaves the devres
action queued. When the driver is unbound, devres will execute
led_classdev_unregister() a second time on the same LED class device pointer.
Will this result in list corruption and use-after-free?

> +				goto update_kobjects;
> +			}
> +
> +				/* Pairs with smp_load_acquire in attribute show/store functions */
> +				smp_store_release(&drvdata->rgb_registered, true);
> +		}
> +	}

[ ... ]

> @@ -971,9 +1593,11 @@ static int claw_probe(struct hid_device *hdev, u8 ep)
>  	spin_lock_init(&drvdata->cmd_lock);
>  	spin_lock_init(&drvdata->mode_lock);
>  	spin_lock_init(&drvdata->profile_lock);
> +	spin_lock_init(&drvdata->frame_lock);
>  	init_completion(&drvdata->send_cmd_complete);
>  	INIT_DELAYED_WORK(&drvdata->cfg_resume, &cfg_resume_fn);
>  	INIT_DELAYED_WORK(&drvdata->cfg_setup, &cfg_setup_fn);
> +	INIT_DELAYED_WORK(&drvdata->rgb_queue, &claw_rgb_queue_fn);

[Severity: High]
Is the rgb_queue delayed work properly cancelled during system suspend?

In claw_suspend(), the driver cancels cfg_resume and cfg_setup, but it does
not appear to cancel the newly introduced rgb_queue delayed work. Because
rgb_queue is queued on system_wq (which is not freezable), could it execute
after the device has been suspended? This might lead to USB I/O errors and
possible hardware lockups.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522015518.1111290-1-derekjohn.clark@gmail.com?part=3