Cleanup tcp_try_coalesce, fix module lockup on ixgbe

This patch addresses several issues in the way we were tracking the truesize in tcp_try_coalesce.

First it was using ksize which prevents us from having a 0 sized head frag and getting a usable result. To resolve that this patch uses the end pointer which is set based off either ksize, or the frag_size supplied in build_skb. This allows us to compute the original truesize of the entire buffer and remove that value leaving us with just what was added as pages.

The second issue was the use of skb->len if there is a mergeable head frag. We should only need to remove the size of an data aligned sk_buff from our current skb->truesize to compute the delta for a buffer with a reused head. By using skb->len the value of truesize was being artificially reduced which means that head frags could use more memory than buffers using standard allocations.

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

Cc Eric Dumazet <[email protected]>

Cc Jeff Kirsher <[email protected]>

---

 net/ipv4/tcp_input.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

net/ipv4/tcp_input.c +3 -5

index c6f78e2..3cb273a 100644
@@ -4565,12 +4565,10 @@ merge:
 	if (skb_headlen(from) == 0 &&
 	    (skb_shinfo(to)->nr_frags +
 	     skb_shinfo(from)->nr_frags <= MAX_SKB_FRAGS)) {
-		WARN_ON_ONCE(from->head_frag);
-		delta = from->truesize - ksize(from->head) -
-			SKB_DATA_ALIGN(sizeof(struct sk_buff));
-
-		WARN_ON_ONCE(delta < len);
+		delta = from->truesize -
+			SKB_TRUESIZE(skb_end_pointer(from) - from->head);
 copyfrags:
+		WARN_ON_ONCE(delta < len);
 		memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
 		       skb_shinfo(from)->frags,
 		       skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));

Quoted 17 lines

This patch addresses several issues in the way we were tracking the truesize in tcp_try_coalesce.

First it was using ksize which prevents us from having a 0 sized head frag and getting a usable result. To resolve that this patch uses the end pointer which is set based off either ksize, or the frag_size supplied in build_skb. This allows us to compute the original truesize of the entire buffer and remove that value leaving us with just what was added as pages.

The second issue was the use of skb->len if there is a mergeable head frag. We should only need to remove the size of an data aligned sk_buff from our current skb->truesize to compute the delta for a buffer with a reused head. By using skb->len the value of truesize was being artificially reduced which means that head frags could use more memory than buffers using standard allocations.

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

Acked-by Eric Dumazet <[email protected]>

From: Eric Dumazet<[email protected]>

GRO is very optimistic in skb truesize estimates, only taking into account the used part of fragments.

Be conservative, and use more precise computation, so that bloated GRO skbs can be collapsed eventually.

Signed-off-by Eric Dumazet<[email protected]>

Cc Alexander Duyck<[email protected]>

Cc Jeff Kirsher<[email protected]>

---
  net/core/skbuff.c |   11 ++++++++---
  1 file changed, 8 insertions(+), 3 deletions(-)

net/core/skbuff.c +7 -2

index 9e8caa0..e1f8bba 100644
@@ -2871,6 +2871,7 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
  	unsigned int len = skb_gro_len(skb);
  	unsigned int offset = skb_gro_offset(skb);
  	unsigned int headlen = skb_headlen(skb);
+	unsigned int delta_truesize;
 
  	if (p->len + len>= 65536)
  		return -E2BIG;

@@ -2900,11 +2901,14 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
  		frag->page_offset += offset;
  		skb_frag_size_sub(frag, offset);
 
+		/* all fragments truesize : remove (head size + sk_buff) */
+		delta_truesize = skb->truesize - SKB_TRUESIZE(skb_end_pointer(skb) - skb->head);
+
  		skb->truesize -= skb->data_len;
  		skb->len -= skb->data_len;
  		skb->data_len = 0;
 
-		NAPI_GRO_CB(skb)->free = 1;
+		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE;
  		goto done;
  	} else if (skb->head_frag) {
  		int nr_frags = pinfo->nr_frags;

@@ -2929,6 +2933,7 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
  		memcpy(frag + 1, skbinfo->frags, sizeof(*frag) * skbinfo->nr_frags);
  		/* We dont need to clear skbinfo->nr_frags here */
 
+		delta_truesize = skb->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff));
  		NAPI_GRO_CB(skb)->free = NAPI_GRO_FREE_STOLEN_HEAD;
  		goto done;
  	} else if (skb_gro_len(p) != pinfo->gso_size)

@@ -2971,7 +2976,7 @@ int skb_gro_receive(struct sk_buff **head, struct sk_buff *skb)
  	p = nskb;
 
  merge:
-	p->truesize += skb->truesize - len;
+	delta_truesize = skb->truesize;
  	if (offset>  headlen) {
  		unsigned int eat = offset - headlen;
 

From: Eric Dumazet <[email protected]>

GRO is very optimistic in skb truesize estimates, only taking into account the used part of fragments.

Be conservative, and use more precise computation, so that bloated GRO skbs can be collapsed eventually.

Signed-off-by Eric Dumazet <[email protected]>

This change reorders the code related to the use of an skb->head_frag so it is placed before we check the rest of the frags. This allows the code to read more linearly instead of like some sort of loop.

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

Cc Eric Dumazet <[email protected]>

Cc Jeff Kirsher <[email protected]>

---

 net/ipv4/tcp_input.c |   42 +++++++++++++++++++++++++-----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

Quoted 5 lines

This change reorders the code related to the use of an skb->head_frag so it is placed before we check the rest of the frags. This allows the code to read more linearly instead of like some sort of loop.

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

Acked-by Eric Dumazet <[email protected]>

This change cleans up the last bits of tcp_try_coalesce so that we only need one goto which jumps to the end of the function. The idea is to make the code more readable by putting things in a linear order so that we start execution at the top of the function, and end it at the bottom.

I also made a slight tweak to the code for handling frags when we are a clone. Instead of making it an if (clone) loop else nr_frags = 0 I changed the logic so that if (!clone) we just set the number of frags to 0 which disables the for loop anyway.

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

Cc Eric Dumazet <[email protected]>

Cc Jeff Kirsher <[email protected]>

---

 net/ipv4/tcp_input.c |   55 ++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 26 deletions(-)

Quoted 11 lines

This change cleans up the last bits of tcp_try_coalesce so that we only need one goto which jumps to the end of the function. The idea is to make the code more readable by putting things in a linear order so that we start execution at the top of the function, and end it at the bottom.

I also made a slight tweak to the code for handling frags when we are a clone. Instead of making it an if (clone) loop else nr_frags = 0 I changed the logic so that if (!clone) we just set the number of frags to 0 which disables the for loop anyway.

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

Thanks a lot Alex, this patch serie looks very good.

Acked-by Eric Dumazet <[email protected]>

While testing the TCP changes I had to fix an issue in order to be able to load and unload the module.

The recent patch that added thermal sensor support added a use after free bug on module unload with an 82598 adapter in the system. To resolve the issue I have updated the code so that when we free the info_kobj we set it back to NULL.

I suspect there are likely other bugs present, but I will leave that for another patch that can undergo more testing.

I am submitting this directly to net-next since this fixes a fairly serious bug that will lock up the ixgbe module until the system is rebooted.

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