[1/4] vhost-user: fix crash on socket disconnect.

Commit Message

Ilya Maximets March 30, 2016, 3:14 p.m. UTC

After disconnect of vhost-user socket (Ex.: host application crash)
QEMU will crash with segmentation fault while virtio driver unbinding
inside the guest.

This happens because vhost_net_cleanup() called while stopping
virtqueue #0 because of CHR_EVENT_CLOSED event arriving.
After that stopping of virtqueue #1 crashes because of cleaned and
freed device memory:

[-------------------------------- cut -----------------------------------]
[guest]# echo -n '0000:00:01.0' > /sys/bus/pci/drivers/virtio-pci/unbind
qemu: Failed to read msg header. Read 0 instead of 12. Original request 11.
qemu: Failed to read msg header. Read 0 instead of 12. Original request 11.
qemu: Failed to read msg header. Read 0 instead of 12. Original request 11.

Child terminated with signal = 0xb (SIGSEGV)
GDBserver exiting
[-------------------------------- cut -----------------------------------]
[host]# gdb
Breakpoint 1 vhost_user_cleanup(dev) at hw/virtio/vhost-user.c
(gdb) bt
#0  vhost_user_cleanup # executes 'dev->opaque = 0;'
#1  vhost_dev_cleanup
#2  vhost_net_cleanup  # After return from #1: g_free(net)
#3  vhost_user_stop
#4  net_vhost_user_event (<...>, event=5) /* CHR_EVENT_CLOSED */
#5  qemu_chr_be_event (<...>, event=event@entry=5)
#6  tcp_chr_disconnect
#7  tcp_chr_sync_read
#8  qemu_chr_fe_read_all
#9  vhost_user_read
#10 vhost_user_get_vring_base
#11 vhost_virtqueue_stop (vq=0xffe190, idx=0)
#12 vhost_dev_stop
#13 vhost_net_stop_one
#14 vhost_net_stop
#15 virtio_net_vhost_status
#16 virtio_net_set_status
#17 virtio_set_status
<...>
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
qemu_chr_fe_write_all (s=s@entry=0x0, <...>) at qemu-char.c:302
302         if (s->replay && replay_mode == REPLAY_MODE_PLAY) {
(gdb) bt
#0  qemu_chr_fe_write_all (s=s@entry=0x0, <...>)
#1  vhost_user_write
#2  vhost_user_get_vring_base
#3  vhost_virtqueue_stop (vq=0xffe190, idx=1) # device already freed here
#4  vhost_dev_stop
#5  vhost_net_stop_one
#6  vhost_net_stop
#7  virtio_net_vhost_status
#8  virtio_net_set_status
#9  virtio_set_status
<...>
[-------------------------------- cut -----------------------------------]

Fix that by introducing of reference counter for vhost_net device
and freeing memory only after dropping of last reference.

Signed-off-by: Ilya Maximets <[e-mail]>
---
 hw/net/vhost_net.c       | 22 +++++++++++++++++++---
 hw/net/virtio-net.c      | 32 ++++++++++++++++++++++++--------
 include/net/vhost-user.h |  1 +
 include/net/vhost_net.h  |  1 +
 net/filter.c             |  1 +
 net/vhost-user.c         | 43 ++++++++++++++++++++++++++++++++++---------
 6 files changed, 80 insertions(+), 20 deletions(-)

Patch

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 6e1032f..55d5456 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -296,6 +296,7 @@  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
         if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
                 dev->use_guest_notifier_mask = false;
         }
+        put_vhost_net(ncs[i].peer);
      }
 
     r = k->set_guest_notifiers(qbus->parent, total_queues * 2, true);
