From patchwork Wed Mar 30 15:14:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 603427 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3qZrpY1tVdz9s5l for ; Thu, 31 Mar 2016 02:16:53 +1100 (AEDT) Received: from localhost ([::1]:54786 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alHrX-0001vZ-Fi for incoming@patchwork.ozlabs.org; Wed, 30 Mar 2016 11:16:51 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58599) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alHpO-00075F-Pw for qemu-devel@nongnu.org; Wed, 30 Mar 2016 11:14:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alHpI-0001fs-8b for qemu-devel@nongnu.org; Wed, 30 Mar 2016 11:14:38 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:41400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alHpH-0001ep-SZ for qemu-devel@nongnu.org; Wed, 30 Mar 2016 11:14:32 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O4U00FM3YC7VK70@mailout2.w1.samsung.com> for qemu-devel@nongnu.org; Wed, 30 Mar 2016 16:14:31 +0100 (BST) X-AuditID: cbfec7f5-f792a6d000001302-a8-56fbed566811 Received: from eusync1.samsung.com ( [203.254.199.211]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id 31.E9.04866.65DEBF65; Wed, 30 Mar 2016 16:14:30 +0100 (BST) Received: from imaximets.rnd.samsung.ru ([106.109.129.180]) by eusync1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0O4U00ACPYBSOO90@eusync1.samsung.com>; Wed, 30 Mar 2016 16:14:30 +0100 (BST) From: Ilya Maximets To: qemu-devel@nongnu.org, "Michael S. Tsirkin" Date: Wed, 30 Mar 2016 18:14:06 +0300 Message-id: <1459350849-31989-2-git-send-email-i.maximets@samsung.com> X-Mailer: git-send-email 2.5.0 In-reply-to: <1459350849-31989-1-git-send-email-i.maximets@samsung.com> References: <1459350849-31989-1-git-send-email-i.maximets@samsung.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrPJMWRmVeSWpSXmKPExsVy+t/xy7phb3+HGdzbpGVxpf0nu8WyS5+Z LP7/esVqcbx3B4vF5NlSDqweT65tZvJ4v+8qm0ffllWMAcxRXDYpqTmZZalF+nYJXBl/Wq6y FkxOqPh7ehdTA+MU7y5GTg4JAROJP4+3s0HYYhIX7q0Hsrk4hASWMkrsmjKXHcJpZZJYcmce E0gVm4COxKnVRxhBbBEBB4kvTT/A4swCJRKHfp1kB7GFgeJLH60Bs1kEVCXOHj0PVsMr4CbR +/ETkM0BtE1OYsGFdJAwp4C7xPKN/1lAbCGgkl0/pjBPYORdwMiwilE0tTS5oDgpPddIrzgx t7g0L10vOT93EyMkWL7uYFx6zOoQowAHoxIP7wbp32FCrIllxZW5hxglOJiVRHhXvgQK8aYk VlalFuXHF5XmpBYfYpTmYFES5525632IkEB6YklqdmpqQWoRTJaJg1OqgfFmv/nktjn7eQst d3l7/Lsn0/H9vrnW0qtalft37cjni9Dc7/Pt7v/HAvmrp8gcnRH8tqqYb47wHGddX8nig98c r/vEP7aN7lqvofqkIDUq1v/mqWfxr+/p9XQdt6i96hCbtOnAJ6uPp9r+TyssePumNFDJouid 8WZBLo61KV0L+r6/NZPcsUyJpTgj0VCLuag4EQBy9QYxEgIAAA== X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 210.118.77.12 Cc: Ilya Maximets , Jason Wang , Dyasly Sergey Subject: [Qemu-devel] [PATCH 1/4] vhost-user: fix crash on socket disconnect. X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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(-) 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); }