Re: [PATCH 2/2] virtio_net: fixing XDP for fully checksummed packets handling

[Heng Qi <hengqi@linux.alibaba.com>]

On Thu, 20 Jun 2024 16:33:35 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Jun 18, 2024 at 11:17 AM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > On Tue, 18 Jun 2024 11:10:26 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Mon, Jun 17, 2024 at 9:15 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > > The XDP program can't correctly handle partially checksummed
> > > > packets, but works fine with fully checksummed packets.
> > >
> > > Not sure this is ture, if I was not wrong, XDP can try to calculate checksum.
> >
> > XDP's interface serves a full checksum,
> 
> What do you mean by "serve" here? I mean, XDP can calculate the
> checksum and fill it in the packet by itself.
> 

Yes, XDP can parse and calculate checksums for all packets.
However, the bpf_csum_diff and bpf_l4_csum_replace APIs provided by XDP assume
that the packets being processed are fully checksumed packets. That is,
after the XDP program modified the packets, the incremental checksum can be
calculated (for example, samples/bpf/tcbpf1_kern.c, samples/bpf/test_lwt_bpf.c).

Therefore, partially checksummed packets cannot be processed normally in these
examples and need to be discarded.

> > and this is why we disabled the
> > offloading of VIRTIO_NET_F_GUEST_CSUM when loading XDP.
> 
> If we trust the device to disable VIRTIO_NET_F_GUEST_CSUM, any reason
> to check VIRTIO_NET_HDR_F_NEEDS_CSUM again in the receive path?

There doesn't seem to be a mandatory constraint in the spec that devices that
haven't negotiated VIRTIO_NET_F_GUEST_CSUM cannot set NEEDS_CSUM bit, so I check this.

Thanks.

> 
> >
> > Thanks.
> 
> Thanks
> 
> >
> > >
> > > Thanks
> > >
> > > > If the
> > > > device has already validated fully checksummed packets, then
> > > > the driver doesn't need to re-validate them, saving CPU resources.
> > > >
> > > > Additionally, the driver does not drop all partially checksummed
> > > > packets when VIRTIO_NET_F_GUEST_CSUM is not negotiated. This is
> > > > not a bug, as the driver has always done this.
> > > >
> > > > Fixes: 436c9453a1ac ("virtio-net: keep vnet header zeroed after processing XDP")
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 20 +++++++++++++++++++-
> > > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index aa70a7ed8072..ea10db9a09fa 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -1360,6 +1360,10 @@ static struct sk_buff *receive_small_xdp(struct net_device *dev,
> > > >         if (unlikely(hdr->hdr.gso_type))
> > > >                 goto err_xdp;
> > > >
> > > > +       /* Partially checksummed packets must be dropped. */
> > > > +       if (unlikely(hdr->hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> > > > +               goto err_xdp;
> > > > +
> > > >         buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> > > >                 SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > > >
> > > > @@ -1677,6 +1681,10 @@ static void *mergeable_xdp_get_buf(struct virtnet_info *vi,
> > > >         if (unlikely(hdr->hdr.gso_type))
> > > >                 return NULL;
> > > >
> > > > +       /* Partially checksummed packets must be dropped. */
> > > > +       if (unlikely(hdr->hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> > > > +               return NULL;
> > > > +
> > > >         /* Now XDP core assumes frag size is PAGE_SIZE, but buffers
> > > >          * with headroom may add hole in truesize, which
> > > >          * make their length exceed PAGE_SIZE. So we disabled the
> > > > @@ -1943,6 +1951,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > >         struct net_device *dev = vi->dev;
> > > >         struct sk_buff *skb;
> > > >         struct virtio_net_common_hdr *hdr;
> > > > +       u8 flags;
> > > >
> > > >         if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > >                 pr_debug("%s: short packet %i\n", dev->name, len);
> > > > @@ -1951,6 +1960,15 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > >                 return;
> > > >         }
> > > >
> > > > +       /* 1. Save the flags early, as the XDP program might overwrite them.
> > > > +        * These flags ensure packets marked as VIRTIO_NET_HDR_F_DATA_VALID
> > > > +        * stay valid after XDP processing.
> > > > +        * 2. XDP doesn't work with partially checksummed packets (refer to
> > > > +        * virtnet_xdp_set()), so packets marked as
> > > > +        * VIRTIO_NET_HDR_F_NEEDS_CSUM get dropped during XDP processing.
> > > > +        */
> > > > +       flags = ((struct virtio_net_common_hdr *)buf)->hdr.flags;
> > > > +
> > > >         if (vi->mergeable_rx_bufs)
> > > >                 skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> > > >                                         stats);
> > > > @@ -1966,7 +1984,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > >         if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > > >                 virtio_skb_set_hash(&hdr->hash_v1_hdr, skb);
> > > >
> > > > -       if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > > > +       if (flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > > >                 skb->ip_summed = CHECKSUM_UNNECESSARY;
> > > >
> > > >         if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > > >
> > >
> >
>