Re: [PATCH 21/24] vhost-user: use guest buffer directly in vu_handle_tx()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9424 bytes --]

On Fri, Feb 02, 2024 at 03:11:48PM +0100, Laurent Vivier wrote:
> Check the buffer address is correctly in the mmap'ed memory.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c     |  6 +++++
>  packet.h     |  2 ++
>  tap.c        | 39 +++++++++++++++++++++++++++----
>  tap.h        |  1 +
>  vhost_user.c | 66 ++++++++++++++++++++++++++++++++--------------------
>  5 files changed, 84 insertions(+), 30 deletions(-)
> 
> 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 == 0)

So, IIUC, you're using p->buf_size == 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];
>  };
>  
> +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) == ETH_P_ARP) {
> -			PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf));
> +			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			packet_add(pkt, l2_len, (char *)eh);
>  			arp(c, pkt);
> @@ -656,7 +656,7 @@ resume:
>  			continue;
>  
>  		if (iph->protocol == IPPROTO_ICMP) {
> -			PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf));
> +			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			if (c->no_icmp)
>  				continue;
> @@ -675,7 +675,7 @@ resume:
>  			continue;
>  
>  		if (iph->protocol == IPPROTO_UDP) {
> -			PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf));
> +			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			packet_add(pkt, l2_len, (char *)eh);
>  			if (dhcp(c, pkt))
> @@ -815,7 +815,7 @@ resume:
>  		}
>  
>  		if (proto == IPPROTO_ICMPV6) {
> -			PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf));
> +			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			if (c->no_icmp)
>  				continue;
> @@ -839,7 +839,7 @@ resume:
>  		uh = (struct udphdr *)l4h;
>  
>  		if (proto == IPPROTO_UDP) {
> -			PACKET_POOL_P(pkt, 1, in->buf, sizeof(pkt_buf));
> +			PACKET_POOL_P(pkt, 1, in->buf, in->buf_size);
>  
>  			packet_add(pkt, l4_len, l4h);
>  
> @@ -1291,6 +1291,23 @@ static void tap_sock_tun_init(struct ctx *c)
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
>  }
>  
> +void tap_sock_update_buf(void *base, size_t size)
> +{
> +	int i;
> +
> +	pool_tap4_storage.buf = base;
> +	pool_tap4_storage.buf_size = size;
> +	pool_tap6_storage.buf = base;
> +	pool_tap6_storage.buf_size = size;
> +
> +	for (i = 0; i < TAP_SEQS; i++) {
> +		tap4_l4[i].p.buf = base;
> +		tap4_l4[i].p.buf_size = size;
> +		tap6_l4[i].p.buf = base;
> +		tap6_l4[i].p.buf_size = size;
> +	}
> +}
> +
>  /**
>   * tap_sock_init() - Create and set up AF_UNIX socket or tuntap file descriptor
>   * @c:		Execution context
> @@ -1302,10 +1319,22 @@ void tap_sock_init(struct ctx *c)
>  
>  	pool_tap4_storage = PACKET_INIT(pool_tap4, TAP_MSGS, pkt_buf, sz);
>  	pool_tap6_storage = PACKET_INIT(pool_tap6, TAP_MSGS, pkt_buf, sz);
> +	if (c->mode == MODE_VU) {
> +		pool_tap4_storage.buf = NULL;
> +		pool_tap4_storage.buf_size = 0;
> +		pool_tap6_storage.buf = NULL;
> +		pool_tap6_storage.buf_size = 0;
> +	}
>  
>  	for (i = 0; i < TAP_SEQS; i++) {
>  		tap4_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz);
>  		tap6_l4[i].p = PACKET_INIT(pool_l4, UIO_MAXIOV, pkt_buf, sz);
> +		if (c->mode == MODE_VU) {
> +			tap4_l4[i].p.buf = NULL;
> +			tap4_l4[i].p.buf_size = 0;
> +			tap6_l4[i].p.buf = NULL;
> +			tap6_l4[i].p.buf_size = 0;
> +		}

Can't you use your tap_sock_update_buf() function above to do this
initialization?

>  	}
>  
>  	if (c->fd_tap != -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);
>  }
>  
> +int vu_packet_check_range(void *buf, size_t offset, size_t len, const char *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 = buf; dev_region->mmap_addr; dev_region++) {
> +		if ((char *)dev_region->mmap_addr <= 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,
>  		}
>  	}
>  
> +	/* XXX */

What's this XXX for?

> +	ASSERT(vdev->nregions < VHOST_USER_MAX_RAM_SLOTS - 1);
> +	vdev->regions[vdev->nregions].mmap_addr = 0; /* mark EOF for vu_packet_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;
>  }
>  
> @@ -650,8 +675,8 @@ static void vu_handle_tx(VuDev *vdev, int index)
>  	VuVirtq *vq = &vdev->vq[index];
>  	int hdrlen = vdev->hdrlen;
>  	struct timespec now;
> -	char *p;
> -	size_t n;
> +	unsigned int indexes[VIRTQUEUE_MAX_SIZE];
> +	int count;
>  
>  	if (index % 2 != 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)
>  
>  	clock_gettime(CLOCK_MONOTONIC, &now);
>  
> -	p = pkt_buf;
> -
>  	pool_flush_all();
>  
> +	count = 0;
>  	while (1) {
>  		VuVirtqElement *elem;
> -		unsigned int out_num;
> -		struct iovec sg[VIRTQUEUE_MAX_SIZE], *out_sg;
>  
>  		ASSERT(index == VHOST_USER_TX_QUEUE);
>  		elem = vu_queue_pop(vdev, vq, sizeof(VuVirtqElement), buffer[index]);
> @@ -675,32 +697,26 @@ static void vu_handle_tx(VuDev *vdev, int index)
>  			break;
>  		}
>  
> -		out_num = elem->out_num;
> -		out_sg = 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 == 1);
>  
> -		if (hdrlen) {
> -			unsigned sg_num;
> -
> -			sg_num = iov_copy(sg, ARRAY_SIZE(sg), out_sg, out_num,
> -					  hdrlen, -1);
> -			out_num = sg_num;
> -			out_sg = sg;
> -		}
> -
> -		n = iov_to_buf(out_sg, out_num, 0, p, TAP_BUF_FILL);
> -
> -		packet_add_all(c, n, p);
> -
> -		p += n;
> +		packet_add_all(c, elem->out_sg[0].iov_len - hdrlen,
> +			       (char *)elem->out_sg[0].iov_base + hdrlen);
> +		indexes[count] = elem->index;
> +		count++;
> +	}
> +	tap_handler_all(c, &now);
>  
> -		vu_queue_push(vdev, vq, elem, 0);
> +	if (count) {
> +		int i;
> +		for (i = 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);
>  }
>  
>  void vu_kick_cb(struct ctx *c, union epoll_ref ref)

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]