[PATCH] vhost_user: remove ASSERT() on iovec number

[PATCH] vhost_user: remove ASSERT() on iovec number

[Laurent Vivier]

Replace ASSERT() on the number of iovec in the element and on
the first entry length by a debug() message.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 vu_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/vu_common.c b/vu_common.c
index 431fba6be0c0..d34ae6dc1df3 100644
--- a/vu_common.c
+++ b/vu_common.c
@@ -195,8 +195,12 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
 				        hdrlen);
 		} else {
 			/* vnet header can be in a separate iovec */
-			ASSERT(elem[count].out_num == 2);
-			ASSERT(elem[count].out_sg[0].iov_len == (size_t)hdrlen);
+			if (elem[count].out_num != 2)
+				debug("virtio-net tranmit queue contains more than one buffer ([%d]: %u)",
+				      count, elem[count].out_num);
+			if (elem[count].out_sg[0].iov_len != (size_t)hdrlen)
+				debug("virtio-net transmit queue entry not aligned on hdrlen ([%d]: %d != %zu)",
+				count, hdrlen, elem[count].out_sg[0].iov_len);
 			tap_add_packet(vdev->context,
 				       elem[count].out_sg[1].iov_len,
 				       (char *)elem[count].out_sg[1].iov_base);
-- 
@@ -195,8 +195,12 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
 				        hdrlen);
 		} else {
 			/* vnet header can be in a separate iovec */
-			ASSERT(elem[count].out_num == 2);
-			ASSERT(elem[count].out_sg[0].iov_len == (size_t)hdrlen);
+			if (elem[count].out_num != 2)
+				debug("virtio-net tranmit queue contains more than one buffer ([%d]: %u)",
+				      count, elem[count].out_num);
+			if (elem[count].out_sg[0].iov_len != (size_t)hdrlen)
+				debug("virtio-net transmit queue entry not aligned on hdrlen ([%d]: %d != %zu)",
+				count, hdrlen, elem[count].out_sg[0].iov_len);
 			tap_add_packet(vdev->context,
 				       elem[count].out_sg[1].iov_len,
 				       (char *)elem[count].out_sg[1].iov_base);
-- 
2.47.1

Re: [PATCH] vhost_user: remove ASSERT() on iovec number

[Stefano Brivio]

On Mon, 20 Jan 2025 14:15:22 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> Replace ASSERT() on the number of iovec in the element and on
> the first entry length by a debug() message.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  vu_common.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/vu_common.c b/vu_common.c
> index 431fba6be0c0..d34ae6dc1df3 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -195,8 +195,12 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
>  				        hdrlen);
>  		} else {
>  			/* vnet header can be in a separate iovec */
> -			ASSERT(elem[count].out_num == 2);
> -			ASSERT(elem[count].out_sg[0].iov_len == (size_t)hdrlen);
> +			if (elem[count].out_num != 2)
> +				debug("virtio-net tranmit queue contains more than one buffer ([%d]: %u)",

s/tranmit/transmit/

Applied with this change.

-- 
Stefano

Re: [PATCH] vhost_user: remove ASSERT() on iovec number

[David Gibson]

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

On Mon, Jan 20, 2025 at 02:15:22PM +0100, Laurent Vivier wrote:
> Replace ASSERT() on the number of iovec in the element and on
> the first entry length by a debug() message.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Removing the ASSERT() makes sense, but is it safe to carry on to the
tap_add_packet() if the packet is not in the layout we expect?  Should
we be bailing out of the function (effectively dropping the packet)
instead?

> ---
>  vu_common.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/vu_common.c b/vu_common.c
> index 431fba6be0c0..d34ae6dc1df3 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -195,8 +195,12 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
>  				        hdrlen);
>  		} else {
>  			/* vnet header can be in a separate iovec */
> -			ASSERT(elem[count].out_num == 2);
> -			ASSERT(elem[count].out_sg[0].iov_len == (size_t)hdrlen);
> +			if (elem[count].out_num != 2)
> +				debug("virtio-net tranmit queue contains more than one buffer ([%d]: %u)",
> +				      count, elem[count].out_num);
> +			if (elem[count].out_sg[0].iov_len != (size_t)hdrlen)
> +				debug("virtio-net transmit queue entry not aligned on hdrlen ([%d]: %d != %zu)",
> +				count, hdrlen, elem[count].out_sg[0].iov_len);
>  			tap_add_packet(vdev->context,
>  				       elem[count].out_sg[1].iov_len,
>  				       (char *)elem[count].out_sg[1].iov_base);

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

Re: [PATCH] vhost_user: remove ASSERT() on iovec number

[Stefano Brivio]

On Tue, 21 Jan 2025 14:20:28 +1030
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Mon, Jan 20, 2025 at 02:15:22PM +0100, Laurent Vivier wrote:
> > Replace ASSERT() on the number of iovec in the element and on
> > the first entry length by a debug() message.
> > 
> > Signed-off-by: Laurent Vivier <lvivier@redhat.com>  
> 
> Removing the ASSERT() makes sense, but is it safe to carry on to the
> tap_add_packet() if the packet is not in the layout we expect?

Safe for us and for TCP yes, but it's bad to mangle UDP packets like
that.

> Should we be bailing out of the function (effectively dropping the packet)
> instead?

I even proposed that in:

  https://archives.passt.top/passt-dev/20250115233302.23b24862@elisabeth/

and forgot about it during review. :(

This is applied now. Laurent, a follow-up patch would be appreciated.

-- 
Stefano