From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 71B4D5A026D for ; Fri, 9 Feb 2024 06:01:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1707454877; bh=vkU/D8VzOfl6koLEtF4IXtyeDOt7bD23CwJrx7tXaK4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DBqREfq7U6TOZxC1r2NSlpB35M3/LyWNYFtRv7k/hkTC9gcJrny9tjAhCtHgbvQEf 7Z0Jr2DAbiJ5qlFja5+hmfmx4JHiqEOMeGwrED0rIZLrZcRVOZ+4GIFBQHq9vmXSoi Hk7x8LWU13QzrLX4QKXzW3JMcq99E3p1+9q3SuprRXTPKWqq9gIC4uO9LK+B4t67i1 xkZ41vJOUef6qKQbV0Mp5udUEEcVMKpkteMr4+tPeqTopUIhcd42PlkdpElTvZlu/T J1KUOvFLRF05/3CMr4edn8N4pOfqcWQyw97z6oy7ZtJuj99WaBFe3LyE7uaa4nNd+s Yo00KXoYPsBvg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TWMBj3jlYz4wcs; Fri, 9 Feb 2024 16:01:17 +1100 (AEDT) Date: Fri, 9 Feb 2024 15:26:46 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 21/24] vhost-user: use guest buffer directly in vu_handle_tx() Message-ID: References: <20240202141151.3762941-1-lvivier@redhat.com> <20240202141151.3762941-22-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2goHjX22csSXTmo1" Content-Disposition: inline In-Reply-To: <20240202141151.3762941-22-lvivier@redhat.com> Message-ID-Hash: 6ICQWET3PC3XKB647HKYCECP46E4OZUU X-Message-ID-Hash: 6ICQWET3PC3XKB647HKYCECP46E4OZUU X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --2goHjX22csSXTmo1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 02, 2024 at 03:11:48PM +0100, Laurent Vivier wrote: > Check the buffer address is correctly in the mmap'ed memory. >=20 > Signed-off-by: Laurent Vivier > --- > packet.c | 6 +++++ > packet.h | 2 ++ > tap.c | 39 +++++++++++++++++++++++++++---- > tap.h | 1 + > vhost_user.c | 66 ++++++++++++++++++++++++++++++++-------------------- > 5 files changed, 84 insertions(+), 30 deletions(-) >=20 > diff --git a/packet.c b/packet.c > index af2a539a1794..3c5fc39df6d7 100644 > --- a/packet.c > +++ b/packet.c > @@ -25,6 +25,12 @@ > static int packet_check_range(const struct pool *p, size_t offset, size_= t len, > const char *start, const char *func, int line) > { > + ASSERT(p->buf); > + > + if (p->buf_size =3D=3D 0) So, IIUC, you're using p->buf_size =3D=3D 0 essentially as a flag to indicate that this packet pool is reference packets in an external (i.e.. guest for vhost-user) pool rather than passt allocated memory. Some comments on the data structure definition, and/or some "theory of operation" comments describing the two cases would probably help to make it easier to follow. > + return vu_packet_check_range((void *)p->buf, offset, len, start, > + func, line); > + > if (start < p->buf) { > if (func) { > trace("add packet start %p before buffer start %p, " > diff --git a/packet.h b/packet.h > index 8377dcf678bb..0aec6d9410aa 100644 > --- a/packet.h > +++ b/packet.h > @@ -22,6 +22,8 @@ struct pool { > struct iovec pkt[1]; > }; > =20 > +int vu_packet_check_range(void *buf, size_t offset, size_t len, > + const char *start, const char *func, int line); > void packet_add_do(struct pool *p, size_t len, const char *start, > const char *func, int line); > void *packet_get_do(const struct pool *p, const size_t idx, > diff --git a/tap.c b/tap.c > index c2a917bc00ca..930e48689497 100644 > --- a/tap.c > +++ b/tap.c > @@ -626,7 +626,7 @@ resume: > if (!eh) > continue; > if (ntohs(eh->h_proto) =3D=3D ETH_P_ARP) { > - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); > + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > =20 > packet_add(pkt, l2_len, (char *)eh); > arp(c, pkt); > @@ -656,7 +656,7 @@ resume: > continue; > =20 > if (iph->protocol =3D=3D IPPROTO_ICMP) { > - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); > + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > =20 > if (c->no_icmp) > continue; > @@ -675,7 +675,7 @@ resume: > continue; > =20 > if (iph->protocol =3D=3D IPPROTO_UDP) { > - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); > + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > =20 > packet_add(pkt, l2_len, (char *)eh); > if (dhcp(c, pkt)) > @@ -815,7 +815,7 @@ resume: > } > =20 > if (proto =3D=3D IPPROTO_ICMPV6) { > - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); > + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > =20 > if (c->no_icmp) > continue; > @@ -839,7 +839,7 @@ resume: > uh =3D (struct udphdr *)l4h; > =20 > if (proto =3D=3D IPPROTO_UDP) { > - PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf)); > + PACKET_POOL_P(pkt, 1, in->buf, in->buf_size); > =20 > packet_add(pkt, l4_len, l4h); > =20 > @@ -1291,6 +1291,23 @@ static void tap_sock_tun_init(struct ctx *c) > epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); > } > =20 > +void tap_sock_update_buf(void *base, size_t size) > +{ > + int i; > + > + pool_tap4_storage.buf =3D base; > + pool_tap4_storage.buf_size =3D size; > + pool_tap6_storage.buf =3D base; > + pool_tap6_storage.buf_size =3D size; > + > + for (i =3D 0; i < TAP_SEQS; i++) { > + tap4_l4[i].p.buf =3D base; > + tap4_l4[i].p.buf_size =3D size; > + tap6_l4[i].p.buf =3D base; > + tap6_l4[i].p.buf_size =3D size; > + } > +} > + > /** > * tap_sock_init() - Create and set up AF_UNIX socket or tuntap file des= criptor > * @c: Execution context > @@ -1302,10 +1319,22 @@ void tap_sock_init(struct ctx *c) > =20 > pool_tap4_storage =3D PACKET_INIT(pool_tap4, TAP_MSGS, pkt_buf, sz); > pool_tap6_storage =3D PACKET_INIT(pool_tap6, TAP_MSGS, pkt_buf, sz); > + if (c->mode =3D=3D MODE_VU) { > + pool_tap4_storage.buf =3D NULL; > + pool_tap4_storage.buf_size =3D 0; > + pool_tap6_storage.buf =3D NULL; > + pool_tap6_storage.buf_size =3D 0; > + } > =20 > for (i =3D 0; i < TAP_SEQS; i++) { > tap4_l4[i].p =3D PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz); > tap6_l4[i].p =3D PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz); > + if (c->mode =3D=3D MODE_VU) { > + tap4_l4[i].p.buf =3D NULL; > + tap4_l4[i].p.buf_size =3D 0; > + tap6_l4[i].p.buf =3D NULL; > + tap6_l4[i].p.buf_size =3D 0; > + } Can't you use your tap_sock_update_buf() function above to do this initialization? > } > =20 > if (c->fd_tap !=3D -1) { /* Passed as --fd */ > diff --git a/tap.h b/tap.h > index ee839d4f09dc..6823c9b32313 100644 > --- a/tap.h > +++ b/tap.h > @@ -82,6 +82,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, > void tap_handler_passt(struct ctx *c, uint32_t events, > const struct timespec *now); > void tap_sock_reset(struct ctx *c); > +void tap_sock_update_buf(void *base, size_t size); > void tap_sock_init(struct ctx *c); > void pool_flush_all(void); > void tap_handler_all(struct ctx *c, const struct timespec *now); > diff --git a/vhost_user.c b/vhost_user.c > index 2acd72398e3a..9cc07c8312c0 100644 > --- a/vhost_user.c > +++ b/vhost_user.c > @@ -334,6 +334,25 @@ static bool map_ring(VuDev *vdev, VuVirtq *vq) > return !(vq->vring.desc && vq->vring.used && vq->vring.avail); > } > =20 > +int vu_packet_check_range(void *buf, size_t offset, size_t len, const ch= ar *start, > + const char *func, int line) > +{ > + VuDevRegion *dev_region; > + Ah.. and if IIUC, in the indirect buffer case, the buf pointer in the pool is a pointer to a vector of VuDevRegion rather than a buffer. I think I'd prefer to see struct pool changed to include a union to make it clear that there are two quite different interpretations of the buf pointer. > + for (dev_region =3D buf; dev_region->mmap_addr; dev_region++) { > + if ((char *)dev_region->mmap_addr <=3D start && > + start + offset + len < (char *)dev_region->mmap_addr + > + dev_region->mmap_offset + > + dev_region->size) > + return 0; > + } > + if (func) { > + trace("cannot find region, %s:%i", func, line); > + } > + > + return -1; > +} > + > /* > * #syscalls:passt mmap munmap > */ > @@ -400,6 +419,12 @@ static bool vu_set_mem_table_exec(VuDev *vdev, > } > } > =20 > + /* XXX */ What's this XXX for? > + ASSERT(vdev->nregions < VHOST_USER_MAX_RAM_SLOTS - 1); > + vdev->regions[vdev->nregions].mmap_addr =3D 0; /* mark EOF for vu_packe= t_check_range() */ > + > + tap_sock_update_buf(vdev->regions, 0); If you use a union, you could make the pool point to a while VuDev with nregions as well as the actual region list and bounds check without needing this hack. > + > return false; > } > =20 > @@ -650,8 +675,8 @@ static void vu_handle_tx(VuDev *vdev, int index) > VuVirtq *vq =3D &vdev->vq[index]; > int hdrlen =3D vdev->hdrlen; > struct timespec now; > - char *p; > - size_t n; > + unsigned int indexes[VIRTQUEUE_MAX_SIZE]; > + int count; > =20 > if (index % 2 !=3D VHOST_USER_TX_QUEUE) { > debug("index %d is not an TX queue", index); > @@ -660,14 +685,11 @@ static void vu_handle_tx(VuDev *vdev, int index) > =20 > clock_gettime(CLOCK_MONOTONIC, &now); > =20 > - p =3D pkt_buf; > - > pool_flush_all(); > =20 > + count =3D 0; > while (1) { > VuVirtqElement *elem; > - unsigned int out_num; > - struct iovec sg[VIRTQUEUE_MAX_SIZE], *out_sg; > =20 > ASSERT(index =3D=3D VHOST_USER_TX_QUEUE); > elem =3D vu_queue_pop(vdev, vq, sizeof(VuVirtqElement), buffer[index]); > @@ -675,32 +697,26 @@ static void vu_handle_tx(VuDev *vdev, int index) > break; > } > =20 > - out_num =3D elem->out_num; > - out_sg =3D elem->out_sg; > - if (out_num < 1) { > + if (elem->out_num < 1) { The change from out_num local to elem->out_num seems like an unrelated stylistic change that could be folded into the earlier patch. > debug("virtio-net header not in first element"); > break; > } > + ASSERT(elem->out_num =3D=3D 1); > =20 > - if (hdrlen) { > - unsigned sg_num; > - > - sg_num =3D iov_copy(sg, ARRAY_SIZE(sg), out_sg, out_num, > - hdrlen, -1); > - out_num =3D sg_num; > - out_sg =3D sg; > - } > - > - n =3D iov_to_buf(out_sg, out_num, 0, p, TAP_BUF_FILL); > - > - packet_add_all(c, n, p); > - > - p +=3D n; > + packet_add_all(c, elem->out_sg[0].iov_len - hdrlen, > + (char *)elem->out_sg[0].iov_base + hdrlen); > + indexes[count] =3D elem->index; > + count++; > + } > + tap_handler_all(c, &now); > =20 > - vu_queue_push(vdev, vq, elem, 0); > + if (count) { > + int i; > + for (i =3D 0; i < count; i++) > + vu_queue_fill_by_index(vdev, vq, indexes[i], 0, i); > + vu_queue_flush(vdev, vq, count); > vu_queue_notify(vdev, vq); > } > - tap_handler_all(c, &now); > } > =20 > void vu_kick_cb(struct ctx *c, union epoll_ref ref) --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --2goHjX22csSXTmo1 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXFqXUACgkQzQJF27ox 2Gfx7hAAnQ2TnZboFuHiC29TaKQ7ViK5V9E/bzXSGR6ulKMZZM2FpKqUe3U7rdQd KzizRq6KXQd9Z96wrxVSUCnjd4dFAnWujdsx7KndwTDiPjNOszsFBITDl3UKKT38 9dvIjLpzIRcefgwQSBXKvqEVaMB5CDz2uNob2ti6IjI9xOvuOQOVdvIZcK/HY9cU F/J/v83OwbjJSA+uDhCa/QuxXAqupglNFg1xJMm9C0L1Sw6DIhBEo/EpP1IshI8P r8OjaezMK7qDJYiaT7Ga4NmolWp2vl9cntWzDAULfTemXOwQyLf5vVzOiVv4juSY UDvny75F8/SPa/I2kKZ71u+HiYkx3vb4D4EkLJXhXXYCcLCeI78sZtCqTRgKnHMY buj/aTgFRc4g2rSlXH7ACtPZhyTpkYWhlScBr3WiQ7rN6N16oDjhM0R3B1OVwaQr 3eQenLXas2KHfqSYDyEAgEq/CbfM59DWr/xHIEJPFVDR3IpAo4NfAfJwUoMeskx7 OdCEYqMvpqoRjDWCoI02jFirL5rb4xMeAYTTw7m1LZHCQEibXi+hF7s/GuaW+dw4 RrSDHHziQbhC3N39QcHouukczKmokg4ZxBU+/ohjMFxEptmTtIEkRmNIvUWNfRPK ll2iDhrJupxjCo9b6vneptpBczrIcGZs+2F+JDVSE2h2QCAE2Ug= =HZF3 -----END PGP SIGNATURE----- --2goHjX22csSXTmo1--