@@ -306,7 +307,7 @@  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
 
     for (i = 0; i < total_queues; i++) {
         r = vhost_net_start_one(get_vhost_net(ncs[i].peer), dev);
-
+        put_vhost_net(ncs[i].peer);
         if (r < 0) {
             goto err_start;
         }
@@ -317,6 +318,7 @@  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
 err_start:
     while (--i >= 0) {
         vhost_net_stop_one(get_vhost_net(ncs[i].peer), dev);
+        put_vhost_net(ncs[i].peer);
     }
     e = k->set_guest_notifiers(qbus->parent, total_queues * 2, false);
     if (e < 0) {
@@ -337,6 +339,7 @@  void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
 
     for (i = 0; i < total_queues; i++) {
         vhost_net_stop_one(get_vhost_net(ncs[i].peer), dev);
+        put_vhost_net(ncs[i].peer);
     }
 
     r = k->set_guest_notifiers(qbus->parent, total_queues * 2, false);
@@ -398,16 +401,25 @@  VHostNetState *get_vhost_net(NetClientState *nc)
     return vhost_net;
 }
 
+void put_vhost_net(NetClientState *nc)
+{
+    if (nc && nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
+        vhost_user_put_vhost_net(nc);
+    }
+}
+
 int vhost_set_vring_enable(NetClientState *nc, int enable)
 {
     VHostNetState *net = get_vhost_net(nc);
     const VhostOps *vhost_ops = net->dev.vhost_ops;
+    int res = 0;
 
     if (vhost_ops->vhost_set_vring_enable) {
-        return vhost_ops->vhost_set_vring_enable(&net->dev, enable);
+        res = vhost_ops->vhost_set_vring_enable(&net->dev, enable);
     }
 
-    return 0;
+    put_vhost_net(nc);
+    return res;
 }
 
 #else
@@ -466,6 +478,10 @@  VHostNetState *get_vhost_net(NetClientState *nc)
     return 0;
 }
 
+void put_vhost_net(NetClientState *nc)
+{
+}
+
 int vhost_set_vring_enable(NetClientState *nc, int enable)
 {
     return 0;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5798f87..e5456ef 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -121,6 +121,7 @@  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
     if (!get_vhost_net(nc->peer)) {
         return;
     }
+    put_vhost_net(nc->peer);
 
     if ((virtio_net_started(n, status) && !nc->peer->link_down) ==
         !!n->vhost_started) {
@@ -521,6 +522,8 @@  static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     NetClientState *nc = qemu_get_queue(n->nic);
+    VHostNetState *net;
+    uint64_t res;
 
     /* Firstly sync all virtio-net possible supported features */
     features |= n->host_features;
@@ -544,10 +547,15 @@  static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
         virtio_clear_feature(&features, VIRTIO_NET_F_HOST_UFO);
     }
 
-    if (!get_vhost_net(nc->peer)) {
-        return features;
+    net = get_vhost_net(nc->peer);
+    if (!net) {
+        res = features;
+    } else {
+        res = vhost_net_get_features(net, features);
+        put_vhost_net(nc->peer);
     }
-    return vhost_net_get_features(get_vhost_net(nc->peer), features);
+
+    return res;
 }
 
 static uint64_t virtio_net_bad_features(VirtIODevice *vdev)
@@ -615,11 +623,13 @@  static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
 
     for (i = 0;  i < n->max_queues; i++) {
         NetClientState *nc = qemu_get_subqueue(n->nic, i);
+        VHostNetState *net = get_vhost_net(nc->peer);
 
-        if (!get_vhost_net(nc->peer)) {
+        if (!net) {
             continue;
         }
-        vhost_net_ack_features(get_vhost_net(nc->peer), features);
+        vhost_net_ack_features(net, features);
+        put_vhost_net(nc->peer);
     }
 
     if (virtio_has_feature(features, VIRTIO_NET_F_CTRL_VLAN)) {
@@ -1698,8 +1708,13 @@  static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(idx));
+    VHostNetState *net = get_vhost_net(nc->peer);
+    bool res;
+
     assert(n->vhost_started);
-    return vhost_net_virtqueue_pending(get_vhost_net(nc->peer), idx);
+    res = vhost_net_virtqueue_pending(net, idx);
+    put_vhost_net(nc->peer);
+    return res;
 }
 
 static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
@@ -1707,9 +1722,10 @@  static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     NetClientState *nc = qemu_get_subqueue(n->nic, vq2q(idx));
+    VHostNetState *net = get_vhost_net(nc->peer);
     assert(n->vhost_started);
-    vhost_net_virtqueue_mask(get_vhost_net(nc->peer),
-                             vdev, idx, mask);
+    vhost_net_virtqueue_mask(net, vdev, idx, mask);
+    put_vhost_net(nc->peer);
 }
 
 static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
