UDP GSO Segmentation clean-ups

From: Alexander Duyck <[email protected]>

We need to record the number of segments that will be generated when this frame is segmented. The expectation is that if gso_size is set then gso_segs is set as well. Without this some drivers such as ixgbe get confused if they attempt to offload this as they record 0 segments for the entire packet instead of the correct value.

Reviewed-by Eric Dumazet <[email protected]>

Signed-off-by Alexander Duyck <[email protected]>

---
 net/ipv4/udp.c |    2 ++
 1 file changed, 2 insertions(+)

net/ipv4/udp.c +0 -0

index dd3102a37ef9..e07db83b311e 100644

From: Alexander Duyck <[email protected]>

We should verify that we can pull the UDP header before we attempt to do so. Otherwise if this fails we have no way of knowing and GSO will not work correctly.

Signed-off-by Alexander Duyck <[email protected]>

---

From: Alexander Duyck <[email protected]>

We should verify that we can pull the UDP header before we attempt to do so. Otherwise if this fails we have no way of knowing and GSO will not work correctly.

Quoted 5 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

We should verify that we can pull the UDP header before we attempt to do so. Otherwise if this fails we have no way of knowing and GSO will not work correctly.

This already happened in the callers udp[46]_ufo_fragment

From: Alexander Duyck <[email protected]>

There is no point in passing MSS as a parameter for for the GSO segmentation call as it is already available via the shared info for the skb itself.

Signed-off-by Alexander Duyck <[email protected]>

---

Quoted 2 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

Quoted 6 lines

There is no point in passing MSS as a parameter for for the GSO segmentation call as it is already available via the shared info for the skb itself.

Signed-off-by Alexander Duyck <[email protected]>

---

Reviewed-by Eric Dumazet <[email protected]>

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

Quoted 12 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

net/ipv4/udp_offload.c struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb, · line 229

Quoted 8 lines

net/ipv4/udp_offload.c +8 -0

+     uh = udp_hdr(segs);
+
+     /* compute checksum adjustment based on old length versus new */
+     newlen = htons(sizeof(*uh) + mss);
+     check = ~csum_fold((__force __wsum)((__force u32)uh->check +
+                                         ((__force u32)uh->len ^ 0xFFFF) +
+                                         (__force u32)newlen));
+

Can't this use csum_sub() instead of this ^ 0xFFFF trick ?

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

I think this may be fixing a checksum bug in the original code as well since the checksum that was passed included the UDP header in the checksum computation, but then excluded it for the adjustment on the last frame. I believe this may have an effect on things in the cases where the two differ by bits that would result in things crossing the byte boundaries.

                /* last packet can be partial gso_size */

net/ipv4/udp_offload.c +0 -3

-               if (!seg->next)
-                       csum_replace2(&uh->check, htons(mss),
-                                     htons(seg->len - hdrlen - sizeof(*uh)));

Quoted 10 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

Quoted 5 lines

I think this may be fixing a checksum bug in the original code as well since the checksum that was passed included the UDP header in the checksum computation, but then excluded it for the adjustment on the last frame. I believe this may have an effect on things in the cases where the two differ by bits that would result in things crossing the byte boundaries.

The replacement code, below, subtracts original payload size then adds the new payload size. mss here excludes the udp header size.

Quoted 4 lines

                /* last packet can be partial gso_size */

net/ipv4/udp_offload.c +0 -3

-               if (!seg->next)
-                       csum_replace2(&uh->check, htons(mss),
-                                     htons(seg->len - hdrlen - sizeof(*uh)));

Quoted 12 lines · Willem de Bruijn

Quoted 10 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

Quoted 13 lines

Quoted 5 lines

I think this may be fixing a checksum bug in the original code as well since the checksum that was passed included the UDP header in the checksum computation, but then excluded it for the adjustment on the last frame. I believe this may have an effect on things in the cases where the two differ by bits that would result in things crossing the byte boundaries.

The replacement code, below, subtracts original payload size then adds the new payload size. mss here excludes the udp header size.

Quoted 4 lines

                /* last packet can be partial gso_size */

net/ipv4/udp_offload.c +0 -3

-               if (!seg->next)
-                       csum_replace2(&uh->check, htons(mss),
-                                     htons(seg->len - hdrlen - sizeof(*uh)));

That is my point. When you calculated your checksum you included the UDP header in the calculation.

net/ipv4/udp_offload.c +0 -3

-       return __udp_gso_segment(gso_skb, features,
-                                udp_v4_check(sizeof(struct udphdr) + mss,
-                                             iph->saddr, iph->daddr, 0));

Basically the problem is in one spot you are adding the sizeof(struct udphdr) + mss and then in another you are cancelling it out as mss and trying to account for it by also dropping the UDP header from the payload length of the value you are adding. That works in the cases where the effect doesn't cause any issues with the byte ordering, however I think when mss + 8 crosses a byte boundary it can lead to issues since the calculation is done on a byte swapped value.

Quoted 35 lines · Alexander Duyck

Quoted 12 lines · Willem de Bruijn

Quoted 10 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch is meant to allow us to avoid having to recompute the checksum from scratch and have it passed as a parameter.

Instead of taking that approach we can take advantage of the fact that the length that was used to compute the existing checksum is included in the UDP header. If we cancel that out by adding the value XOR with 0xFFFF we can then just add the new length in and fold that into the new result.

Quoted 13 lines

Quoted 5 lines

I think this may be fixing a checksum bug in the original code as well since the checksum that was passed included the UDP header in the checksum computation, but then excluded it for the adjustment on the last frame. I believe this may have an effect on things in the cases where the two differ by bits that would result in things crossing the byte boundaries.

The replacement code, below, subtracts original payload size then adds the new payload size. mss here excludes the udp header size.

Quoted 4 lines

                /* last packet can be partial gso_size */

net/ipv4/udp_offload.c +0 -3

-               if (!seg->next)
-                       csum_replace2(&uh->check, htons(mss),
-                                     htons(seg->len - hdrlen - sizeof(*uh)));

That is my point. When you calculated your checksum you included the UDP header in the calculation.

net/ipv4/udp_offload.c +0 -3

-       return __udp_gso_segment(gso_skb, features,
-                                udp_v4_check(sizeof(struct udphdr) + mss,
-                                             iph->saddr, iph->daddr, 0));

Quoted 7 lines

Basically the problem is in one spot you are adding the sizeof(struct udphdr) + mss and then in another you are cancelling it out as mss and trying to account for it by also dropping the UDP header from the payload length of the value you are adding. That works in the cases where the effect doesn't cause any issues with the byte ordering, however I think when mss + 8 crosses a byte boundary it can lead to issues since the calculation is done on a byte swapped value.

Do you mean that the issue is that the arithmetic operations on a __be16 in csum_replace2 may be incorrect if they exceed the least significant byte?

csum_replace2 is used in many locations in the stack to adjust a network byte order csum when the payload length changes (e.g., iph->tot_len in inet_gro_complete).

Or am I missing something specific about the udphdr calculations?

From: Alexander Duyck <[email protected]>

This patch allows us to take care of unrolling the first segment and the last segment

of the loop for processing the segmented skb. Part of the motivation for this is that it makes it easier to process the fact that the first fame and all of the frames in between should be mostly identical in terms of header data, and the last frame has differences in the length and partial checksum.

In addition I am dropping the header length calculation since we don't really need it for anything but the last frame and it can be easily obtained by just pulling the data_len and offset of tail from the transport header.

Signed-off-by Alexander Duyck <[email protected]>

---

v2: New break-out patch based on one patch from earlier series

 net/ipv4/udp_offload.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

net/ipv4/udp_offload.c +6 -5

index 5c4bb8b9e83e..946d06d2aa0c 100644
@@ -193,7 +193,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
        struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
        struct sock *sk = gso_skb->sk;
        unsigned int sum_truesize = 0;
-       unsigned int hdrlen;
        struct udphdr *uh;
        unsigned int mss;
        __sum16 check;

@@ -206,7 +205,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
        if (!pskb_may_pull(gso_skb, sizeof(*uh)))
                goto out;
 
-       hdrlen = gso_skb->data - skb_mac_header(gso_skb);
        skb_pull(gso_skb, sizeof(*uh));
 
        /* clear destructor to avoid skb_segment assigning it to tail */

