Re: [PATCH] virtio_console: add console port to list before hvc_alloc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] virtio_console: race between __send_to_port() and out_intr() in add_port()
--

commit e1686190742677c49792956e3ec4f167b8b193e9
Author: Murat Seker <muratseker@meta.com>

virtio_console: add console port to list before hvc_alloc()

This commit adds the console port to pdrvdata.consoles before calling
hvc_alloc() in init_port_console() so that put_chars() can find the port
when register_console() replays the kernel log buffer. If hvc_alloc()
fails, the port is removed from pdrvdata.consoles before freeing the
vtermno.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 019bcae81af5c..a31fdac658913 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1280,18 +1280,28 @@ static int init_port_console(struct port *port)
>  		return ret;
>  
>  	port->cons.vtermno = ret;
> +
> +	/*
> +	 * hvc_alloc() may register the console, which replays the log buffer
> +	 * through put_chars().  put_chars() finds the port on this list, so
> +	 * add it first or the replayed messages are dropped.
> +	 */
> +	spin_lock_irq(&pdrvdata_lock);
> +	list_add_tail(&port->cons.list, &pdrvdata.consoles);
> +	spin_unlock_irq(&pdrvdata_lock);
> +
>  	port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);

[Severity: Critical]
When probing a non-multiport device with a preferred hvc console, can this
cause a race between __send_to_port() and out_intr() on port->out_vq?

In virtcons_probe(), init_vqs() sets out_intr() as the callback for
portdev->out_vqs[0] and virtio_device_ready() enables virtqueue interrupts
before add_port() is called.

In add_port(), when !use_multiport(port->portdev) is true,
init_port_console() is called before the port is added to
port->portdev->ports:

drivers/char/virtio_console.c:add_port() {
	...
	else if (!use_multiport(port->portdev)) {
		/*
		 * If we're not using multiport support,
		 * this has to be a console port.
		 */
		err = init_port_console(port);
		if (err)
			goto free_inbufs;
	}

	spin_lock_irq(&portdev->ports_lock);
	list_add_tail(&port->list, &port->portdev->ports);
	spin_unlock_irq(&portdev->ports_lock);
	...
}

Because port->cons.list is now in pdrvdata.consoles during hvc_alloc(),
hvc_alloc() -> hvc_check_console() -> register_console() synchronously
replays the kernel log buffer through hvc_console_print() -> put_chars() ->
__send_to_port() while port is still not in portdev->ports.

__send_to_port() acquires port->outvq_lock with spin_lock_irqsave(), queues
the buffer on out_vq, kicks the host, and spins waiting for completion:

drivers/char/virtio_console.c:__send_to_port() {
	...
	/* Tell Host to go! */
	virtqueue_kick(out_vq);

	if (!in_blocking) {
		...
		while (!(buf = virtqueue_get_buf(out_vq, &len))
			&& !virtqueue_is_broken(out_vq))
			cpu_relax();
	}
	...
}

If the host completes the buffer and raises an interrupt on another CPU
before add_port() adds the port to portdev->ports, out_intr() runs and
find_port_by_vq() returns NULL:

drivers/char/virtio_console.c:out_intr() {
	struct port *port;

	port = find_port_by_vq(vq->vdev->priv, vq);
	if (!port) {
		flush_bufs(vq, false);
		return;
	}
	...
}

Wouldn't flush_bufs() then call virtqueue_get_buf() and free_buf() on
out_vq without holding port->outvq_lock, corrupting the virtqueue ring
concurrently with __send_to_port() and stealing the completed buffer so
that __send_to_port() spins forever with interrupts disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101022435.3466820-1-muratseker@meta.com?part=1