Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] virtio_console: cons.hvc NULL races in resize_console() and port_fops_open() - [High] virtio_console: missing __rcu on cons.hvc and unsafe init_port_console() store --
commit 67f46e55590e3a9043a67c038c41030d7d15c8e0 Author: Yaohan Li <[email protected]> virtio_console: synchronize HVC polling with port removal This commit protects the hvc_poll() call in in_intr() with an RCU read-side critical section and clears port->cons.hvc followed by synchronize_rcu() in unplug_port() before calling hvc_remove(). > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 019bcae81af5c..a3ee4af106d17 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1551,10 +1551,22 @@ static void unplug_port(struct port *port) > spin_unlock_irq(&port->inbuf_lock); > > if (is_console_port(port)) { > + struct hvc_struct *cons_hvc = port->cons.hvc; > + > spin_lock_irq(&pdrvdata_lock); > list_del(&port->cons.list); > spin_unlock_irq(&pdrvdata_lock); > - hvc_remove(port->cons.hvc); > + > + /* > + * See in_intr(): a data vq callback may be running > + * hvc_poll() on the adapter. Publish a NULL adapter so no > + * new poll can start, wait for in-flight ones, and only > + * then remove the adapter. > + */ > + rcu_assign_pointer(port->cons.hvc, NULL); > + synchronize_rcu(); [Severity: High] Could clearing port->cons.hvc here introduce races with resize_console() and port_fops_open()? Previously, unplug_port() never cleared port->cons.hvc, so once a port was initialized as a console port, port->cons.hvc remained non-NULL and is_console_port(port) returned true for the rest of the port's lifetime. With port->cons.hvc now set to NULL before synchronize_rcu() and hvc_remove(), if hvc_open() -> notifier_add_vio() -> resize_console() runs concurrently in drivers/char/virtio_console.c: static void resize_console(struct port *port) { struct virtio_device *vdev; /* The port could have been hot-unplugged */ if (!port || !is_console_port(port)) return; vdev = port->portdev->vdev; /* Don't test F_SIZE at all if we're rproc: not a valid feature! */ if (!is_rproc_serial(vdev) && virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE)) hvc_resize(port->cons.hvc, port->cons.ws); } is_console_port(port) checks port->cons.hvc once, and then hvc_resize() reloads port->cons.hvc without RCU protection or caching the pointer in a local variable. If unplug_port() clears port->cons.hvc between those two reads, wouldn't hvc_resize() receive NULL and dereference &hp->lock? Similarly, in port_fops_open() in drivers/char/virtio_console.c: if (is_console_port(port)) { ret = -ENXIO; goto out; } /* Allow only one process to open a particular port at a time */ spin_lock_irq(&port->inbuf_lock); if (port->guest_connected) { spin_unlock_irq(&port->inbuf_lock); ret = -EBUSY; goto out; } port->guest_connected = true; If port_fops_open() looks up a console port via find_port_by_devt() just before unplug_port() removes port->list from portdev->ports, and reaches the is_console_port(port) check while unplug_port() is sleeping in synchronize_rcu() or hvc_remove() (before cdev_del()), both port->cons.hvc and port->guest_connected have already been cleared. Wouldn't that allow the unplugging console port to bypass the -ENXIO and -EBUSY checks, set port->guest_connected = true, and send a VIRTIO_CONSOLE_PORT_OPEN control message to the host? > @@ -1820,8 +1833,19 @@ static void in_intr(struct virtqueue *vq) > > wake_up_interruptible(&port->waitqueue); > > - if (is_console_port(port) && hvc_poll(port->cons.hvc)) > + /* > + * The vq callback calls hvc_poll() on the hvc adapter, which > + * hvc_remove() frees on unplug without any lock shared with this > + * hard-IRQ path (calling hvc_poll() with port->inbuf_lock held > + * would deadlock: the hvc ops take that lock themselves). Take an > + * RCU read lock across the poll and have the unplug path NULL out > + * the adapter and wait for in-flight callbacks before removing it. > + */ > + rcu_read_lock(); > + cons_hvc = rcu_dereference(port->cons.hvc); > + if (cons_hvc && hvc_poll(cons_hvc)) > hvc_kick(); > + rcu_read_unlock(); [Severity: High] Should struct console::hvc be annotated with __rcu, and does init_port_console() need to avoid publishing an error or partially initialized pointer to in_intr()? In drivers/char/virtio_console.c, struct console still declares: struct console { ... /* The hvc device associated with this console port */ struct hvc_struct *hvc; ... }; Without the __rcu annotation on hvc, rcu_assign_pointer() and rcu_dereference() trigger Sparse address-space warnings. In addition, on multiport devices, add_port() populates port->in_vq and links the port before the host sends VIRTIO_CONSOLE_CONSOLE_PORT, so in_intr() can already run when handle_control_message() calls init_port_console(port): port->cons.vtermno = ret; port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE); if (IS_ERR(port->cons.hvc)) { ret = PTR_ERR(port->cons.hvc); dev_err(port->dev, "error %d allocating hvc for port\n", ret); port->cons.hvc = NULL; ida_free(&vtermno_ida, port->cons.vtermno); return ret; } spin_lock_irq(&pdrvdata_lock); list_add_tail(&port->cons.list, &pdrvdata.consoles); spin_unlock_irq(&pdrvdata_lock); port->guest_connected = true; If hvc_alloc() returns an ERR_PTR, port->cons.hvc transiently holds that non-NULL error pointer until it is reset to NULL after dev_err(). If in_intr() runs during that window, cons_hvc = rcu_dereference(port->cons.hvc) will be non-NULL and hvc_poll(cons_hvc) will dereference the ERR_PTR. Even when hvc_alloc() succeeds, port->cons.hvc is stored without rcu_assign_pointer() and before port->cons.list is added to pdrvdata.consoles, so a concurrent in_intr() -> hvc_poll() -> get_chars() -> find_port_by_vtermno() can fail to find the console and return -EPIPE (triggering tty_hangup()). Should init_port_console() store the hvc_alloc() result in a local variable, validate it with IS_ERR(), add port->cons.list to pdrvdata.consoles, and then publish port->cons.hvc via rcu_assign_pointer()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