@@ -219,7 +217,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                return segs;
        }
 
-       uh = udp_hdr(segs);
+       seg = segs;
+       uh = udp_hdr(seg);
 
        /* compute checksum adjustment based on old length versus new */
        newlen = htons(sizeof(*uh) + mss);

@@ -227,25 +226,31 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                                            ((__force u32)uh->len ^ 0xFFFF) +
                                            (__force u32)newlen));
 
-       for (seg = segs; seg; seg = seg->next) {
-               uh = udp_hdr(seg);
+       for (;;) {
+               seg->destructor = sock_wfree;
+               seg->sk = sk;
+               sum_truesize += seg->truesize;

net/ipv4/udp_offload.c +12 -3

                uh->len = newlen;
                uh->check = check;
 
-               seg->destructor = sock_wfree;
-               seg->sk = sk;
-               sum_truesize += seg->truesize;
+               seg = seg->next;
+               uh = udp_hdr(seg);
        }
 
+       /* last packet can be partial gso_size, account for that in checksum */
+       newlen = htons(skb_tail_pointer(seg) - skb_transport_header(seg) +
+                      seg->data_len);
+       check = ~csum_fold((__force __wsum)((__force u32)uh->check +
+                                           ((__force u32)uh->len ^ 0xFFFF)  +
+                                           (__force u32)newlen));
+       uh->len = newlen;
+       uh->check = check;
+
+       /* update refcount for the packet */
        refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
 out:
        return segs;
 

Quoted 4 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch allows us to take care of unrolling the first segment and the last segment

Only the last segment needs to be unrolled, right?

Quoted 59 lines

of the loop for processing the segmented skb. Part of the motivation for this is that it makes it easier to process the fact that the first fame and all of the frames in between should be mostly identical in terms of header data, and the last frame has differences in the length and partial checksum.

In addition I am dropping the header length calculation since we don't really need it for anything but the last frame and it can be easily obtained by just pulling the data_len and offset of tail from the transport header.

Signed-off-by Alexander Duyck <[email protected]>

---

v2: New break-out patch based on one patch from earlier series

 net/ipv4/udp_offload.c |   35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

net/ipv4/udp_offload.c +6 -5

index 5c4bb8b9e83e..946d06d2aa0c 100644
@@ -193,7 +193,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
        struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
        struct sock *sk = gso_skb->sk;
        unsigned int sum_truesize = 0;
-       unsigned int hdrlen;
        struct udphdr *uh;
        unsigned int mss;
        __sum16 check;

@@ -206,7 +205,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
        if (!pskb_may_pull(gso_skb, sizeof(*uh)))
                goto out;
 
-       hdrlen = gso_skb->data - skb_mac_header(gso_skb);
        skb_pull(gso_skb, sizeof(*uh));
 
        /* clear destructor to avoid skb_segment assigning it to tail */

@@ -219,7 +217,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                return segs;
        }
 
-       uh = udp_hdr(segs);
+       seg = segs;
+       uh = udp_hdr(seg);
 
        /* compute checksum adjustment based on old length versus new */
        newlen = htons(sizeof(*uh) + mss);

@@ -227,25 +226,31 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                                            ((__force u32)uh->len ^ 0xFFFF) +
                                            (__force u32)newlen));
 
