[PATCH] qemu-kvm: fix crash on reboot with vhost-net

[PATCH] qemu-kvm: fix crash on reboot with vhost-net

[Michael S. Tsirkin]

When vhost-net is disabled on reboot, we set msix mask notifier
to NULL to disable further mask/unmask notifications.
Code currently tries to pass this NULL to notifier,
leading to a crash.  The right thing to do is:
- if vector is masked, we don't need to notify backend,
  just disable future notifications
- if vector is unmasked, invoke callback to unassign backend,
  then disable future notifications

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Note: this patch is for qemu-kvm, the code in question
is not in qemu.git.

 hw/msix.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/msix.c b/hw/msix.c
index 3ec8805..43361b5 100644
--- a/hw/msix.c
+++ b/hw/msix.c
@@ -613,9 +613,18 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)
     if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
         return 0;
 
-    if (dev->msix_mask_notifier)
-        r = dev->msix_mask_notifier(dev, vector, opaque,
-                                    msix_is_masked(dev, vector));
+    if (dev->msix_mask_notifier && !msix_is_masked(dev, vector)) {
+        /* Switching notifiers while vector is unmasked:
+         * mask the old one, unmask the new one. */
+        if (dev->msix_mask_notifier_opaque[vector]) {
+            r = dev->msix_mask_notifier(dev, vector,
+                                        dev->msix_mask_notifier_opaque[vector],
+                                        1);
+        }
+        if (r >= 0 && opaque) {
+            r = dev->msix_mask_notifier(dev, vector, opaque, 0);
+        }
+    }
     if (r >= 0)
         dev->msix_mask_notifier_opaque[vector] = opaque;
     return r;
-- 
1.7.1.rc1.22.g3163

Re: [PATCH] qemu-kvm: fix crash on reboot with vhost-net

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> When vhost-net is disabled on reboot, we set msix mask notifier
> to NULL to disable further mask/unmask notifications.
> Code currently tries to pass this NULL to notifier,
> leading to a crash.  The right thing to do is:
> - if vector is masked, we don't need to notify backend,
>   just disable future notifications
> - if vector is unmasked, invoke callback to unassign backend,
>   then disable future notifications
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I don't fully understand this.

> diff --git a/hw/msix.c b/hw/msix.c
> index 3ec8805..43361b5 100644
> --- a/hw/msix.c
> +++ b/hw/msix.c
> @@ -613,9 +613,18 @@ int msix_set_mask_notifier(PCIDevice *dev, unsigned vector, void *opaque)

we have a msix_set_mask_notifier() function that, low and behold, we
also use it to unmask the notifier, just passing a NULL opaque.

I can live with this (althought I would preffer two functions)

>      if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector])
>          return 0;
>  
> -    if (dev->msix_mask_notifier)
> -        r = dev->msix_mask_notifier(dev, vector, opaque,
> -                                    msix_is_masked(dev, vector));
> +    if (dev->msix_mask_notifier && !msix_is_masked(dev, vector)) {

Now things got interesting.  We check if device has a mask notifier, and
if that vector is not masked (notice the _not_ part)

> +        /* Switching notifiers while vector is unmasked:
> +         * mask the old one, unmask the new one. */
> +        if (dev->msix_mask_notifier_opaque[vector]) {

ha ha ha, but it has a mask!

> +            r = dev->msix_mask_notifier(dev, vector,
> +                                        dev->msix_mask_notifier_opaque[vector],
> +                                        1);

No problem, we _mask_ it.

(from pci.h)
typedef int (*msix_mask_notifier_func)(PCIDevice *, unsigned vector,
				       void *opaque, int masked);

(notice that we called masked = 1)

There is only one user of msix_mask_notifier(), we look at it.

it is virtio_pci_mask_notifier()

    int r = kvm_set_irqfd(dev->msix_irq_entries[vector].gsi,
                          event_notifier_get_fd(notifier),
                          !masked);

Notice the interesting !masked part, kvm_set_irqfd() and
msix_mask_notifier() use inverse logic.

Go along:

    if (masked) {
        qemu_set_fd_handler(event_notifier_get_fd(notifier),
                            virtio_pci_guest_notifier_read, NULL, vq);
    } else {
        qemu_set_fd_handler(event_notifier_get_fd(notifier),
                            NULL, NULL, NULL);
    }

masked = 1, so we are assigning that event notifier with this vq
(vq is the opaque that we just passed, that is our
dev->msix_mask_notifier_opaque[vector], that is the one that we want to
get rid of.  Assigning it to one fd handler looks suspcious at least.
it seems that what we really want is the other branch of the if
(i.e. just disable that fd handler).

> +        }
> +        if (r >= 0 && opaque) {
> +            r = dev->msix_mask_notifier(dev, vector, opaque, 0);
> +        }

now, if we are _not_ unmasking the notifier, we just enabled it again.
humm, actually, we unmasked it again.  But remember the inverse logic
all around, here mask == 0, i.e. we are disabling the handler.  at this
point I would have though that we wanted to "enable" it.

> +    }
>      if (r >= 0)
>          dev->msix_mask_notifier_opaque[vector] = opaque;
>      return r;

Here, we assign the notifier opaque only if there hasn't been an error.
I am not fully sure that this is fully correct, because if 1st call to
msix_mask_notifier() success and second fails, I gess that a NULL value
is better than the old value.

But all of this is supposing that I haven't lost track at this point of
what is masked/unmasked :(

I think that at least the if(masked) branches in
virtio_pci_mask_notifier() needs to be changed.

Later, Juan.