Linux Virtualization - Apr 2018 - [PATCH 3/6] virtio_console: free buffers after reset

Turns out virtio console tries to take a buffer out of an active vq.
Works by sheer luck, and is explicitly forbidden by spec.  And while
going over it I saw that error handling is also broken -
failure is easy to trigger if I force allocations to fail.

Lightly tested.

Michael S. Tsirkin (6):
  virtio_console: don't tie bufs to a vq
  virtio: add ability to iterate over vqs
  virtio_console: free buffers after reset
  virtio_console: drop custom control queue cleanup
  virtio_console: move removal code
  virtio_console: reset on out of memory

 drivers/char/virtio_console.c | 155 ++++++++++++++++++++----------------------
 include/linux/virtio.h        |   3 +
 2 files changed, 75 insertions(+), 83 deletions(-)

-- 
MST

an allocated buffer doesn't need to be tied to a vq -
only vq->vdev is ever used. Pass the function the
just what it needs - the vdev.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/char/virtio_console.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 468f061..3e56f32 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
 	}
 }
 
-static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t
buf_size,
 				     int pages)
 {
 	struct port_buffer *buf;
@@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq,
size_t buf_size,
 		return buf;
 	}
 
-	if (is_rproc_serial(vq->vdev)) {
+	if (is_rproc_serial(vdev)) {
 		/*
 		 * Allocate DMA memory from ancestor. When a virtio
 		 * device is created by remoteproc, the DMA memory is
 		 * associated with the grandparent device:
 		 * vdev => rproc => platform-dev.
 		 */
-		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+		if (!vdev->dev.parent || !vdev->dev.parent->parent)
 			goto free_buf;
-		buf->dev = vq->vdev->dev.parent->parent;
+		buf->dev = vdev->dev.parent->parent;
 
 		/* Increase device refcnt to avoid freeing it */
 		get_device(buf->dev);
@@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const char
__user *ubuf,
 
 	count = min((size_t)(32 * 1024), count);
 
-	buf = alloc_buf(port->out_vq, count, 0);
+	buf = alloc_buf(port->portdev->vdev, count, 0);
 	if (!buf)
 		return -ENOMEM;
 
@@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info
*pipe,
 	if (ret < 0)
 		goto error_out;
 
-	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
+	buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
 	if (!buf) {
 		ret = -ENOMEM;
 		goto error_out;
@@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq,
spinlock_t *lock)
 
 	nr_added_bufs = 0;
 	do {
-		buf = alloc_buf(vq, PAGE_SIZE, 0);
+		buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
 		if (!buf)
 			break;
 
-- 
MST

Console driver is out of spec. The spec says:
	A driver MUST NOT decrement the available idx on a live
	virtqueue (ie. there is no way to ?unexpose? buffers).
and it does exactly that by trying to detach unused buffers
without doing a device reset first.

Defer detaching the buffers until device unplug.

Of course this means we might get an interrupt for
a vq without an attached port now. Handle that by
discarding the consumed buffer.

Reported-by: Tiwei Bie <tiwei.bie at intel.com>
Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
CC: stable at kernel.org
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/char/virtio_console.c | 49 +++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3e56f32..26a66ff 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1402,7 +1402,6 @@ static int add_port(struct ports_device *portdev, u32 id)
 {
 	char debugfs_name[16];
 	struct port *port;
-	struct port_buffer *buf;
 	dev_t devt;
 	unsigned int nr_added_bufs;
 	int err;
@@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32 id)
 	return 0;
 
 free_inbufs:
-	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf, true);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:
@@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref)
 
 static void remove_port_data(struct port *port)
 {
-	struct port_buffer *buf;
-
 	spin_lock_irq(&port->inbuf_lock);
 	/* Remove unused data this port might have received. */
 	discard_port_data(port);
 	spin_unlock_irq(&port->inbuf_lock);
 
-	/* Remove buffers we queued up for the Host to send us data in. */
-	do {
-		spin_lock_irq(&port->inbuf_lock);
-		buf = virtqueue_detach_unused_buf(port->in_vq);
-		spin_unlock_irq(&port->inbuf_lock);
-		if (buf)
-			free_buf(buf, true);
-	} while (buf);
-
 	spin_lock_irq(&port->outvq_lock);
 	reclaim_consumed_buffers(port);
 	spin_unlock_irq(&port->outvq_lock);
-
-	/* Free pending buffers from the out-queue. */
-	do {
-		spin_lock_irq(&port->outvq_lock);
-		buf = virtqueue_detach_unused_buf(port->out_vq);
-		spin_unlock_irq(&port->outvq_lock);
-		if (buf)
-			free_buf(buf, true);
-	} while (buf);
 }
 
 /*
@@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct
*work)
 	spin_unlock(&portdev->c_ivq_lock);
 }
 
+static void flush_bufs(struct virtqueue *vq, bool can_sleep)
+{
+	struct port_buffer *buf;
+	unsigned int len;
+
+	while ((buf = virtqueue_get_buf(vq, &len)))
+		free_buf(buf, can_sleep);
+}
+
 static void out_intr(struct virtqueue *vq)
 {
 	struct port *port;
 
 	port = find_port_by_vq(vq->vdev->priv, vq);
-	if (!port)
+	if (!port) {
+		flush_bufs(vq, false);
 		return;
+	}
 
 	wake_up_interruptible(&port->waitqueue);
 }
@@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq)
 	unsigned long flags;
 
 	port = find_port_by_vq(vq->vdev->priv, vq);
-	if (!port)
+	if (!port) {
+		flush_bufs(vq, false);
 		return;
+	}
 
 	spin_lock_irqsave(&port->inbuf_lock, flags);
 	port->inbuf = get_inbuf(port);
@@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = {
 
 static void remove_vqs(struct ports_device *portdev)
 {
+	struct virtqueue *vq;
+
+	virtio_device_for_each_vq(portdev->vdev, vq) {
+		struct port_buffer *buf;
+
+		flush_bufs(vq, true);
+		while ((buf = virtqueue_detach_unused_buf(vq)))
+			free_buf(buf, true);
+	}
 	portdev->vdev->config->del_vqs(portdev->vdev);
 	kfree(portdev->in_vqs);
 	kfree(portdev->out_vqs);
-- 
MST

For cleanup it's helpful to be able to simply scan all vqs and discard
all data. Add an iterator to do that.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 include/linux/virtio.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 988c735..fa1b5da 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -157,6 +157,9 @@ int virtio_device_freeze(struct virtio_device *dev);
 int virtio_device_restore(struct virtio_device *dev);
 #endif
 
+#define virtio_device_for_each_vq(vdev, vq) \
+	list_for_each_entry(vq, &vdev->vqs, list)
+
 /**
  * virtio_driver - operations for a virtio I/O driver
  * @driver: underlying device driver (populate name and owner).
-- 
MST

We now cleanup all VQs on device removal - no need
to handle the control VQ specially.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/char/virtio_console.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 26a66ff..2d87ce5 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1988,21 +1988,6 @@ static void remove_vqs(struct ports_device *portdev)
 	kfree(portdev->out_vqs);
 }
 
-static void remove_controlq_data(struct ports_device *portdev)
-{
-	struct port_buffer *buf;
-	unsigned int len;
-
-	if (!use_multiport(portdev))
-		return;
-
-	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-		free_buf(buf, true);
-
-	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-		free_buf(buf, true);
-}
-
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
@@ -2163,7 +2148,6 @@ static void virtcons_remove(struct virtio_device *vdev)
 	 * have to just stop using the port, as the vqs are going
 	 * away.
 	 */
-	remove_controlq_data(portdev);
 	remove_vqs(portdev);
 	kfree(portdev);
 }
