* Re: [PATCH] virtio_console: synchronize HVC polling with port removal
2026-11-04 9:45 [PATCH] virtio_console: synchronize HVC polling with port removal Yaohan Li
@ 2026-11-04 9:57 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-11-04 9:57 UTC (permalink / raw)
To: Yaohan Li; +Cc: virtualization, Michael S. Tsirkin, Eugenio Perez
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 <shunsheng1218@gmail.com>
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/179378553951.72222.12851179500568566393@gmail.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread