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(-)
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;
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);MST
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
On 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);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!
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);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
- 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"
- 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
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);
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);@@ -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);- }
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);- }
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);
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).
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);
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 },
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:
On Fri, Apr 20, 2018 at 09:17:59PM +0300, Michael S. Tsirkin wrote:
Turns out virtio console tries to take a buffer out of an active vq. Works by sheer luck, and is explicitly forbidden by spec. And while going over it I saw that error handling is also broken - failure is easy to trigger if I force allocations to fail.
Lightly tested.
Amit - any feedback before I push these patches?
Michael S. Tsirkin (6): virtio_console: don't tie bufs to a vq virtio: add ability to iterate over vqs virtio_console: free buffers after reset virtio_console: drop custom control queue cleanup virtio_console: move removal code virtio_console: reset on out of memory
drivers/char/virtio_console.c | 155 ++++++++++++++++++++---------------------- include/linux/virtio.h | 3 + 2 files changed, 75 insertions(+), 83 deletions(-)
-- MST
On 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
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
(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
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?
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?
AmitI 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
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?
AmitI 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
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?
AmitI 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.
linux-stable-mirror@lists.linaro.org
-
Amit Shah -
Amit Shah -
Greg Kroah-Hartman -
Jason Wang -
Michael S. Tsirkin