@@ -2208,7 +2192,6 @@ static int virtcons_freeze(struct virtio_device *vdev)
 	 */
 	if (use_multiport(portdev))
 		virtqueue_disable_cb(portdev->c_ivq);
-	remove_controlq_data(portdev);
 
 	list_for_each_entry(port, &portdev->ports, list) {
 		virtqueue_disable_cb(port->in_vq);
-- 
MST

Will make it reusable for error handling.

Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/char/virtio_console.c | 72 +++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 2d87ce5..e8480fe 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1988,6 +1988,42 @@ static void remove_vqs(struct ports_device *portdev)
 	kfree(portdev->out_vqs);
 }
 
+static void virtcons_remove(struct virtio_device *vdev)
+{
+	struct ports_device *portdev;
+	struct port *port, *port2;
+
+	portdev = vdev->priv;
+
+	spin_lock_irq(&pdrvdata_lock);
+	list_del(&portdev->list);
+	spin_unlock_irq(&pdrvdata_lock);
+
+	/* Disable interrupts for vqs */
+	vdev->config->reset(vdev);
+	/* Finish up work that's lined up */
+	if (use_multiport(portdev))
+		cancel_work_sync(&portdev->control_work);
+	else
+		cancel_work_sync(&portdev->config_work);
+
+	list_for_each_entry_safe(port, port2, &portdev->ports, list)
+		unplug_port(port);
+
+	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
+
+	/*
+	 * When yanking out a device, we immediately lose the
+	 * (device-side) queues.  So there's no point in keeping the
+	 * guest side around till we drop our final reference.  This
+	 * also means that any ports which are in an open state will
+	 * have to just stop using the port, as the vqs are going
+	 * away.
+	 */
+	remove_vqs(portdev);
+	kfree(portdev);
+}
+
 /*
  * Once we're further in boot, we get probed like any other virtio
  * device.
@@ -2116,42 +2152,6 @@ static int virtcons_probe(struct virtio_device *vdev)
 	return err;
 }
 
-static void virtcons_remove(struct virtio_device *vdev)
-{
-	struct ports_device *portdev;
-	struct port *port, *port2;
-
-	portdev = vdev->priv;
-
-	spin_lock_irq(&pdrvdata_lock);
-	list_del(&portdev->list);
-	spin_unlock_irq(&pdrvdata_lock);
-
-	/* Disable interrupts for vqs */
-	vdev->config->reset(vdev);
-	/* Finish up work that's lined up */
-	if (use_multiport(portdev))
-		cancel_work_sync(&portdev->control_work);
-	else
-		cancel_work_sync(&portdev->config_work);
-
-	list_for_each_entry_safe(port, port2, &portdev->ports, list)
-		unplug_port(port);
-
-	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
-
-	/*
-	 * When yanking out a device, we immediately lose the
-	 * (device-side) queues.  So there's no point in keeping the
-	 * guest side around till we drop our final reference.  This
-	 * also means that any ports which are in an open state will
-	 * have to just stop using the port, as the vqs are going
-	 * away.
-	 */
-	remove_vqs(portdev);
-	kfree(portdev);
-}
-
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_CONSOLE, VIRTIO_DEV_ANY_ID },
 	{ 0 },
