[PATCH net 0/4] virtio-net: fixes for mergeable XDP receive path

[PATCH net 0/4] virtio-net: fixes for mergeable XDP receive path

[Bui Quang Minh]

Hi everyone,

This series contains fixes for XDP receive path in virtio-net
- Patch 1: add a missing check for the received data length with our
allocated buffer size in mergeable mode
- Patch 2: remove a redundant truesize check with PAGE_SIZE in mergeable
mode
- Patch 3: add a helper for mergeable received data length check to avoid
repeated code
- Patch 4: fix the flaky drivers/net/ping.py selftest due to frame drop
when the receive buffer is prefilled before XDP is set but is used after
XDP is set. It's because of current restriction that
headroom + frame's length + tailroom < PAGE_SIZE. XDP does not have this
restriction, so we can lift the restriction here and continue processing
the frame.

Thanks,
Quang Minh.

Bui Quang Minh (4):
  virtio-net: ensure the received length does not exceed allocated size
  virtio-net: remove redundant truesize check with PAGE_SIZE
  virtio-net: create a helper to check received mergeable buffer's
    length
  virtio-net: allow more allocated space for mergeable XDP

 drivers/net/virtio_net.c | 91 ++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 37 deletions(-)

-- 
2.43.0

[PATCH net 1/4] virtio-net: ensure the received length does not exceed allocated size

[Bui Quang Minh]

In xdp_linearize_page, when reading the following buffers from the ring,
we forget to check the received length with the true allocate size. This
can lead to an out-of-bound read. This commit adds that missing check.

Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
---
 drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e53ba600605a..2a130a3e50ac 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
  * across multiple buffers (num_buf > 1), and we make sure buffers
  * have enough headroom.
  */
