[net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

[net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

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

With this set applied I am was able to get UDP GSO traffic to pass over
vxlan tunnels in both offloaded modes and non-offloaded modes for ixgbe and
ixgbevf.

I submitted the driver specific patches earlier as an RFC:
https://patchwork.ozlabs.org/project/netdev/list/?series=42477&archive=both&state=*

v2: Updated patches based on feedback from Eric Dumazet
    Split first patch into several patches based on feedback from Eric

---

Alexander Duyck (8):
      udp: Record gso_segs when supporting UDP segmentation offload
      udp: Verify that pulling UDP header in GSO segmentation doesn't fail
      udp: Do not pass MSS as parameter to GSO segmentation
      udp: Do not pass checksum as a parameter to GSO segmentation
      udp: Partially unroll handling of first segment and last segment
      udp: Add support for software checksum and GSO_PARTIAL with GSO offload
      udp: Do not copy destructor if one is not present
      net: Add NETIF_F_GSO_UDP_L4 to list of GSO offloads with fallback


 include/linux/netdev_features.h |    3 +
 include/net/udp.h               |    3 -
 net/ipv4/udp.c                  |    2 +
 net/ipv4/udp_offload.c          |  104 ++++++++++++++++++++++++++-------------
 net/ipv6/udp_offload.c          |   16 ------
 5 files changed, 74 insertions(+), 54 deletions(-)

[net-next PATCH v2 1/8] udp: Record gso_segs when supporting UDP segmentation offload

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <edumazet@google.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 net/ipv4/udp.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index dd3102a37ef9..e07db83b311e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -793,6 +793,8 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
 
 		skb_shinfo(skb)->gso_size = cork->gso_size;
 		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
+		skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
+							 cork->gso_size);
 		goto csum_partial;
 	}
 

Re: [net-next PATCH v2 1/8] udp: Record gso_segs when supporting UDP segmentation offload

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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 <edumazet@google.com>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  net/ipv4/udp.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index dd3102a37ef9..e07db83b311e 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -793,6 +793,8 @@ static int udp_send_skb(struct sk_buff *skb, struct flowi4 *fl4,
>
>                 skb_shinfo(skb)->gso_size = cork->gso_size;
>                 skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
> +               skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(len - sizeof(uh),
> +                                                        cork->gso_size);
>                 goto csum_partial;
>         }

Acked-by: Willem de Bruijn <willemb@google.com>

But we have to be careful that both UDP GSO and UFO packets can traverse
qdisc_pkt_len_init. This does seem to fix the UDP GSO case, as it adds the
header size to each segment. But that is odd, as the code precedes UDP
GSO, so must have been intended to estimate UFO size on the wire. Only
external sources can generate UFO. It seems like we need to not add
sizeof(struct udphdr) to hdrlen in the SKB_GSO_DODGY branch.

[net-next PATCH v2 2/8] udp: Verify that pulling UDP header in GSO segmentation doesn't fail

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---

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

 net/ipv4/udp_offload.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 006257092f06..8303fff42940 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -191,14 +191,17 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 				  netdev_features_t features,
 				  unsigned int mss, __sum16 check)
 {
+	struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
 	struct sock *sk = gso_skb->sk;
 	unsigned int sum_truesize = 0;
-	struct sk_buff *segs, *seg;
 	unsigned int hdrlen;
 	struct udphdr *uh;
 
 	if (gso_skb->len <= sizeof(*uh) + mss)
-		return ERR_PTR(-EINVAL);
+		goto out;
+
+	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));
@@ -230,7 +233,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	}
 
 	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
-
+out:
 	return segs;
 }
 EXPORT_SYMBOL_GPL(__udp_gso_segment);

Re: [net-next PATCH v2 2/8] udp: Verify that pulling UDP header in GSO segmentation doesn't fail

[Eric Dumazet]

On 05/04/2018 11:29 AM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> 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 <alexander.h.duyck@intel.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [net-next PATCH v2 2/8] udp: Verify that pulling UDP header in GSO segmentation doesn't fail

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:29 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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

Re: [net-next PATCH v2 2/8] udp: Verify that pulling UDP header in GSO segmentation doesn't fail

[Alexander Duyck]

On Sat, May 5, 2018 at 1:12 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, May 4, 2018 at 8:29 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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