-       for (seg = segs; seg; seg = seg->next) {
-               uh = udp_hdr(seg);
+       for (;;) {
+               seg->destructor = sock_wfree;
+               seg->sk = sk;
+               sum_truesize += seg->truesize;

net/ipv4/udp_offload.c struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb, · line 235

Quoted 10 lines

net/ipv4/udp_offload.c +2 -7

 
-               /* last packet can be partial gso_size */
-               if (!seg->next) {
-                       newlen = htons(seg->len - hdrlen);
-                       check = ~csum_fold((__force __wsum)((__force u32)uh->check +
-                                                           ((__force u32)uh->len ^ 0xFFFF) +
-                                                           (__force u32)newlen));
-               }
+               if (!seg->next)
+                       break;

Not critical, but I find replacing

  for (seg = segs; seg; seg = seg->next) {
    uh = udp_hdr(seg);
    ...
  }

with

  uh = udp_hdr(seg);
  for (;;) {
    ...
    if (!seg->next)
      break;

    uh = udp_hdr(seg);

}

much less easy to parse and it inflates patch size. How about just

- for (seg = segs; seg; seg = seg->next)
+ for( seg = segs; seg->next; seg = seg->next)

Quoted 25 lines

net/ipv4/udp_offload.c +12 -3

                uh->len = newlen;
                uh->check = check;
 
-               seg->destructor = sock_wfree;
-               seg->sk = sk;
-               sum_truesize += seg->truesize;
+               seg = seg->next;
+               uh = udp_hdr(seg);
        }
 
+       /* last packet can be partial gso_size, account for that in checksum */
+       newlen = htons(skb_tail_pointer(seg) - skb_transport_header(seg) +
+                      seg->data_len);
+       check = ~csum_fold((__force __wsum)((__force u32)uh->check +
+                                           ((__force u32)uh->len ^ 0xFFFF)  +
+                                           (__force u32)newlen));
+       uh->len = newlen;
+       uh->check = check;
+
+       /* update refcount for the packet */
        refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
 out:
        return segs;
 

From: Alexander Duyck <[email protected]>

This patch adds support for a software provided checksum and GSO_PARTIAL segmentation support. With this we can offload UDP segmentation on devices that only have partial support for tunnels.

Since we are no longer needing the hardware checksum we can drop the checks in the segmentation code that were verifying if it was present.

Signed-off-by Alexander Duyck <[email protected]>

---
 net/ipv4/udp_offload.c |   28 ++++++++++++++++++----------
 net/ipv6/udp_offload.c |   11 +----------
 2 files changed, 19 insertions(+), 20 deletions(-)

net/ipv4/udp_offload.c +7 -0

index 946d06d2aa0c..fd94bbb369b2 100644
@@ -217,6 +217,13 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                return segs;
        }
 
+       /* GSO partial and frag_list segmentation only requires splitting
+        * the frame into an MSS multiple and possibly a remainder, both
+        * cases return a GSO skb. So update the mss now.
+        */
+       if (skb_is_gso(segs))
+               mss *= skb_shinfo(segs)->gso_segs;
+
        seg = segs;
        uh = udp_hdr(seg);
 

Quoted 34 lines · Alexander Duyck

From: Alexander Duyck <[email protected]>

This patch adds support for a software provided checksum and GSO_PARTIAL segmentation support. With this we can offload UDP segmentation on devices that only have partial support for tunnels.

Since we are no longer needing the hardware checksum we can drop the checks in the segmentation code that were verifying if it was present.

Signed-off-by Alexander Duyck <[email protected]>

---
 net/ipv4/udp_offload.c |   28 ++++++++++++++++++----------
 net/ipv6/udp_offload.c |   11 +----------
 2 files changed, 19 insertions(+), 20 deletions(-)

net/ipv4/udp_offload.c +7 -0

index 946d06d2aa0c..fd94bbb369b2 100644
@@ -217,6 +217,13 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                return segs;
        }
 
+       /* GSO partial and frag_list segmentation only requires splitting
+        * the frame into an MSS multiple and possibly a remainder, both
+        * cases return a GSO skb. So update the mss now.
+        */
+       if (skb_is_gso(segs))
+               mss *= skb_shinfo(segs)->gso_segs;
+
        seg = segs;
        uh = udp_hdr(seg);
 

net/ipv4/udp_offload.c struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb, · line 250

Quoted 7 lines

net/ipv4/udp_offload.c +4 -0

@@ -237,6 +244,11 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
                uh->len = newlen;
                uh->check = check;
 
+               if (seg->ip_summed == CHECKSUM_PARTIAL)
+                       gso_reset_checksum(seg, ~check);
+               else
+                       uh->check = gso_make_checksum(seg, ~check);

Here and below, this needs

  if (uh->check == 0)
    uh->check = CSUM_MANGLED_0;

similar to __skb_udp_tunnel_segment?

From: Alexander Duyck <[email protected]>

This patch makes it so that if a destructor is not present we avoid trying to update the skb socket or any reference counting that would be associated with the NULL socket and/or descriptor. By doing this we can support traffic coming from another namespace without any issues.