-static struct page *xdp_linearize_page(struct receive_queue *rq,
+static struct page *xdp_linearize_page(struct net_device *dev,
+				       struct receive_queue *rq,
 				       int *num_buf,
 				       struct page *p,
 				       int offset,
@@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
 	page_off += *len;
 
 	while (--*num_buf) {
-		unsigned int buflen;
+		unsigned int headroom, tailroom, room;
+		unsigned int truesize, buflen;
 		void *buf;
+		void *ctx;
 		int off;
 
-		buf = virtnet_rq_get_buf(rq, &buflen, NULL);
+		buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
 		if (unlikely(!buf))
 			goto err_buf;
 
 		p = virt_to_head_page(buf);
 		off = buf - page_address(p);
 
+		truesize = mergeable_ctx_to_truesize(ctx);
+		headroom = mergeable_ctx_to_headroom(ctx);
+		tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
+		room = SKB_DATA_ALIGN(headroom + tailroom);
+
+		if (unlikely(buflen > truesize - room)) {
+			put_page(p);
+			pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
+				 dev->name, buflen,
+				 (unsigned long)(truesize - room));
+			DEV_STATS_INC(dev, rx_length_errors);
+			goto err_buf;
+		}
+
 		/* guard against a misconfigured or uncooperative backend that
 		 * is sending packet larger than the MTU.
 		 */
@@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
 		headroom = vi->hdr_len + header_offset;
 		buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
 			SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
-		xdp_page = xdp_linearize_page(rq, &num_buf, page,
+		xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
 					      offset, header_offset,
 					      &tlen);
 		if (!xdp_page)
@@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
 	 */
 	if (!xdp_prog->aux->xdp_has_frags) {
 		/* linearize data for XDP */
-		xdp_page = xdp_linearize_page(rq, num_buf,
+		xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
 					      *page, offset,
 					      XDP_PACKET_HEADROOM,
 					      len);
-- 
2.43.0

[PATCH net 2/4] virtio-net: remove redundant truesize check with PAGE_SIZE

[Bui Quang Minh]

The truesize is guaranteed not to exceed PAGE_SIZE in
get_mergeable_buf_len(). It is saved in mergeable context, which is not
changeable by the host side, so the check in receive path is quite
redundant.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 2a130a3e50ac..6f9fedad4a5e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2144,9 +2144,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 {
 	struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
 	unsigned int headroom, tailroom, room;
-	unsigned int truesize, cur_frag_size;
 	struct skb_shared_info *shinfo;
 	unsigned int xdp_frags_truesz = 0;
+	unsigned int truesize;
 	struct page *page;
 	skb_frag_t *frag;
 	int offset;
@@ -2194,9 +2194,8 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 		tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
 		room = SKB_DATA_ALIGN(headroom + tailroom);
 
-		cur_frag_size = truesize;
-		xdp_frags_truesz += cur_frag_size;
-		if (unlikely(len > truesize - room || cur_frag_size > PAGE_SIZE)) {
+		xdp_frags_truesz += truesize;
+		if (unlikely(len > truesize - room)) {
 			put_page(page);
 			pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
 				 dev->name, len, (unsigned long)(truesize - room));
-- 
2.43.0

[PATCH net 3/4] virtio-net: create a helper to check received mergeable buffer's length

[Bui Quang Minh]

Currently, we have repeated code to check the received mergeable buffer's
length with allocated size. This commit creates a helper to do that and
converts current code to use it.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6f9fedad4a5e..844cb2a78be0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -778,6 +778,26 @@ static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
 	return (unsigned long)mrg_ctx & ((1 << MRG_CTX_HEADER_SHIFT) - 1);
 }
 
+static int check_mergeable_len(struct net_device *dev, void *mrg_ctx,
+			       unsigned int len)
+{
+	unsigned int headroom, tailroom, room, truesize;
+
+	truesize = mergeable_ctx_to_truesize(mrg_ctx);
+	headroom = mergeable_ctx_to_headroom(mrg_ctx);
+	tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
+	room = SKB_DATA_ALIGN(headroom + tailroom);
+
+	if (len > truesize - room) {
+		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
+			 dev->name, len, (unsigned long)(truesize - room));
+		DEV_STATS_INC(dev, rx_length_errors);
+		return -1;
+	}
+
+	return 0;
+}
+
 static struct sk_buff *virtnet_build_skb(void *buf, unsigned int buflen,
 					 unsigned int headroom,
 					 unsigned int len)
@@ -1819,8 +1839,7 @@ static struct page *xdp_linearize_page(struct net_device *dev,
 	page_off += *len;
 
 	while (--*num_buf) {
-		unsigned int headroom, tailroom, room;
-		unsigned int truesize, buflen;
+		unsigned int buflen;
 		void *buf;
 		void *ctx;
 		int off;
@@ -1832,17 +1851,8 @@ static struct page *xdp_linearize_page(struct net_device *dev,
 		p = virt_to_head_page(buf);
 		off = buf - page_address(p);
 
-		truesize = mergeable_ctx_to_truesize(ctx);
-		headroom = mergeable_ctx_to_headroom(ctx);
-		tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
-		room = SKB_DATA_ALIGN(headroom + tailroom);
-
-		if (unlikely(buflen > truesize - room)) {
+		if (check_mergeable_len(dev, ctx, buflen)) {
 			put_page(p);
-			pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-				 dev->name, buflen,
-				 (unsigned long)(truesize - room));
-			DEV_STATS_INC(dev, rx_length_errors);
 			goto err_buf;
 		}
 
@@ -2143,7 +2153,6 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 				      struct virtnet_rq_stats *stats)
 {
 	struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
-	unsigned int headroom, tailroom, room;
 	struct skb_shared_info *shinfo;
 	unsigned int xdp_frags_truesz = 0;
 	unsigned int truesize;
@@ -2189,20 +2198,14 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
 		page = virt_to_head_page(buf);
 		offset = buf - page_address(page);
 
-		truesize = mergeable_ctx_to_truesize(ctx);
-		headroom = mergeable_ctx_to_headroom(ctx);
-		tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
-		room = SKB_DATA_ALIGN(headroom + tailroom);
-
-		xdp_frags_truesz += truesize;
-		if (unlikely(len > truesize - room)) {
+		if (check_mergeable_len(dev, ctx, len)) {
 			put_page(page);
-			pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-				 dev->name, len, (unsigned long)(truesize - room));
-			DEV_STATS_INC(dev, rx_length_errors);
 			goto err;
 		}
 
+		truesize = mergeable_ctx_to_truesize(ctx);
+		xdp_frags_truesz += truesize;
+
 		frag = &shinfo->frags[shinfo->nr_frags++];
 		skb_frag_fill_page_desc(frag, page, offset, len);
 		if (page_is_pfmemalloc(page))
@@ -2416,18 +2419,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 	struct sk_buff *head_skb, *curr_skb;
 	unsigned int truesize = mergeable_ctx_to_truesize(ctx);
 	unsigned int headroom = mergeable_ctx_to_headroom(ctx);
-	unsigned int tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
-	unsigned int room = SKB_DATA_ALIGN(headroom + tailroom);
 
 	head_skb = NULL;
 	u64_stats_add(&stats->bytes, len - vi->hdr_len);
 
-	if (unlikely(len > truesize - room)) {
-		pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-			 dev->name, len, (unsigned long)(truesize - room));
-		DEV_STATS_INC(dev, rx_length_errors);
+	if (check_mergeable_len(dev, ctx, len))
 		goto err_skb;
-	}
 
 	if (unlikely(vi->xdp_enabled)) {
 		struct bpf_prog *xdp_prog;
@@ -2462,17 +2459,10 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
 		u64_stats_add(&stats->bytes, len);
 		page = virt_to_head_page(buf);
 
-		truesize = mergeable_ctx_to_truesize(ctx);
-		headroom = mergeable_ctx_to_headroom(ctx);
-		tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
-		room = SKB_DATA_ALIGN(headroom + tailroom);
-		if (unlikely(len > truesize - room)) {
-			pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
-				 dev->name, len, (unsigned long)(truesize - room));
-			DEV_STATS_INC(dev, rx_length_errors);
+		if (check_mergeable_len(dev, ctx, len))
 			goto err_skb;
-		}
 
+		truesize = mergeable_ctx_to_truesize(ctx);
 		curr_skb  = virtnet_skb_append_frag(head_skb, curr_skb, page,
 						    buf, len, truesize);
 		if (!curr_skb)
-- 
2.43.0

[PATCH net 4/4] virtio-net: allow more allocated space for mergeable XDP

[Bui Quang Minh]

When the mergeable receive buffer is prefilled before XDP is set, it
does not reserve the space for XDP_PACKET_HEADROOM and skb_shared_info.
So when XDP is set and this buffer is used to receive frame, we need to
create a new buffer with reserved headroom, tailroom and copy the frame
data over. Currently, the new buffer's size is restricted to PAGE_SIZE
only. If the frame data's length + headroom + tailroom exceeds
PAGE_SIZE, the frame is dropped.

However, it seems like there is no restriction on the total size in XDP.
So we can just increase the size of new buffer to 2 * PAGE_SIZE in that
case and continue to process the frame.

In my opinion, the current drop behavior is fine and expected so this
commit is just an improvement not a bug fix.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 844cb2a78be0..663cec686045 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2277,13 +2277,26 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
 					      len);
 		if (!xdp_page)
 			return NULL;
+
+		*frame_sz = PAGE_SIZE;
 	} else {
+		unsigned int total_len;
+
 		xdp_room = SKB_DATA_ALIGN(XDP_PACKET_HEADROOM +
 					  sizeof(struct skb_shared_info));
-		if (*len + xdp_room > PAGE_SIZE)
+		total_len = *len + xdp_room;
+
+		/* This must never happen because len cannot exceed PAGE_SIZE */
+		if (unlikely(total_len > 2 * PAGE_SIZE))
 			return NULL;
 
-		xdp_page = alloc_page(GFP_ATOMIC);
+		if (total_len > PAGE_SIZE) {
+			xdp_page = alloc_pages(GFP_ATOMIC, 1);
+			*frame_sz = 2 * PAGE_SIZE;
+		} else {
+			xdp_page = alloc_page(GFP_ATOMIC);
+			*frame_sz = PAGE_SIZE;
+		}
 		if (!xdp_page)
 			return NULL;
 
@@ -2291,8 +2304,6 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
 		       page_address(*page) + offset, *len);
 	}
 
-	*frame_sz = PAGE_SIZE;
-
 	put_page(*page);
 
 	*page = xdp_page;
-- 
2.43.0

Re: [PATCH net 1/4] virtio-net: ensure the received length does not exceed allocated size

[Jason Wang]

On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> In xdp_linearize_page, when reading the following buffers from the ring,
> we forget to check the received length with the true allocate size. This
> can lead to an out-of-bound read. This commit adds that missing check.
>
> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")

I think we should cc stable.

> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e53ba600605a..2a130a3e50ac 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>   * across multiple buffers (num_buf > 1), and we make sure buffers
>   * have enough headroom.
>   */
> -static struct page *xdp_linearize_page(struct receive_queue *rq,
> +static struct page *xdp_linearize_page(struct net_device *dev,
> +                                      struct receive_queue *rq,
>                                        int *num_buf,
>                                        struct page *p,
>                                        int offset,
> @@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
>         page_off += *len;
>
>         while (--*num_buf) {
> -               unsigned int buflen;
> +               unsigned int headroom, tailroom, room;
> +               unsigned int truesize, buflen;
>                 void *buf;
> +               void *ctx;
>                 int off;
>
> -               buf = virtnet_rq_get_buf(rq, &buflen, NULL);
> +               buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
>                 if (unlikely(!buf))
>                         goto err_buf;
>
>                 p = virt_to_head_page(buf);
>                 off = buf - page_address(p);
>
> +               truesize = mergeable_ctx_to_truesize(ctx);

This won't work for receive_small_xdp().

> +               headroom = mergeable_ctx_to_headroom(ctx);
> +               tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
> +               room = SKB_DATA_ALIGN(headroom + tailroom);
> +
> +               if (unlikely(buflen > truesize - room)) {
> +                       put_page(p);
> +                       pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> +                                dev->name, buflen,
> +                                (unsigned long)(truesize - room));
> +                       DEV_STATS_INC(dev, rx_length_errors);
> +                       goto err_buf;
> +               }

I wonder if this issue only affect XDP should we check other places?

> +
>                 /* guard against a misconfigured or uncooperative backend that
>                  * is sending packet larger than the MTU.
>                  */
> @@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
>                 headroom = vi->hdr_len + header_offset;
>                 buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
>                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> -               xdp_page = xdp_linearize_page(rq, &num_buf, page,
> +               xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
>                                               offset, header_offset,
>                                               &tlen);
>                 if (!xdp_page)
> @@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
>          */
>         if (!xdp_prog->aux->xdp_has_frags) {
>                 /* linearize data for XDP */
> -               xdp_page = xdp_linearize_page(rq, num_buf,
> +               xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
>                                               *page, offset,
>                                               XDP_PACKET_HEADROOM,
>                                               len);
> --
> 2.43.0
>

Re: [PATCH net 2/4] virtio-net: remove redundant truesize check with PAGE_SIZE

[Jason Wang]

On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> The truesize is guaranteed not to exceed PAGE_SIZE in
> get_mergeable_buf_len(). It is saved in mergeable context, which is not
> changeable by the host side,

This really depends on the security model.

> so the check in receive path is quite
> redundant.
>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/net/virtio_net.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 2a130a3e50ac..6f9fedad4a5e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2144,9 +2144,9 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
>  {
>         struct virtio_net_hdr_mrg_rxbuf *hdr = buf;
>         unsigned int headroom, tailroom, room;
> -       unsigned int truesize, cur_frag_size;
>         struct skb_shared_info *shinfo;
>         unsigned int xdp_frags_truesz = 0;
> +       unsigned int truesize;
>         struct page *page;
>         skb_frag_t *frag;
>         int offset;
> @@ -2194,9 +2194,8 @@ static int virtnet_build_xdp_buff_mrg(struct net_device *dev,
>                 tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
>                 room = SKB_DATA_ALIGN(headroom + tailroom);
>
> -               cur_frag_size = truesize;
> -               xdp_frags_truesz += cur_frag_size;
> -               if (unlikely(len > truesize - room || cur_frag_size > PAGE_SIZE)) {
> +               xdp_frags_truesz += truesize;
> +               if (unlikely(len > truesize - room)) {
>                         put_page(page);
>                         pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>                                  dev->name, len, (unsigned long)(truesize - room));
> --
> 2.43.0
>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH net 3/4] virtio-net: create a helper to check received mergeable buffer's length

[Jason Wang]

On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> Currently, we have repeated code to check the received mergeable buffer's
> length with allocated size. This commit creates a helper to do that and
> converts current code to use it.
>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>

I think it would be better to introduce this as patch 1, so a
mergeable XDP path can use that directly.

This will have a smaller changeset.

Thanks

Re: [PATCH net 4/4] virtio-net: allow more allocated space for mergeable XDP

[Jason Wang]

On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> When the mergeable receive buffer is prefilled before XDP is set, it
> does not reserve the space for XDP_PACKET_HEADROOM and skb_shared_info.
> So when XDP is set and this buffer is used to receive frame, we need to
> create a new buffer with reserved headroom, tailroom and copy the frame
> data over. Currently, the new buffer's size is restricted to PAGE_SIZE
> only. If the frame data's length + headroom + tailroom exceeds
> PAGE_SIZE, the frame is dropped.
>
> However, it seems like there is no restriction on the total size in XDP.
> So we can just increase the size of new buffer to 2 * PAGE_SIZE in that
> case and continue to process the frame.
>
> In my opinion, the current drop behavior is fine and expected so this
> commit is just an improvement not a bug fix.

Then this should go for net-next.

>
> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> ---
>  drivers/net/virtio_net.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 844cb2a78be0..663cec686045 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2277,13 +2277,26 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
>                                               len);
>                 if (!xdp_page)
>                         return NULL;
> +
> +               *frame_sz = PAGE_SIZE;
>         } else {
> +               unsigned int total_len;
> +
>                 xdp_room = SKB_DATA_ALIGN(XDP_PACKET_HEADROOM +
>                                           sizeof(struct skb_shared_info));
> -               if (*len + xdp_room > PAGE_SIZE)
> +               total_len = *len + xdp_room;
> +
> +               /* This must never happen because len cannot exceed PAGE_SIZE */
> +               if (unlikely(total_len > 2 * PAGE_SIZE))
>                         return NULL;
>
> -               xdp_page = alloc_page(GFP_ATOMIC);
> +               if (total_len > PAGE_SIZE) {
> +                       xdp_page = alloc_pages(GFP_ATOMIC, 1);

I'm not sure it's worth optimizing the corner case here that may bring
burdens for maintenance.

And a good optimization here is to reduce the logic duplication by
reusing xdp_linearize_page().


> +                       *frame_sz = 2 * PAGE_SIZE;
> +               } else {
> +                       xdp_page = alloc_page(GFP_ATOMIC);
> +                       *frame_sz = PAGE_SIZE;
> +               }
>                 if (!xdp_page)
>                         return NULL;
>
> @@ -2291,8 +2304,6 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
>                        page_address(*page) + offset, *len);
>         }
>
> -       *frame_sz = PAGE_SIZE;
> -
>         put_page(*page);
>
>         *page = xdp_page;
> --

Thanks

> 2.43.0
>

Re: [PATCH net 1/4] virtio-net: ensure the received length does not exceed allocated size

[Bui Quang Minh]

On 6/26/25 09:34, Jason Wang wrote:
> On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> <minhquangbui99@gmail.com> wrote:
>> In xdp_linearize_page, when reading the following buffers from the ring,
>> we forget to check the received length with the true allocate size. This
>> can lead to an out-of-bound read. This commit adds that missing check.
>>
>> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> I think we should cc stable.

Okay, I'll do that in next version.

>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>> ---
>>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index e53ba600605a..2a130a3e50ac 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>>    * across multiple buffers (num_buf > 1), and we make sure buffers
>>    * have enough headroom.
>>    */
>> -static struct page *xdp_linearize_page(struct receive_queue *rq,
>> +static struct page *xdp_linearize_page(struct net_device *dev,
>> +                                      struct receive_queue *rq,
>>                                         int *num_buf,
>>                                         struct page *p,
>>                                         int offset,
>> @@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
>>          page_off += *len;
>>
>>          while (--*num_buf) {
>> -               unsigned int buflen;
>> +               unsigned int headroom, tailroom, room;
>> +               unsigned int truesize, buflen;
>>                  void *buf;
>> +               void *ctx;
>>                  int off;
>>
>> -               buf = virtnet_rq_get_buf(rq, &buflen, NULL);
>> +               buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
>>                  if (unlikely(!buf))
>>                          goto err_buf;
>>
>>                  p = virt_to_head_page(buf);
>>                  off = buf - page_address(p);
>>
>> +               truesize = mergeable_ctx_to_truesize(ctx);
> This won't work for receive_small_xdp().

If it is small mode, the num_buf == 1 and we don't get into the while loop.

>
>> +               headroom = mergeable_ctx_to_headroom(ctx);
>> +               tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
>> +               room = SKB_DATA_ALIGN(headroom + tailroom);
>> +
>> +               if (unlikely(buflen > truesize - room)) {
>> +                       put_page(p);
>> +                       pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>> +                                dev->name, buflen,
>> +                                (unsigned long)(truesize - room));
>> +                       DEV_STATS_INC(dev, rx_length_errors);
>> +                       goto err_buf;
>> +               }
> I wonder if this issue only affect XDP should we check other places?

In small mode, we check the len with GOOD_PACKET_LEN in receive_small. 
In mergeable mode, we have some checks over the place and this is the 
only one I see we miss. In xsk, we check inside buf_to_xdp. However, in 
the big mode, I feel like there is a bug.

In add_recvbuf_big, 1 first page + vi->big_packets_num_skbfrags pages. 
The pages are managed by a linked list. The vi->big_packets_num_skbfrags 
is set in virtnet_set_big_packets

     vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS : 
DIV_ROUND_UP(mtu, PAGE_SIZE);

So the vi->big_packets_num_skbfrags can be fewer than MAX_SKB_FRAGS.

In receive_big, we call to page_to_skb, there is a check

     if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
         /* error case */
     }

But because the number of allocated buffer is 
vi->big_packets_num_skbfrags + 1 and vi->big_packets_num_skbfrags can be 
fewer than MAX_SKB_FRAGS, the check seems not enough

     while (len) {
         unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
         skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
                 frag_size, truesize);
         len -= frag_size;
         page = (struct page *)page->private;
         offset = 0;
     }

In the following while loop, we keep running based on len without NULL 
check the pages linked list, so it may result into NULL pointer dereference.

What do you think?

Thanks,
Quang Minh.

>
>> +
>>                  /* guard against a misconfigured or uncooperative backend that
>>                   * is sending packet larger than the MTU.
>>                   */
>> @@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
>>                  headroom = vi->hdr_len + header_offset;
>>                  buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
>>                          SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> -               xdp_page = xdp_linearize_page(rq, &num_buf, page,
>> +               xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
>>                                                offset, header_offset,
>>                                                &tlen);
>>                  if (!xdp_page)
>> @@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
>>           */
>>          if (!xdp_prog->aux->xdp_has_frags) {
>>                  /* linearize data for XDP */
>> -               xdp_page = xdp_linearize_page(rq, num_buf,
>> +               xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
>>                                                *page, offset,
>>                                                XDP_PACKET_HEADROOM,
>>                                                len);
>> --
>> 2.43.0
>>

Re: [PATCH net 3/4] virtio-net: create a helper to check received mergeable buffer's length

[Bui Quang Minh]

On 6/26/25 09:38, Jason Wang wrote:
> On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> <minhquangbui99@gmail.com> wrote:
>> Currently, we have repeated code to check the received mergeable buffer's
>> length with allocated size. This commit creates a helper to do that and
>> converts current code to use it.
>>
>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> I think it would be better to introduce this as patch 1, so a
> mergeable XDP path can use that directly.
>
> This will have a smaller changeset.

I'm just concerned that it might make backporting the fix harder because 
the fix depends on this refactor and this refactor touches some function 
that may create conflict.

Thanks,
Quang Minh.

Re: [PATCH net 1/4] virtio-net: ensure the received length does not exceed allocated size

[Jason Wang]

On Thu, Jun 26, 2025 at 11:34 PM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> On 6/26/25 09:34, Jason Wang wrote:
> > On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> > <minhquangbui99@gmail.com> wrote:
> >> In xdp_linearize_page, when reading the following buffers from the ring,
> >> we forget to check the received length with the true allocate size. This
> >> can lead to an out-of-bound read. This commit adds that missing check.
> >>
> >> Fixes: 4941d472bf95 ("virtio-net: do not reset during XDP set")
> > I think we should cc stable.
>
> Okay, I'll do that in next version.
>
> >
> >> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> >> ---
> >>   drivers/net/virtio_net.c | 27 ++++++++++++++++++++++-----
> >>   1 file changed, 22 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index e53ba600605a..2a130a3e50ac 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1797,7 +1797,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
> >>    * across multiple buffers (num_buf > 1), and we make sure buffers
> >>    * have enough headroom.
> >>    */
> >> -static struct page *xdp_linearize_page(struct receive_queue *rq,
> >> +static struct page *xdp_linearize_page(struct net_device *dev,
> >> +                                      struct receive_queue *rq,
> >>                                         int *num_buf,
> >>                                         struct page *p,
> >>                                         int offset,
> >> @@ -1818,17 +1819,33 @@ static struct page *xdp_linearize_page(struct receive_queue *rq,
> >>          page_off += *len;
> >>
> >>          while (--*num_buf) {
> >> -               unsigned int buflen;
> >> +               unsigned int headroom, tailroom, room;
> >> +               unsigned int truesize, buflen;
> >>                  void *buf;
> >> +               void *ctx;
> >>                  int off;
> >>
> >> -               buf = virtnet_rq_get_buf(rq, &buflen, NULL);
> >> +               buf = virtnet_rq_get_buf(rq, &buflen, &ctx);
> >>                  if (unlikely(!buf))
> >>                          goto err_buf;
> >>
> >>                  p = virt_to_head_page(buf);
> >>                  off = buf - page_address(p);
> >>
> >> +               truesize = mergeable_ctx_to_truesize(ctx);
> > This won't work for receive_small_xdp().
>
> If it is small mode, the num_buf == 1 and we don't get into the while loop.

You are right, it might be worth mentioning this somewhere.

>
> >
> >> +               headroom = mergeable_ctx_to_headroom(ctx);
> >> +               tailroom = headroom ? sizeof(struct skb_shared_info) : 0;
> >> +               room = SKB_DATA_ALIGN(headroom + tailroom);
> >> +
> >> +               if (unlikely(buflen > truesize - room)) {
> >> +                       put_page(p);
> >> +                       pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> >> +                                dev->name, buflen,
> >> +                                (unsigned long)(truesize - room));
> >> +                       DEV_STATS_INC(dev, rx_length_errors);
> >> +                       goto err_buf;
> >> +               }
> > I wonder if this issue only affect XDP should we check other places?
>
> In small mode, we check the len with GOOD_PACKET_LEN in receive_small.
> In mergeable mode, we have some checks over the place and this is the
> only one I see we miss. In xsk, we check inside buf_to_xdp. However, in
> the big mode, I feel like there is a bug.
>
> In add_recvbuf_big, 1 first page + vi->big_packets_num_skbfrags pages.
> The pages are managed by a linked list. The vi->big_packets_num_skbfrags
> is set in virtnet_set_big_packets
>
>      vi->big_packets_num_skbfrags = guest_gso ? MAX_SKB_FRAGS :
> DIV_ROUND_UP(mtu, PAGE_SIZE);
>
> So the vi->big_packets_num_skbfrags can be fewer than MAX_SKB_FRAGS.
>
> In receive_big, we call to page_to_skb, there is a check
>
>      if (unlikely(len > MAX_SKB_FRAGS * PAGE_SIZE)) {
>          /* error case */
>      }
>
> But because the number of allocated buffer is
> vi->big_packets_num_skbfrags + 1 and vi->big_packets_num_skbfrags can be
> fewer than MAX_SKB_FRAGS, the check seems not enough
>
>      while (len) {
>          unsigned int frag_size = min((unsigned)PAGE_SIZE - offset, len);
>          skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, offset,
>                  frag_size, truesize);
>          len -= frag_size;
>          page = (struct page *)page->private;
>          offset = 0;
>      }
>
> In the following while loop, we keep running based on len without NULL
> check the pages linked list, so it may result into NULL pointer dereference.
>
> What do you think?

This looks like a bug, let's fix it.

Thanks

>
> Thanks,
> Quang Minh.
>
> >
> >> +
> >>                  /* guard against a misconfigured or uncooperative backend that
> >>                   * is sending packet larger than the MTU.
> >>                   */
> >> @@ -1917,7 +1934,7 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> >>                  headroom = vi->hdr_len + header_offset;
> >>                  buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> >>                          SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> >> -               xdp_page = xdp_linearize_page(rq, &num_buf, page,
> >> +               xdp_page = xdp_linearize_page(dev, rq, &num_buf, page,
> >>                                                offset, header_offset,
> >>                                                &tlen);
> >>                  if (!xdp_page)
> >> @@ -2252,7 +2269,7 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
> >>           */
> >>          if (!xdp_prog->aux->xdp_has_frags) {
> >>                  /* linearize data for XDP */
> >> -               xdp_page = xdp_linearize_page(rq, num_buf,
> >> +               xdp_page = xdp_linearize_page(vi->dev, rq, num_buf,
> >>                                                *page, offset,
> >>                                                XDP_PACKET_HEADROOM,
> >>                                                len);
> >> --
> >> 2.43.0
> >>
>

Re: [PATCH net 3/4] virtio-net: create a helper to check received mergeable buffer's length

[Jason Wang]

On Thu, Jun 26, 2025 at 11:38 PM Bui Quang Minh
<minhquangbui99@gmail.com> wrote:
>
> On 6/26/25 09:38, Jason Wang wrote:
> > On Thu, Jun 26, 2025 at 12:10 AM Bui Quang Minh
> > <minhquangbui99@gmail.com> wrote:
> >> Currently, we have repeated code to check the received mergeable buffer's
> >> length with allocated size. This commit creates a helper to do that and
> >> converts current code to use it.
> >>
> >> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
> > I think it would be better to introduce this as patch 1, so a
> > mergeable XDP path can use that directly.
> >
> > This will have a smaller changeset.
>
> I'm just concerned that it might make backporting the fix harder because
> the fix depends on this refactor and this refactor touches some function
> that may create conflict.

We can make it a single patch that contains:

1) new helper
2) fixes

as long as the changeset meets the requirement of -stable.

Thanks

>
> Thanks,
> Quang Minh.
>