From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49077) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTrMl-00071a-4v for qemu-devel@nongnu.org; Mon, 03 Dec 2018 11:46:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gTrMh-0006He-4k for qemu-devel@nongnu.org; Mon, 03 Dec 2018 11:46:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53398) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gTrMg-0006FB-SM for qemu-devel@nongnu.org; Mon, 03 Dec 2018 11:46:35 -0500 References: <20181203100608.28538-1-jasowang@redhat.com> <20181203100608.28538-5-jasowang@redhat.com> From: Eric Blake Message-ID: Date: Mon, 3 Dec 2018 10:46:13 -0600 MIME-Version: 1.0 In-Reply-To: <20181203100608.28538-5-jasowang@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V4 for 3.1 4/4] virtio-net-test: add large tx buffer test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org, peter.maydell@linaro.org Cc: mst@redhat.com, ppandit@redhat.com, liq3ea@163.com, liq3ea@gmail.com, pbonzini@redhat.com, thuth@redhat.com On 12/3/18 4:06 AM, Jason Wang wrote: > This test tries to build a packet whose size is greater than INT_MAX > which tries to trigger integer overflow in qemu_net_queue_append_iov() > which may result OOB. Can you also add a packet just slightly larger than NET_BUFSIZE (68k) to show that we aren't having any further issues at even smaller (and more likely) values of oversized packets? > > Signed-off-by: Jason Wang > --- > tests/virtio-net-test.c | 44 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > +++ b/tests/virtio-net-test.c > @@ -245,6 +245,49 @@ static void pci_basic(gconstpointer data) > g_free(dev); > qtest_shutdown(qs); > } > + > +static void large_tx(gconstpointer data) > +{ > + QVirtioPCIDevice *dev; > + QOSState *qs; > + QVirtQueuePCI *tx, *rx; > + QVirtQueue *vq; > + uint64_t req_addr; > + uint32_t free_head; > + size_t alloc_size = UINT_MAX / 64; > + int i; > + > + qs = pci_test_start("-netdev hubport,id=hp0,hubid=0 " > + "-device virtio-net-pci,netdev=hp0 "); Why the trailing space? > + dev = virtio_net_pci_init(qs->pcibus, PCI_SLOT); > + > + rx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 0); > + tx = (QVirtQueuePCI *)qvirtqueue_setup(&dev->vdev, qs->alloc, 1); > + > + driver_init(&dev->vdev); > + vq = &tx->vq; > + > + /* Bypass the limitation by pointing several descriptors to a single > + * smaller area */ > + req_addr = guest_alloc(qs->alloc, alloc_size); > + free_head = qvirtqueue_add(vq, req_addr, alloc_size, false, true); > + > + for (i = 0; i < 64; i++) { > + qvirtqueue_add(vq, req_addr, alloc_size, false, i == 63 ? > + false : true); Any time I see both 'true' and 'false' in a ?: operator, I have to wonder why you didn't just write the simpler version directly on the condition. This is the same as 'i != 63'. > + } > + qvirtqueue_kick(&dev->vdev, vq, free_head); > + > + qvirtio_wait_used_elem(&dev->vdev, vq, free_head, NULL, > + QVIRTIO_NET_TIMEOUT_US); > + > + qvirtqueue_cleanup(dev->vdev.bus, &tx->vq, qs->alloc); > + qvirtqueue_cleanup(dev->vdev.bus, &rx->vq, qs->alloc); > + qvirtio_pci_device_disable(dev); > + g_free(dev->pdev); > + g_free(dev); > + qtest_shutdown(qs); > +} > #endif > > static void hotplug(void) > @@ -270,6 +313,7 @@ int main(int argc, char **argv) > qtest_add_data_func("/virtio/net/pci/basic", send_recv_test, pci_basic); > qtest_add_data_func("/virtio/net/pci/rx_stop_cont", > stop_cont_test, pci_basic); > + qtest_add_data_func("/virtio/net/pci/large_tx", NULL, large_tx); > #endif > qtest_add_func("/virtio/net/pci/hotplug", hotplug); > I can reproduce Peter's complaints of this introducing noise to 'make check': qemu-system-x86_64: warning: hub 0 is not connected to host network With patches 2-4 applied but patch 1 omitted, 'make check' fails with: Broken pipe tests/libqtest.c:125: kill_qemu() detected QEMU death from signal 11 (Segmentation fault) (core dumped) GTester: last random seed: R02S9569aabcb2d834a9dadcce272c5588db so the test is definitely triggering the problem as patched by part 1. Although I'm not confident enough of what the test is doing for an R-b, and would like it to be less noisy, I can at least add: Tested-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org