netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net v2 0/2] virtio-net: xsk: rx: fix the frame's length check
@ 2025-06-21 14:49 Bui Quang Minh
  2025-06-21 14:49 ` [PATCH net v2 1/2] " Bui Quang Minh
  2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
  0 siblings, 2 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-21 14:49 UTC (permalink / raw)
  To: netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization,
	linux-kernel, bpf, Bui Quang Minh

Hi everyone,

This series contains 2 patches for the zerocopy XDP receive path in virtio
net
- Patch 1: there is a difference between first buffer and the following
buffers in this receive path. While the first buffer contains virtio
header, the following ones do not. So the length of the remaining region
for frame data is also different in 2 cases. The current maximum frame's
length check is only correct for the following buffers not the first one.
- Patch 2: no functional change. The tricky xdp->data adjustment due to
the above difference is moved to buf_to_xdp() so that this helper contains
all logic to build xdp_buff and the tricky adjustment does not scatter
over different functions.

Version 2 changes:
- Patch 1: fix kdoc

Thanks,
Quang Minh.

Bui Quang Minh (2):
  virtio-net: xsk: rx: fix the frame's length check
  virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()

 drivers/net/virtio_net.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

-- 
2.43.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check
  2025-06-21 14:49 [PATCH net v2 0/2] virtio-net: xsk: rx: fix the frame's length check Bui Quang Minh
@ 2025-06-21 14:49 ` Bui Quang Minh
  2025-06-24  8:25   ` Xuan Zhuo
  2025-06-24 10:02   ` Paolo Abeni
  2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
  1 sibling, 2 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-21 14:49 UTC (permalink / raw)
  To: netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization,
	linux-kernel, bpf, Bui Quang Minh

When calling buf_to_xdp, the len argument is the frame data's length
without virtio header's length (vi->hdr_len). We check that len with

	xsk_pool_get_rx_frame_size() + vi->hdr_len

to ensure the provided len does not larger than the allocated chunk
size. The additional vi->hdr_len is because in virtnet_add_recvbuf_xsk,
we use part of XDP_PACKET_HEADROOM for virtio header and ask the vhost
to start placing data from

	hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
not
	hard_start + XDP_PACKET_HEADROOM

But the first buffer has virtio_header, so the maximum frame's length in
the first buffer can only be

	xsk_pool_get_rx_frame_size()
not
	xsk_pool_get_rx_frame_size() + vi->hdr_len

like in the current check.

This commit adds an additional argument to buf_to_xdp differentiate
between the first buffer and other ones to correctly calculate the maximum
frame's length.

Fixes: a4e7ba702701 ("virtio_net: xsk: rx: support recv small mode")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/virtio_net.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e53ba600605a..1eb237cd5d0b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1127,15 +1127,29 @@ static void check_sq_full_and_disable(struct virtnet_info *vi,
 	}
 }
 
+/* Note that @len is the length of received data without virtio header */
 static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
