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