virtio: fix descriptor counting in virtqueue_pop

[Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop

Posted by Wolfgang Bumiller 6 years, 7 months ago

While changing the s/g list allocation, commit 3b3b0628
also changed the descriptor counting to count iovec entries
as split by cpu_physical_memory_map(). Previously only the
actual descriptor entries were counted and the split into
the iovec happened afterwards in virtqueue_map().
Count the entries again instead to avoid erroneous
"Looped descriptor" errors.

Reported-by: Hans Middelhoek <h.middelhoek@ospito.nl>
Link: https://forum.proxmox.com/threads/vm-crash-with-memory-hotplug.35904/
Fixes: 3b3b0628217e ("virtio: slim down allocation of VirtQueueElements")
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
 hw/virtio/virtio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 890b4d7eb7..33bb770177 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -834,7 +834,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     int64_t len;
     VirtIODevice *vdev = vq->vdev;
     VirtQueueElement *elem = NULL;
-    unsigned out_num, in_num;
+    unsigned out_num, in_num, elem_entries;
     hwaddr addr[VIRTQUEUE_MAX_SIZE];
     struct iovec iov[VIRTQUEUE_MAX_SIZE];
     VRingDesc desc;
@@ -852,7 +852,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     smp_rmb();
 
     /* When we start there are none of either input nor output. */
-    out_num = in_num = 0;
+    out_num = in_num = elem_entries = 0;
 
     max = vq->vring.num;
 
@@ -922,7 +922,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
         }
 
         /* If we've got too many, that implies a descriptor loop. */
-        if ((in_num + out_num) > max) {
+        if (++elem_entries > max) {
             virtio_error(vdev, "Looped descriptor");
             goto err_undo_map;
         }
-- 
2.11.0

Re: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop

Posted by Alexandre DERUMIER 6 years, 6 months ago

Hi,

has somebody reviewed this patch ?

I'm also able de reproduce the vm crash like the proxmox user.
This patch is fixing it for me too.

Regards,

Alexandre


----- Mail original -----
De: "Wolfgang Bumiller" <w.bumiller@proxmox.com>
À: "qemu-devel" <qemu-devel@nongnu.org>
Cc: "pbonzini" <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
Envoyé: Mercredi 20 Septembre 2017 08:09:33
Objet: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop

While changing the s/g list allocation, commit 3b3b0628 
also changed the descriptor counting to count iovec entries 
as split by cpu_physical_memory_map(). Previously only the 
actual descriptor entries were counted and the split into 
the iovec happened afterwards in virtqueue_map(). 
Count the entries again instead to avoid erroneous 
"Looped descriptor" errors. 

Reported-by: Hans Middelhoek <h.middelhoek@ospito.nl> 
Link: https://forum.proxmox.com/threads/vm-crash-with-memory-hotplug.35904/ 
Fixes: 3b3b0628217e ("virtio: slim down allocation of VirtQueueElements") 
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> 
--- 
hw/virtio/virtio.c | 6 +++--- 
1 file changed, 3 insertions(+), 3 deletions(-) 

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c 
index 890b4d7eb7..33bb770177 100644 
--- a/hw/virtio/virtio.c 
+++ b/hw/virtio/virtio.c 
@@ -834,7 +834,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
int64_t len; 
VirtIODevice *vdev = vq->vdev; 
VirtQueueElement *elem = NULL; 
- unsigned out_num, in_num; 
+ unsigned out_num, in_num, elem_entries; 
hwaddr addr[VIRTQUEUE_MAX_SIZE]; 
struct iovec iov[VIRTQUEUE_MAX_SIZE]; 
VRingDesc desc; 
@@ -852,7 +852,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
smp_rmb(); 

/* When we start there are none of either input nor output. */ 
- out_num = in_num = 0; 
+ out_num = in_num = elem_entries = 0; 

max = vq->vring.num; 

@@ -922,7 +922,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
} 

