Re: [Qemu-devel] [PATCH] vhost: fix crash on virtio_error while device stop

[Michael S. Tsirkin]

On Fri, Dec 08, 2017 at 05:54:18PM +0300, Ilya Maximets wrote:
> On 07.12.2017 20:27, Michael S. Tsirkin wrote:
> > On Thu, Dec 07, 2017 at 09:39:36AM +0300, Ilya Maximets wrote:
> >> On 06.12.2017 19:45, Michael S. Tsirkin wrote:
> >>> On Wed, Dec 06, 2017 at 04:06:18PM +0300, Ilya Maximets wrote:
> >>>> In case virtio error occured after vhost_dev_close(), qemu will crash
> >>>> in nested cleanup while checking IOMMU flag because dev->vdev already
> >>>> set to zero and resources are already freed.
> >>>>
> >>>> Example:
> >>>>
> >>>> Program received signal SIGSEGV, Segmentation fault.
> >>>> vhost_virtqueue_stop at hw/virtio/vhost.c:1155
> >>>>
> >>>>     #0  vhost_virtqueue_stop at hw/virtio/vhost.c:1155
> >>>>     #1  vhost_dev_stop at hw/virtio/vhost.c:1594
> >>>>     #2  vhost_net_stop_one at hw/net/vhost_net.c:289
> >>>>     #3  vhost_net_stop at hw/net/vhost_net.c:368
> >>>>
> >>>>             Nested call to vhost_net_stop(). First time was at #14.
> >>>>
> >>>>     #4  virtio_net_vhost_status at hw/net/virtio-net.c:180
> >>>>     #5  virtio_net_set_status (status=79) at hw/net/virtio-net.c:254
> >>>>     #6  virtio_set_status at hw/virtio/virtio.c:1146
> >>>>     #7  virtio_error at hw/virtio/virtio.c:2455
> >>>>
> >>>>         virtqueue_get_head() failed here.
> >>>>
> >>>>     #8  virtqueue_get_head at hw/virtio/virtio.c:543
> >>>>     #9  virtqueue_drop_all at hw/virtio/virtio.c:984
> >>>>     #10 virtio_net_drop_tx_queue_data at hw/net/virtio-net.c:240
> >>>>     #11 virtio_bus_set_host_notifier at hw/virtio/virtio-bus.c:297
> >>>>     #12 vhost_dev_disable_notifiers at hw/virtio/vhost.c:1431
> >>>>
> >>>>         vhost_dev_stop() was executed here. dev->vdev == NULL now.
> >>>>
> >>>>     #13 vhost_net_stop_one at hw/net/vhost_net.c:290
> >>>>     #14 vhost_net_stop at hw/net/vhost_net.c:368
> >>>>     #15 virtio_net_vhost_status at hw/net/virtio-net.c:180
> >>>>     #16 virtio_net_set_status (status=15) at hw/net/virtio-net.c:254
> >>>>     #17 qmp_set_link ("hostnet0", up=false) at net/net.c:1430
> >>>>     #18 chr_closed_bh at net/vhost-user.c:214
> >>>>     #19 aio_bh_call at util/async.c:90
> >>>>     #20 aio_bh_poll at util/async.c:118
> >>>>     #21 aio_dispatch at util/aio-posix.c:429
> >>>>     #22 aio_ctx_dispatch at util/async.c:261
> >>>>     #23 g_main_context_dispatch
> >>>>     #24 glib_pollfds_poll at util/main-loop.c:213
> >>>>     #25 os_host_main_loop_wait at util/main-loop.c:261
> >>>>     #26 main_loop_wait at util/main-loop.c:515
> >>>>     #27 main_loop () at vl.c:1917
> >>>>     #28 main at vl.c:4795
> >>>>
> >>>> Above backtrace captured from qemu crash on vhost disconnect while
> >>>> virtio driver in guest was in failed state.
> >>>>
> >>>> We can just add checking for 'vdev' in 'vhost_dev_has_iommu()' but
> >>>> it will assert further trying to free already freed ioeventfds. The
> >>>> real problem is that we're allowing nested calls to 'vhost_net_stop'.
> >>>>
> >>>> This patch is aimed to forbid nested calls to 'vhost_net_stop' to avoid
> >>>> any possible double frees and segmentation faults doue to using of
> >>>> already freed resources by setting 'vhost_started' flag to zero prior
> >>>> to 'vhost_net_stop' call.
> >>>>
> >>>> Signed-off-by: Ilya Maximets <address@hidden>
> >>>> ---
> >>>>
> >>>> This issue was already addressed more than a year ago by the following
> >>>> patch: 
> >>>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06732.html
> >>>> but it was dropped without review due to not yet implemented 
> >>>> re-connection
> >>>> of vhost-user. Re-connection implementation lately fixed most of the
> >>>> nested calls, but few of them still exists. For example, above backtrace
> >>>> captured after 'virtqueue_get_head()' failure on vhost-user 
> >>>> disconnection.
> >>>>
> >>>>  hw/net/virtio-net.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index 38674b0..4d95a18 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -177,8 +177,8 @@ static void virtio_net_vhost_status(VirtIONet *n, 
> >>>> uint8_t status)
> >>>>              n->vhost_started = 0;
> >>>>          }
> >>>>      } else {
> >>>> -        vhost_net_stop(vdev, n->nic->ncs, queues);
> >>>>          n->vhost_started = 0;
> >>>> +        vhost_net_stop(vdev, n->nic->ncs, queues);
> >>>>      }
> >>>>  }
> >>>
> >>> Well the wider context is
> >>>
> >>>
> >>>         n->vhost_started = 1;
> >>>         r = vhost_net_start(vdev, n->nic->ncs, queues);
> >>>         if (r < 0) {
> >>>             error_report("unable to start vhost net: %d: "
> >>>                          "falling back on userspace virtio", -r);
> >>>             n->vhost_started = 0;
> >>>         }
> >>>     } else {
> >>>         vhost_net_stop(vdev, n->nic->ncs, queues);
> >>>         n->vhost_started = 0;
> >>>
> >>> So we set it to 1 before start, we should clear after stop.
> >>
> >> OK. I agree that clearing after is a bit safer. But in this case we need
> >> a separate flag or other way to detect that we're already inside the
> >> 'vhost_net_stop()'.
> >>
> >> What do you think about that old patch:
> >> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg06732.html  ?
> >>
> >> It implements the same thing but introduces additional flag. It even could
> >> be still applicable.
> > 
> > So the issue is that if someone calls set_status nested
> > from within set_status, then status gets over-written
> > because status is only set at the last moment,
> > it's thus actually incorrect most of the time.
> > 
> > But most people just want the new status,
> > so it is better to keep that as part of status.
> > 
> > I think something like the following should do the trick.
> > Thoughts?
> 
> Maybe you're right, but it's really hard to understand the patch below.
> The function 'set_status' doesn't set the status anymore, instead we
> need to set status manually before calling the 'set_status'.

I don't understand this comment.  status is set in virtio_set_status,
same as it was previously.  set_status is a callback.

The issue was that if set_status callback changed the status
again, it would overwrite the old value.
Fix is to firsdt set status then do a callback.

This means we can not pass new value as parameter any longer.
We pass the old one, new value is in vdev->status.

It's true that virtio_net_set_status checks vdev->status
now, so it must be set correctly on unrealize.

> That
> looks very strange. Some of the functions gets 'old_status', others
> the 'new_status'. I'm a bit confused.

OK, fair enough. Fixed - let's pass old status everywhere,
users that need the new one can get it from the vdev.

> And it's not functional in current state:
> 
> hw/net/virtio-net.c:264:28: error: ‘status’ undeclared

Fixed too. new version below.

> > 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>

Still completely untested, sorry about that - hope you can help here.

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 098bdaa..f5d0ee1 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -115,7 +115,7 @@ typedef struct VirtioDeviceClass {
     void (*get_config)(VirtIODevice *vdev, uint8_t *config);
     void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
     void (*reset)(VirtIODevice *vdev);
-    void (*set_status)(VirtIODevice *vdev, uint8_t val);
+    void (*set_status)(VirtIODevice *vdev, uint8_t old_status);
     /* For transitional devices, this is a bitmap of features
      * that are only exposed on the legacy interface but not
      * the modern one.
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 05d1440..b8b07ba 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -814,15 +814,15 @@ static uint64_t virtio_blk_get_features(VirtIODevice 
*vdev, uint64_t features,
     return features;
 }
 
-static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status)
+static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t old_status)
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
-    if (!(status & (VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_DRIVER_OK))) {
+    if (!(vdev->status & (VIRTIO_CONFIG_S_DRIVER | 
VIRTIO_CONFIG_S_DRIVER_OK))) {
         assert(!s->dataplane_started);
     }
 
-    if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return;
     }
 
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 9470bd7..881b1ff 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -616,7 +616,7 @@ static void guest_reset(VirtIOSerial *vser)
     }
 }
 
-static void set_status(VirtIODevice *vdev, uint8_t status)
+static void set_status(VirtIODevice *vdev, uint8_t old_status)
 {
     VirtIOSerial *vser;
     VirtIOSerialPort *port;
@@ -625,7 +625,7 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
     port = find_port_by_id(vser, 0);
 
     if (port && !use_multiport(port->vser)
-        && (status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         /*
          * Non-multiport guests won't be able to tell us guest
          * open/close status.  Such guests can only have a port at id
@@ -634,7 +634,7 @@ static void set_status(VirtIODevice *vdev, uint8_t status)
          */
         port->guest_connected = true;
     }
-    if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         guest_reset(vser);
     }
 
diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c
index 0e42f0d..abb817b 100644
--- a/hw/input/virtio-input.c
+++ b/hw/input/virtio-input.c
@@ -188,12 +188,12 @@ static uint64_t virtio_input_get_features(VirtIODevice 
*vdev, uint64_t f,
     return f;
 }
 
-static void virtio_input_set_status(VirtIODevice *vdev, uint8_t val)
+static void virtio_input_set_status(VirtIODevice *vdev, uint8_t old_val)
 {
     VirtIOInputClass *vic = VIRTIO_INPUT_GET_CLASS(vdev);
     VirtIOInput *vinput = VIRTIO_INPUT(vdev);
 
-    if (val & VIRTIO_CONFIG_S_DRIVER_OK) {
+    if (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) {
         if (!vinput->active) {
             vinput->active = true;
             if (vic->change_active) {
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 38674b0..7851968 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -124,7 +124,7 @@ static void virtio_net_announce_timer(void *opaque)
     virtio_notify_config(vdev);
 }
 
-static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
+static void virtio_net_vhost_status(VirtIONet *n, uint8_t old_status)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     NetClientState *nc = qemu_get_queue(n->nic);
@@ -134,7 +134,7 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t 
status)
         return;
     }
 
-    if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
+    if ((virtio_net_started(n, vdev->status) && !nc->peer->link_down) ==
         !!n->vhost_started) {
         return;
     }
@@ -212,12 +212,12 @@ static bool virtio_net_set_vnet_endian(VirtIODevice 
*vdev, NetClientState *ncs,
     return false;
 }
 
-static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t status)
+static void virtio_net_vnet_endian_status(VirtIONet *n, uint8_t old_status)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
     int queues = n->multiqueue ? n->max_queues : 1;
 
-    if (virtio_net_started(n, status)) {
+    if (virtio_net_started(n, vdev->status)) {
         /* Before using the device, we tell the network backend about the
          * endianness to use when parsing vnet headers. If the backend
          * can't do it, we fallback onto fixing the headers in the core
@@ -225,7 +225,7 @@ static void virtio_net_vnet_endian_status(VirtIONet *n, 
uint8_t status)
          */
         n->needs_vnet_hdr_swap = virtio_net_set_vnet_endian(vdev, n->nic->ncs,
                                                             queues, true);
-    } else if (virtio_net_started(n, vdev->status)) {
+    } else if (virtio_net_started(n, old_status)) {
         /* After using the device, we need to reset the network backend to
          * the default (guest native endianness), otherwise the guest may
          * lose network connectivity if it is rebooted into a different
@@ -243,15 +243,15 @@ static void virtio_net_drop_tx_queue_data(VirtIODevice 
*vdev, VirtQueue *vq)
     }
 }
 
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t 
old_status)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     VirtIONetQueue *q;
     int i;
     uint8_t queue_status;
 
-    virtio_net_vnet_endian_status(n, status);
-    virtio_net_vhost_status(n, status);
+    virtio_net_vnet_endian_status(n, old_status);
+    virtio_net_vhost_status(n, old_status);
 
     for (i = 0; i < n->max_queues; i++) {
         NetClientState *ncs = qemu_get_subqueue(n->nic, i);
@@ -261,7 +261,7 @@ static void virtio_net_set_status(struct VirtIODevice 
*vdev, uint8_t status)
         if ((!n->multiqueue && i != 0) || i >= n->curr_queues) {
             queue_status = 0;
         } else {
-            queue_status = status;
+            queue_status = vdev->status;
         }
         queue_started =
             virtio_net_started(n, queue_status) && !n->vhost_started;
@@ -2048,9 +2048,11 @@ static void virtio_net_device_unrealize(DeviceState 
*dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIONet *n = VIRTIO_NET(dev);
     int i, max_queues;
+    uint8_t old_status = vdev->status;
 
     /* This will stop vhost backend if appropriate. */
-    virtio_net_set_status(vdev, 0);
+    vdev->status = 0;
+    virtio_net_set_status(vdev, old_status);
 
     g_free(n->netclient_name);
     n->netclient_name = NULL;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 9c1bea8..5a561e4 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -108,11 +108,11 @@ static void vhost_scsi_stop(VHostSCSI *s)
     vhost_scsi_common_stop(vsc);
 }
 
-static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
+static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t old_val)
 {
     VHostSCSI *s = VHOST_SCSI(vdev);
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
-    bool start = (val & VIRTIO_CONFIG_S_DRIVER_OK);
+    bool start = vdev->status & VIRTIO_CONFIG_S_DRIVER_OK;
 
     if (vsc->dev.started == start) {
         return;
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index f7561e2..289a27e 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -38,11 +38,11 @@ static const int user_feature_bits[] = {
     VHOST_INVALID_FEATURE_BIT
 };
 
-static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
+static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t old_status)
 {
     VHostUserSCSI *s = (VHostUserSCSI *)vdev;
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(s);
-    bool start = (status & VIRTIO_CONFIG_S_DRIVER_OK) && vdev->vm_running;
+    bool start = (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && 
vdev->vm_running;
 
     if (vsc->dev.started == start) {
         return;
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 5ec1c6a..d3f445b 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -154,10 +154,10 @@ static void vhost_vsock_stop(VirtIODevice *vdev)
     vhost_dev_disable_notifiers(&vsock->vhost_dev, vdev);
 }
 
-static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t status)
+static void vhost_vsock_set_status(VirtIODevice *vdev, uint8_t old_status)
 {
     VHostVSock *vsock = VHOST_VSOCK(vdev);
-    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+    bool should_start = vdev->status & VIRTIO_CONFIG_S_DRIVER_OK;
 
     if (!vdev->vm_running) {
         should_start = false;
@@ -370,11 +370,14 @@ static void vhost_vsock_device_unrealize(DeviceState 
*dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostVSock *vsock = VHOST_VSOCK(dev);
+    uint8_t old_status;
 
     vhost_vsock_post_load_timer_cleanup(vsock);
 
     /* This will stop vhost backend if appropriate. */
-    vhost_vsock_set_status(vdev, 0);
+    old_status = vdev->status;
+    vdev->status = 0;
+    vhost_vsock_set_status(vdev, old_status);
 
     vhost_dev_cleanup(&vsock->vhost_dev);
     virtio_cleanup(vdev);
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 37cde38..84e897a 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -468,12 +468,12 @@ static void virtio_balloon_device_reset(VirtIODevice 
*vdev)
     }
 }
 
-static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
+static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t old_status)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
 
     if (!s->stats_vq_elem && vdev->vm_running &&
-        (status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 1)) {
+        (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 
1)) {
         /* poll stats queue for the element we have discarded when the VM
          * was stopped */
         virtio_balloon_receive_stats(vdev, s->svq);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index ad564b0..4981858 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1128,6 +1128,8 @@ static int virtio_validate_features(VirtIODevice *vdev)
 int virtio_set_status(VirtIODevice *vdev, uint8_t val)
 {
     VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint8_t old_status;
+
     trace_virtio_set_status(vdev, val);
 
     if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
@@ -1140,10 +1142,13 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val)
             }
         }
     }
+
+    old_status = vdev->status;
+    vdev->status = val;
+
     if (k->set_status) {
-        k->set_status(vdev, val);
+        k->set_status(vdev, old_status);
     }
-    vdev->status = val;
     return 0;
 }