Re: [PATCH v2] virtio_console: fix buffer ownership and unplug races in the read path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] virtio_console: NULL portdev dereference and vq access in in_intr() on unplug
- [Medium] virtio_console: poll and O_NONBLOCK read mismatch when inbuf_claimed is set
- [Medium] virtio_console: fill_readbuf() returns 0 instead of -ENODEV on port unplug
--

commit c34e695a42ae05ed26e613884cc4fae84545b23a
Author: Yaohan Li <shunsheng1218@gmail.com>

virtio_console: fix buffer ownership and unplug races in the read path

This commit introduces an ownership claim protocol (port->inbuf_claimed) and
an unplug flag (port->unplugging) under port->inbuf_lock to serialize buffer
access in fill_readbuf() and prevent input virtqueue operations during port
teardown.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 019bcae81af5c..191dceb77df1b 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c

[ ... ]

> @@ -541,10 +558,28 @@ static bool port_has_data(struct port *port)
>  
>  	ret = false;
>  	spin_lock_irqsave(&port->inbuf_lock, flags);
> +
> +	/* The port is going away; its vqs are being torn down. */
> +	if (port->unplugging || !port->portdev)
> +		goto out_unlock;
> +
> +	/*
> +	 * A reader owns the claimed buffer, possibly sleeping in
> +	 * copy_to_user().  Report data as pending for the
> +	 * EOF/host-disconnect logic, but do not prefetch the next
> +	 * buffer: the claimer's unconsumed remainder must go back to
> +	 * inbuf first, or the prefetched buffer would be dropped.
> +	 */
> +	if (port->inbuf_claimed) {
> +		ret = true;
> +		goto out_unlock;
> +	}
> +
>  	port->inbuf = get_inbuf(port);
>  	if (port->inbuf)
>  		ret = true;
>  
> +out_unlock:
>  	spin_unlock_irqrestore(&port->inbuf_lock, flags);
>  	return ret;
>  }
> @@ -692,24 +727,111 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
>   * Give out the data that's requested from the buffer that we have
>   * queued up.
>   */
> +static bool will_read_block(struct port *port);
> +
>  static ssize_t fill_readbuf(struct port *port, u8 __user *out_buf,
> -			    size_t out_count, bool to_user)
> +			    size_t out_count, bool to_user, bool nonblock)
>  {
>  	struct port_buffer *buf;
>  	unsigned long flags;
> +	bool free_requeue_buf = false;
> +	int err;
> +
> +	if (!out_count)
> +		return 0;
> +
> +retry:
> +	spin_lock_irqsave(&port->inbuf_lock, flags);
>  
> -	if (!out_count || !port_has_data(port))
> +	/* The port is going away; its vqs are being torn down. */
> +	if (port->unplugging || !port->portdev) {
> +		spin_unlock_irqrestore(&port->inbuf_lock, flags);
>  		return 0;
> +	}

[Severity: Medium]
Should fill_readbuf() return -ENODEV instead of 0 when to_user is true and
port->unplugging || !port->portdev is set?

In port_fops_read() and in the second wait path of fill_readbuf(), hot-unplug
(!port->guest_connected) returns -ENODEV to userspace to distinguish port
removal from a normal host disconnect (0):

drivers/char/virtio_console.c:port_fops_read() {
	if (!port->guest_connected)
		return -ENODEV;
}

However, if a blocking userspace reader waits on port->inbuf_claimed in
fill_readbuf() and is woken by unplug_port() setting port->unplugging = true,
jumping to retry hits this check and returns 0 instead of -ENODEV.

Similarly, if unplug_port() runs after port_fops_read() checks
!port->guest_connected, or between the two spin_lock_irq(&port->inbuf_lock)
critical sections in unplug_port() (where port->unplugging = true is set
before port->guest_connected = false), port_has_data() returns false and
port_fops_read() or fill_readbuf() can return 0 or -EAGAIN instead of
-ENODEV.

>  
> +	/*
> +	 * Another reader (a userspace read() or the HVC console poll)
> +	 * owns the claimed buffer, possibly sleeping in copy_to_user().
> +	 * Prefetching the next buffer here would drop or reorder the
> +	 * claimed data once the owner returns its unconsumed remainder,
> +	 * so report no data instead.  The HVC poll will simply retry;
> +	 * a userspace reader waits for the claim to be released.
> +	 */
> +	if (port->inbuf_claimed) {
> +		spin_unlock_irqrestore(&port->inbuf_lock, flags);
> +		if (!to_user)
> +			return 0;
> +		if (nonblock)
> +			return -EAGAIN;

[Severity: Medium]
Can returning true from port_has_data() while returning -EAGAIN here cause a
busy loop between poll() and non-blocking read() when port->inbuf_claimed is
set?

When one reader has claimed port->inbuf and dropped port->inbuf_lock across
copy_to_user(), port_has_data() returns true, so will_read_block() evaluates
to false:

drivers/char/virtio_console.c:will_read_block() {
	if (!port->guest_connected) {
		/* Port got hot-unplugged. Let's exit. */
		return false;
	}
	return !port_has_data(port) && port->host_connected;
}

drivers/char/virtio_console.c:port_fops_poll() {
	...
	if (!will_read_block(port))
		ret |= EPOLLIN | EPOLLRDNORM;
}

A peer thread polling the same descriptor with O_NONBLOCK will see
EPOLLIN | EPOLLRDNORM from port_fops_poll(), call port_fops_read() ->
fill_readbuf(), and immediately get -EAGAIN while port->inbuf_claimed remains
true. In addition, a blocking reader waiting in fill_readbuf() at
wait_event_freezable(port->waitqueue, !will_read_block(port)) will wake up as
soon as another reader claims a buffer, even though no buffer can be acquired
yet.

[ ... ]

> @@ -1791,7 +1971,15 @@ static void in_intr(struct virtqueue *vq)
>  	}
>  
>  	spin_lock_irqsave(&port->inbuf_lock, flags);
> -	port->inbuf = get_inbuf(port);
> +
> +	/*
> +	 * A reader may own an exclusively claimed buffer, possibly
> +	 * sleeping in copy_to_user().  Do not prefetch the next buffer
> +	 * now: the claimer's unconsumed remainder must go back to
> +	 * port->inbuf first, or the prefetched buffer would be lost.
> +	 */
> +	if (!port->unplugging && !port->inbuf_claimed)
> +		port->inbuf = get_inbuf(port);
>  
>  	/*
>  	 * Normally the port should not accept data when the port is

[Severity: High]
If in_intr() races with unplug_port(), can falling through after skipping
get_inbuf() dereference a NULL port->portdev or touch the virtqueue during
teardown?

When in_intr() runs concurrently with unplug_port(), find_port_by_vq() can
obtain a port reference just before unplug_port() removes the port from
portdev->ports. In unplug_port():

drivers/char/virtio_console.c:unplug_port() {
	spin_lock_irq(&port->inbuf_lock);
	port->unplugging = true;
	spin_unlock_irq(&port->inbuf_lock);
	...
	spin_lock_irq(&port->inbuf_lock);
	if (port->guest_connected) {
		...
		port->guest_connected = false;
		port->host_connected = false;
		...
	}
	spin_unlock_irq(&port->inbuf_lock);
	...
	spin_lock_irq(&port->outvq_lock);
	port->portdev = NULL;
	spin_unlock_irq(&port->outvq_lock);
}

When in_intr() then acquires port->inbuf_lock, the new check skips
port->inbuf = get_inbuf(port), but execution continues directly into:

drivers/char/virtio_console.c:in_intr() {
	if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev))
		discard_port_data(port);

	/* Send a SIGIO indicating new data in case the process asked for it */
	send_sigio_to_port(port);

	spin_unlock_irqrestore(&port->inbuf_lock, flags);

	wake_up_interruptible(&port->waitqueue);

	if (is_console_port(port) && hvc_poll(port->cons.hvc))
		hvc_kick();
}

Because unplug_port() cleared port->guest_connected, in_intr() evaluates
is_rproc_serial(port->portdev->vdev). If unplug_port() has already set
port->portdev = NULL under outvq_lock, this dereferences a NULL port->portdev
before reaching the !port->portdev check inside discard_port_data(). Even if
port->portdev is not yet NULL, discard_port_data() will call get_inbuf() and
add_inbuf() on the unplugging virtqueue (or while port->inbuf_claimed is
held), and in_intr() can then call hvc_poll(port->cons.hvc) after
hvc_remove().

Should in_intr() bail out when port->unplugging || !port->portdev is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105114535.972013-1-shunsheng1218@gmail.com?part=1