Re: [PATCH v7 3/3] HID: roccat: avoid unlocked report buffer access in roccat_poll()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: roccat: lost wakeup race with disconnect in roccat_poll()
--

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

HID: roccat: avoid unlocked report buffer access in roccat_poll()

This commit acquires cbuf_lock when checking the per-device report buffer
indices in roccat_poll() and moves the roccat_device_available() check
before poll_wait().

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 098dd85aa036a..2782fb5773d9d 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -160,12 +160,18 @@ static ssize_t roccat_read(struct file *file, char __user *buffer,
>  static __poll_t roccat_poll(struct file *file, poll_table *wait)
>  {
>  	struct roccat_reader *reader = file->private_data;
> -	poll_wait(file, &reader->device->wait, wait);
> -	if (reader->cbuf_start != reader->device->cbuf_end)
> -		return EPOLLIN | EPOLLRDNORM;
> +	bool avail;
> +
>  	if (!roccat_device_available(reader->device))
>  		return EPOLLERR | EPOLLHUP;
> -	return 0;
> +
> +	poll_wait(file, &reader->device->wait, wait);
> +
> +	mutex_lock(&reader->device->cbuf_lock);
> +	avail = (reader->cbuf_start != reader->device->cbuf_end);
> +	mutex_unlock(&reader->device->cbuf_lock);
> +
> +	return avail ? EPOLLIN | EPOLLRDNORM : 0;
>  }
>  
>  static int roccat_open(struct inode *inode, struct file *file)

[Severity: High]
Can moving the roccat_device_available() check before poll_wait() without
re-checking it afterwards cause a lost wakeup on disconnect?

If roccat_disconnect() runs on another CPU after roccat_device_available()
returns true and before poll_wait() adds the caller to reader->device->wait:

drivers/hid/hid-roccat.c:roccat_disconnect() {
	...
	if (!WARN_ON(!atomic_cmpxchg(&device->exist, 1, 0)))
		device_destroy(device->dev->class, MKDEV(roccat_major, minor));

	if (device->open) {
		hid_hw_close(device->hid);
		wake_up_interruptible(&device->wait);
	}
	...
}

the disconnect wakeup fires before the task is on the waitqueue.
roccat_poll() then registers on reader->device->wait, sees an empty buffer
(avail == false), and returns 0, leaving poll(), select(), or epoll_wait()
sleeping indefinitely on the disconnected device.

Also, in roccat_read(), unread events in the circular buffer are drained
before disconnect is reported:

drivers/hid/hid-roccat.c:roccat_read() {
	...
	/* no data? */
	if (reader->cbuf_start == device->cbuf_end) {
		add_wait_queue(&device->wait, &wait);
		set_current_state(TASK_INTERRUPTIBLE);

		/* wait for data */
		while (reader->cbuf_start == device->cbuf_end) {
			...
			if (!roccat_device_available(device)) {
				retval = -EIO;
				break;
			}
	...
}

When roccat_device_available() is checked before reader->cbuf_start !=
reader->device->cbuf_end, doesn't roccat_poll() return EPOLLERR | EPOLLHUP
without EPOLLIN | EPOLLRDNORM even when unread reports remain in the
circular buffer?

Should poll_wait() remain at the start of roccat_poll(), followed by checking
both the circular buffer under cbuf_lock and roccat_device_available()?

([`drivers/hid/hid-roccat.c:160`](drivers/hid/hid-roccat.c#L160))

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