All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] virtio-console: spec compliance fixes
@ 2018-04-20 18:18 Michael S. Tsirkin
  2018-04-20 18:18   ` Michael S. Tsirkin
                   ` (11 more replies)
  0 siblings, 12 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
@ 2018-04-20 18:18   ` Michael S. Tsirkin
  2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
                     ` (10 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 1/6] virtio_console: don't tie bufs to a vq
@ 2018-04-20 18:18   ` Michael S. Tsirkin
  0 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 3/6] virtio_console: free buffers after reset
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
@ 2018-04-20 18:18   ` Michael S. Tsirkin
  2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
                     ` (10 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang, stable

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 2/6] virtio: add ability to iterate over vqs
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
  2018-04-20 18:18   ` Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18   ` Michael S. Tsirkin
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 2/6] virtio: add ability to iterate over vqs
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2018-04-20 18:18   ` Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` [PATCH 4/6] virtio_console: drop custom control queue cleanup Michael S. Tsirkin
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 3/6] virtio_console: free buffers after reset
@ 2018-04-20 18:18   ` Michael S. Tsirkin
  0 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable,
	virtualization, stable

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 4/6] virtio_console: drop custom control queue cleanup
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (4 preceding siblings ...)
  2018-04-20 18:18 ` [PATCH 4/6] virtio_console: drop custom control queue cleanup Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` [PATCH 5/6] virtio_console: move removal code Michael S. Tsirkin
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 4/6] virtio_console: drop custom control queue cleanup
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (3 preceding siblings ...)
  2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` Michael S. Tsirkin
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 5/6] virtio_console: move removal code
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (5 preceding siblings ...)
  2018-04-20 18:18 ` Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` Michael S. Tsirkin
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 5/6] virtio_console: move removal code
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (6 preceding siblings ...)
  2018-04-20 18:18 ` [PATCH 5/6] virtio_console: move removal code Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` [PATCH 6/6] virtio_console: reset on out of memory Michael S. Tsirkin
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 6/6] virtio_console: reset on out of memory
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (7 preceding siblings ...)
  2018-04-20 18:18 ` Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-20 18:18 ` Michael S. Tsirkin
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* [PATCH 6/6] virtio_console: reset on out of memory
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (8 preceding siblings ...)
  2018-04-20 18:18 ` [PATCH 6/6] virtio_console: reset on out of memory Michael S. Tsirkin
