[v2 PATCH 0/4] Cleanup tcp_try_coalesce, fix module lockup on ixgbe

This set represents the original patch I had done for trying to cleanup
tcp_try_coalesce broken into 3 patches.  The first one illustrates how I
believe we should be updating the truesize based on either the size of
sk_buff in the case of head reuse, or sk_buff plus the size of the head in
the case of a headlen of 0.

The other two patches go through and reorder things so there isn't as much
need for gotos.  I believe it makes the code much more readable since it
starts at the top and finishes at the bottom instead of looping through the
entire code path a few times.

The last patch addresses an issue I ran into on ixgbe.  It turns out the
recent HWMON patch added a but on 82598 adapters that caused the driver to
get hung in module unload.  I figured I would submit it directly since it
is a small change to fix an issue that could have a larger impact.

On that note I am going to sleep now and will respond to any reviews and/or
comments in the morning.

---

Alexander Duyck (4):
      ixgbe: Fix use after free on module remove
      tcp: move stats merge to the end of tcp_try_coalesce
      tcp: Move code related to head frag in tcp_try_coalesce
      tcp: Fix truesize accounting in tcp_try_coalesce


 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    4 +
 net/ipv4/tcp_input.c                           |   83 +++++++++++++-----------
 2 files changed, 49 insertions(+), 38 deletions(-)

-- 

Thanks,

Alex

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 <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <redacted>
---

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

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c6f78e2..3cb273a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

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

@@ -4600,7 +4598,7 @@ copyfrags:
 		skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
 				   page, offset, skb_headlen(from));
 		*fragstolen = true;
-		delta = len; /* we dont know real truesize... */
+		delta = from->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff));
 		goto copyfrags;
 	}
 	return false;

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 <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <redacted>
---

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

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 3cb273a..41fa5df 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -4562,9 +4562,31 @@ merge:
 	if (skb_has_frag_list(to) || skb_has_frag_list(from))
 		return false;
 
-	if (skb_headlen(from) == 0 &&
-	    (skb_shinfo(to)->nr_frags +
-	     skb_shinfo(from)->nr_frags <= MAX_SKB_FRAGS)) {
+	if (skb_headlen(from) != 0) {
+		struct page *page;
+		unsigned int offset;
+
+		if (skb_shinfo(to)->nr_frags +
+		    skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
+			return false;
+
+		if (!from->head_frag || skb_cloned(from))
+			return false;
+
+		delta = from->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff));
+
+		page = virt_to_head_page(from->head);
+		offset = from->data - (unsigned char *)page_address(page);
+
+		skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
+				   page, offset, skb_headlen(from));
+		*fragstolen = true;
+		goto copyfrags;
+	} else {
+		if (skb_shinfo(to)->nr_frags +
+		    skb_shinfo(from)->nr_frags > MAX_SKB_FRAGS)
+			return false;
+
 		delta = from->truesize -
 			SKB_TRUESIZE(skb_end_pointer(from) - from->head);
 copyfrags:

@@ -4587,20 +4609,6 @@ copyfrags:
 		to->data_len += len;
 		goto merge;
 	}
-	if (from->head_frag && !skb_cloned(from)) {
-		struct page *page;
-		unsigned int offset;
-
-		if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
-			return false;
-		page = virt_to_head_page(from->head);
-		offset = from->data - (unsigned char *)page_address(page);
-		skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
-				   page, offset, skb_headlen(from));
-		*fragstolen = true;
-		delta = from->truesize - SKB_DATA_ALIGN(sizeof(struct sk_buff));
-		goto copyfrags;
-	}
 	return false;
 }
 

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 <redacted>
Cc: Jeff Kirsher <redacted>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
index aa41fb7..f81c166 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c

@@ -185,8 +185,10 @@ static void ixgbe_sysfs_del_adapter(struct ixgbe_adapter *adapter)
 		hwmon_device_unregister(adapter->ixgbe_hwmon_buff.device);
 #endif /* CONFIG_IXGBE_HWMON */
 
-	if (adapter->info_kobj != NULL)
+	if (adapter->info_kobj != NULL) {
 		kobject_put(adapter->info_kobj);
+		adapter->info_kobj = NULL;
+	}
 }
 
 /* called from ixgbe_main.c */

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 <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <redacted>
---

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

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 41fa5df..84e69e0 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c

