Re: [PATCH v2 3/4] HID: roccat: examine readers to check whether the device is opened

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Caching `list_empty()` state outside `devices_lock` in `roccat_release()` creates a TOCTOU race with `roccat_open()` and `roccat_disconnect()`, leading to device state corruption and leaked hardware open counts.
--

commit 8f711f83c5a599f7dd523be3cd4112f138217007
Author: Dmitry Antipov <dmantipov@yandex.ru>

HID: roccat: examine readers to check whether the device is opened

This patch replaces the explicit open counter in struct roccat_device with
a check against list_empty(&device->readers). The goal is to determine if
active readers exist without maintaining a separate integer count.

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 007778922c21a..d1df4ec34a1e6 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -213,18 +209,20 @@ static int roccat_release(struct inode *inode, struct file *file)
>  {
>  	struct roccat_reader *reader = file->private_data;
>  	struct roccat_device *device = reader->device;
> +	bool open;
>  
>  	if (WARN_ON_ONCE(!device))
>  		return -ENODEV;
>  
>  	mutex_lock(&device->readers_lock);
>  	list_del(&reader->node);
> +	open = !list_empty(&device->readers);
>  	mutex_unlock(&device->readers_lock);

[Severity: High]
Does evaluating the list state here and dropping the lock introduce a race
condition?

If another thread calls roccat_open() before devices_lock is acquired below,
it will observe an empty list, power on the device, and add a new reader.

>  	kfree(reader);
>  
>  	mutex_lock(&devices_lock);
>  
> -	if (!--device->open) {
> +	if (!open) {
>  		/* removing last reader */
>  		if (device->exist) {
>  			hid_hw_power(device->hid, PM_HINT_NORMAL);
>  			hid_hw_close(device->hid);
>  		}

[Severity: High]
Since this uses the cached open flag, could it unconditionally power down and
close the hardware here, cutting off the newly added reader from the
concurrent roccat_open() call?

[ ... ]
> @@ -370,7 +369,11 @@ void roccat_disconnect(int minor)
>  
>  	devices[minor] = NULL;
>  
> -	if (device->open) {
> +	mutex_lock(&device->readers_lock);
> +	open = !list_empty(&device->readers);
> +	mutex_unlock(&device->readers_lock);
> +
> +	if (open) {
>  		hid_hw_close(device->hid);
>  		wake_up_interruptible(&device->wait);
>  	} else {

[Severity: High]
Can a concurrent roccat_disconnect() and roccat_release() race here,
resulting in neither function closing the hardware?

If roccat_release() removes the last reader, it evaluates the local open
variable to false. If roccat_disconnect() then acquires readers_lock, it will
also evaluate open to false since the list is already empty, and it will skip
calling hid_hw_close().

When roccat_release() subsequently acquires devices_lock, it observes that
device->exist is 0 (set earlier by roccat_disconnect()) and also skips calling
hid_hw_close().

Will this result in a permanent leak of the HID hardware open count?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914120240.1625442-1-dmantipov@yandex.ru?part=3