From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E86A9C43603 for ; Thu, 19 Dec 2019 22:36:53 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B3E7E24676 for ; Thu, 19 Dec 2019 22:36:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="oknEmw2u" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B3E7E24676 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:48444 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ii4Pc-0004t2-Rx for qemu-devel@archiver.kernel.org; Thu, 19 Dec 2019 17:36:52 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:39684) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ii4Ox-0004Ua-Ty for qemu-devel@nongnu.org; Thu, 19 Dec 2019 17:36:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ii4Ow-0006aU-3m for qemu-devel@nongnu.org; Thu, 19 Dec 2019 17:36:11 -0500 Received: from mail.kernel.org ([198.145.29.99]:55802) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ii4Ov-0006Ts-Or for qemu-devel@nongnu.org; Thu, 19 Dec 2019 17:36:10 -0500 Received: from localhost (c-67-164-102-47.hsd1.ca.comcast.net [67.164.102.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 22AED206EC; Thu, 19 Dec 2019 22:36:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1576794968; bh=jnhZinny5FDsNaBpY5u/Ewx8bBBI6DkcRM+qOj8ivNA=; h=Date:From:To:cc:Subject:In-Reply-To:References:From; b=oknEmw2us6FqBnGdr9Eisbm1oZmC25dlhZnLHbPPSajXyLbzpIiJ3rIImLRDDbDcs i2nWdVYuqH7RAgQ70sbVqJRjrLgH/sPmO5NF3mpM+s08J6BrP4zFHSuCi4usZ5ilVy wOpgQM+zODuL/cnYv9X2CXtEpG27O08VUx+JWfh0= Date: Thu, 19 Dec 2019 14:36:07 -0800 (PST) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-T480s To: Christian Schoenebeck Subject: Re: [PATCH] 9p: init_in_iov_from_pdu can truncate the size In-Reply-To: <2734053.qHGH40kZ6z@silver> Message-ID: References: <20191219004251.23763-1-sstabellini@kernel.org> <2734053.qHGH40kZ6z@silver> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 198.145.29.99 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: anthony.perard@citrix.com, groug@kaod.org, Stefano Stabellini , qemu-devel@nongnu.org, Stefano Stabellini Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Thu, 19 Dec 2019, Christian Schoenebeck wrote: > On Donnerstag, 19. Dezember 2019 01:42:51 CET Stefano Stabellini wrote: > > From: Stefano Stabellini > > > > init_in_iov_from_pdu might not be able to allocate the full buffer size > > requested, which comes from the client and could be larger than the > > transport has available at the time of the request. Specifically, this > > can happen with read operations, with the client requesting a read up to > > the max allowed, which might be more than the transport has available at > > the time. > > I haven't looked thoroughly at this yet, but that's about addressing a > temporary, not a permanent transport buffer size limitation, right? Yes, that is correct. > Because if it was a permanent one, then probably an adjusted (lowered) > msize should be returned on R_version response to client as well. > > I wonder why I never triggered this issue, because I was experimenting with > huge msize values for 9pfs performance checks. Was there anything specific to > trigger this issue? Lots of heavy usage by a Java application booting. Nothing like Java to stress the system :-) > > Today the implementation of init_in_iov_from_pdu throws an error, both > > Xen and Virtio. > > > > Instead, change the V9fsTransport interface so that the size becomes a > > pointer and can be limited by the implementation of > > init_in_iov_from_pdu. > > > > Change both the Xen and Virtio implementations to set the size to the > > size of the buffer they managed to allocate, instead of throwing an > > error. > > > > Signed-off-by: Stefano Stabellini > > CC: groug@kaod.org > > CC: anthony.perard@citrix.com > > --- > > hw/9pfs/9p.c | 22 +++++++++++++++------- > > hw/9pfs/9p.h | 2 +- > > hw/9pfs/virtio-9p-device.c | 10 +++------- > > hw/9pfs/xen-9p-backend.c | 12 ++++-------- > > 4 files changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index bdf7919abf..d6c89ce608 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -1682,22 +1682,30 @@ out_nofid: > > * with qemu_iovec_destroy(). > > */ > > static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu, > > - size_t skip, size_t size, > > + size_t skip, size_t *size, > > bool is_write) > > { > > QEMUIOVector elem; > > struct iovec *iov; > > unsigned int niov; > > + size_t alloc_size = *size + skip; > > > > if (is_write) { > > - pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, size + > > skip); + pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov, > > alloc_size); } else { > > - pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + > > skip); + pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, > > &alloc_size); + } > > + > > + if (alloc_size < skip) > > + { > > + *size = 0; > > + } else { > > + *size = alloc_size - skip; > > } > > > > Code style nitpicking: > > ERROR: that open brace { should be on the previous line > #56: FILE: hw/9pfs/9p.c:1699: > + if (alloc_size < skip) > + { Oops, sorry! I can fix that. > > > > static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp, > > @@ -1722,7 +1730,7 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, > > V9fsFidState *fidp, } > > offset += err; > > > > - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false); > > + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &read_count, false); > > err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0, > > ((char *)fidp->fs.xattr.value) + off, > > read_count); > > @@ -1852,7 +1860,7 @@ static void coroutine_fn v9fs_read(void *opaque) > > QEMUIOVector qiov; > > int32_t len; > > > > - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, max_count, > > false); + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset + 4, > > &max_count, false); qemu_iovec_init(&qiov, qiov_full.niov); > > do { > > qemu_iovec_reset(&qiov); > > @@ -2085,7 +2093,7 @@ static void coroutine_fn v9fs_write(void *opaque) > > return; > > } > > offset += err; > > - v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, count, true); > > + v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, &count, true); > > trace_v9fs_write(pdu->tag, pdu->id, fid, off, count, qiov_full.niov); > > > > fidp = get_fid(pdu, fid); > > diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h > > index 8883761b2c..50f7e21da6 100644 > > --- a/hw/9pfs/9p.h > > +++ b/hw/9pfs/9p.h > > @@ -365,7 +365,7 @@ struct V9fsTransport { > > ssize_t (*pdu_vunmarshal)(V9fsPDU *pdu, size_t offset, const char > > *fmt, va_list ap); > > void (*init_in_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov, > > - unsigned int *pniov, size_t size); > > + unsigned int *pniov, size_t *size); > > void (*init_out_iov_from_pdu)(V9fsPDU *pdu, struct iovec **piov, > > unsigned int *pniov, size_t size); void (*push_and_notify)(V9fsPDU > > *pdu); > > diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c > > index 775e8ff766..68873c3f5f 100644 > > --- a/hw/9pfs/virtio-9p-device.c > > +++ b/hw/9pfs/virtio-9p-device.c > > @@ -145,19 +145,15 @@ static ssize_t virtio_pdu_vunmarshal(V9fsPDU *pdu, > > size_t offset, } > > > > static void virtio_init_in_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov, > > - unsigned int *pniov, size_t size) > > + unsigned int *pniov, size_t *size) > > { > > V9fsState *s = pdu->s; > > V9fsVirtioState *v = container_of(s, V9fsVirtioState, state); > > VirtQueueElement *elem = v->elems[pdu->idx]; > > size_t buf_size = iov_size(elem->in_sg, elem->in_num); > > > > - if (buf_size < size) { > > - VirtIODevice *vdev = VIRTIO_DEVICE(v); > > - > > - virtio_error(vdev, > > - "VirtFS reply type %d needs %zu bytes, buffer has > > %zu", - pdu->id + 1, size, buf_size); > > + if (buf_size < *size) { > > + *size = buf_size; > > } > > > > *piov = elem->in_sg; > > diff --git a/hw/9pfs/xen-9p-backend.c b/hw/9pfs/xen-9p-backend.c > > index 3f54a21c76..3994a356d4 100644 > > --- a/hw/9pfs/xen-9p-backend.c > > +++ b/hw/9pfs/xen-9p-backend.c > > @@ -187,7 +187,7 @@ static void xen_9pfs_init_out_iov_from_pdu(V9fsPDU *pdu, > > static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU *pdu, > > struct iovec **piov, > > unsigned int *pniov, > > - size_t size) > > + size_t *size) > > { > > Xen9pfsDev *xen_9pfs = container_of(pdu->s, Xen9pfsDev, state); > > Xen9pfsRing *ring = &xen_9pfs->rings[pdu->tag % xen_9pfs->num_rings]; > > @@ -197,15 +197,11 @@ static void xen_9pfs_init_in_iov_from_pdu(V9fsPDU > > *pdu, g_free(ring->sg); > > > > ring->sg = g_malloc0(sizeof(*ring->sg) * 2); > > - xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, size); > > + xen_9pfs_in_sg(ring, ring->sg, &num, pdu->idx, *size); > > > > buf_size = iov_size(ring->sg, num); > > - if (buf_size < size) { > > - xen_pv_printf(&xen_9pfs->xendev, 0, "Xen 9pfs request type %d" > > - "needs %zu bytes, buffer has %zu\n", pdu->id, size, > > - buf_size); > > - xen_be_set_state(&xen_9pfs->xendev, XenbusStateClosing); > > - xen_9pfs_disconnect(&xen_9pfs->xendev); > > + if (buf_size < *size) { > > + *size = buf_size; > > } > > > > *piov = ring->sg; > > Best regards, > Christian Schoenebeck > > >