@ 2018-04-20 18:18 ` Michael S. Tsirkin
  2018-04-24 18:41 ` [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
  2018-04-24 18:41 ` Michael S. Tsirkin
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-20 18:18   ` Michael S. Tsirkin
  (?)
  (?)
@ 2018-04-21  7:30   ` Greg Kroah-Hartman
  2018-04-24 18:56     ` Michael S. Tsirkin
  2018-04-24 18:56     ` Michael S. Tsirkin
  -1 siblings, 2 replies; 35+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-21  7:30 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, virtualization, stable,
	Tiwei Bie, Jason Wang

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>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-20 18:18   ` Michael S. Tsirkin
  (?)
@ 2018-04-21  7:30   ` Greg Kroah-Hartman
  -1 siblings, 0 replies; 35+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-21  7:30 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, Amit Shah, linux-kernel, stable, virtualization

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>

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 3/6] virtio_console: free buffers after reset
  2018-04-20 18:18   ` Michael S. Tsirkin
  (?)
  (?)
@ 2018-04-24  2:40   ` Jason Wang
  -1 siblings, 0 replies; 35+ messages in thread
From: Jason Wang @ 2018-04-24  2:40 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, stable



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);

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 3/6] virtio_console: free buffers after reset
  2018-04-20 18:18   ` Michael S. Tsirkin
  (?)
@ 2018-04-24  2:40   ` Jason Wang
  -1 siblings, 0 replies; 35+ messages in thread
From: Jason Wang @ 2018-04-24  2:40 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable,
	virtualization, stable



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);

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (10 preceding siblings ...)
  2018-04-24 18:41 ` [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
@ 2018-04-24 18:41 ` Michael S. Tsirkin
  2018-04-25 14:01   ` Amit Shah
                     ` (3 more replies)
  11 siblings, 4 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-24 18:41 UTC (permalink / raw)
  To: linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
                   ` (9 preceding siblings ...)
  2018-04-20 18:18 ` Michael S. Tsirkin
@ 2018-04-24 18:41 ` Michael S. Tsirkin
  2018-04-24 18:41 ` Michael S. Tsirkin
  11 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-24 18:41 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-21  7:30   ` Greg Kroah-Hartman
  2018-04-24 18:56     ` Michael S. Tsirkin
@ 2018-04-24 18:56     ` Michael S. Tsirkin
  2018-04-25  5:50       ` Greg Kroah-Hartman
  2018-04-25  5:50       ` Greg Kroah-Hartman
  1 sibling, 2 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-24 18:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, virtualization, stable,
	Tiwei Bie, Jason Wang

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-21  7:30   ` Greg Kroah-Hartman
@ 2018-04-24 18:56     ` Michael S. Tsirkin
  2018-04-24 18:56     ` Michael S. Tsirkin
  1 sibling, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-24 18:56 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Arnd Bergmann, Amit Shah, linux-kernel, stable, virtualization

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-24 18:56     ` Michael S. Tsirkin
  2018-04-25  5:50       ` Greg Kroah-Hartman
@ 2018-04-25  5:50       ` Greg Kroah-Hartman
  1 sibling, 0 replies; 35+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-25  5:50 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, virtualization, stable,
	Tiwei Bie, Jason Wang

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 1/6] virtio_console: don't tie bufs to a vq
  2018-04-24 18:56     ` Michael S. Tsirkin
@ 2018-04-25  5:50       ` Greg Kroah-Hartman
  2018-04-25  5:50       ` Greg Kroah-Hartman
  1 sibling, 0 replies; 35+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-25  5:50 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, Amit Shah, linux-kernel, stable, virtualization

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-24 18:41 ` Michael S. Tsirkin
@ 2018-04-25 14:01   ` Amit Shah
  2018-05-03  3:34   ` Amit Shah
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 35+ messages in thread
From: Amit Shah @ 2018-04-25 14:01 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Amit Shah, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang



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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-24 18:41 ` Michael S. Tsirkin
  2018-04-25 14:01   ` Amit Shah
  2018-05-03  3:34   ` Amit Shah
@ 2018-05-03  3:34   ` Amit Shah
  2018-05-03  3:45   ` Amit Shah
  3 siblings, 0 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-03  3:34 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
	virtualization, stable, Tiwei Bie, Jason Wang

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-24 18:41 ` Michael S. Tsirkin
  2018-04-25 14:01   ` Amit Shah
@ 2018-05-03  3:34   ` Amit Shah
  2018-05-03  3:34   ` Amit Shah
  2018-05-03  3:45   ` Amit Shah
  3 siblings, 0 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-03  3:34 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, linux-kernel,
	stable, virtualization

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-04-24 18:41 ` Michael S. Tsirkin
                     ` (2 preceding siblings ...)
  2018-05-03  3:34   ` Amit Shah
@ 2018-05-03  3:45   ` Amit Shah
  2018-05-03 19:28     ` Michael S. Tsirkin
  2018-05-03 19:28     ` Michael S. Tsirkin
  3 siblings, 2 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-03  3:45 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
	virtualization, stable, Tiwei Bie, Jason Wang

(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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-03  3:45   ` Amit Shah
@ 2018-05-03 19:28     ` Michael S. Tsirkin
  2018-05-06 17:56       ` Amit Shah
                         ` (2 more replies)
  2018-05-03 19:28     ` Michael S. Tsirkin
  1 sibling, 3 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-05-03 19:28 UTC (permalink / raw)
  To: Amit Shah
  Cc: linux-kernel, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
	virtualization, stable, Tiwei Bie, Jason Wang

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-03  3:45   ` Amit Shah
  2018-05-03 19:28     ` Michael S. Tsirkin
@ 2018-05-03 19:28     ` Michael S. Tsirkin
  1 sibling, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-05-03 19:28 UTC (permalink / raw)
  To: Amit Shah
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, linux-kernel,
	stable, virtualization

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-03 19:28     ` Michael S. Tsirkin
@ 2018-05-06 17:56       ` Amit Shah
  2018-05-06 17:56       ` Amit Shah
  2018-05-06 18:24       ` Amit Shah
  2 siblings, 0 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-06 17:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Amit Shah, linux-kernel, Amit Shah, Arnd Bergmann,
	Greg Kroah-Hartman, virtualization, stable, Tiwei Bie,
	Jason Wang

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-03 19:28     ` Michael S. Tsirkin
  2018-05-06 17:56       ` Amit Shah
@ 2018-05-06 17:56       ` Amit Shah
  2018-05-06 18:24       ` Amit Shah
  2 siblings, 0 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-06 17:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, linux-kernel,
	stable, virtualization, 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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-03 19:28     ` Michael S. Tsirkin
  2018-05-06 17:56       ` Amit Shah
  2018-05-06 17:56       ` Amit Shah
@ 2018-05-06 18:24       ` Amit Shah
  2018-05-06 19:52         ` Michael S. Tsirkin
  2018-05-06 19:52         ` Michael S. Tsirkin
  2 siblings, 2 replies; 35+ messages in thread
From: Amit Shah @ 2018-05-06 18:24 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-06 18:24       ` Amit Shah
@ 2018-05-06 19:52         ` Michael S. Tsirkin
  2018-05-06 19:52         ` Michael S. Tsirkin
  1 sibling, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-05-06 19:52 UTC (permalink / raw)
  To: Amit Shah
  Cc: linux-kernel, Arnd Bergmann, Greg Kroah-Hartman, virtualization,
	stable, Tiwei Bie, Jason Wang

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH 0/6] virtio-console: spec compliance fixes
  2018-05-06 18:24       ` Amit Shah
  2018-05-06 19:52         ` Michael S. Tsirkin
@ 2018-05-06 19:52         ` Michael S. Tsirkin
  1 sibling, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-05-06 19:52 UTC (permalink / raw)
  To: Amit Shah
  Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, stable, virtualization

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/

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH 0/6] virtio-console: spec compliance fixes
@ 2018-04-20 18:18 Michael S. Tsirkin
  0 siblings, 0 replies; 35+ messages in thread
From: Michael S. Tsirkin @ 2018-04-20 18:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman, stable, virtualization

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2018-05-06 19:52 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-20 18:18 [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 1/6] virtio_console: don't tie bufs to a vq Michael S. Tsirkin
2018-04-20 18:18   ` Michael S. Tsirkin
2018-04-21  7:30   ` Greg Kroah-Hartman
2018-04-21  7:30   ` Greg Kroah-Hartman
2018-04-24 18:56     ` Michael S. Tsirkin
2018-04-24 18:56     ` Michael S. Tsirkin
2018-04-25  5:50       ` Greg Kroah-Hartman
2018-04-25  5:50       ` Greg Kroah-Hartman
2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 3/6] virtio_console: free buffers after reset Michael S. Tsirkin
2018-04-20 18:18   ` Michael S. Tsirkin
2018-04-24  2:40   ` Jason Wang
2018-04-24  2:40   ` Jason Wang
2018-04-20 18:18 ` [PATCH 2/6] virtio: add ability to iterate over vqs Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 4/6] virtio_console: drop custom control queue cleanup Michael S. Tsirkin
2018-04-20 18:18 ` Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 5/6] virtio_console: move removal code Michael S. Tsirkin
2018-04-20 18:18 ` Michael S. Tsirkin
2018-04-20 18:18 ` [PATCH 6/6] virtio_console: reset on out of memory Michael S. Tsirkin
2018-04-20 18:18 ` Michael S. Tsirkin
2018-04-24 18:41 ` [PATCH 0/6] virtio-console: spec compliance fixes Michael S. Tsirkin
2018-04-24 18:41 ` Michael S. Tsirkin
2018-04-25 14:01   ` Amit Shah
2018-05-03  3:34   ` Amit Shah
2018-05-03  3:34   ` Amit Shah
2018-05-03  3:45   ` Amit Shah
2018-05-03 19:28     ` Michael S. Tsirkin
2018-05-06 17:56       ` Amit Shah
2018-05-06 17:56       ` Amit Shah
2018-05-06 18:24       ` Amit Shah
2018-05-06 19:52         ` Michael S. Tsirkin
2018-05-06 19:52         ` Michael S. Tsirkin
2018-05-03 19:28     ` Michael S. Tsirkin
  -- strict thread matches above, loose matches on Subject: below --
2018-04-20 18:18 Michael S. Tsirkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.