diff --git a/include/net/vhost-user.h b/include/net/vhost-user.h
index 85109f6..6bdaa1a 100644
--- a/include/net/vhost-user.h
+++ b/include/net/vhost-user.h
@@ -13,5 +13,6 @@ 
 
 struct vhost_net;
 struct vhost_net *vhost_user_get_vhost_net(NetClientState *nc);
+void vhost_user_put_vhost_net(NetClientState *nc);
 
 #endif /* VHOST_USER_H_ */
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 3389b41..577bfa8 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -29,6 +29,7 @@  void vhost_net_virtqueue_mask(VHostNetState *net, VirtIODevice *dev,
                               int idx, bool mask);
 int vhost_net_notify_migration_done(VHostNetState *net, char* mac_addr);
 VHostNetState *get_vhost_net(NetClientState *nc);
+void put_vhost_net(NetClientState *nc);
 
 int vhost_set_vring_enable(NetClientState * nc, int enable);
 #endif
diff --git a/net/filter.c b/net/filter.c
index 1c4fc5a..28403aa 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -214,6 +214,7 @@  static void netfilter_complete(UserCreatable *uc, Error **errp)
 
     if (get_vhost_net(ncs[0])) {
         error_setg(errp, "Vhost is not supported");
+        put_vhost_net(ncs[0]);
         return;
     }
 
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 1b9e73a..9026df3 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -22,6 +22,7 @@  typedef struct VhostUserState {
     NetClientState nc;
     CharDriverState *chr;
     VHostNetState *vhost_net;
+    int refcnt;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -29,13 +30,41 @@  typedef struct VhostUserChardevProps {
     bool is_unix;
 } VhostUserChardevProps;
 
+static void vhost_user_net_ref(VhostUserState *s)
+{
+    if (!s->vhost_net) {
+        return;
+    }
+    s->refcnt++;
+}
+
+static void vhost_user_net_unref(VhostUserState *s)
+{
+    if (!s->vhost_net) {
+        return;
+    }
+
+    if (--s->refcnt == 0) {
+        vhost_net_cleanup(s->vhost_net);
+        s->vhost_net = NULL;
+    }
+}
+
 VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
     assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
+    vhost_user_net_ref(s);
     return s->vhost_net;
 }
 
+void vhost_user_put_vhost_net(NetClientState *nc)
+{
+    VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
+    assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
+    vhost_user_net_unref(s);
+}
+
 static int vhost_user_running(VhostUserState *s)
 {
     return (s->vhost_net) ? 1 : 0;
@@ -54,10 +83,7 @@  static void vhost_user_stop(int queues, NetClientState *ncs[])
             continue;
         }
 
-        if (s->vhost_net) {
-            vhost_net_cleanup(s->vhost_net);
-            s->vhost_net = NULL;
-        }
+        vhost_user_net_unref(s);
     }
 }
 
@@ -81,6 +107,7 @@  static int vhost_user_start(int queues, NetClientState *ncs[])
         options.net_backend = ncs[i];
         options.opaque      = s->chr;
         s->vhost_net = vhost_net_init(&options);
+        vhost_user_net_ref(s);
         if (!s->vhost_net) {
             error_report("failed to init vhost_net for queue %d", i);
             goto err;
@@ -119,7 +146,9 @@  static ssize_t vhost_user_receive(NetClientState *nc, const uint8_t *buf,
         /* extract guest mac address from the RARP message */
         memcpy(mac_addr, &buf[6], 6);
 
+        vhost_user_net_ref(s);
         r = vhost_net_notify_migration_done(s->vhost_net, mac_addr);
+        vhost_user_net_unref(s);
 
         if ((r != 0) && (display_rarp_failure)) {
             fprintf(stderr,
@@ -136,11 +165,7 @@  static void vhost_user_cleanup(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
 
-    if (s->vhost_net) {
-        vhost_net_cleanup(s->vhost_net);
-        s->vhost_net = NULL;
-    }
-
+    vhost_user_net_unref(s);
     qemu_purge_queued_packets(nc);
 }