Re: [PATCH] vhost_user: fix multibuffer from linux

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

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

On Wed, Jan 15, 2025 at 11:33:02PM +0100, Stefano Brivio wrote:
> On Wed, 15 Jan 2025 17:22:30 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
> > Under some conditions, linux can provide several buffers
> > in the same element (multiple entries in the iovec array).
> > 
> > I didn't identify what changed between the kernel guest that
> > provides one buffer and the one that provides several
> > (doesn't seem to be a kernel change or a configuration change).
> 
> Perhaps memory pressure, or different page accounting between kernels?
> 
> > Fix the following assert:
> > 
> > ASSERTION FAILED in virtqueue_map_desc (virtio.c:402): num_sg < max_num_sg
> > 
> > What I can see is the buffer can be splitted in two iovecs:
> >   - vnet header
> >   - packet data
> > 
> > This change manages this special case but the real fix will be to allow
> > tap_add_packet() to manage iovec array.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Applied.

So, we obviously want this as a short term fix.  However, this seems
like it's still not very robust.  IIUC, kernel in theory could
arbitrarily split the packet, not just breaking it neatly at the
header boundary.  I think we should try to handle this more generally.

> I just wonder, if it makes sense as a follow-up:
> 
> > ---
> >  vu_common.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/vu_common.c b/vu_common.c
> > index 6d365bea5fe2..431fba6be0c0 100644
> > --- a/vu_common.c
> > +++ b/vu_common.c
> > @@ -18,6 +18,8 @@
> >  #include "pcap.h"
> >  #include "vu_common.h"
> >  
> > +#define VU_MAX_TX_BUFFER_NB	2
> > +
> >  /**
> >   * vu_packet_check_range() - Check if a given memory zone is contained in
> >   * 			     a mapped guest memory region
> > @@ -168,10 +170,15 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
> >  
> >  	count = 0;
> >  	out_sg_count = 0;
> > -	while (count < VIRTQUEUE_MAX_SIZE) {
> > +	while (count < VIRTQUEUE_MAX_SIZE &&
> > +	       out_sg_count + VU_MAX_TX_BUFFER_NB <= VIRTQUEUE_MAX_SIZE) {
> >  		int ret;
> >  
> > -		vu_set_element(&elem[count], &out_sg[out_sg_count], NULL);
> > +		elem[count].out_num = VU_MAX_TX_BUFFER_NB;
> > +		elem[count].out_sg = &out_sg[out_sg_count];
> > +		elem[count].in_num = 0;
> > +		elem[count].in_sg = NULL;
> > +
> >  		ret = vu_queue_pop(vdev, vq, &elem[count]);
> >  		if (ret < 0)
> >  			break;
> > @@ -181,11 +188,20 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
> >  			warn("virtio-net transmit queue contains no out buffers");
> >  			break;
> >  		}
> > -		ASSERT(elem[count].out_num == 1);
> > +		if (elem[count].out_num == 1) {
> > +			tap_add_packet(vdev->context,
> > +				       elem[count].out_sg[0].iov_len - hdrlen,
> > +				       (char *)elem[count].out_sg[0].iov_base +
> > +				        hdrlen);
> > +		} else {
> > +			/* vnet header can be in a separate iovec */
> > +			ASSERT(elem[count].out_num == 2);
> 
> I suppose we don't have strong guarantees about this. What about
> discarding the packet with a debug() message, at least until we have a
> more elegant solution, if this happens?
> 
> For UDP and ICMP, that's the best thing we can do.
> 
> For TCP, we could just discard a part of it, and the peer would tell
> our guest, but it's surely not practical to look into the packet here,
> so dropping it altogether would look reasonable.
> 
> > +			ASSERT(elem[count].out_sg[0].iov_len == (size_t)hdrlen);
> 
> And similarly here (with an err() message), even though there's probably
> an issue in the hypervisor if this happens, but it doesn't mean we're
> doomed.

Right.  If the kernel is splitting every packet this way, then we
probably are doomed, but we don't know it for certain.  In general
ASSERT()s should indicate a definite bug in *our* code, not unexpected
behaviour from something we interact with, so err() and dropping the
packet would be more appropriate IMO.

> 
> > +			tap_add_packet(vdev->context,
> > +				       elem[count].out_sg[1].iov_len,
> > +				       (char *)elem[count].out_sg[1].iov_base);
> > +		}
> >  
> > -		tap_add_packet(vdev->context,
> > -			       elem[count].out_sg[0].iov_len - hdrlen,
> > -			       (char *)elem[count].out_sg[0].iov_base + hdrlen);
> >  		count++;
> >  	}
> >  	tap_handler(vdev->context, now);
> 

-- 
David Gibson (he or they)	| 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 --]