Re: [PATCH v3] 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 1 potential
issue to consider:
- [Medium] virtio_console: unplug race in fill_readbuf() and
  port_fops_read() returns 0
--

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

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

This commit tracks ownership of port input buffers across unlocked reads
and adds an unplugging state to prevent use-after-free and buffer reorder
races during concurrent reads and port hot-unplug.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 019bcae81af5c..d1757017d9bd0 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
[ ... ]
> @@ -534,19 +551,48 @@ static void discard_port_data(struct port *port)
>  			 err);
>  }
>  
> -static bool port_has_data(struct port *port)
> +/*
> + * Input state of a port, snapshotted under inbuf_lock.  The distinction
> + * between a buffer that can be acquired now (AVAILABLE) and one that is
> + * owned by another reader (CLAIMED) matters for poll() and for blocking
> + * reads: only AVAILABLE must be reported as readable, while both mean
> + * that unread data may still be pending for the EOF logic.
> + */
> +enum port_input_state {
> +	PORT_INPUT_EMPTY,
> +	PORT_INPUT_AVAILABLE,
> +	PORT_INPUT_CLAIMED,
> +	PORT_INPUT_UNPLUGGED,
> +};
> +
> +/* Callers must not hold inbuf_lock. */
> +static enum port_input_state port_input_state(struct port *port)
>  {
>  	unsigned long flags;
> -	bool ret;
> +	enum port_input_state state;
>  
> -	ret = false;
>  	spin_lock_irqsave(&port->inbuf_lock, flags);
> -	port->inbuf = get_inbuf(port);
> -	if (port->inbuf)
> -		ret = true;
> +
> +	if (port->unplugging || !port->portdev) {
> +		state = PORT_INPUT_UNPLUGGED;
> +	} else if (port->inbuf_claimed) {
> +		state = PORT_INPUT_CLAIMED;
> +	} else {
> +		port->inbuf = get_inbuf(port);
> +		state = port->inbuf ? PORT_INPUT_AVAILABLE
> +				    : PORT_INPUT_EMPTY;
> +	}
>  
>  	spin_unlock_irqrestore(&port->inbuf_lock, flags);
> -	return ret;
> +	return state;
> +}
> +
> +static bool port_has_data(struct port *port)
> +{
> +	enum port_input_state state = port_input_state(port);
> +
> +	return state == PORT_INPUT_AVAILABLE ||
> +	       state == PORT_INPUT_CLAIMED;
>  }
[ ... ]
> @@ -692,24 +738,112 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
[ ... ]
> +	port->inbuf = get_inbuf(port);
>  	buf = port->inbuf;
> +	if (!buf) {
> +		spin_unlock_irqrestore(&port->inbuf_lock, flags);
> +		if (!to_user)
> +			return 0;
> +		if (nonblock)
> +			return READ_ONCE(port->host_connected) ? -EAGAIN : 0;
> +		/*
> +		 * The data we saw was consumed by another reader while we
> +		 * waited for its claim.  Keep blocking like the fops-level
> +		 * wait instead of reporting a spurious EOF: wait for new
> +		 * data, a disconnect or the unplug fence.
> +		 */
> +		err = wait_event_freezable(port->waitqueue,
> +					   !will_read_block(port));
> +		if (err)
> +			return err;
> +		if (!READ_ONCE(port->guest_connected))
> +			return -ENODEV;
> +		if (!READ_ONCE(port->host_connected) && !port_has_data(port))
> +			return 0;
> +		goto retry;
> +	}

[Severity: Medium]
Can fill_readbuf() and port_fops_read() still return 0 instead of -ENODEV
when a read races with unplug_port()?

unplug_port() sets port->unplugging = true under inbuf_lock, drops the
lock to remove port->list under ports_lock, and only then re-acquires
inbuf_lock to clear guest_connected and host_connected.  Furthermore,
port_has_data() returns false for PORT_INPUT_UNPLUGGED, and
port->host_connected is read outside inbuf_lock:

CPU0 (fill_readbuf)               CPU1 (unplug_port)
wait_event_freezable(...)
                                  spin_lock_irq(&port->inbuf_lock)
                                  port->unplugging = true
                                  spin_unlock_irq(&port->inbuf_lock)
                                  ...
                                  spin_lock_irq(&port->inbuf_lock)
  !READ_ONCE(port->guest_connected)
    -> false
                                  port->guest_connected = false
                                  port->host_connected = false
                                  spin_unlock_irq(&port->inbuf_lock)
  !READ_ONCE(port->host_connected)
    && !port_has_data(port)
    -> true (PORT_INPUT_UNPLUGGED)
  return 0;  <-- returns EOF instead of -ENODEV

The same split between port_input_state() (or get_inbuf()) and
READ_ONCE(port->host_connected) exists in the non-blocking path above and
at bothPORT_INPUT_EMPTY checks in port_fops_read(): if unplug_port() runs
after port_input_state() returns PORT_INPUT_EMPTY, it clears
port->host_connected and the reader returns 0 instead of -ENODEV.

In addition, because unplug_port() sets port->unplugging before clearing
port->guest_connected, port_fops_poll() can observe port->guest_connected
as true and then call will_read_block(), which sees PORT_INPUT_UNPLUGGED
and returns false, reporting EPOLLIN | EPOLLRDNORM on an unplugging port
with no data.

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