/* If we've got too many, that implies a descriptor loop. */ 
- if ((in_num + out_num) > max) { 
+ if (++elem_entries > max) { 
virtio_error(vdev, "Looped descriptor"); 
goto err_undo_map; 
} 
-- 
2.11.0

Re: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop

Posted by Stefan Hajnoczi 6 years, 5 months ago

On Thu, Oct 05, 2017 at 08:03:35PM +0200, Alexandre DERUMIER wrote:
> Hi,
> 
> has somebody reviewed this patch ?
> 
> I'm also able de reproduce the vm crash like the proxmox user.
> This patch is fixing it for me too.

This patch should go through Michael Tsirkin's tree.  I have pinged him
separately in case this email thread got buried in his inbox.

Stefan

> 
> Regards,
> 
> Alexandre
> 
> 
> ----- Mail original -----
> De: "Wolfgang Bumiller" <w.bumiller@proxmox.com>
> À: "qemu-devel" <qemu-devel@nongnu.org>
> Cc: "pbonzini" <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
> Envoyé: Mercredi 20 Septembre 2017 08:09:33
> Objet: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop
> 
> While changing the s/g list allocation, commit 3b3b0628 
> also changed the descriptor counting to count iovec entries 
> as split by cpu_physical_memory_map(). Previously only the 
> actual descriptor entries were counted and the split into 
> the iovec happened afterwards in virtqueue_map(). 
> Count the entries again instead to avoid erroneous 
> "Looped descriptor" errors. 
> 
> Reported-by: Hans Middelhoek <h.middelhoek@ospito.nl> 
> Link: https://forum.proxmox.com/threads/vm-crash-with-memory-hotplug.35904/ 
> Fixes: 3b3b0628217e ("virtio: slim down allocation of VirtQueueElements") 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> 
> --- 
> hw/virtio/virtio.c | 6 +++--- 
> 1 file changed, 3 insertions(+), 3 deletions(-) 
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c 
> index 890b4d7eb7..33bb770177 100644 
> --- a/hw/virtio/virtio.c 
> +++ b/hw/virtio/virtio.c 
> @@ -834,7 +834,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> int64_t len; 
> VirtIODevice *vdev = vq->vdev; 
> VirtQueueElement *elem = NULL; 
> - unsigned out_num, in_num; 
> + unsigned out_num, in_num, elem_entries; 
> hwaddr addr[VIRTQUEUE_MAX_SIZE]; 
> struct iovec iov[VIRTQUEUE_MAX_SIZE]; 
> VRingDesc desc; 
> @@ -852,7 +852,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> smp_rmb(); 
> 
> /* When we start there are none of either input nor output. */ 
> - out_num = in_num = 0; 
> + out_num = in_num = elem_entries = 0; 
> 
> max = vq->vring.num; 
> 
> @@ -922,7 +922,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> } 
> 
> /* If we've got too many, that implies a descriptor loop. */ 
> - if ((in_num + out_num) > max) { 
> + if (++elem_entries > max) { 
> virtio_error(vdev, "Looped descriptor"); 
> goto err_undo_map; 
> } 
> -- 
> 2.11.0 
> 
>

Re: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop

Posted by Michael S. Tsirkin 6 years, 5 months ago

On Fri, Nov 10, 2017 at 03:41:37PM +0000, Stefan Hajnoczi wrote:
> On Thu, Oct 05, 2017 at 08:03:35PM +0200, Alexandre DERUMIER wrote:
> > Hi,
> > 
> > has somebody reviewed this patch ?
> > 
> > I'm also able de reproduce the vm crash like the proxmox user.
> > This patch is fixing it for me too.
> 
> This patch should go through Michael Tsirkin's tree.  I have pinged him
> separately in case this email thread got buried in his inbox.
> 
> Stefan

Isn't this upstream?
I see it as commit 37ef70be6af7e9f2a6f852c68f74bd98dac2664b there.


> > 
> > Regards,
> > 
> > Alexandre
> > 
> > 
> > ----- Mail original -----
> > De: "Wolfgang Bumiller" <w.bumiller@proxmox.com>
> > À: "qemu-devel" <qemu-devel@nongnu.org>
> > Cc: "pbonzini" <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
> > Envoyé: Mercredi 20 Septembre 2017 08:09:33
> > Objet: [Qemu-devel] [PATCH] virtio: fix descriptor counting in virtqueue_pop
> > 
> > While changing the s/g list allocation, commit 3b3b0628 
> > also changed the descriptor counting to count iovec entries 
> > as split by cpu_physical_memory_map(). Previously only the 
> > actual descriptor entries were counted and the split into 
> > the iovec happened afterwards in virtqueue_map(). 
> > Count the entries again instead to avoid erroneous 
> > "Looped descriptor" errors. 
> > 
> > Reported-by: Hans Middelhoek <h.middelhoek@ospito.nl> 
> > Link: https://forum.proxmox.com/threads/vm-crash-with-memory-hotplug.35904/ 
> > Fixes: 3b3b0628217e ("virtio: slim down allocation of VirtQueueElements") 
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> 
> > --- 
> > hw/virtio/virtio.c | 6 +++--- 
> > 1 file changed, 3 insertions(+), 3 deletions(-) 
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c 
> > index 890b4d7eb7..33bb770177 100644 
> > --- a/hw/virtio/virtio.c 
> > +++ b/hw/virtio/virtio.c 
> > @@ -834,7 +834,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> > int64_t len; 
> > VirtIODevice *vdev = vq->vdev; 
> > VirtQueueElement *elem = NULL; 
> > - unsigned out_num, in_num; 
> > + unsigned out_num, in_num, elem_entries; 
> > hwaddr addr[VIRTQUEUE_MAX_SIZE]; 
> > struct iovec iov[VIRTQUEUE_MAX_SIZE]; 
> > VRingDesc desc; 
> > @@ -852,7 +852,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> > smp_rmb(); 
> > 
> > /* When we start there are none of either input nor output. */ 
> > - out_num = in_num = 0; 
> > + out_num = in_num = elem_entries = 0; 
> > 
> > max = vq->vring.num; 
> > 
> > @@ -922,7 +922,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz) 
> > } 
> > 
> > /* If we've got too many, that implies a descriptor loop. */ 
> > - if ((in_num + out_num) > max) { 
> > + if (++elem_entries > max) { 
> > virtio_error(vdev, "Looped descriptor"); 
> > goto err_undo_map; 
> > } 
> > -- 
> > 2.11.0 
> > 
> >