Signed-off-by Alexander Duyck <[email protected]>

This patch set addresses a number of issues I found while sorting out enabling UDP GSO Segmentation support for ixgbe/ixgbevf. Specifically there were a number of issues related to the checksum and such that seemed to cause either minor irregularities or kernel panics in the case of the offload request being allowed to traverse between name spaces.

Quoted 5 lines · Alexander Duyck

This patch set addresses a number of issues I found while sorting out enabling UDP GSO Segmentation support for ixgbe/ixgbevf. Specifically there were a number of issues related to the checksum and such that seemed to cause either minor irregularities or kernel panics in the case of the offload request being allowed to traverse between name spaces.

Were you able to traverse GSO packets between network namespace before adding to NETIF_F_GSO_SOFTWARE? It does appear that veth includes NETIF_F_GSO_ENCAP_ALL, which also allows GSO.

In either case, it should not be possible for GSO packets to arrive on a veth device, as that can result in queuing the GSO packet to a recipient socket. In this regard veth is like loopback and must exclude GSO support.

I'll take a look.

Quoted 11 lines · Willem de Bruijn

Quoted 5 lines · Alexander Duyck

This patch set addresses a number of issues I found while sorting out enabling UDP GSO Segmentation support for ixgbe/ixgbevf. Specifically there were a number of issues related to the checksum and such that seemed to cause either minor irregularities or kernel panics in the case of the offload request being allowed to traverse between name spaces.

Were you able to traverse GSO packets between network namespace before adding to NETIF_F_GSO_SOFTWARE? It does appear that veth includes NETIF_F_GSO_ENCAP_ALL, which also allows GSO.

Without that change the tunnel wouldn't pass the requests between namespaces. However with it I was able to easily test the software checksum code as otherwise the socket was returning EIO when the hardware checksum was disabled.

Quoted 4 lines

In either case, it should not be possible for GSO packets to arrive on a veth device, as that can result in queuing the GSO packet to a recipient socket. In this regard veth is like loopback and must exclude GSO support.

Quoted 1 line

I'll take a look.

I suspect it was probably sending veth UDP segmentation offload requests. For now I can probably drop he patch that was adding it and it can be added later to individual drivers if needed.

Quoted 23 lines · Alexander Duyck

Quoted 11 lines · Willem de Bruijn

Quoted 5 lines · Alexander Duyck

This patch set addresses a number of issues I found while sorting out enabling UDP GSO Segmentation support for ixgbe/ixgbevf. Specifically there were a number of issues related to the checksum and such that seemed to cause either minor irregularities or kernel panics in the case of the offload request being allowed to traverse between name spaces.

Were you able to traverse GSO packets between network namespace before adding to NETIF_F_GSO_SOFTWARE? It does appear that veth includes NETIF_F_GSO_ENCAP_ALL, which also allows GSO.

Without that change the tunnel wouldn't pass the requests between namespaces. However with it I was able to easily test the software checksum code as otherwise the socket was returning EIO when the hardware checksum was disabled.

Quoted 4 lines

In either case, it should not be possible for GSO packets to arrive on a veth device, as that can result in queuing the GSO packet to a recipient socket. In this regard veth is like loopback and must exclude GSO support.

Quoted 5 lines

Quoted 1 line

I'll take a look.

I suspect it was probably sending veth UDP segmentation offload requests. For now I can probably drop he patch that was adding it and it can be added later to individual drivers if needed.

I just tested udpgso_bench_tx over veth (on a commit without your patchset).

Having NETIF_F_GSO_UDP_L4 in NETIF_F_GSO_ENCAP_ALL and NETIF_F_GSO_ENCAP_ALL in VETH_FEATURES is sufficient to receive large packets on the veth peer.

This is clearly a bug, as is for any device that may loop packets onto a local socket. Such as macvlan in bridge mode.

I will have to revise commit 83aa025f535f ("udp: add gso support to virtual devices")

It remains useful to have this capability on the bonding device. I might remove the flag from NETIF_F_GSO_ENCAP_ALL and add it specifically to that device.

This is also all relevant to future work of NETIF_F_GSO_SOFTWARE.