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

[Alexandre DERUMIER]

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" <address@hidden>
À: "qemu-devel" <address@hidden>
Cc: "pbonzini" <address@hidden>, "Michael S. Tsirkin" <address@hidden>
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 <address@hidden> 
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 <address@hidden> 
--- 
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