-- 
MST

When out of memory and we can't add ctrl vq buffers,
probe fails. Unfortunately the error handling is
out of spec: it calls del_vqs without bothering
to reset the device first.

To fix, call the full cleanup function in this case.

Cc: stable at vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
 drivers/char/virtio_console.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index e8480fe..2108551 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -2090,6 +2090,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 
 	spin_lock_init(&portdev->ports_lock);
 	INIT_LIST_HEAD(&portdev->ports);
+	INIT_LIST_HEAD(&portdev->list);
 
 	virtio_device_ready(portdev->vdev);
 
@@ -2107,8 +2108,15 @@ static int virtcons_probe(struct virtio_device *vdev)
 		if (!nr_added_bufs) {
 			dev_err(&vdev->dev,
 				"Error allocating buffers for control queue\n");
-			err = -ENOMEM;
-			goto free_vqs;
+			/*
+			 * The host might want to notify mgmt sw about device
+			 * add failure.
+			 */
+			__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+					   VIRTIO_CONSOLE_DEVICE_READY, 0);
+			/* Device was functional: we need full cleanup. */
+			virtcons_remove(vdev);
+			return -ENOMEM;
 		}
 	} else {
 		/*
@@ -2139,11 +2147,6 @@ static int virtcons_probe(struct virtio_device *vdev)
 
 	return 0;
 
-free_vqs:
-	/* The host might want to notify mgmt sw about device add failure */
-	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
-			   VIRTIO_CONSOLE_DEVICE_READY, 0);
-	remove_vqs(portdev);
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
 free:
