From patchwork Tue Oct 27 09:31:07 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Will Deacon X-Patchwork-Id: 7495221 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 8E03D9F36A for ; Tue, 27 Oct 2015 09:31:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 61B742084D for ; Tue, 27 Oct 2015 09:31:23 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 181032070B for ; Tue, 27 Oct 2015 09:31:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753938AbbJ0JbR (ORCPT ); Tue, 27 Oct 2015 05:31:17 -0400 Received: from foss.arm.com ([217.140.101.70]:55559 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753879AbbJ0JbP (ORCPT ); Tue, 27 Oct 2015 05:31:15 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8B97B4A; Tue, 27 Oct 2015 02:31:08 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 631063F2E5; Tue, 27 Oct 2015 02:31:12 -0700 (PDT) Date: Tue, 27 Oct 2015 09:31:07 +0000 From: Will Deacon To: Sasha Levin Cc: Dmitry Vyukov , syzkaller@googlegroups.com, Sasha Levin , Pekka Enberg , Asias He , penberg@cs.helsinki.fi, Cyrill Gorcunov , matt@ozlabs.org, Michael Ellerman , Prasad Joshi , marc.zyngier@arm.com, "Aneesh Kumar K.V" , mingo@elte.hu, gorcunov@openvz.org, kvm@vger.kernel.org, Kostya Serebryany , Evgenii Stepanov , Alexey Samsonov , Alexander Potapenko Subject: Re: Network hangs when communicating with host Message-ID: <20151027093106.GA1689@arm.com> References: <56213324.3010901@oracle.com> <5624B639.4030603@arm.com> <5624FC39.2060708@oracle.com> <5626489B.1020309@oracle.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <5626489B.1020309@oracle.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP [apologies for the delay -- I've been off for a week and am catching up on email] On Tue, Oct 20, 2015 at 09:58:51AM -0400, Sasha Levin wrote: > On 10/20/2015 09:42 AM, Dmitry Vyukov wrote: > > I now have another issue. My binary fails to mmap a file within lkvm > > sandbox. The same binary works fine on host and in qemu. I've added > > strace into sandbox script, and here is the output: > > > > [pid 837] openat(AT_FDCWD, "syzkaller-shm048878722", O_RDWR|O_CLOEXEC) = 5 > > [pid 837] mmap(NULL, 1048576, PROT_READ|PROT_WRITE, MAP_SHARED, 5, > > 0) = -1 EINVAL (Invalid argument) > > > > I don't see anything that can potentially cause EINVAL here. Is it > > possible that lkvm somehow affects kernel behavior here? > > > > I run lkvm as: > > > > $ taskset 1 /kvmtool/lkvm sandbox --disk syz-0 --mem=2048 --cpus=2 > > --kernel /arch/x86/boot/bzImage --network mode=user --sandbox > > /workdir/kvm/syz-0.sh > > It's possible that something in the virtio-9p layer is broken. I'll give > it a look in the evening. I ended up with the patch below, but it's really ugly and I didn't get round to posting it. Will --->8 From 7cbcdfef1b9f094db4bf75676f22339f3164e103 Mon Sep 17 00:00:00 2001 From: Will Deacon Date: Fri, 17 Apr 2015 17:31:36 +0100 Subject: [PATCH] kvmtool: virtio-net: fix VIRTIO_NET_F_MRG_RXBUF usage in rx thread When merging virtio-net buffers using the VIRTIO_NET_F_MRG_RXBUF feature, the first buffer added to the used ring should indicate the total number of buffers used to hold the packet. Unfortunately, kvmtool has a number of issues when constructing these merged buffers: - Commit 5131332e3f1a ("kvmtool: convert net backend to support bi-endianness") introduced a strange loop counter, which resulted in hdr->num_buffers being set redundantly the first time round - When adding the buffers to the ring, we actually add them one-by-one, allowing the guest to see the header before we've inserted the rest of the data buffers... - ... which is made worse because we non-atomically increment the num_buffers count in the header each time we insert a new data buffer Consequently, the guest quickly becomes confused in its net rx code and the whole thing grinds to a halt. This is easily exemplified by trying to boot a root filesystem over NFS, which seldom succeeds. This patch resolves the issues by allowing us to insert items into the used ring without updating the index. Once the full payload has been added and num_buffers corresponds to the total size, we *then* publish the buffers to the guest. Cc: Marc Zyngier Cc: Sasha Levin Signed-off-by: Will Deacon --- include/kvm/virtio.h | 2 ++ virtio/core.c | 32 +++++++++++++++++++++++++------- virtio/net.c | 31 +++++++++++++++---------------- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/include/kvm/virtio.h b/include/kvm/virtio.h index 768ee9668d44..8324ba7d38be 100644 --- a/include/kvm/virtio.h +++ b/include/kvm/virtio.h @@ -112,6 +112,8 @@ static inline bool virt_queue__available(struct virt_queue *vq) return virtio_guest_to_host_u16(vq, vq->vring.avail->idx) != vq->last_avail_idx; } +void virt_queue__used_idx_advance(struct virt_queue *queue, u16 jump); +struct vring_used_elem * virt_queue__set_used_elem_no_update(struct virt_queue *queue, u32 head, u32 len, u16 offset); struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, u32 head, u32 len); bool virtio_queue__should_signal(struct virt_queue *vq); diff --git a/virtio/core.c b/virtio/core.c index 3b6e4d7cd045..d6ac289d450e 100644 --- a/virtio/core.c +++ b/virtio/core.c @@ -21,22 +21,17 @@ const char* virtio_trans_name(enum virtio_trans trans) return "unknown"; } -struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, u32 head, u32 len) +void virt_queue__used_idx_advance(struct virt_queue *queue, u16 jump) { - struct vring_used_elem *used_elem; u16 idx = virtio_guest_to_host_u16(queue, queue->vring.used->idx); - used_elem = &queue->vring.used->ring[idx % queue->vring.num]; - used_elem->id = virtio_host_to_guest_u32(queue, head); - used_elem->len = virtio_host_to_guest_u32(queue, len); - /* * Use wmb to assure that used elem was updated with head and len. * We need a wmb here since we can't advance idx unless we're ready * to pass the used element to the guest. */ wmb(); - idx++; + idx += jump; queue->vring.used->idx = virtio_host_to_guest_u16(queue, idx); /* @@ -45,6 +40,29 @@ struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, u32 * an updated idx. */ wmb(); +} + +struct vring_used_elem * +virt_queue__set_used_elem_no_update(struct virt_queue *queue, u32 head, + u32 len, u16 offset) +{ + struct vring_used_elem *used_elem; + u16 idx = virtio_guest_to_host_u16(queue, queue->vring.used->idx); + + idx += offset; + used_elem = &queue->vring.used->ring[idx % queue->vring.num]; + used_elem->id = virtio_host_to_guest_u32(queue, head); + used_elem->len = virtio_host_to_guest_u32(queue, len); + + return used_elem; +} + +struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, u32 head, u32 len) +{ + struct vring_used_elem *used_elem; + + used_elem = virt_queue__set_used_elem_no_update(queue, head, len, 0); + virt_queue__used_idx_advance(queue, 1); return used_elem; } diff --git a/virtio/net.c b/virtio/net.c index 9784520336b1..afee75333edb 100644 --- a/virtio/net.c +++ b/virtio/net.c @@ -80,14 +80,12 @@ static void virtio_net_fix_tx_hdr(struct virtio_net_hdr *hdr, struct net_dev *nd hdr->csum_offset = virtio_guest_to_host_u16(&ndev->vdev, hdr->csum_offset); } -static void virtio_net_fix_rx_hdr(struct virtio_net_hdr_mrg_rxbuf *hdr, struct net_dev *ndev) +static void virtio_net_fix_rx_hdr(struct virtio_net_hdr *hdr, struct net_dev *ndev) { - hdr->hdr.hdr_len = virtio_host_to_guest_u16(&ndev->vdev, hdr->hdr.hdr_len); - hdr->hdr.gso_size = virtio_host_to_guest_u16(&ndev->vdev, hdr->hdr.gso_size); - hdr->hdr.csum_start = virtio_host_to_guest_u16(&ndev->vdev, hdr->hdr.csum_start); - hdr->hdr.csum_offset = virtio_host_to_guest_u16(&ndev->vdev, hdr->hdr.csum_offset); - if (has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF)) - hdr->num_buffers = virtio_host_to_guest_u16(&ndev->vdev, hdr->num_buffers); + hdr->hdr_len = virtio_host_to_guest_u16(&ndev->vdev, hdr->hdr_len); + hdr->gso_size = virtio_host_to_guest_u16(&ndev->vdev, hdr->gso_size); + hdr->csum_start = virtio_host_to_guest_u16(&ndev->vdev, hdr->csum_start); + hdr->csum_offset = virtio_host_to_guest_u16(&ndev->vdev, hdr->csum_offset); } static void *virtio_net_rx_thread(void *p) @@ -123,7 +121,7 @@ static void *virtio_net_rx_thread(void *p) .iov_len = sizeof(buffer), }; struct virtio_net_hdr_mrg_rxbuf *hdr; - int i; + u16 num_buffers; len = ndev->ops->rx(&dummy_iov, 1, ndev); if (len < 0) { @@ -132,7 +130,7 @@ static void *virtio_net_rx_thread(void *p) goto out_err; } - copied = i = 0; + copied = num_buffers = 0; head = virt_queue__get_iov(vq, iov, &out, &in, kvm); hdr = iov[0].iov_base; while (copied < len) { @@ -140,19 +138,20 @@ static void *virtio_net_rx_thread(void *p) memcpy_toiovec(iov, buffer + copied, iovsize); copied += iovsize; - if (i++ == 0) - virtio_net_fix_rx_hdr(hdr, ndev); - if (has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF)) { - u16 num_buffers = virtio_guest_to_host_u16(vq, hdr->num_buffers); - hdr->num_buffers = virtio_host_to_guest_u16(vq, num_buffers + 1); - } - virt_queue__set_used_elem(vq, head, iovsize); + virt_queue__set_used_elem_no_update(vq, head, iovsize, num_buffers++); if (copied == len) break; while (!virt_queue__available(vq)) sleep(0); head = virt_queue__get_iov(vq, iov, &out, &in, kvm); } + + virtio_net_fix_rx_hdr(&hdr->hdr, ndev); + if (has_virtio_feature(ndev, VIRTIO_NET_F_MRG_RXBUF)) + hdr->num_buffers = virtio_host_to_guest_u16(vq, num_buffers); + + virt_queue__used_idx_advance(vq, num_buffers); + /* We should interrupt guest right now, otherwise latency is huge. */ if (virtio_queue__should_signal(vq)) ndev->vdev.ops->signal_vq(kvm, &ndev->vdev, id);