virtio: fail device if set_event_notifier fails

The function virtio_notify_irqfd used to ignore the return code of event_notifier_set. Let's fail the device should this occur.

Signed-off-by Halil Pasic <[email protected]>

---

This patch is most likely flawed because virtio_notify_irqfd
is probably supposed to be thread-safe and neither strerror
nor virtio_error are that. Anyway lets get the discussion started with this.

There was a suggestion by Michael to make event_notifier_set void and assert inside. After some thinking, I settled at the opinion, that neither doing nothing nor crashing the guest is a good idea should this failure happen in production. So I came up with this.

Looking forward to your feedback.

---
 hw/virtio/virtio.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

hw/virtio/virtio.c +7 -1

index 23483c7..e05f3e5 100644
@@ -1555,6 +1555,8 @@ static bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
 void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
 {
     bool should_notify;
+    int rc;
+
     rcu_read_lock();
     should_notify = virtio_should_notify(vdev, vq);
     rcu_read_unlock();

@@ -1581,7 +1583,11 @@ void virtio_notify_irqfd(VirtIODevice *vdev, VirtQueue *vq)
      * to an atomic operation.
      */
     virtio_set_isr(vq->vdev, 0x1);
-    event_notifier_set(&vq->guest_notifier);
+    rc = event_notifier_set(&vq->guest_notifier);
+    if (unlikely(rc)) {
+        virtio_error(vdev, "guest notifier broken for vdev at %p (%s)", vdev,
+                     strerror(-rc));
+    }
 }
 
 static void virtio_irq(VirtQueue *vq)

Quoted 2 lines · On Thu, 2 Mar 2017 19:59:42 +0100 Halil Pasic <[email protected]> wrote:

The function virtio_notify_irqfd used to ignore the return code of event_notifier_set. Let's fail the device should this occur.

I'm wondering if there are reasons for event_notifier_set() to fail beyond "we've hit an internal race and should make an effort to fix that one, or else we have completely messed up in qemu". Marking the device broken tells the guest that there's something wrong with the device, but I think we want qemu bug reports when there's something broken with the irqfd.

Quoted 6 lines · Cornelia Huck

Quoted 2 lines · On Thu, 2 Mar 2017 19:59:42 +0100 Halil Pasic <[email protected]> wrote:

The function virtio_notify_irqfd used to ignore the return code of event_notifier_set. Let's fail the device should this occur.

Quoted 7 lines

I'm wondering if there are reasons for event_notifier_set() to fail beyond "we've hit an internal race and should make an effort to fix that one, or else we have completely messed up in qemu". Marking the device broken tells the guest that there's something wrong with the device, but I think we want qemu bug reports when there's something broken with the irqfd.

That's why the error is logged. I understand virtio_error like something suitable for indicating bugs.

What do you suggest? Forcing a dump? I would rather leave it to the user to figure out how important is the state sitting in the machine and the device, and how much effort does (s)he want to put into recovering from the failure.

Quoted 18 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 2 lines · On Thu, 2 Mar 2017 19:59:42 +0100 Halil Pasic <[email protected]> wrote:

The function virtio_notify_irqfd used to ignore the return code of event_notifier_set. Let's fail the device should this occur.

Quoted 7 lines

I'm wondering if there are reasons for event_notifier_set() to fail beyond "we've hit an internal race and should make an effort to fix that one, or else we have completely messed up in qemu". Marking the device broken tells the guest that there's something wrong with the device, but I think we want qemu bug reports when there's something broken with the irqfd.

That's why the error is logged. I understand virtio_error like something suitable for indicating bugs.

Quoted 4 lines

What do you suggest? Forcing a dump? I would rather leave it to the user to figure out how important is the state sitting in the machine and the device, and how much effort does (s)he want to put into recovering from the failure.

