[PATCH 0/2] virtio_net: fixes for checksum offloading and XDP handling

[PATCH 0/2] virtio_net: fixes for checksum offloading and XDP handling

[Heng Qi]

This series of patches aim to address two specific issues identified in
the virtio_net driver related to checksum offloading and XDP processing of
fully checksummed packets.

The first patch corrects the handling of checksum offloading in the driver.
The second patch addresses an issue where the XDP program had no trouble
with fully checksummed packets.

Heng Qi (2):
  virtio_net: checksum offloading handling fix
  virtio_net: fixing XDP for fully checksummed packets handling

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

-- 
2.32.0.3.g01195cf9f

[PATCH 1/2] virtio_net: checksum offloading handling fix

[Heng Qi]

In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle
partially checksummed packets, and the validation of fully checksummed
packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM
negotiation. However, the specification erroneously stated:

  "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
   to zero and SHOULD supply a fully checksummed packet to the driver."

This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM
negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag.
Essentially, the device can facilitate the validation of these packets'
checksums - a process known as RX checksum offloading - removing the need
for the driver to do so.

This scenario is currently not implemented in the driver and requires
correction. The necessary specification correction[1] has been made and
approved in the virtio TC vote.
[1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html

Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available")
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 61a57d134544..aa70a7ed8072 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5666,8 +5666,16 @@ static int virtnet_probe(struct virtio_device *vdev)
 			dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
 		/* (!csum && gso) case will be fixed by register_netdev() */
 	}
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
-		dev->features |= NETIF_F_RXCSUM;
+
+	/* 1. With VIRTIO_NET_F_GUEST_CSUM negotiation, the driver doesn't
+	 * need to calculate checksums for partially checksummed packets,
+	 * as they're considered valid by the upper layer.
+	 * 2. Without VIRTIO_NET_F_GUEST_CSUM negotiation, the driver only
+	 * receives fully checksummed packets. The device may assist in
+	 * validating these packets' checksums, so the driver won't have to.
+	 */
+	dev->features |= NETIF_F_RXCSUM;
+
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
 	    virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6))
 		dev->features |= NETIF_F_GRO_HW;
-- 
2.32.0.3.g01195cf9f

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

[Heng Qi]

The XDP program can't correctly handle partially checksummed
packets, but works fine with fully checksummed packets. 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

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Jiri Pirko]

Mon, Jun 17, 2024 at 03:15:23PM CEST, hengqi@linux.alibaba.com wrote:
>In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle
>partially checksummed packets, and the validation of fully checksummed
>packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM
>negotiation. However, the specification erroneously stated:
>
>  "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
>   to zero and SHOULD supply a fully checksummed packet to the driver."
>
>This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM
>negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag.
>Essentially, the device can facilitate the validation of these packets'
>checksums - a process known as RX checksum offloading - removing the need
>for the driver to do so.
>
>This scenario is currently not implemented in the driver and requires
>correction. The necessary specification correction[1] has been made and
>approved in the virtio TC vote.
>[1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html
>
>Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available")
>Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Jason Wang]

On Mon, Jun 17, 2024 at 9:15 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle
> partially checksummed packets, and the validation of fully checksummed
> packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM
> negotiation. However, the specification erroneously stated:
>
>   "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
>    to zero and SHOULD supply a fully checksummed packet to the driver."
>
> This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM
> negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag.
> Essentially, the device can facilitate the validation of these packets'
> checksums - a process known as RX checksum offloading - removing the need
> for the driver to do so.
>
> This scenario is currently not implemented in the driver and requires
> correction. The necessary specification correction[1] has been made and
> approved in the virtio TC vote.
> [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html
>
> Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available")
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---

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

(Should we manually do checksum if RXCUSM is disabled?)

Thanks

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Heng Qi]

On Tue, 18 Jun 2024 11:01:27 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Mon, Jun 17, 2024 at 9:15 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > In virtio spec 0.95, VIRTIO_NET_F_GUEST_CSUM was designed to handle
> > partially checksummed packets, and the validation of fully checksummed
> > packets by the device is independent of VIRTIO_NET_F_GUEST_CSUM
> > negotiation. However, the specification erroneously stated:
> >
> >   "If VIRTIO_NET_F_GUEST_CSUM is not negotiated, the device MUST set flags
> >    to zero and SHOULD supply a fully checksummed packet to the driver."
> >
> > This statement is inaccurate because even without VIRTIO_NET_F_GUEST_CSUM
> > negotiation, the device can still set the VIRTIO_NET_HDR_F_DATA_VALID flag.
> > Essentially, the device can facilitate the validation of these packets'
> > checksums - a process known as RX checksum offloading - removing the need
> > for the driver to do so.
> >
> > This scenario is currently not implemented in the driver and requires
> > correction. The necessary specification correction[1] has been made and
> > approved in the virtio TC vote.
> > [1] https://lists.oasis-open.org/archives/virtio-comment/202401/msg00011.html
> >
> > Fixes: 4f49129be6fa ("virtio-net: Set RXCSUM feature if GUEST_CSUM is available")
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > ---
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> (Should we manually do checksum if RXCUSM is disabled?)
> 

Currently we do not allow RXCUSM to be disabled.

Thanks.

> Thanks
> 

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

[Jason Wang]

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.

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
>

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

[Heng Qi]

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, and this is why we disabled the
offloading of VIRTIO_NET_F_GUEST_CSUM when loading XDP.

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
> >
> 

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Jakub Kicinski]

On Tue, 18 Jun 2024 11:09:02 +0800 Heng Qi wrote:
> > (Should we manually do checksum if RXCUSM is disabled?)
> >   
> 
> Currently we do not allow RXCUSM to be disabled.

You don't have to disable checksuming in the device.
Just ignore VIRTIO_NET_HDR_F_DATA_VALID if user cleared NETIF_F_RXCSUM.
I know some paranoid workloads which do actually want the kernel to
calculate the checksum.

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Heng Qi]

On Tue, 18 Jun 2024 18:15:16 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 18 Jun 2024 11:09:02 +0800 Heng Qi wrote:
> > > (Should we manually do checksum if RXCUSM is disabled?)
> > >   
> > 
> > Currently we do not allow RXCUSM to be disabled.
> 
> You don't have to disable checksuming in the device.

Yes, it is up to the device itself to decide whether to validate checksum.
What I mean is that we don't allow users to disable the driver's
NETIF_F_RXCSUM flag.

> Just ignore VIRTIO_NET_HDR_F_DATA_VALID if user cleared NETIF_F_RXCSUM.

Right.

Thanks.

> I know some paranoid workloads which do actually want the kernel to
> calculate the checksum.
> 

Re: [PATCH 0/2] virtio_net: fixes for checksum offloading and XDP handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon, 17 Jun 2024 21:15:22 +0800 you wrote:
> This series of patches aim to address two specific issues identified in
> the virtio_net driver related to checksum offloading and XDP processing of
> fully checksummed packets.
> 
> The first patch corrects the handling of checksum offloading in the driver.
> The second patch addresses an issue where the XDP program had no trouble
> with fully checksummed packets.
> 
> [...]

Here is the summary with links:
  - [1/2] virtio_net: checksum offloading handling fix
    https://git.kernel.org/netdev/net/c/604141c036e1
  - [2/2] virtio_net: fixing XDP for fully checksummed packets handling
    https://git.kernel.org/netdev/net/c/703eec1b2422

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Jakub Kicinski]

On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote:
> > > Currently we do not allow RXCUSM to be disabled.  
> > 
> > You don't have to disable checksuming in the device.  
> 
> Yes, it is up to the device itself to decide whether to validate checksum.
> What I mean is that we don't allow users to disable the driver's
> NETIF_F_RXCSUM flag.

I understand. What I'm suggesting is that you send a follow up patch
that allows it.

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Heng Qi]

在 2024/6/19 下午11:08, Jakub Kicinski 写道:
> On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote:
>>>> Currently we do not allow RXCUSM to be disabled.
>>> You don't have to disable checksuming in the device.
>> Yes, it is up to the device itself to decide whether to validate checksum.
>> What I mean is that we don't allow users to disable the driver's
>> NETIF_F_RXCSUM flag.
> I understand. What I'm suggesting is that you send a follow up patch
> that allows it.

OK, will do it.

Thanks.

Re: [PATCH 1/2] virtio_net: checksum offloading handling fix

[Jason Wang]

On Wed, Jun 19, 2024 at 11:44 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
> 在 2024/6/19 下午11:08, Jakub Kicinski 写道:
> > On Wed, 19 Jun 2024 10:02:58 +0800 Heng Qi wrote:
> >>>> Currently we do not allow RXCUSM to be disabled.
> >>> You don't have to disable checksuming in the device.
> >> Yes, it is up to the device itself to decide whether to validate checksum.
> >> What I mean is that we don't allow users to disable the driver's
> >> NETIF_F_RXCSUM flag.
> > I understand. What I'm suggesting is that you send a follow up patch
> > that allows it.

Exactly my point as well.

>
> OK, will do it.
>
> Thanks.

Thanks

>
>

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

[Jason Wang]

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.

> 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?

>
> 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
> > >
> >
>

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

[Heng Qi]

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
> > > >
> > >
> >
> 

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

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 05:28:48PM +0800, Heng Qi wrote:
> 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.

The spec says:

\item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
  VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
  set: if so, the packet checksum at offset \field{csum_offset} 
  from \field{csum_start} and any preceding checksums
  have been validated.  The checksum on the packet is incomplete and
  if bit VIRTIO_NET_HDR_F_RSC_INFO is not set in \field{flags},
  then \field{csum_start} and \field{csum_offset} indicate how to calculate it
  (see Packet Transmission point 1).


So yes, NEEDS_CSUM without VIRTIO_NET_F_GUEST_CSUM is at best undefined.
Please do not try to use it unless VIRTIO_NET_F_GUEST_CSUM is set.

And if you want to be flexible, ignore it unless VIRTIO_NET_F_GUEST_CSUM
has been negotiated.





> > 
> > >
> > > 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
> > > > >
> > > >
> > >
> > 

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

[Heng Qi]

On Thu, 20 Jun 2024 06:19:01 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 05:28:48PM +0800, Heng Qi wrote:
> > 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.
> 
> The spec says:
> 
> \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
>   VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
>   set: if so, the packet checksum at offset \field{csum_offset} 
>   from \field{csum_start} and any preceding checksums
>   have been validated.  The checksum on the packet is incomplete and
>   if bit VIRTIO_NET_HDR_F_RSC_INFO is not set in \field{flags},
>   then \field{csum_start} and \field{csum_offset} indicate how to calculate it
>   (see Packet Transmission point 1).
> 
> 
> So yes, NEEDS_CSUM without VIRTIO_NET_F_GUEST_CSUM is at best undefined.
> Please do not try to use it unless VIRTIO_NET_F_GUEST_CSUM is set.

I've seen it before, but thought something like
 "The device MUST NOT set the NEEDS_CSUM bit if GUEST_CSUM has not been negotiated"
would be clearer.

Furthermore, it is still possible for a malicious device to set the bit.

Thanks.

> 
> And if you want to be flexible, ignore it unless VIRTIO_NET_F_GUEST_CSUM
> has been negotiated.
> 
> 
> 
> 
> 
> > > 
> > > >
> > > > 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
> > > > > >
> > > > >
> > > >
> > > 
> 

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

[Heng Qi]

On Thu, 20 Jun 2024 18:27:16 +0800, Heng Qi <hengqi@linux.alibaba.com> wrote:
> On Thu, 20 Jun 2024 06:19:01 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 05:28:48PM +0800, Heng Qi wrote:
> > > 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.
> > 
> > The spec says:
> > 
> > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> >   VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
> >   set: if so, the packet checksum at offset \field{csum_offset} 
> >   from \field{csum_start} and any preceding checksums
> >   have been validated.  The checksum on the packet is incomplete and
> >   if bit VIRTIO_NET_HDR_F_RSC_INFO is not set in \field{flags},
> >   then \field{csum_start} and \field{csum_offset} indicate how to calculate it
> >   (see Packet Transmission point 1).
> > 
> > 
> > So yes, NEEDS_CSUM without VIRTIO_NET_F_GUEST_CSUM is at best undefined.
> > Please do not try to use it unless VIRTIO_NET_F_GUEST_CSUM is set.
> 
> I've seen it before, but thought something like
>  "The device MUST NOT set the NEEDS_CSUM bit if GUEST_CSUM has not been negotiated"
> would be clearer.
> 
> Furthermore, it is still possible for a malicious device to set the bit.

Hint:
We previously checked and used DATA_VALID and NEEDS_CSUM bits, but never checked
to see if GUEST_CSUM was negotiated.

> 
> Thanks.
> 
> > 
> > And if you want to be flexible, ignore it unless VIRTIO_NET_F_GUEST_CSUM
> > has been negotiated.
> > 
> > 
> > 
> > 
> > 
> > > > 
> > > > >
> > > > > 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
> > > > > > >
> > > > > >
> > > > >
> > > > 
> > 
> 

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

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 06:38:49PM +0800, Heng Qi wrote:
> On Thu, 20 Jun 2024 18:27:16 +0800, Heng Qi <hengqi@linux.alibaba.com> wrote:
> > On Thu, 20 Jun 2024 06:19:01 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 05:28:48PM +0800, Heng Qi wrote:
> > > > 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.
> > > 
> > > The spec says:
> > > 
> > > \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > >   VIRTIO_NET_HDR_F_NEEDS_CSUM bit in \field{flags} can be
> > >   set: if so, the packet checksum at offset \field{csum_offset} 
> > >   from \field{csum_start} and any preceding checksums
> > >   have been validated.  The checksum on the packet is incomplete and
> > >   if bit VIRTIO_NET_HDR_F_RSC_INFO is not set in \field{flags},
> > >   then \field{csum_start} and \field{csum_offset} indicate how to calculate it
> > >   (see Packet Transmission point 1).
> > > 
> > > 
> > > So yes, NEEDS_CSUM without VIRTIO_NET_F_GUEST_CSUM is at best undefined.
> > > Please do not try to use it unless VIRTIO_NET_F_GUEST_CSUM is set.
> > 
> > I've seen it before, but thought something like
> >  "The device MUST NOT set the NEEDS_CSUM bit if GUEST_CSUM has not been negotiated"
> > would be clearer.
> > 
> > Furthermore, it is still possible for a malicious device to set the bit.
> 
> Hint:
> We previously checked and used DATA_VALID and NEEDS_CSUM bits, but never checked
> to see if GUEST_CSUM was negotiated.


That would be out of spec then. Might be too late to fix.

> > 
> > Thanks.
> > 
> > > 
> > > And if you want to be flexible, ignore it unless VIRTIO_NET_F_GUEST_CSUM
> > > has been negotiated.
> > > 
> > > 
> > > 
> > > 
> > > 
> > > > > 
> > > > > >
> > > > > > 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
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > 
> > > 
> >