[Qemu-devel] [PATCH] dataplane: avoid reentrancy during virtio_blk_data_plane_stop()

[Qemu-devel] [PATCH] dataplane: avoid reentrancy during virtio_blk_data_plane_stop()

[Stefan Hajnoczi]

When dataplane is stopping, the s->vdev->binding->set_host_notifier(...,
false) call can invoke the virtqueue handler if an ioeventfd
notification is pending.  This causes hw/virtio-blk.c to invoke
virtio_blk_data_plane_start() before virtio_blk_data_plane_stop()
returns!

The result is that we try to restart dataplane while trying to stop it
and the following assertion is raised:

  msix_set_mask_notifier: Assertion `!dev->msix_mask_notifier' failed.

Although the code was intended to prevent this scenario, the s->started
boolean isn't enough.  Add s->stopping so that we can postpone clearing
s->started until we've completely stopped dataplane.

This way, virtqueue handler calls during virtio_blk_data_plane_stop()
are ignored.  When dataplane is legitimately started again later we
already self-kick ourselves to resume processing.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/dataplane/virtio-blk.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/dataplane/virtio-blk.c b/hw/dataplane/virtio-blk.c
index 4b26faa..3f2da22 100644
--- a/hw/dataplane/virtio-blk.c
+++ b/hw/dataplane/virtio-blk.c
@@ -40,6 +40,7 @@ typedef struct {
 
 struct VirtIOBlockDataPlane {
     bool started;
+    bool stopping;
     QEMUBH *start_bh;
     QemuThread thread;
 
@@ -357,7 +358,7 @@ static void *data_plane_thread(void *opaque)
 
     do {
         event_poll(&s->event_poll);
-    } while (s->started || s->num_reqs > 0);
+    } while (!s->stopping || s->num_reqs > 0);
     return NULL;
 }
 
@@ -486,10 +487,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
 void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 {
-    if (!s->started) {
+    if (!s->started || s->stopping) {
         return;
     }
-    s->started = false;
+    s->stopping = true;
     trace_virtio_blk_data_plane_stop(s);
 
     /* Stop thread or cancel pending thread creation BH */
@@ -511,4 +512,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     s->vdev->binding->set_guest_notifiers(s->vdev->binding_opaque, 1, false);
 
     vring_teardown(&s->vring);
+    s->started = false;
+    s->stopping = false;
 }
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH] dataplane: avoid reentrancy during virtio_blk_data_plane_stop()

[Stefan Hajnoczi]

On Tue, Jan 15, 2013 at 05:19:38PM +0100, Stefan Hajnoczi wrote:
> When dataplane is stopping, the s->vdev->binding->set_host_notifier(...,
> false) call can invoke the virtqueue handler if an ioeventfd
> notification is pending.  This causes hw/virtio-blk.c to invoke
> virtio_blk_data_plane_start() before virtio_blk_data_plane_stop()
> returns!
> 
> The result is that we try to restart dataplane while trying to stop it
> and the following assertion is raised:
> 
>   msix_set_mask_notifier: Assertion `!dev->msix_mask_notifier' failed.
> 
> Although the code was intended to prevent this scenario, the s->started
> boolean isn't enough.  Add s->stopping so that we can postpone clearing
> s->started until we've completely stopped dataplane.
> 
> This way, virtqueue handler calls during virtio_blk_data_plane_stop()
> are ignored.  When dataplane is legitimately started again later we
> already self-kick ourselves to resume processing.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/dataplane/virtio-blk.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan