[PATCH net] virtio-net: fix incorrect flags recording in big mode

[PATCH net] virtio-net: fix incorrect flags recording in big mode

[Xuan Zhuo]

The purpose of commit 703eec1b2422 ("virtio_net: fixing XDP for fully
checksummed packets handling") is to record the flags in advance, as
their value may be overwritten in the XDP case. However, the flags
recorded under big mode are incorrect, because in big mode, the passed
buf does not point to the rx buffer, but rather to the page of the
submitted buffer. This commit fixes this issue.

For the small mode, the commit c11a49d58ad2 ("virtio_net: Fix mismatched
buf address when unmapping for small packets") fixed it.

Fixes: 703eec1b2422 ("virtio_net: fixing XDP for fully checksummed packets handling")
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 975bdc5dab84..6e6e74390955 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2630,13 +2630,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 	 */
 	flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
 
-	if (vi->mergeable_rx_bufs)
+	if (vi->mergeable_rx_bufs) {
 		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
 					stats);
-	else if (vi->big_packets)
+	} else if (vi->big_packets) {
+		void *p;
+
+		p = page_address((struct page *)buf);
+		flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
+
 		skb = receive_big(dev, vi, rq, buf, len, stats);
-	else
+	} else {
 		skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
+	}
 
 	if (unlikely(!skb))
 		return;
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net] virtio-net: fix incorrect flags recording in big mode

[Jason Wang]

On Fri, Sep 19, 2025 at 9:35 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> The purpose of commit 703eec1b2422 ("virtio_net: fixing XDP for fully
> checksummed packets handling") is to record the flags in advance, as
> their value may be overwritten in the XDP case. However, the flags
> recorded under big mode are incorrect, because in big mode, the passed
> buf does not point to the rx buffer, but rather to the page of the
> submitted buffer. This commit fixes this issue.
>
> For the small mode, the commit c11a49d58ad2 ("virtio_net: Fix mismatched
> buf address when unmapping for small packets") fixed it.
>
> Fixes: 703eec1b2422 ("virtio_net: fixing XDP for fully checksummed packets handling")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 975bdc5dab84..6e6e74390955 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2630,13 +2630,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>          */
>         flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
>
> -       if (vi->mergeable_rx_bufs)
> +       if (vi->mergeable_rx_bufs) {
>                 skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
>                                         stats);
> -       else if (vi->big_packets)
> +       } else if (vi->big_packets) {
> +               void *p;
> +
> +               p = page_address((struct page *)buf);
> +               flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> +

Patch looks good but a I have a nit:

It looks better to move this above?

if (vi->big_packets) {
               void *p = page_address((struct page *)buf);
               flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
} else
               flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;

To avoid twice the calculations and reuse the comment.

>                 skb = receive_big(dev, vi, rq, buf, len, stats);
> -       else
> +       } else {
>                 skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
> +       }
>
>         if (unlikely(!skb))
>                 return;
> --
> 2.32.0.3.g01195cf9f
>

Thanks

Re: [PATCH net] virtio-net: fix incorrect flags recording in big mode

[Xuan Zhuo]

On Fri, 19 Sep 2025 10:11:55 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Fri, Sep 19, 2025 at 9:35 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > The purpose of commit 703eec1b2422 ("virtio_net: fixing XDP for fully
> > checksummed packets handling") is to record the flags in advance, as
> > their value may be overwritten in the XDP case. However, the flags
> > recorded under big mode are incorrect, because in big mode, the passed
> > buf does not point to the rx buffer, but rather to the page of the
> > submitted buffer. This commit fixes this issue.
> >
> > For the small mode, the commit c11a49d58ad2 ("virtio_net: Fix mismatched
> > buf address when unmapping for small packets") fixed it.
> >
> > Fixes: 703eec1b2422 ("virtio_net: fixing XDP for fully checksummed packets handling")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 975bdc5dab84..6e6e74390955 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2630,13 +2630,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >          */
> >         flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
> >
> > -       if (vi->mergeable_rx_bufs)
> > +       if (vi->mergeable_rx_bufs) {
> >                 skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> >                                         stats);
> > -       else if (vi->big_packets)
> > +       } else if (vi->big_packets) {
> > +               void *p;
> > +
> > +               p = page_address((struct page *)buf);
> > +               flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> > +
>
> Patch looks good but a I have a nit:
>
> It looks better to move this above?
>
> if (vi->big_packets) {

This should be
	if (!vi->mergeable_rx_bufs && vi->big_packets)


>                void *p = page_address((struct page *)buf);
>                flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> } else
>                flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;


I'm also torn between these two approaches. I am ok, if you prefer this.

Thanks



>
> To avoid twice the calculations and reuse the comment.
>
> >                 skb = receive_big(dev, vi, rq, buf, len, stats);
> > -       else
> > +       } else {
> >                 skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
> > +       }
> >
> >         if (unlikely(!skb))
> >                 return;
> > --
> > 2.32.0.3.g01195cf9f
> >
>
> Thanks
>
>

Re: [PATCH net] virtio-net: fix incorrect flags recording in big mode

[Michael S. Tsirkin]

On Fri, Sep 19, 2025 at 09:34:50AM +0800, Xuan Zhuo wrote:
> The purpose of commit 703eec1b2422 ("virtio_net: fixing XDP for fully
> checksummed packets handling") is to record the flags in advance, as
> their value may be overwritten in the XDP case. However, the flags
> recorded under big mode are incorrect, because in big mode, the passed
> buf does not point to the rx buffer, but rather to the page of the
> submitted buffer. This commit fixes this issue.
> 
> For the small mode, the commit c11a49d58ad2 ("virtio_net: Fix mismatched
> buf address when unmapping for small packets") fixed it.
> 
> Fixes: 703eec1b2422 ("virtio_net: fixing XDP for fully checksummed packets handling")
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


Thanks for the patch! Yet something to improve:

> ---
>  drivers/net/virtio_net.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 975bdc5dab84..6e6e74390955 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2630,13 +2630,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  	 */
>  	flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
>  
> -	if (vi->mergeable_rx_bufs)
> +	if (vi->mergeable_rx_bufs) {
>  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
>  					stats);
> -	else if (vi->big_packets)
> +	} else if (vi->big_packets) {
> +		void *p;
> +
> +		p = page_address((struct page *)buf);

I'd move this assignment to where p is declared:
		void *p = page_address((struct page *)buf);


> +		flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> +
>  		skb = receive_big(dev, vi, rq, buf, len, stats);
> -	else
> +	} else {
>  		skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
> +	}
>  
>  	if (unlikely(!skb))
>  		return;


Indeed but let's please not do the initial 
       flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
for the big mode at all, it's confusing, and only works because struct
page is bigger than struct virtio_net_common_hdr.

pls move it into the clauses for mergeable and small.


> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH net] virtio-net: fix incorrect flags recording in big mode

[Michael S. Tsirkin]

On Fri, Sep 19, 2025 at 10:11:55AM +0800, Jason Wang wrote:
> On Fri, Sep 19, 2025 at 9:35 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > The purpose of commit 703eec1b2422 ("virtio_net: fixing XDP for fully
> > checksummed packets handling") is to record the flags in advance, as
> > their value may be overwritten in the XDP case. However, the flags
> > recorded under big mode are incorrect, because in big mode, the passed
> > buf does not point to the rx buffer, but rather to the page of the
> > submitted buffer. This commit fixes this issue.
> >
> > For the small mode, the commit c11a49d58ad2 ("virtio_net: Fix mismatched
> > buf address when unmapping for small packets") fixed it.
> >
> > Fixes: 703eec1b2422 ("virtio_net: fixing XDP for fully checksummed packets handling")
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 975bdc5dab84..6e6e74390955 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2630,13 +2630,19 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >          */
> >         flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
> >
> > -       if (vi->mergeable_rx_bufs)
> > +       if (vi->mergeable_rx_bufs) {
> >                 skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> >                                         stats);
> > -       else if (vi->big_packets)
> > +       } else if (vi->big_packets) {
> > +               void *p;
> > +
> > +               p = page_address((struct page *)buf);
> > +               flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> > +
> 
> Patch looks good but a I have a nit:
> 
> It looks better to move this above?
> 
> if (vi->big_packets) {
>                void *p = page_address((struct page *)buf);
>                flags = ((struct virtio_net_common_hdr *)p)->hdr.flags;
> } else
>                flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
> 
> To avoid twice the calculations and reuse the comment.


I think duplicating a bit of code is better than branching twice.
So I would move the flags assignment down instead.


> >                 skb = receive_big(dev, vi, rq, buf, len, stats);
> > -       else
> > +       } else {
> >                 skb = receive_small(dev, vi, rq, buf, ctx, len, xdp_xmit, stats);
> > +       }
> >
> >         if (unlikely(!skb))
> >                 return;
> > --
> > 2.32.0.3.g01195cf9f
> >
> 
> Thanks