Re: [PATCH] virtio_console: synchronize HVC polling with port removal

[sashiko-bot@kernel.org]

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