[v2] virtio-net: xsk: rx: fix the frame's length check

[PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check

Posted by Bui Quang Minh 3 months, 2 weeks ago

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

Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check

Posted by Paolo Abeni 3 months, 2 weeks ago

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

Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check

Posted by Bui Quang Minh 3 months, 2 weeks ago

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.

Re: [PATCH net v2 1/2] virtio-net: xsk: rx: fix the frame's length check

Posted by Xuan Zhuo 3 months, 2 weeks ago

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
>