Re: [PATCH AUTOSEL 6.18-5.10] virtio_console: fix control queue race during restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] virtio_console: dropped host control replies in virtcons_restore()
--

commit 896c2558dbc3180e4d84cf033ee9bf699250ed69
Author: Sungho Bae <baver.bae@lge.com>

virtio_console: fix control queue race during restore

This commit defers populating the control receive queue (portdev->c_ivq) in
virtcons_restore() until after iterating over portdev->ports to prevent
concurrent list modification from control_work_handler().

Link: https://patch.msgid.link/20260603183757.21587-4-baver.bae@gmail.com

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 088182e54debd..6dd0cf78bf839 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -2136,9 +2136,6 @@ static int virtcons_restore(struct virtio_device *vdev)
>  
>  	virtio_device_ready(portdev->vdev);
>  
> -	if (use_multiport(portdev))
> -		fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
> -
>  	list_for_each_entry(port, &portdev->ports, list) {
>  		port->in_vq = portdev->in_vqs[port->id];
>  		port->out_vq = portdev->out_vqs[port->id];
> @@ -2155,6 +2152,18 @@ static int virtcons_restore(struct virtio_device *vdev)
>  		if (port->guest_connected)
>  			send_control_msg(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
>  	}
> +
> +	/*
> +	 * Populate the control receive queue only after the list iteration
> +	 * is complete. If we fill this queue before iterating, the host could
> +	 * immediately deliver a VIRTIO_CONSOLE_PORT_REMOVE message.
> +	 * This would trigger the control workqueue, which modifies the
> +	 * portdev->ports list concurrently with the unprotected loop above,
> +	 * leading to a Use-After-Free and list corruption.
> +	 */
> +	if (use_multiport(portdev))
> +		fill_queue(portdev->c_ivq, &portdev->c_ivq_lock);
> +
>  	return 0;
>  }
>  #endif

[Severity: High]
Won't deferring fill_queue(portdev->c_ivq, &portdev->c_ivq_lock) until after
the loop cause the host's control replies to be dropped during restore?

During freeze, virtcons_freeze() clears port->host_connected for every port
and destroys all virtqueues:

drivers/char/virtio_console.c:virtcons_freeze() {
	...
	list_for_each_entry(port, &portdev->ports, list) {
		virtqueue_disable_cb(port->in_vq);
		virtqueue_disable_cb(port->out_vq);
		/*
		 * We'll ask the host later if the new invocation has
		 * the port opened or closed.
		 */
		port->host_connected = false;
		remove_port_data(port);
	}
	remove_vqs(portdev);
	...
}

In virtcons_restore(), the list_for_each_entry() loop queries the host for
each port's open/close status via send_control_msg() -> __send_control_msg():

drivers/char/virtio_console.c:virtcons_restore() {
	...
	list_for_each_entry(port, &portdev->ports, list) {
		...
		/* Get port open/close status on the host */
		send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 1);
		...
	}
}

Because __send_control_msg() kicks portdev->c_ovq and busy-waits
synchronously until the host consumes the VIRTIO_CONSOLE_PORT_READY request:

drivers/char/virtio_console.c:__send_control_msg() {
	...
	if (virtqueue_add_outbuf(vq, sg, 1, &portdev->cpkt, GFP_ATOMIC) == 0) {
		virtqueue_kick(vq);\
		while (!virtqueue_get_buf(vq, &len)
			&& !virtqueue_is_broken(vq))
			cpu_relax();
	}
	...
}

the host processes VIRTIO_CONSOLE_PORT_READY and immediately attempts to send
back VIRTIO_CONSOLE_PORT_OPEN (along with VIRTIO_CONSOLE_CONSOLE_PORT and
VIRTIO_CONSOLE_PORT_NAME) on portdev->c_ivq before __send_control_msg()
returns.

If fill_queue(portdev->c_ivq, &portdev->c_ivq_lock) has not been called yet,
portdev->c_ivq has zero receive buffers when the host replies, causing those
VIRTIO_CONSOLE_PORT_OPEN replies to be dropped and leaving
port->host_connected permanently stuck at false after restore (which causes
port_fops_write() to block or fail with -EAGAIN, port_fops_read() to return
EOF, and port_fops_poll() to report EPOLLHUP).

Would it work to keep fill_queue(portdev->c_ivq, &portdev->c_ivq_lock) before
the loop, but disable the c_ivq callback or defer scheduling control_work
until the loop finishes so the host has receive buffers available while list
traversal is still protected from concurrent control_work_handler() runs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-390-sashal@kernel.org?part=1