Ah. Okay I didn't see that. I may add a comment somewhere indicating
that is the case as it is a bit buried with all the calls.

Thanks.

- Alex

[net-next PATCH v2 3/8] udp: Do not pass MSS as parameter to GSO segmentation

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---

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

 include/net/udp.h      |    2 +-
 net/ipv4/udp_offload.c |    6 ++++--
 net/ipv6/udp_offload.c |    2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 05990746810e..8bd83b044ecd 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -176,7 +176,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 				  netdev_features_t features,
-				  unsigned int mss, __sum16 check);
+				  __sum16 check);
 
 static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb)
 {
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 8303fff42940..f21b63adcbbc 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -189,14 +189,16 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 				  netdev_features_t features,
-				  unsigned int mss, __sum16 check)
+				  __sum16 check)
 {
 	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;
 
+	mss = skb_shinfo(gso_skb)->gso_size;
 	if (gso_skb->len <= sizeof(*uh) + mss)
 		goto out;
 
@@ -247,7 +249,7 @@ static struct sk_buff *__udp4_gso_segment(struct sk_buff *gso_skb,
 	if (!can_checksum_protocol(features, htons(ETH_P_IP)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features, mss,
+	return __udp_gso_segment(gso_skb, features,
 				 udp_v4_check(sizeof(struct udphdr) + mss,
 					      iph->saddr, iph->daddr, 0));
 }
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index f7b85b1e6b3e..dea03ec09715 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -26,7 +26,7 @@ static struct sk_buff *__udp6_gso_segment(struct sk_buff *gso_skb,
 	if (!can_checksum_protocol(features, htons(ETH_P_IPV6)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features, mss,
+	return __udp_gso_segment(gso_skb, features,
 				 udp_v6_check(sizeof(struct udphdr) + mss,
 					      &ip6h->saddr, &ip6h->daddr, 0));
 }

Re: [net-next PATCH v2 3/8] udp: Do not pass MSS as parameter to GSO segmentation

[Eric Dumazet]

On 05/04/2018 11:29 AM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> 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 <alexander.h.duyck@intel.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [net-next PATCH v2 3/8] udp: Do not pass MSS as parameter to GSO segmentation

[Willem de Bruijn]

On Fri, May 4, 2018 at 10:08 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/04/2018 11:29 AM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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 <alexander.h.duyck@intel.com>
>> ---
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>

Acked-by: Willem de Bruijn <willemb@google.com>

[net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

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

 include/net/udp.h      |    3 +--
 net/ipv4/udp_offload.c |   35 +++++++++++++++++++++--------------
 net/ipv6/udp_offload.c |    7 +------
 3 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 8bd83b044ecd..9289b6425032 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -175,8 +175,7 @@ struct sk_buff **udp_gro_receive(struct sk_buff **head, struct sk_buff *skb,
 int udp_gro_complete(struct sk_buff *skb, int nhoff, udp_lookup_t lookup);
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
-				  netdev_features_t features,
-				  __sum16 check);
+				  netdev_features_t features);
 
 static inline struct udphdr *udp_gro_udphdr(struct sk_buff *skb)
 {
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index f21b63adcbbc..5c4bb8b9e83e 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -188,8 +188,7 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
 EXPORT_SYMBOL(skb_udp_tunnel_segment);
 
 struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
-				  netdev_features_t features,
-				  __sum16 check)
+				  netdev_features_t features)
 {
 	struct sk_buff *seg, *segs = ERR_PTR(-EINVAL);
 	struct sock *sk = gso_skb->sk;
@@ -197,6 +196,8 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	unsigned int hdrlen;
 	struct udphdr *uh;
 	unsigned int mss;
+	__sum16 check;
+	__be16 newlen;
 
 	mss = skb_shinfo(gso_skb)->gso_size;
 	if (gso_skb->len <= sizeof(*uh) + mss)
@@ -218,17 +219,28 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 		return segs;
 	}
 
+	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));
+
 	for (seg = segs; seg; seg = seg->next) {
 		uh = udp_hdr(seg);
-		uh->len = htons(seg->len - hdrlen);
-		uh->check = check;
 
 		/* last packet can be partial gso_size */
-		if (!seg->next)
-			csum_replace2(&uh->check, htons(mss),
-				      htons(seg->len - hdrlen - sizeof(*uh)));
+		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));
+		}
+
+		uh->len = newlen;
+		uh->check = check;
 
