Re: [PATCH 2/2] virtio_input: stop callbacks before unregistering input device

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

New issues:
- [High] Breaking early from the event processing loop truncates the input packet, violating input subsystem synchronization rules and leading to stuck keys.

Pre-existing issues:
- [High] Missing virtqueue_detach_unused_buf() for the 'events' virtqueue leads to DMA mapping leaks during device teardown and suspend.
--

--- Patch [2]: [PATCH 2/2] virtio_input: stop callbacks before unregistering input device ---
commit 8b35f1100c300e75de17028044efd92590e5c2f0
Author: Karl Mehltretter <[email protected]>

virtio_input: stop callbacks before unregistering input device

This commit alters the teardown sequence in virtinput_remove() to reset the
virtio device prior to unregistering the input device. It also adds checks
for vi->ready in virtinput_recv_events() to prevent requeuing buffers or
kicking the queue during teardown.

> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e6828..b6843601d0aea 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -49,9 +49,12 @@ static void virtinput_recv_events(struct virtqueue *vq)
>  				    le16_to_cpu(event->code),
>  				    le32_to_cpu(event->value));
>  			spin_lock_irqsave(&vi->lock, flags);
> +			if (!vi->ready)
> +				break;

[Severity: High]
Does breaking out of this loop truncate the input packet?

If the device begins teardown or suspend while the loop is midway through
processing a packet, breaking early aborts reading the remaining events from
the used ring. If an EV_KEY release was processed just before the break, its
corresponding EV_SYN is left in the queue and subsequently destroyed when
the device resets.

Because EV_SYN is required to flush the state to userspace handlers, this
can lead to lost button releases and stuck keys. Should this use "continue"
instead of "break" to safely process the full packet while preventing buffer
requeue during teardown?

>  			virtinput_queue_evtbuf(vi, event);
>  		}
> -		virtqueue_kick(vq);
> +		if (vi->ready)
> +			virtqueue_kick(vq);
>  	}
>  	spin_unlock_irqrestore(&vi->lock, flags);
>  }

[ ... ]

> @@ -350,8 +353,9 @@ static void virtinput_remove(struct virtio_device *vdev)
>  	vi->ready = false;
>  	spin_unlock_irqrestore(&vi->lock, flags);
>  
> -	input_unregister_device(vi->idev);
> +	/* Stop callbacks before unregistering the input device. */
>  	virtio_reset_device(vdev);
> +	input_unregister_device(vi->idev);
>  	while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)

[Severity: High]
This is a pre-existing issue, but does this teardown sequence leak DMA
mappings for the events virtqueue?

During initialization, vi->evt is populated with buffers that establish DMA
mappings. During virtinput_remove() (and similarly in virtinput_freeze()),
the driver correctly drains vi->sts using virtqueue_detach_unused_buf() to
trigger dma_unmap_page(), but it skips vi->evt entirely before calling
vdev->config->del_vqs(vdev).

Since vring_del_virtqueue() only frees the queue descriptors and does not
automatically unmap unused buffers left inside the ring, skipping vi->evt
leaks its DMA mappings on every suspend/resume or driver unbind.

>  		kfree(buf);
>  	vdev->config->del_vqs(vdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2