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

[net-next PATCH 0/5] 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 will include the driver patches as a seperate RFC following this series.

---

Alexander Duyck (5):
      udp: Record gso_segs when supporting UDP segmentation offload
      udp: Do not pass checksum or MSS as parameters
      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          |  102 ++++++++++++++++++++++++++-------------
 net/ipv6/udp_offload.c          |   16 ------
 5 files changed, 73 insertions(+), 53 deletions(-)

[net-next PATCH 1/5] 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.

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

[net-next PATCH 2/5] udp: Do not pass checksum or MSS as parameters

[Alexander Duyck]

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

This patch is meant to be a start at cleaning up some of the UDP GSO
segmentation code. Specifically we were passing mss and a recomputed
checksum when we really didn't need to. The function itself could derive
that information based on the already provided checksum, the length of the
packet stored in the UDP header, and the gso_size.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
 include/net/udp.h      |    3 +-
 net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
 net/ipv6/udp_offload.c |    7 +-----
 3 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 05990746810e..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,
-				  unsigned int mss, __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 006257092f06..946d06d2aa0c 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -188,19 +188,23 @@ 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,
-				  unsigned int mss, __sum16 check)
+				  netdev_features_t features)
 {
+	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;
+	unsigned int mss;
+	__sum16 check;
+	__be16 newlen;
 
+	mss = skb_shinfo(gso_skb)->gso_size;
 	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));
 
 	/* clear destructor to avoid skb_segment assigning it to tail */
@@ -213,24 +217,42 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
 		return segs;
 	}
 