-		uh->check = ~uh->check;
 		seg->destructor = sock_wfree;
 		seg->sk = sk;
 		sum_truesize += seg->truesize;
@@ -243,15 +255,10 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 static struct sk_buff *__udp4_gso_segment(struct sk_buff *gso_skb,
 					  netdev_features_t features)
 {
-	const struct iphdr *iph = ip_hdr(gso_skb);
-	unsigned int mss = skb_shinfo(gso_skb)->gso_size;
-
 	if (!can_checksum_protocol(features, htons(ETH_P_IP)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features,
-				 udp_v4_check(sizeof(struct udphdr) + mss,
-					      iph->saddr, iph->daddr, 0));
+	return __udp_gso_segment(gso_skb, features);
 }
 
 static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index dea03ec09715..61e34f1d2fa2 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -20,15 +20,10 @@
 static struct sk_buff *__udp6_gso_segment(struct sk_buff *gso_skb,
 					  netdev_features_t features)
 {
-	const struct ipv6hdr *ip6h = ipv6_hdr(gso_skb);
-	unsigned int mss = skb_shinfo(gso_skb)->gso_size;
-
 	if (!can_checksum_protocol(features, htons(ETH_P_IPV6)))
 		return ERR_PTR(-EIO);
 
-	return __udp_gso_segment(gso_skb, features,
-				 udp_v6_check(sizeof(struct udphdr) + mss,
-					      &ip6h->saddr, &ip6h->daddr, 0));
+	return __udp_gso_segment(gso_skb, features);
 }
 
 static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Eric Dumazet]

On 05/04/2018 11:30 AM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> 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.
> 

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

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

On Fri, May 4, 2018 at 1:19 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/04/2018 11:30 AM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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.
>>
>
>>
>> +     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 ?

I could but that actually adds more instructions to all this since
csum_sub will perform the inversion across a 32b checksum when we only
need to bitflip a 16 bit value. I had considered doing (u16)(~uh->len)
but thought type casing it more than once would be a pain as well.

What I wanted to avoid is having to do the extra math to account for
the rollover. Adding 3 16 bit values will result in at most a 18 bit
value which can then be folded. Doing it this way we avoid that extra
add w/ carry logic that is needed for csum_add/sub.

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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.

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

>                 /* last packet can be partial gso_size */
> -               if (!seg->next)
> -                       csum_replace2(&uh->check, htons(mss),
> -                                     htons(seg->len - hdrlen - sizeof(*uh)));

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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.
>
> The replacement code, below, subtracts original payload size then adds
> the new payload size. mss here excludes the udp header size.
>
>>                 /* last packet can be partial gso_size */
>> -               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.

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

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Willem de Bruijn]

On Sat, May 5, 2018 at 7:39 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>>
>>> 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.
>>
>> The replacement code, below, subtracts original payload size then adds
>> the new payload size. mss here excludes the udp header size.
>>
>>>                 /* last packet can be partial gso_size */
>>> -               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.
>
> -       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.

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?

Re: [net-next PATCH v2 4/8] udp: Do not pass checksum as a parameter to GSO segmentation

[Alexander Duyck]

On Sun, May 6, 2018 at 10:17 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Sat, May 5, 2018 at 7:39 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Sat, May 5, 2018 at 3:01 AM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>>>
>>>> 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.
>>>
>>> The replacement code, below, subtracts original payload size then adds
>>> the new payload size. mss here excludes the udp header size.
>>>
>>>>                 /* last packet can be partial gso_size */
>>>> -               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.
>>
>> -       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.
>
> 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?

Actually it looks like the math I was applying to test this was off.

Basically the part I wasn't a fan of is the fact that we account for
the UDP header in the first calculation but not in the next. I guess
in the grand scheme of things though you are just dropping it from
both the value being removed and the value being added so it works out
do to the fact that the checksum can be associative.

I guess I was just being too literal in my thinking. Still an
expensive way of doing this though. I'll update the patch description.

Thanks.

- Alex

[net-next PATCH v2 5/8] udp: Partially unroll handling of first segment and last segment

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---

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

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 5c4bb8b9e83e..946d06d2aa0c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -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;
 
