[PATCH 0/6] virtio-console: spec compliance fixes

[PATCH 0/6] virtio-console: spec compliance fixes

[Michael S. Tsirkin]

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

[PATCH 1/6] virtio_console: don't tie bufs to a vq

[Michael S. Tsirkin]

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@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

Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq

[Greg Kroah-Hartman]

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@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>

Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq

[Michael S. Tsirkin]

On Sat, Apr 21, 2018 at 09:30:05AM +0200, Greg Kroah-Hartman wrote:
> 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@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>


Thanks!
I have some questions about this one:

Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
Cc: <stable@vger.kernel.org> # 3.3.x
Signed-off-by: Ingo Molnar <mingo@elte.hu>

1. what does the kernel version mean? can I omit it?
2. so when I rebase to add the tag, this changes commit IDs for
   following tags in the same tree, breaking their tags
   in the process. Pretty annoying. Any idea how to do it better?

Thanks!

-- 
MST

Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq

[Greg Kroah-Hartman]

On Tue, Apr 24, 2018 at 09:56:33PM +0300, Michael S. Tsirkin wrote:
> On Sat, Apr 21, 2018 at 09:30:05AM +0200, Greg Kroah-Hartman wrote:
> > 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@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>
> 
> 
> Thanks!
> I have some questions about this one:
> 
> Cc: <stable@vger.kernel.org> # 3.3.x: a1f84a3: sched: Check for idle
> Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
> Cc: <stable@vger.kernel.org> # 3.3.x: fd21073: sched: Fix affinity logic
> Cc: <stable@vger.kernel.org> # 3.3.x
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 1. what does the kernel version mean? can I omit it?

Did you read the document?, it explains that the version can be used to
say "this kernel version and newer"

> 2. so when I rebase to add the tag, this changes commit IDs for
>    following tags in the same tree, breaking their tags
>    in the process. Pretty annoying. Any idea how to do it better?

You only put tags there if you want me to pick up pre-requisite patches
that are already in Linus's tree.  If you have a patch series that all
needs to go into stable, just add the "cc: stable@" to the tags on all
of them and I'll pick them up in the correct order then.

hope this helps,

greg k-h

[PATCH 3/6] virtio_console: free buffers after reset

[Michael S. Tsirkin]

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@intel.com>
Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
CC: stable@kernel.org
Signed-off-by: Michael S. Tsirkin <mst@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

Re: [PATCH 3/6] virtio_console: free buffers after reset

[Jason Wang]

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@intel.com>
> Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach")
> CC: stable@kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@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);

[PATCH 2/6] virtio: add ability to iterate over vqs

[Michael S. Tsirkin]

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@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

[PATCH 4/6] virtio_console: drop custom control queue cleanup

[Michael S. Tsirkin]

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

Signed-off-by: Michael S. Tsirkin <mst@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

[PATCH 5/6] virtio_console: move removal code

[Michael S. Tsirkin]

Will make it reusable for error handling.

Signed-off-by: Michael S. Tsirkin <mst@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

[PATCH 6/6] virtio_console: reset on out of memory

[Michael S. Tsirkin]

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@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@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

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Michael S. Tsirkin]

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
> 

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

On 24 April 2018 11:41:29 AM GMT-07:00, "Michael S. Tsirkin" <mst@redhat.com> 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?

I'm traveling this week, will be able to take a look early next week.

                Amit

-- 
http://amitshah.net

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

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/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

(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
-- 
http://amitshah.net/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Michael S. Tsirkin]

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/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

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/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Amit Shah]

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/

Re: [PATCH 0/6] virtio-console: spec compliance fixes

[Michael S. Tsirkin]

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/