-- 
MST

On Fri, Apr 20, 2018 at 09:18:01PM +0300, Michael S. Tsirkin
wrote:
> an allocated buffer doesn't need to be tied to a vq -
> only vq->vdev is ever used. Pass the function the
> just what it needs - the vdev.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> ---
>  drivers/char/virtio_console.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 468f061..3e56f32 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -422,7 +422,7 @@ static void reclaim_dma_bufs(void)
>  	}
>  }
>  
> -static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t
buf_size,
> +static struct port_buffer *alloc_buf(struct virtio_device *vdev, size_t
buf_size,
>  				     int pages)
>  {
>  	struct port_buffer *buf;
> @@ -445,16 +445,16 @@ static struct port_buffer *alloc_buf(struct virtqueue
*vq, size_t buf_size,
>  		return buf;
>  	}
>  
> -	if (is_rproc_serial(vq->vdev)) {
> +	if (is_rproc_serial(vdev)) {
>  		/*
>  		 * Allocate DMA memory from ancestor. When a virtio
>  		 * device is created by remoteproc, the DMA memory is
>  		 * associated with the grandparent device:
>  		 * vdev => rproc => platform-dev.
>  		 */
> -		if (!vq->vdev->dev.parent ||
!vq->vdev->dev.parent->parent)
> +		if (!vdev->dev.parent || !vdev->dev.parent->parent)
>  			goto free_buf;
> -		buf->dev = vq->vdev->dev.parent->parent;
> +		buf->dev = vdev->dev.parent->parent;
>  
>  		/* Increase device refcnt to avoid freeing it */
>  		get_device(buf->dev);
> @@ -838,7 +838,7 @@ static ssize_t port_fops_write(struct file *filp, const
char __user *ubuf,
>  
>  	count = min((size_t)(32 * 1024), count);
>  
> -	buf = alloc_buf(port->out_vq, count, 0);
> +	buf = alloc_buf(port->portdev->vdev, count, 0);
>  	if (!buf)
>  		return -ENOMEM;
>  
> @@ -957,7 +957,7 @@ static ssize_t port_fops_splice_write(struct
pipe_inode_info *pipe,
>  	if (ret < 0)
>  		goto error_out;
>  
> -	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
> +	buf = alloc_buf(port->portdev->vdev, 0, pipe->nrbufs);
>  	if (!buf) {
>  		ret = -ENOMEM;
>  		goto error_out;
> @@ -1374,7 +1374,7 @@ static unsigned int fill_queue(struct virtqueue *vq,
spinlock_t *lock)
>  
>  	nr_added_bufs = 0;
>  	do {
> -		buf = alloc_buf(vq, PAGE_SIZE, 0);
> +		buf = alloc_buf(vq->vdev, PAGE_SIZE, 0);
>  		if (!buf)
>  			break;
>  
> -- 
> MST
<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

On 2018?04?21? 02:18, Michael S. Tsirkin wrote:
> Console driver is out of spec. The spec says:
> 	A driver MUST NOT decrement the available idx on a live
> 	virtqueue (ie. there is no way to ?unexpose? buffers).
> and it does exactly that by trying to detach unused buffers
> without doing a device reset first.
>
> Defer detaching the buffers until device unplug.
>
> Of course this means we might get an interrupt for
> a vq without an attached port now. Handle that by
> discarding the consumed buffer.
>
> Reported-by: Tiwei Bie <tiwei.bie at intel.com>
> Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer
detach")
> CC: stable at kernel.org
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
I wonder whether or not we can have some BUG_ON() in 
virtqueue_detach_unused_buf() to detect such bugs (e.g by checking status?).

Thanks
> ---
>   drivers/char/virtio_console.c | 49
+++++++++++++++++++++----------------------
>   1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 3e56f32..26a66ff 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1402,7 +1402,6 @@ static int add_port(struct ports_device *portdev, u32
id)
>   {
>   	char debugfs_name[16];
>   	struct port *port;
> -	struct port_buffer *buf;
>   	dev_t devt;
>   	unsigned int nr_added_bufs;
>   	int err;
> @@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32
id)
>   	return 0;
>   
>   free_inbufs:
> -	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> -		free_buf(buf, true);
>   free_device:
>   	device_destroy(pdrvdata.class, port->dev->devt);
>   free_cdev:
> @@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref)
>   
>   static void remove_port_data(struct port *port)
>   {
> -	struct port_buffer *buf;
> -
>   	spin_lock_irq(&port->inbuf_lock);
>   	/* Remove unused data this port might have received. */
>   	discard_port_data(port);
>   	spin_unlock_irq(&port->inbuf_lock);
>   
> -	/* Remove buffers we queued up for the Host to send us data in. */
> -	do {
> -		spin_lock_irq(&port->inbuf_lock);
> -		buf = virtqueue_detach_unused_buf(port->in_vq);
> -		spin_unlock_irq(&port->inbuf_lock);
> -		if (buf)
> -			free_buf(buf, true);
> -	} while (buf);
> -
>   	spin_lock_irq(&port->outvq_lock);
>   	reclaim_consumed_buffers(port);
>   	spin_unlock_irq(&port->outvq_lock);
> -
> -	/* Free pending buffers from the out-queue. */
> -	do {
> -		spin_lock_irq(&port->outvq_lock);
> -		buf = virtqueue_detach_unused_buf(port->out_vq);
> -		spin_unlock_irq(&port->outvq_lock);
> -		if (buf)
> -			free_buf(buf, true);
> -	} while (buf);
>   }
>   
>   /*
> @@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct
*work)
>   	spin_unlock(&portdev->c_ivq_lock);
>   }
>   
> +static void flush_bufs(struct virtqueue *vq, bool can_sleep)
> +{
> +	struct port_buffer *buf;
> +	unsigned int len;
> +
> +	while ((buf = virtqueue_get_buf(vq, &len)))
> +		free_buf(buf, can_sleep);
> +}
> +
>   static void out_intr(struct virtqueue *vq)
>   {
>   	struct port *port;
>   
>   	port = find_port_by_vq(vq->vdev->priv, vq);
> -	if (!port)
> +	if (!port) {
> +		flush_bufs(vq, false);
>   		return;
> +	}
>   
>   	wake_up_interruptible(&port->waitqueue);
>   }
> @@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq)
>   	unsigned long flags;
>   
>   	port = find_port_by_vq(vq->vdev->priv, vq);
> -	if (!port)
> +	if (!port) {
> +		flush_bufs(vq, false);
>   		return;
> +	}
>   
>   	spin_lock_irqsave(&port->inbuf_lock, flags);
>   	port->inbuf = get_inbuf(port);
> @@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = {
>   
>   static void remove_vqs(struct ports_device *portdev)
>   {
> +	struct virtqueue *vq;
> +
> +	virtio_device_for_each_vq(portdev->vdev, vq) {
> +		struct port_buffer *buf;
> +
> +		flush_bufs(vq, true);
> +		while ((buf = virtqueue_detach_unused_buf(vq)))
> +			free_buf(buf, true);
> +	}
>   	portdev->vdev->config->del_vqs(portdev->vdev);
>   	kfree(portdev->in_vqs);
>   	kfree(portdev->out_vqs);

On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin
wrote:
> Turns out virtio console tries to take a buffer out of an active vq.
> Works by sheer luck, and is explicitly forbidden by spec.  And while
> going over it I saw that error handling is also broken -
> failure is easy to trigger if I force allocations to fail.
> 
> Lightly tested.
Amit - any feedback before I push these patches?
> Michael S. Tsirkin (6):
>   virtio_console: don't tie bufs to a vq
>   virtio: add ability to iterate over vqs
>   virtio_console: free buffers after reset
>   virtio_console: drop custom control queue cleanup
>   virtio_console: move removal code
>   virtio_console: reset on out of memory
> 
>  drivers/char/virtio_console.c | 155
++++++++++++++++++++----------------------
>  include/linux/virtio.h        |   3 +
>  2 files changed, 75 insertions(+), 83 deletions(-)
> 
> -- 
> MST
>

On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin
wrote:
> On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > Turns out virtio console tries to take a buffer out of an active vq.
> > Works by sheer luck, and is explicitly forbidden by spec.  And while
> > going over it I saw that error handling is also broken -
> > failure is easy to trigger if I force allocations to fail.
> > 
> > Lightly tested.
> 
> Amit - any feedback before I push these patches?
The changes look good spec-wise.

There are a bunch of tests in avocado-vt that test virtio-console
functionality.  Can you give those a try before pushing?


		Amit
-- 
http://amitshah.net/

On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah
wrote:
> (apologies if you received a dup)
> 
> On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
> > > Turns out virtio console tries to take a buffer out of an active
vq.
> > > Works by sheer luck, and is explicitly forbidden by spec.  And
while
> > > going over it I saw that error handling is also broken -
> > > failure is easy to trigger if I force allocations to fail.
> > > 
> > > Lightly tested.
> > 
> > Amit - any feedback before I push these patches?
> 
> The changes look good spec-wise.
> 
> There are a bunch of tests in avocado-vt that test virtio-console
> functionality.  Can you give those a try before pushing?
> 
> 		Amit
I pushed before I did that test, will try to find the time later.  BTW
do you still want to be on the maintainers list?
> -- 
> http://amitshah.net/

On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin
wrote:
> On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > (apologies if you received a dup)
> > 
> > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin
wrote:
> > > > Turns out virtio console tries to take a buffer out of an
active vq.
> > > > Works by sheer luck, and is explicitly forbidden by spec. 
And while
> > > > going over it I saw that error handling is also broken -
> > > > failure is easy to trigger if I force allocations to fail.
> > > > 
> > > > Lightly tested.
> > > 
> > > Amit - any feedback before I push these patches?
> > 
> > The changes look good spec-wise.
> > 
> > There are a bunch of tests in avocado-vt that test virtio-console
> > functionality.  Can you give those a try before pushing?
> > 
> > 		Amit
> 
> I pushed before I did that test, will try to find the time later.  BTW
> do you still want to be on the maintainers list?
Yes, I want to be on the maintainers list; but won't mind
co-maintainers.  The recent changes seem to be spec-related, and I'd
expect you to have a good handle on that anyway.

		Amit
-- 
http://amitshah.net/

On Sun, May 06, 2018 at 08:24:30PM +0200, Amit Shah
wrote:
> On (Thu) 03 May 2018 [22:28:32], Michael S. Tsirkin wrote:
> > On Thu, May 03, 2018 at 05:45:29AM +0200, Amit Shah wrote:
> > > (apologies if you received a dup)
> > > 
> > > On (Tue) 24 Apr 2018 [21:41:29], Michael S. Tsirkin wrote:
> > > > On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin
wrote:
> > > > > Turns out virtio console tries to take a buffer out of
an active vq.
> > > > > Works by sheer luck, and is explicitly forbidden by
spec.  And while
> > > > > going over it I saw that error handling is also broken
-
> > > > > failure is easy to trigger if I force allocations to
fail.
> > > > > 
> > > > > Lightly tested.
> > > > 
> > > > Amit - any feedback before I push these patches?
> > > 
> > > The changes look good spec-wise.
> > > 
> > > There are a bunch of tests in avocado-vt that test virtio-console
> > > functionality.  Can you give those a try before pushing?
> > > 
> > > 		Amit
> > 
> > I pushed before I did that test, will try to find the time later.  BTW
> > do you still want to be on the maintainers list?
> 
> Yes, I want to be on the maintainers list; but won't mind
> co-maintainers.  The recent changes seem to be spec-related, and I'd
> expect you to have a good handle on that anyway.
> 
> 		Amit
If you can do extra testing that would be appreciated though.
> -- 
> http://amitshah.net/