How likely are those logged messages being brought to attention of the admin? Does any management software flag machines with such error messages? (that's more of a general question)

I'd like to have some kind of trigger that rings an alarm bell so that the admin might consider reporting this, but I don't have a good idea on how to do that either...

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 2 lines · On Thu, 2 Mar 2017 19:59:42 +0100 Halil Pasic <[email protected]> wrote:

The function virtio_notify_irqfd used to ignore the return code of event_notifier_set. Let's fail the device should this occur.

Quoted 7 lines

I'm wondering if there are reasons for event_notifier_set() to fail beyond "we've hit an internal race and should make an effort to fix that one, or else we have completely messed up in qemu". Marking the device broken tells the guest that there's something wrong with the device, but I think we want qemu bug reports when there's something broken with the irqfd.

That's why the error is logged. I understand virtio_error like something suitable for indicating bugs.

Quoted 9 lines

Quoted 4 lines

What do you suggest? Forcing a dump? I would rather leave it to the user to figure out how important is the state sitting in the machine and the device, and how much effort does (s)he want to put into recovering from the failure.

How likely are those logged messages being brought to attention of the admin? Does any management software flag machines with such error messages? (that's more of a general question)

I admit, I did not investigate this thoroughly, also because the patch is flawed regarding multi-thread anyway. After a quick investigation it seems the linux guest won't auto-reset the device so the guest should end up with a not working device. I think it's pretty likely that the admin will check the logs if the device was important.

I agree fully that it's a very general question, and I do not feel competent for answering it.

Quoted 4 lines

I'd like to have some kind of trigger that rings an alarm bell so that the admin might consider reporting this, but I don't have a good idea on how to do that either...

There are tools for aggregating and processing logs, and triggering alarm bells too (for example ELK= logstash + Kibana + Elasticsearch). AFAIK logs are the most common way to deal with such stuff. But I'm far form being an expert. Of course logs are only as good as the messages landing in them...

Quoted 7 lines · On Fri, 3 Mar 2017 14:08:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 2 Mar 2017 19:59:42 +0100
Halil Pasic <[email protected]> wrote:

Quoted 5 lines

I admit, I did not investigate this thoroughly, also because the patch is flawed regarding multi-thread anyway. After a quick investigation it seems the linux guest won't auto-reset the device so the guest should end up with a not working device. I think it's pretty likely that the admin will check the logs if the device was important.

Thinking a bit more about this, it seems setting the device broken is not the right solution for exactly that reason. Setting the virtio device broken is a way to signal the guest to 'you did something broken; please reset the device and start anew' (and that's how current callers use it). In our case, this is not the guest's fault.

Maybe go back to the assert 'solution'? But I'm not sure that's enough if production builds disable asserts...

Quoted 10 lines · Cornelia Huck

Quoted 7 lines · On Fri, 3 Mar 2017 14:08:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 2 Mar 2017 19:59:42 +0100
Halil Pasic <[email protected]> wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I admit, I did not investigate this thoroughly, also because the patch is flawed regarding multi-thread anyway. After a quick investigation it seems the linux guest won't auto-reset the device so the guest should end up with a not working device. I think it's pretty likely that the admin will check the logs if the device was important.

Thinking a bit more about this, it seems setting the device broken is not the right solution for exactly that reason. Setting the virtio device broken is a way to signal the guest to 'you did something broken; please reset the device and start anew' (and that's how current callers use it). In our case, this is not the guest's fault.

Do we have something to just say stuff broken without blaming the guest? And device reset might not be that stupid at all in the given situation, if we want to save what can be saved from the perspective of the guest. (After reset stuff should work again until we hit the race again -- and since turning ioeventfd on/off should not happen that often during normal operation it could help limit damage suffered -- e.g. controlled shutdown).

Quoted 5 lines

Maybe go back to the assert 'solution'? But I'm not sure that's enough if production builds disable asserts...

I will wait a bit, maybe other virtio folks are going to have an opinion too.

My concern about the assert solution is that for production it is either too rigorous (kill off, hopefully with a dump) or not enough (as you have mentioned, if NDEBUG assert does nothing).

I think there are setups where a loss of device does not have to be fatal, and I would not like to be the one who makes it fatal (for the guest).

Quoted 24 lines · On Mon, 6 Mar 2017 16:21:13 +0100 Halil Pasic <[email protected]> wrote:

Quoted 10 lines · Cornelia Huck

Quoted 7 lines · On Fri, 3 Mar 2017 14:08:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 2 Mar 2017 19:59:42 +0100
Halil Pasic <[email protected]> wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I admit, I did not investigate this thoroughly, also because the patch is flawed regarding multi-thread anyway. After a quick investigation it seems the linux guest won't auto-reset the device so the guest should end up with a not working device. I think it's pretty likely that the admin will check the logs if the device was important.

Thinking a bit more about this, it seems setting the device broken is not the right solution for exactly that reason. Setting the virtio device broken is a way to signal the guest to 'you did something broken; please reset the device and start anew' (and that's how current callers use it). In our case, this is not the guest's fault.

Quoted 6 lines

Do we have something to just say stuff broken without blaming the guest? And device reset might not be that stupid at all in the given situation, if we want to save what can be saved from the perspective of the guest. (After reset stuff should work again until we hit the race again -- and since turning ioeventfd on/off should not happen that often during normal operation it could help limit damage suffered -- e.g. controlled shutdown).

Checking again, the spec says

DEVICE_NEEDS_RESET (64) Indicates that the device has experienced an error from which it can’t recover.

Nothing about 'guest error'.

The only problem is that legacy devices don't have that state, which means they'll have a broken device through no fault of their own.

Quoted 9 lines

Quoted 5 lines

Maybe go back to the assert 'solution'? But I'm not sure that's enough if production builds disable asserts...

I will wait a bit, maybe other virtio folks are going to have an opinion too.

Quoted 8 lines

My concern about the assert solution is that for production it is either too rigorous (kill off, hopefully with a dump) or not enough (as you have mentioned, if NDEBUG assert does nothing).

I think there are setups where a loss of device does not have to be fatal, and I would not like to be the one who makes it fatal (for the guest).

Basically, it's a host bug (and not a bug specific to a certain device). Moving the device which was impacted to a broken state may be a useful mitigation.

But yes, let's hear some other opinions.

Quoted 62 lines · Cornelia Huck

Quoted 24 lines · On Mon, 6 Mar 2017 16:21:13 +0100 Halil Pasic <[email protected]> wrote:

Quoted 10 lines · Cornelia Huck

Quoted 7 lines · On Fri, 3 Mar 2017 14:08:37 +0100 Halil Pasic <[email protected]> wrote:

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Fri, 3 Mar 2017 13:43:32 +0100 Halil Pasic <[email protected]> wrote:

Quoted 2 lines · Cornelia Huck

On Thu, 2 Mar 2017 19:59:42 +0100
Halil Pasic <[email protected]> wrote:

[...]

Quoted 11 lines

Quoted 5 lines

I admit, I did not investigate this thoroughly, also because the patch is flawed regarding multi-thread anyway. After a quick investigation it seems the linux guest won't auto-reset the device so the guest should end up with a not working device. I think it's pretty likely that the admin will check the logs if the device was important.

Thinking a bit more about this, it seems setting the device broken is not the right solution for exactly that reason. Setting the virtio device broken is a way to signal the guest to 'you did something broken; please reset the device and start anew' (and that's how current callers use it). In our case, this is not the guest's fault.

Quoted 6 lines

Do we have something to just say stuff broken without blaming the guest? And device reset might not be that stupid at all in the given situation, if we want to save what can be saved from the perspective of the guest. (After reset stuff should work again until we hit the race again -- and since turning ioeventfd on/off should not happen that often during normal operation it could help limit damage suffered -- e.g. controlled shutdown).

Checking again, the spec says

DEVICE_NEEDS_RESET (64) Indicates that the device has experienced an error from which it can’t recover.

Nothing about 'guest error'.

The only problem is that legacy devices don't have that state, which means they'll have a broken device through no fault of their own.

Quoted 17 lines

Quoted 5 lines

Maybe go back to the assert 'solution'? But I'm not sure that's enough if production builds disable asserts...

I will wait a bit, maybe other virtio folks are going to have an opinion too.

My concern about the assert solution is that for production it is either too rigorous (kill off, hopefully with a dump) or not enough (as you have mentioned, if NDEBUG assert does nothing).

I think there are setups where a loss of device does not have to be fatal, and I would not like to be the one who makes it fatal (for the guest).

Quoted 5 lines

Basically, it's a host bug (and not a bug specific to a certain device). Moving the device which was impacted to a broken state may be a useful mitigation.

But yes, let's hear some other opinions.

We don't support NDEBUG really so I think an assert is fine for now.

Handling unexpected errors more gracefully is laudable but I think we want a more systematic approach than just open-coding it in this specific place.