@@ -4548,15 +4548,13 @@ static bool tcp_try_coalesce(struct sock *sk,
 	int i, delta, len = from->len;
 
 	*fragstolen = false;
+
 	if (tcp_hdr(from)->fin || skb_cloned(to))
 		return false;
+
 	if (len <= skb_tailroom(to)) {
 		BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
-merge:
-		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
-		TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
-		TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
-		return true;
+		goto merge;
 	}
 
 	if (skb_has_frag_list(to) || skb_has_frag_list(from))

@@ -4581,7 +4579,6 @@ merge:
 		skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
 				   page, offset, skb_headlen(from));
 		*fragstolen = true;
-		goto copyfrags;
 	} else {
 		if (skb_shinfo(to)->nr_frags +
 		    skb_shinfo(from)->nr_frags > MAX_SKB_FRAGS)

@@ -4589,27 +4586,33 @@ merge:
 
 		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));
-		skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
-
-		if (skb_cloned(from))
-			for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
-				skb_frag_ref(from, i);
-		else
-			skb_shinfo(from)->nr_frags = 0;
-
-		to->truesize += delta;
-		atomic_add(delta, &sk->sk_rmem_alloc);
-		sk_mem_charge(sk, delta);
-		to->len += len;
-		to->data_len += len;
-		goto merge;
 	}
-	return false;
+
+	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));
+	skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
+
+	if (!skb_cloned(from))
+		skb_shinfo(from)->nr_frags = 0;
+
+	/* if the skb is cloned this does nothing since we set nr_frags to 0 */
+	for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
+		skb_frag_ref(from, i);
+
+	to->truesize += delta;
+	atomic_add(delta, &sk->sk_rmem_alloc);
+	sk_mem_charge(sk, delta);
+	to->len += len;
+	to->data_len += len;
+
+merge:
+	NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
+	TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
+	TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
+	return true;
 }
 
 static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)

On Thu, 2012-05-03 at 00:18 -0700, Alexander Duyck wrote:

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

On Thu, 2012-05-03 at 00:19 -0700, Alexander Duyck wrote:

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 <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <redacted>
---

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

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

On Thu, 2012-05-03 at 00:19 -0700, Alexander Duyck wrote:

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 <redacted>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <redacted>
---

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

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

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

From: Eric Dumazet <redacted>
Date: Thu, 03 May 2012 09:48:54 +0200

On Thu, 2012-05-03 at 00:18 -0700, Alexander Duyck wrote:

quoted

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

 ...

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

Applied.

From: Eric Dumazet <redacted>
Date: Thu, 03 May 2012 09:50:56 +0200

On Thu, 2012-05-03 at 00:19 -0700, Alexander Duyck wrote:

quoted

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

 ...

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

Applied.

From: Eric Dumazet <redacted>
Date: Thu, 03 May 2012 09:52:45 +0200

On Thu, 2012-05-03 at 00:19 -0700, Alexander Duyck wrote:

quoted

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

 ...

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

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

Applied.

From: Alexander Duyck <redacted>
Date: Thu, 03 May 2012 00:19:14 -0700

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

Applied.

From: Eric Dumazet <edumazet@google.com>

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 <edumazet@google.com>
Cc: Alexander Duyck <redacted>
Cc: Jeff Kirsher <redacted>
---
 net/core/skbuff.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9e8caa0..e1f8bba 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

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

@@ -2991,7 +2996,7 @@ merge:
 done:
 	NAPI_GRO_CB(p)->count++;
 	p->data_len += len;
-	p->truesize += len;
+	p->truesize += delta_truesize;
 	p->len += len;
 
 	NAPI_GRO_CB(skb)->same_flow = 1;

On 05/03/2012 02:33 AM, Eric Dumazet wrote:

Couldn't sleep so I figured I would review some patches and maybe get a 
few more written before the sun came up.   I was actually thinking of 
trying to get to this before I logged in.  Looks like you have this one 
taken care of already so I will go take care of skb_head_is_locked.

Acked-by: Alexander Duyck <redacted>

From: Eric Dumazet <redacted>
Date: Thu, 03 May 2012 11:33:21 +0200

From: Eric Dumazet <edumazet@google.com>

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 <edumazet@google.com>

Applied.