-				   struct receive_queue *rq, void *buf, u32 len)
+				   struct receive_queue *rq, void *buf,
+				   u32 len, bool first_buf)
 {
 	struct xdp_buff *xdp;
 	u32 bufsize;
 
 	xdp = (struct xdp_buff *)buf;
 
-	bufsize = xsk_pool_get_rx_frame_size(rq->xsk_pool) + vi->hdr_len;
+	/* In virtnet_add_recvbuf_xsk, we use part of XDP_PACKET_HEADROOM for
+	 * virtio header and ask the vhost to fill data from
+	 *         hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
+	 * The first buffer has virtio header so the remaining region for frame
+	 * data is
+	 *         xsk_pool_get_rx_frame_size()
+	 * While other buffers than the first one do not have virtio header, so
+	 * the maximum frame data's length can be
+	 *         xsk_pool_get_rx_frame_size() + vi->hdr_len
+	 */
+	bufsize = xsk_pool_get_rx_frame_size(rq->xsk_pool);
+	if (!first_buf)
+		bufsize += vi->hdr_len;
 
 	if (unlikely(len > bufsize)) {
 		pr_debug("%s: rx error: len %u exceeds truesize %u\n",
@@ -1260,7 +1274,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
 
 		u64_stats_add(&stats->bytes, len);
 
-		xdp = buf_to_xdp(vi, rq, buf, len);
+		xdp = buf_to_xdp(vi, rq, buf, len, false);
 		if (!xdp)
 			goto err;
 
@@ -1358,7 +1372,7 @@ static void virtnet_receive_xsk_buf(struct virtnet_info *vi, struct receive_queu
 
 	u64_stats_add(&stats->bytes, len);
 
-	xdp = buf_to_xdp(vi, rq, buf, len);
+	xdp = buf_to_xdp(vi, rq, buf, len, true);
 	if (!xdp)
 		return;
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-21 14:49 [PATCH net v2 0/2] virtio-net: xsk: rx: fix the frame's length check Bui Quang Minh
  2025-06-21 14:49 ` [PATCH net v2 1/2] " Bui Quang Minh
@ 2025-06-21 14:49 ` Bui Quang Minh
  2025-06-24  8:26   ` Xuan Zhuo
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-21 14:49 UTC (permalink / raw)
  To: netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, virtualization,
	linux-kernel, bpf, Bui Quang Minh

This commit does not do any functional changes. It moves xdp->data
adjustment for buffer other than first buffer to buf_to_xdp() helper so
that the xdp_buff adjustment does not scatter over different functions.

Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/virtio_net.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1eb237cd5d0b..4e942ea1bfa3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
 		return NULL;
 	}
 
-	xsk_buff_set_size(xdp, len);
+	if (first_buf) {
+		xsk_buff_set_size(xdp, len);
+	} else {
+		/* This is the same as xsk_buff_set_size but with the adjusted
+		 * xdp->data.
+		 */
+		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
+		xdp->data -= vi->hdr_len;
+		xdp->data_meta = xdp->data;
+		xdp->data_end = xdp->data + len;
+		xdp->flags = 0;
+	}
+
 	xsk_buff_dma_sync_for_cpu(xdp);
 
 	return xdp;
@@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
 			goto err;
 		}
 
-		memcpy(buf, xdp->data - vi->hdr_len, len);
+		memcpy(buf, xdp->data, len);
 
 		xsk_buff_free(xdp);
 
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check
  2025-06-21 14:49 ` [PATCH net v2 1/2] " Bui Quang Minh
@ 2025-06-24  8:25   ` Xuan Zhuo
  2025-06-24 10:02   ` Paolo Abeni
  1 sibling, 0 replies; 11+ messages in thread
From: Xuan Zhuo @ 2025-06-24  8:25 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf, Bui Quang Minh,
	netdev

On Sat, 21 Jun 2025 21:49:51 +0700, Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> When calling buf_to_xdp, the len argument is the frame data's length
> without virtio header's length (vi->hdr_len). We check that len with
>
> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
>
> to ensure the provided len does not larger than the allocated chunk
> size. The additional vi->hdr_len is because in virtnet_add_recvbuf_xsk,
> we use part of XDP_PACKET_HEADROOM for virtio header and ask the vhost
> to start placing data from
>
> 	hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
> not
> 	hard_start + XDP_PACKET_HEADROOM
>
> But the first buffer has virtio_header, so the maximum frame's length in
> the first buffer can only be
>
> 	xsk_pool_get_rx_frame_size()
> not
> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
>
> like in the current check.
>
> This commit adds an additional argument to buf_to_xdp differentiate
> between the first buffer and other ones to correctly calculate the maximum
> frame's length.
>
> Fixes: a4e7ba702701 ("virtio_net: xsk: rx: support recv small mode")
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

> ---
>  drivers/net/virtio_net.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e53ba600605a..1eb237cd5d0b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1127,15 +1127,29 @@ static void check_sq_full_and_disable(struct virtnet_info *vi,
>  	}
>  }
>
> +/* Note that @len is the length of received data without virtio header */
>  static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
> -				   struct receive_queue *rq, void *buf, u32 len)
> +				   struct receive_queue *rq, void *buf,
> +				   u32 len, bool first_buf)
>  {
>  	struct xdp_buff *xdp;
>  	u32 bufsize;
>
>  	xdp = (struct xdp_buff *)buf;
>
> -	bufsize = xsk_pool_get_rx_frame_size(rq->xsk_pool) + vi->hdr_len;
> +	/* In virtnet_add_recvbuf_xsk, we use part of XDP_PACKET_HEADROOM for
> +	 * virtio header and ask the vhost to fill data from
> +	 *         hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
> +	 * The first buffer has virtio header so the remaining region for frame
> +	 * data is
> +	 *         xsk_pool_get_rx_frame_size()
> +	 * While other buffers than the first one do not have virtio header, so
> +	 * the maximum frame data's length can be
> +	 *         xsk_pool_get_rx_frame_size() + vi->hdr_len
> +	 */
> +	bufsize = xsk_pool_get_rx_frame_size(rq->xsk_pool);
> +	if (!first_buf)
> +		bufsize += vi->hdr_len;
>
>  	if (unlikely(len > bufsize)) {
>  		pr_debug("%s: rx error: len %u exceeds truesize %u\n",
> @@ -1260,7 +1274,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>
>  		u64_stats_add(&stats->bytes, len);
>
> -		xdp = buf_to_xdp(vi, rq, buf, len);
> +		xdp = buf_to_xdp(vi, rq, buf, len, false);
>  		if (!xdp)
>  			goto err;
>
> @@ -1358,7 +1372,7 @@ static void virtnet_receive_xsk_buf(struct virtnet_info *vi, struct receive_queu
>
>  	u64_stats_add(&stats->bytes, len);
>
> -	xdp = buf_to_xdp(vi, rq, buf, len);
> +	xdp = buf_to_xdp(vi, rq, buf, len, true);
>  	if (!xdp)
>  		return;
>
> --
> 2.43.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
@ 2025-06-24  8:26   ` Xuan Zhuo
  2025-06-24  8:27   ` Xuan Zhuo
  2025-06-24 10:17   ` Paolo Abeni
  2 siblings, 0 replies; 11+ messages in thread
From: Xuan Zhuo @ 2025-06-24  8:26 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf, Bui Quang Minh,
	netdev

On Sat, 21 Jun 2025 21:49:52 +0700, Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> This commit does not do any functional changes. It moves xdp->data
> adjustment for buffer other than first buffer to buf_to_xdp() helper so
> that the xdp_buff adjustment does not scatter over different functions.
>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>



> ---
>  drivers/net/virtio_net.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1eb237cd5d0b..4e942ea1bfa3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
>  		return NULL;
>  	}
>
> -	xsk_buff_set_size(xdp, len);
> +	if (first_buf) {
> +		xsk_buff_set_size(xdp, len);
> +	} else {
> +		/* This is the same as xsk_buff_set_size but with the adjusted
> +		 * xdp->data.
> +		 */
> +		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> +		xdp->data -= vi->hdr_len;
> +		xdp->data_meta = xdp->data;
> +		xdp->data_end = xdp->data + len;
> +		xdp->flags = 0;
> +	}
> +
>  	xsk_buff_dma_sync_for_cpu(xdp);
>
>  	return xdp;
> @@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>  			goto err;
>  		}
>
> -		memcpy(buf, xdp->data - vi->hdr_len, len);
> +		memcpy(buf, xdp->data, len);
>
>  		xsk_buff_free(xdp);
>
> --
> 2.43.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
  2025-06-24  8:26   ` Xuan Zhuo
@ 2025-06-24  8:27   ` Xuan Zhuo
  2025-06-24 10:17   ` Paolo Abeni
  2 siblings, 0 replies; 11+ messages in thread
From: Xuan Zhuo @ 2025-06-24  8:27 UTC (permalink / raw)
  To: Bui Quang Minh
  Cc: Michael S. Tsirkin, Jason Wang, Eugenio Pérez, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf, Bui Quang Minh,
	netdev

On Sat, 21 Jun 2025 21:49:52 +0700, Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> This commit does not do any functional changes. It moves xdp->data
> adjustment for buffer other than first buffer to buf_to_xdp() helper so
> that the xdp_buff adjustment does not scatter over different functions.
>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

> ---
>  drivers/net/virtio_net.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1eb237cd5d0b..4e942ea1bfa3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
>  		return NULL;
>  	}
>
> -	xsk_buff_set_size(xdp, len);
> +	if (first_buf) {
> +		xsk_buff_set_size(xdp, len);
> +	} else {
> +		/* This is the same as xsk_buff_set_size but with the adjusted
> +		 * xdp->data.
> +		 */
> +		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> +		xdp->data -= vi->hdr_len;
> +		xdp->data_meta = xdp->data;
> +		xdp->data_end = xdp->data + len;
> +		xdp->flags = 0;
> +	}
> +
>  	xsk_buff_dma_sync_for_cpu(xdp);
>
>  	return xdp;
> @@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>  			goto err;
>  		}
>
> -		memcpy(buf, xdp->data - vi->hdr_len, len);
> +		memcpy(buf, xdp->data, len);
>
>  		xsk_buff_free(xdp);
>
> --
> 2.43.0
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check
  2025-06-21 14:49 ` [PATCH net v2 1/2] " Bui Quang Minh
  2025-06-24  8:25   ` Xuan Zhuo
@ 2025-06-24 10:02   ` Paolo Abeni
  2025-06-24 14:36     ` Bui Quang Minh
  1 sibling, 1 reply; 11+ messages in thread
From: Paolo Abeni @ 2025-06-24 10:02 UTC (permalink / raw)
  To: Bui Quang Minh, netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf

On 6/21/25 4:49 PM, Bui Quang Minh wrote:
> When calling buf_to_xdp, the len argument is the frame data's length
> without virtio header's length (vi->hdr_len). We check that len with
> 
> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
> 
> to ensure the provided len does not larger than the allocated chunk
> size. The additional vi->hdr_len is because in virtnet_add_recvbuf_xsk,
> we use part of XDP_PACKET_HEADROOM for virtio header and ask the vhost
> to start placing data from
> 
> 	hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
> not
> 	hard_start + XDP_PACKET_HEADROOM
> 
> But the first buffer has virtio_header, so the maximum frame's length in
> the first buffer can only be
> 
> 	xsk_pool_get_rx_frame_size()
> not
> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
> 
> like in the current check.
> 
> This commit adds an additional argument to buf_to_xdp differentiate
> between the first buffer and other ones to correctly calculate the maximum
> frame's length.
> 
> Fixes: a4e7ba702701 ("virtio_net: xsk: rx: support recv small mode")

It looks like the checks in the blamed commit above are correct and the
bug has been added with commit 99c861b44eb1f ("virtio_net: xsk: rx:
support recv merge mode")???

/P


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
  2025-06-24  8:26   ` Xuan Zhuo
  2025-06-24  8:27   ` Xuan Zhuo
@ 2025-06-24 10:17   ` Paolo Abeni
  2025-06-24 14:58     ` Bui Quang Minh
  2025-06-29  5:31     ` Bui Quang Minh
  2 siblings, 2 replies; 11+ messages in thread
From: Paolo Abeni @ 2025-06-24 10:17 UTC (permalink / raw)
  To: Bui Quang Minh, netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf

On 6/21/25 4:49 PM, Bui Quang Minh wrote:
> This commit does not do any functional changes. It moves xdp->data
> adjustment for buffer other than first buffer to buf_to_xdp() helper so
> that the xdp_buff adjustment does not scatter over different functions.
> 
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/net/virtio_net.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1eb237cd5d0b..4e942ea1bfa3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
>  		return NULL;
>  	}
>  
> -	xsk_buff_set_size(xdp, len);
> +	if (first_buf) {
> +		xsk_buff_set_size(xdp, len);
> +	} else {
> +		/* This is the same as xsk_buff_set_size but with the adjusted
> +		 * xdp->data.
> +		 */
> +		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
> +		xdp->data -= vi->hdr_len;
> +		xdp->data_meta = xdp->data;
> +		xdp->data_end = xdp->data + len;
> +		xdp->flags = 0;
> +	}
> +
>  	xsk_buff_dma_sync_for_cpu(xdp);
>  
>  	return xdp;
> @@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>  			goto err;
>  		}
>  
> -		memcpy(buf, xdp->data - vi->hdr_len, len);
> +		memcpy(buf, xdp->data, len);
>  
>  		xsk_buff_free(xdp);
>  

I'm unsure if this change is in the right direction because it almost
open-code the existing xsk_buff_set_size() helper - any changes there
should be reflected here, too.

Also AFAICS xdp->data will now carry a different value, and I guess such
change is user-visible from the attached xdp program. The commit message
should at least mentions such fact.

Thanks,

Paolo


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check
  2025-06-24 10:02   ` Paolo Abeni
@ 2025-06-24 14:36     ` Bui Quang Minh
  0 siblings, 0 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-24 14:36 UTC (permalink / raw)
  To: Paolo Abeni, netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf

On 6/24/25 17:02, Paolo Abeni wrote:
> On 6/21/25 4:49 PM, Bui Quang Minh wrote:
>> When calling buf_to_xdp, the len argument is the frame data's length
>> without virtio header's length (vi->hdr_len). We check that len with
>>
>> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
>>
>> to ensure the provided len does not larger than the allocated chunk
>> size. The additional vi->hdr_len is because in virtnet_add_recvbuf_xsk,
>> we use part of XDP_PACKET_HEADROOM for virtio header and ask the vhost
>> to start placing data from
>>
>> 	hard_start + XDP_PACKET_HEADROOM - vi->hdr_len
>> not
>> 	hard_start + XDP_PACKET_HEADROOM
>>
>> But the first buffer has virtio_header, so the maximum frame's length in
>> the first buffer can only be
>>
>> 	xsk_pool_get_rx_frame_size()
>> not
>> 	xsk_pool_get_rx_frame_size() + vi->hdr_len
>>
>> like in the current check.
>>
>> This commit adds an additional argument to buf_to_xdp differentiate
>> between the first buffer and other ones to correctly calculate the maximum
>> frame's length.
>>
>> Fixes: a4e7ba702701 ("virtio_net: xsk: rx: support recv small mode")
> It looks like the checks in the blamed commit above are correct and the
> bug has been added with commit 99c861b44eb1f ("virtio_net: xsk: rx:
> support recv merge mode")???

AFAICS, the small mode has only 1 buffer per frame and that buffer is 
quite the same as first buffer in mergeable mode. That buffer still has 
virtio header (though it's smaller than in mergeable case), so the 
remaining space for data is only xsk_pool_get_rx_frame_size() not 
xsk_pool_get_rx_frame_size() + vi->hdr_len.

Thanks,
Quang Minh.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-24 10:17   ` Paolo Abeni
@ 2025-06-24 14:58     ` Bui Quang Minh
  2025-06-29  5:31     ` Bui Quang Minh
  1 sibling, 0 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-24 14:58 UTC (permalink / raw)
  To: Paolo Abeni, netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf

On 6/24/25 17:17, Paolo Abeni wrote:
> On 6/21/25 4:49 PM, Bui Quang Minh wrote:
>> This commit does not do any functional changes. It moves xdp->data
>> adjustment for buffer other than first buffer to buf_to_xdp() helper so
>> that the xdp_buff adjustment does not scatter over different functions.
>>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   drivers/net/virtio_net.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1eb237cd5d0b..4e942ea1bfa3 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
>>   		return NULL;
>>   	}
>>   
>> -	xsk_buff_set_size(xdp, len);
>> +	if (first_buf) {
>> +		xsk_buff_set_size(xdp, len);
>> +	} else {
>> +		/* This is the same as xsk_buff_set_size but with the adjusted
>> +		 * xdp->data.
>> +		 */
>> +		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
>> +		xdp->data -= vi->hdr_len;
>> +		xdp->data_meta = xdp->data;
>> +		xdp->data_end = xdp->data + len;
>> +		xdp->flags = 0;
>> +	}
>> +
>>   	xsk_buff_dma_sync_for_cpu(xdp);
>>   
>>   	return xdp;
>> @@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>>   			goto err;
>>   		}
>>   
>> -		memcpy(buf, xdp->data - vi->hdr_len, len);
>> +		memcpy(buf, xdp->data, len);
>>   
>>   		xsk_buff_free(xdp);
>>   
> I'm unsure if this change is in the right direction because it almost
> open-code the existing xsk_buff_set_size() helper - any changes there
> should be reflected here, too.
>
> Also AFAICS xdp->data will now carry a different value, and I guess such
> change is user-visible from the attached xdp program. The commit message
> should at least mentions such fact.

The else path in buf_to_xdp is only triggered when it's called in 
xsk_append_merge_buffer. That's also the reason of the memcpy line 
change in xsk_append_merge_buffer. xsk_append_merge_buffer will allocate 
frag and copy xdp_buff's data to that frag. Previously, after 
buf_to_xdp, the xdp->data does not point to correct place yet so when 
memcpy, we need to copy from xdp->data - vi->hdr_len. With this change, 
we will adjust xdp->data = xdp->data - vi->hdr_len right in buf_to_xdp 
and also adjust other fields (xdp->data_end) to the correct place too. 
So I don't think there is a functional change in this commit.

Thanks,
Quang Minh.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp()
  2025-06-24 10:17   ` Paolo Abeni
  2025-06-24 14:58     ` Bui Quang Minh
@ 2025-06-29  5:31     ` Bui Quang Minh
  1 sibling, 0 replies; 11+ messages in thread
From: Bui Quang Minh @ 2025-06-29  5:31 UTC (permalink / raw)
  To: Paolo Abeni, netdev
  Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Jesper Dangaard Brouer,
	John Fastabend, virtualization, linux-kernel, bpf

On 6/24/25 17:17, Paolo Abeni wrote:
> On 6/21/25 4:49 PM, Bui Quang Minh wrote:
>> This commit does not do any functional changes. It moves xdp->data
>> adjustment for buffer other than first buffer to buf_to_xdp() helper so
>> that the xdp_buff adjustment does not scatter over different functions.
>>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   drivers/net/virtio_net.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1eb237cd5d0b..4e942ea1bfa3 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1159,7 +1159,19 @@ static struct xdp_buff *buf_to_xdp(struct virtnet_info *vi,
>>   		return NULL;
>>   	}
>>   
>> -	xsk_buff_set_size(xdp, len);
>> +	if (first_buf) {
>> +		xsk_buff_set_size(xdp, len);
>> +	} else {
>> +		/* This is the same as xsk_buff_set_size but with the adjusted
>> +		 * xdp->data.
>> +		 */
>> +		xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
>> +		xdp->data -= vi->hdr_len;
>> +		xdp->data_meta = xdp->data;
>> +		xdp->data_end = xdp->data + len;
>> +		xdp->flags = 0;
>> +	}
>> +
>>   	xsk_buff_dma_sync_for_cpu(xdp);
>>   
>>   	return xdp;
>> @@ -1284,7 +1296,7 @@ static int xsk_append_merge_buffer(struct virtnet_info *vi,
>>   			goto err;
>>   		}
>>   
>> -		memcpy(buf, xdp->data - vi->hdr_len, len);
>> +		memcpy(buf, xdp->data, len);
>>   
>>   		xsk_buff_free(xdp);
>>   
> I'm unsure if this change is in the right direction because it almost
> open-code the existing xsk_buff_set_size() helper - any changes there
> should be reflected here, too.

I've found out that there is xdp_prepare_buff helper which gives more 
control over the headroom so it can be used here. I'll update in the 
next version.

Thanks,
Quang Minh.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-29  5:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-21 14:49 [PATCH net v2 0/2] virtio-net: xsk: rx: fix the frame's length check Bui Quang Minh
2025-06-21 14:49 ` [PATCH net v2 1/2] " Bui Quang Minh
2025-06-24  8:25   ` Xuan Zhuo
2025-06-24 10:02   ` Paolo Abeni
2025-06-24 14:36     ` Bui Quang Minh
2025-06-21 14:49 ` [PATCH net v2 2/2] virtio-net: xsk: rx: move the xdp->data adjustment to buf_to_xdp() Bui Quang Minh
2025-06-24  8:26   ` Xuan Zhuo
2025-06-24  8:27   ` Xuan Zhuo
2025-06-24 10:17   ` Paolo Abeni
2025-06-24 14:58     ` Bui Quang Minh
2025-06-29  5:31     ` Bui Quang Minh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).