-		/* 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;
 
 		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;

Re: [net-next PATCH v2 5/8] udp: Partially unroll handling of first segment and last segment

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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?

> 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 <alexander.h.duyck@intel.com>
> ---
>
> 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(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 5c4bb8b9e83e..946d06d2aa0c 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -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;
>
> -               /* 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)


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

Re: [net-next PATCH v2 5/8] udp: Partially unroll handling of first segment and last segment

[Alexander Duyck]

On Sat, May 5, 2018 at 1:37 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, May 4, 2018 at 8:30 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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?

I need the first segment as I have to get access to the UDP header to
recompute what the checksum should actually be for the first frame and
all the frames in between.

>> 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 <alexander.h.duyck@intel.com>
>> ---
>>
>> 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(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 5c4bb8b9e83e..946d06d2aa0c 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -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;
>>
>> -               /* 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)
>
>

The problem is I need access to the UDP header before I start the
loop. That was why I pulled seg = segs and the "uh = udp_hdr(seg)" out
seperately

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

[net-next PATCH v2 6/8] udp: Add support for software checksum and GSO_PARTIAL with GSO offload

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---
 net/ipv4/udp_offload.c |   28 ++++++++++++++++++----------
 net/ipv6/udp_offload.c |   11 +----------
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 946d06d2aa0c..fd94bbb369b2 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -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);
 
@@ -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);
+
 		seg = seg->next;
 		uh = udp_hdr(seg);
 	}
@@ -250,6 +262,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);
+
 	/* update refcount for the packet */
 	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
 out:
@@ -257,15 +274,6 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 }
 EXPORT_SYMBOL_GPL(__udp_gso_segment);
 
-static struct sk_buff *__udp4_gso_segment(struct sk_buff *gso_skb,
-					  netdev_features_t features)
-{
-	if (!can_checksum_protocol(features, htons(ETH_P_IP)))
-		return ERR_PTR(-EIO);
-
-	return __udp_gso_segment(gso_skb, features);
-}
-
 static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 					 netdev_features_t features)
 {
@@ -289,7 +297,7 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 		goto out;
 
 	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
-		return __udp4_gso_segment(skb, features);
+		return __udp_gso_segment(skb, features);
 
 	mss = skb_shinfo(skb)->gso_size;
 	if (unlikely(skb->len <= mss))
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index 61e34f1d2fa2..03a2ff3fe1e6 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -17,15 +17,6 @@
 #include <net/ip6_checksum.h>
 #include "ip6_offload.h"
 
-static struct sk_buff *__udp6_gso_segment(struct sk_buff *gso_skb,
-					  netdev_features_t features)
-{
-	if (!can_checksum_protocol(features, htons(ETH_P_IPV6)))
-		return ERR_PTR(-EIO);
-
-	return __udp_gso_segment(gso_skb, features);
-}
-
 static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 					 netdev_features_t features)
 {
@@ -58,7 +49,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 			goto out;
 
 		if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4)
-			return __udp6_gso_segment(skb, features);
+			return __udp_gso_segment(skb, features);
 
 		/* Do software UFO. Complete and fill in the UDP checksum as HW cannot
 		 * do checksum of UDP packets sent as multiple IP fragments.

Re: [net-next PATCH v2 6/8] udp: Add support for software checksum and GSO_PARTIAL with GSO offload

[Willem de Bruijn]

On Sat, May 5, 2018 at 3:31 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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 <alexander.h.duyck@intel.com>
> ---
>  net/ipv4/udp_offload.c |   28 ++++++++++++++++++----------
>  net/ipv6/udp_offload.c |   11 +----------
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 946d06d2aa0c..fd94bbb369b2 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -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);
>
> @@ -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?

Re: [net-next PATCH v2 6/8] udp: Add support for software checksum and GSO_PARTIAL with GSO offload

[Alexander Duyck]

On Sun, May 6, 2018 at 2:50 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Sat, May 5, 2018 at 3:31 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> 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 <alexander.h.duyck@intel.com>
>> ---
>>  net/ipv4/udp_offload.c |   28 ++++++++++++++++++----------
>>  net/ipv6/udp_offload.c |   11 +----------
>>  2 files changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 946d06d2aa0c..fd94bbb369b2 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -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);
>>
>> @@ -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?

Good call, though I think I might change this up a bit and do something like:
uh->check = gso_make_checksum(seg, ~check) ? : CSUM_MANGLED_0;

That way I can avoid the extra read.

Thanks.

- Alex

[net-next PATCH v2 7/8] udp: Do not copy destructor if one is not present

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

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 <alexander.h.duyck@intel.com>
---

v2: Do not overwrite destructor if not sock_wfree as per Eric
    Drop WARN_ON as per Eric

 net/ipv4/udp_offload.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index fd94bbb369b2..e802f6331589 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -195,6 +195,7 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	unsigned int sum_truesize = 0;
 	struct udphdr *uh;
 	unsigned int mss;
+	bool copy_dtor;
 	__sum16 check;
 	__be16 newlen;
 
@@ -208,12 +209,14 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 	skb_pull(gso_skb, sizeof(*uh));
 
 	/* clear destructor to avoid skb_segment assigning it to tail */
