[Qemu-devel] [PATCH 1/1] virtio: fix ioeventfd assignment race

[Cornelia Huck]

The ->set_host_notifier() callback is invoked whenever we want to
switch from or to the generic ioeventfd handler. Currently, all
transports deregister the ioeventfd backing and then re-register
it. This opens a race window where we are without ioeventfd
backing for a time period: In the virtio-blk dataplane case, we
observed notifications coming in from both the vcpu thread and
the iothread.

Let's change pci, mmio and ccw to keep the ioeventfd during
->set_host_notifier() and only switch the ioeventfd handler.

Signed-off-by: Cornelia Huck <address@hidden>
---
 hw/s390x/virtio-ccw.c          | 22 +++++++++++++++++-----
 hw/virtio/virtio-mmio.c        | 27 +++++++++++++++++----------
 hw/virtio/virtio-pci.c         | 28 ++++++++++++++++++----------
 include/hw/virtio/virtio-bus.h |  4 ++++
 4 files changed, 56 insertions(+), 25 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index cb887ba..7b1088e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1196,14 +1196,26 @@ static bool 
virtio_ccw_query_guest_notifiers(DeviceState *d)
 static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign)
 {
     VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+    VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
+    VirtQueue *vq = virtio_get_queue(vdev, n);
 
-    /* Stop using the generic ioeventfd, we are doing eventfd handling
-     * ourselves below */
-    dev->ioeventfd_disabled = assign;
     if (assign) {
-        virtio_ccw_stop_ioeventfd(dev);
+        /* Stop using the generic ioeventfd, we are doing eventfd handling
+         * ourselves below */
+        dev->ioeventfd_disabled = true;
+    };
+    /*
+     * Just switch the handler, don't deassign the ioeventfd.
+     * Otherwise, there's a window where we don't have an
+     * ioeventfd and we may end up with a notification where
+     * we don't expect one.
+     */
+    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    if (!assign) {
+        /* Use generic ioeventfd handler again. */
+        dev->ioeventfd_disabled = false;
     }
-    return virtio_ccw_set_guest2host_notifier(dev, n, assign, false);
+    return 0;
 }
 
 static int virtio_ccw_get_mappings(VirtioCcwDevice *dev)
diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index d4cd91f..aafebdf 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -502,19 +502,26 @@ static int virtio_mmio_set_host_notifier(DeviceState 
*opaque, int n,
                                          bool assign)
 {
     VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+    VirtQueue *vq = virtio_get_queue(vdev, n);
 
-    /* Stop using ioeventfd for virtqueue kick if the device starts using host
-     * notifiers.  This makes it easy to avoid stepping on each others' toes.
-     */
-    proxy->ioeventfd_disabled = assign;
     if (assign) {
-        virtio_mmio_stop_ioeventfd(proxy);
+        /* Stop using the generic ioeventfd, we are doing eventfd handling
+         * ourselves below */
+        proxy->ioeventfd_disabled = true;
+    };
+    /*
+     * Just switch the handler, don't deassign the ioeventfd.
+     * Otherwise, there's a window where we don't have an
+     * ioeventfd and we may end up with a notification where
+     * we don't expect one.
+     */
+    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    if (!assign) {
+        /* Use generic ioeventfd handler again. */
+        proxy->ioeventfd_disabled = false;
     }
-    /* We don't need to start here: it's not needed because backend
-     * currently only stops on status change away from ok,
-     * reset, vmstop and such. If we do add code to start here,
-     * need to check vmstate, device state etc. */
-    return virtio_mmio_set_host_notifier_internal(proxy, n, assign, false);
+    return 0;
 }
 
 /* virtio-mmio device */
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 0dadb66..a91c1e8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1115,18 +1115,26 @@ static int virtio_pci_set_host_notifier(DeviceState *d, 
int n, bool assign)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
 
-    /* Stop using ioeventfd for virtqueue kick if the device starts using host
-     * notifiers.  This makes it easy to avoid stepping on each others' toes.
-     */
-    proxy->ioeventfd_disabled = assign;
+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
+    VirtQueue *vq = virtio_get_queue(vdev, n);
+
     if (assign) {
-        virtio_pci_stop_ioeventfd(proxy);
+        /* Stop using the generic ioeventfd, we are doing eventfd handling
+         * ourselves below */
+        proxy->ioeventfd_disabled = true;
+    };
+    /*
+     * Just switch the handler, don't deassign the ioeventfd.
+     * Otherwise, there's a window where we don't have an
+     * ioeventfd and we may end up with a notification where
+     * we don't expect one.
+     */
+    virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign);
+    if (!assign) {
+        /* Use generic ioeventfd handler again. */
+        proxy->ioeventfd_disabled = false;
     }
-    /* We don't need to start here: it's not needed because backend
-     * currently only stops on status change away from ok,
-     * reset, vmstop and such. If we do add code to start here,
-     * need to check vmstate, device state etc. */
-    return virtio_pci_set_host_notifier_internal(proxy, n, assign, false);
+    return 0;
 }
 
 static void virtio_pci_vmstate_change(DeviceState *d, bool running)
diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
index 3f2c136..98c660a 100644
--- a/include/hw/virtio/virtio-bus.h
+++ b/include/hw/virtio/virtio-bus.h
@@ -52,6 +52,10 @@ typedef struct VirtioBusClass {
     bool (*has_extra_state)(DeviceState *d);
     bool (*query_guest_notifiers)(DeviceState *d);
     int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
+    /*
+     * Switch from/to the generic ioeventfd handler.
+     * assigned==false means 'use generic ioeventfd handler'.
+     */
     int (*set_host_notifier)(DeviceState *d, int n, bool assigned);
     void (*vmstate_change)(DeviceState *d, bool running);
     /*
-- 
2.8.0