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 <[email protected]> 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/[email protected]?part=1
