Message ID | 20170920060933.11593-1-w.bumiller@proxmox.com |
---|---|
State | New |
Headers | show |
Series | virtio: fix descriptor counting in virtqueue_pop | expand |
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;
}
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 > >
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 > > > >
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; }
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(-)