-	WARN_ON_ONCE(gso_skb->destructor != sock_wfree);
-	gso_skb->destructor = NULL;
+	copy_dtor = gso_skb->destructor == sock_wfree;
+	if (copy_dtor)
+		gso_skb->destructor = NULL;
 
 	segs = skb_segment(gso_skb, features);
 	if (unlikely(IS_ERR_OR_NULL(segs))) {
-		gso_skb->destructor = sock_wfree;
+		if (copy_dtor)
+			gso_skb->destructor = sock_wfree;
 		return segs;
 	}
 
@@ -234,9 +237,11 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 					    (__force u32)newlen));
 
 	for (;;) {
-		seg->destructor = sock_wfree;
-		seg->sk = sk;
-		sum_truesize += seg->truesize;
+		if (copy_dtor) {
+			seg->destructor = sock_wfree;
+			seg->sk = sk;
+			sum_truesize += seg->truesize;
+		}
 
 		if (!seg->next)
 			break;
@@ -268,7 +273,9 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 		uh->check = gso_make_checksum(seg, ~check);
 
 	/* update refcount for the packet */
-	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
+	if (copy_dtor)
+		refcount_add(sum_truesize - gso_skb->truesize,
+			     &sk->sk_wmem_alloc);
 out:
 	return segs;
 }

Re: [net-next PATCH v2 7/8] udp: Do not copy destructor if one is not present

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:31 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> 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 <alexander.h.duyck@intel.com>

Acked-by: Willem de Bruijn <willemb@google.com>

[net-next PATCH v2 8/8] net: Add NETIF_F_GSO_UDP_L4 to list of GSO offloads with fallback

[Alexander Duyck]

From: Alexander Duyck <alexander.h.duyck@intel.com>

Enable UDP offload as a generic software offload since we can now handle it
for multiple cases including if the checksum isn't present and via
gso_partial in the case of tunnels.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/linux/netdev_features.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index c87c3a3453c1..efbd8b2c0197 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -184,7 +184,8 @@ enum {
 
 /* List of features with software fallbacks. */
 #define NETIF_F_GSO_SOFTWARE	(NETIF_F_ALL_TSO | \
-				 NETIF_F_GSO_SCTP)
+				 NETIF_F_GSO_SCTP| \
+				 NETIF_F_GSO_UDP_L4)
 
 /*
  * If one device supports one of these features, then enable them

Re: [net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

[Willem de Bruijn]

On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> 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.

Re: [net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

[Alexander Duyck]

On Sat, May 5, 2018 at 3:06 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> 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.

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

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.

Thanks.

- Alex

Re: [net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

[Willem de Bruijn]

On Mon, May 7, 2018 at 2:02 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Sat, May 5, 2018 at 3:06 AM, Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> 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.
>
>> 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.
>
> 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.

Re: [net-next PATCH v2 0/8] UDP GSO Segmentation clean-ups

[Alexander Duyck]

On Wed, May 9, 2018 at 8:39 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, May 7, 2018 at 2:02 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Sat, May 5, 2018 at 3:06 AM, Willem de Bruijn
>> <willemdebruijn.kernel@gmail.com> wrote:
>>> On Fri, May 4, 2018 at 8:28 PM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> 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.
>>
>>> 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.
>>
>> 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.

Sounds like a plan. In the meantime I am going to see about getting
some internal paperwork taken care of to get UDP GSO added to
ixgbe/ixgbevf as an official feature.

I need to finish up some work I am doing on macvlan over the next
couple of weeks so I won't be focusing on this code for the next month
or so.

Thanks.

- Alex