Re: [PATCH v3] virtio_console: Introduce an ID allocator for virtual console numbers

[Thomas Huth]

On 22/11/2022 14.46, Cédric Le Goater wrote:
> When a virtio console port is initialized, it is registered as an hvc
> console using a virtual console number. If a KVM guest is started with
> multiple virtio console devices, the same vtermno (or virtual console
> number) can be used to allocate different hvc consoles, which leads to
> various communication problems later on.
>
> This is also reported in debugfs :
>
>    # grep vtermno /sys/kernel/debug/virtio-ports/*
>    /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
>    /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
>    /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
>    /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3
>
> Replace the next_vtermno global with an ID allocator and start the
> allocation at 1 as it is today. Also recycle IDs when a console port
> is removed.
>
> Signed-off-by: Cédric Le Goater <clg@xxxxxxxx>
> ---
>
>   Changes in v3:
>   - made use of ida_alloc_min()
>   - free ID in case of error
>   
>   Changes in v2:
>   - introduced an ID allocator
>
>   drivers/char/virtio_console.c | 26 +++++++++++---------------
>   1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 9fa3c76a267f..6a821118d553 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -13,6 +13,7 @@
>   #include <linux/fs.h>
>   #include <linux/splice.h>
>   #include <linux/pagemap.h>
> +#include <linux/idr.h>
>   #include <linux/init.h>
>   #include <linux/list.h>
>   #include <linux/poll.h>
> @@ -48,22 +49,11 @@ struct ports_driver_data {
>   	/* List of all the devices we're handling */
>   	struct list_head portdevs;
>   
> -	/*
> -	 * This is used to keep track of the number of hvc consoles
> -	 * spawned by this driver.  This number is given as the first
> -	 * argument to hvc_alloc().  To correctly map an initial
> -	 * console spawned via hvc_instantiate to the console being
> -	 * hooked up via hvc_alloc, we need to pass the same vtermno.
> -	 *
> -	 * We also just assume the first console being initialised was
> -	 * the first one that got used as the initial console.
> -	 */
> -	unsigned int next_vtermno;
> -
>   	/* All the console devices handled by this driver */
>   	struct list_head consoles;
>   };
> -static struct ports_driver_data pdrvdata = { .next_vtermno = 1};
> +
> +static struct ports_driver_data pdrvdata;
>   
>   static DEFINE_SPINLOCK(pdrvdata_lock);
>   static DECLARE_COMPLETION(early_console_added);
> @@ -89,6 +79,8 @@ struct console {
>   	u32 vtermno;
>   };
>   
> +static DEFINE_IDA(vtermno_ida);
> +
>   struct port_buffer {
>   	char *buf;
>   
> @@ -1244,18 +1236,21 @@ static int init_port_console(struct port *port)
>   	 * pointers.  The final argument is the output buffer size: we
>   	 * can do any size, so we put PAGE_SIZE here.
>   	 */
> -	port->cons.vtermno = pdrvdata.next_vtermno;
> +	ret = ida_alloc_min(&vtermno_ida, 1, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
>   
> +	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);
> -	pdrvdata.next_vtermno++;
>   	list_add_tail(&port->cons.list, &pdrvdata.consoles);
>   	spin_unlock_irq(&pdrvdata_lock);
>   	port->guest_connected = true;
> @@ -1532,6 +1527,7 @@ static void unplug_port(struct port *port)
>   		list_del(&port->cons.list);
>   		spin_unlock_irq(&pdrvdata_lock);
>   		hvc_remove(port->cons.hvc);
> +		ida_free(&vtermno_ida, port->cons.vtermno);
>   	}
>   
>   	remove_port_data(port);

Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>