-	for (seg = segs; seg; seg = seg->next) {
-		uh = udp_hdr(seg);
-		uh->len = htons(seg->len - hdrlen);
-		uh->check = check;
+	seg = segs;
+	uh = udp_hdr(seg);
 
-		/* last packet can be partial gso_size */
-		if (!seg->next)
-			csum_replace2(&uh->check, htons(mss),
-				      htons(seg->len - hdrlen - sizeof(*uh)));
+	/* 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));
 
-		uh->check = ~uh->check;
+	for (;;) {
 		seg->destructor = sock_wfree;
 		seg->sk = sk;
 		sum_truesize += seg->truesize;
+
+		if (!seg->next)
+			break;
+
+		uh->len = newlen;
+		uh->check = check;
+
+		seg = seg->next;
+		uh = udp_hdr(seg);
 	}
 
-	refcount_add(sum_truesize - gso_skb->truesize, &sk->sk_wmem_alloc);
+	/* 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;
 }
 EXPORT_SYMBOL_GPL(__udp_gso_segment);
@@ -238,15 +260,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, mss,
-				 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 f7b85b1e6b3e..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, mss,
-				 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,

[net-next PATCH 3/5] 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.

[net-next PATCH 4/5] 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>
---
 net/ipv4/udp_offload.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index fd94bbb369b2..52760660d674 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);
+	copy_dtor = gso_skb->destructor == sock_wfree;
+	WARN_ON_ONCE(gso_skb->destructor && !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;
 }

[net-next PATCH 5/5] 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 1/5] udp: Record gso_segs when supporting UDP segmentation offload

[Eric Dumazet]

On 05/03/2018 05:33 PM, Alexander Duyck 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.
> 
> 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;
>  	}
>  
> 

Yes, this also fixes qdisc_pkt_len_init()

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

Re: [net-next PATCH 2/5] udp: Do not pass checksum or MSS as parameters

[Eric Dumazet]

On 05/03/2018 05:33 PM, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> 
> This patch is meant to be a start at cleaning up some of the UDP GSO
> segmentation code. Specifically we were passing mss and a recomputed
> checksum when we really didn't need to. The function itself could derive
> that information based on the already provided checksum, the length of the
> packet stored in the UDP header, and the gso_size.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
>  include/net/udp.h      |    3 +-
>  net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
>  net/ipv6/udp_offload.c |    7 +-----
>  3 files changed, 41 insertions(+), 30 deletions(-)
>

There are really a lot of changes in this patch (not mentioned in changelog).

Can you please split all this in small patches ?

Re: [net-next PATCH 4/5] udp: Do not copy destructor if one is not present

[Eric Dumazet]

On 05/03/2018 05:33 PM, Alexander Duyck 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>
> ---
>  net/ipv4/udp_offload.c |   19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index fd94bbb369b2..52760660d674 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);
> +	copy_dtor = gso_skb->destructor == sock_wfree;
> +	WARN_ON_ONCE(gso_skb->destructor && !copy_dtor);

Why not simply clear gso_skb->destructor only if copy_dtor is true ?

Then we can get rid of this WARN_ON_ONCE()

>  	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 3/5] udp: Add support for software checksum and GSO_PARTIAL with GSO offload

[Eric Dumazet]

On 05/03/2018 05:33 PM, Alexander Duyck 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;
> +
> 

I do not understand this code.

I am also seeing it in tcp, after commit 07b26c9454a2a ("gso: Support partial splitting at the frag_list pointer")

Presumably this broke tcp_gso_tstamp() , right ?

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

[Alexander Duyck]

On Thu, May 3, 2018 at 8:50 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/03/2018 05:33 PM, Alexander Duyck 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;
>> +
>>
>
> I do not understand this code.

In the case of GSO partial or frag_list segmentation we only split
along certain MSS sized boundaries and we leave the resulting frame as
a GSO skb. As a result the actual frame size is a multiple of MSS so
we were updating the mss by that value.

> I am also seeing it in tcp, after commit 07b26c9454a2a ("gso: Support partial splitting at the frag_list pointer")
>
> Presumably this broke tcp_gso_tstamp() , right ?

I'm not sure it did. The MSS multiplication is there because the frame
now spans multiple MSS chunks in one shot. So tcp_gso_tstamp should
still have the correct behavior, I think.

- Alex

Re: [net-next PATCH 4/5] udp: Do not copy destructor if one is not present

[Alexander Duyck]

On Thu, May 3, 2018 at 6:58 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/03/2018 05:33 PM, Alexander Duyck 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>
>> ---
>>  net/ipv4/udp_offload.c |   19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index fd94bbb369b2..52760660d674 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);
>> +     copy_dtor = gso_skb->destructor == sock_wfree;
>> +     WARN_ON_ONCE(gso_skb->destructor && !copy_dtor);
>
> Why not simply clear gso_skb->destructor only if copy_dtor is true ?
>
> Then we can get rid of this WARN_ON_ONCE()

Sounds good. I think I had code similar to that in an earlier version
of this patch, but when I started breaking things up I got rid of this
code until I realized I needed it due to a kernel panic triggered by
running the offload over a VXLAN from another namespace.

Thanks.

- Alex

Re: [net-next PATCH 2/5] udp: Do not pass checksum or MSS as parameters

[Alexander Duyck]

On Thu, May 3, 2018 at 6:56 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 05/03/2018 05:33 PM, Alexander Duyck wrote:
>> From: Alexander Duyck <alexander.h.duyck@intel.com>
>>
>> This patch is meant to be a start at cleaning up some of the UDP GSO
>> segmentation code. Specifically we were passing mss and a recomputed
>> checksum when we really didn't need to. The function itself could derive
>> that information based on the already provided checksum, the length of the
>> packet stored in the UDP header, and the gso_size.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>  include/net/udp.h      |    3 +-
>>  net/ipv4/udp_offload.c |   61 +++++++++++++++++++++++++++++++-----------------
>>  net/ipv6/udp_offload.c |    7 +-----
>>  3 files changed, 41 insertions(+), 30 deletions(-)
>>
>
> There are really a lot of changes in this patch (not mentioned in changelog).
>
> Can you please split all this in small patches ?

I'll see what I can do for the next version. I think I can come up
with an in-between step that should help to cut this down in size.

Thanks.

- Alex