Skip to contentSkip to conversation

pskb_expand_head() bugfix

RFC In review Hideo AOKI 3 participants 13 messages Mar 25, 2008 – Mar 29, 2008

Subsystems (5)
Networking Network drivers IPv4/IPv6 Networking IPsec +1 more subsystems
lore
Share & tools
lore
author
#2 lore raw

RFC PATCH 2/2 pskb_expand_head() updates truesize

In reply to Hideo AOKI

This patch revises pskb_expand_head() to update truesize.

Applying this change, caller functions don't have to update truesize by themselves. Moreover, skb can keep correct truesize even if allocation size is aligned in pskb_expand_head().

This patch also removes updating truesize in callers since the update doesn't need anymore.

[Note: Updating truesize in audit_expand() is fixed by anther patch.]

Signed-off-by Hideo Aoki <[email protected]>

---

 core/skbuff.c        |    6 ++++++
 ipv4/ipcomp.c        |    1 -
 ipv6/ipcomp6.c       |    1 -
 netlink/af_netlink.c |    3 +--
 4 files changed, 7 insertions(+), 4 deletions(-)
net/core/skbuff.c +6 -0
diff -pruN net-2.6-mod-p1/net/core/skbuff.c net-2.6-mod-p2/net/core/skbuff.c
@@ -701,6 +701,12 @@ int pskb_expand_head(struct sk_buff *skb
 
 	skb_release_data(skb);
 
+#ifdef NET_SKBUFF_DATA_USES_OFFSET
+	skb->truesize += (size - skb->end);
+#else
+	skb->truesize += (size - (skb->end - skb->head));
+#endif
+
 	off = (data + nhead) - skb->head;
 
 	skb->head     = data;
net/ipv4/ipcomp.c +0 -1
diff -pruN net-2.6-mod-p1/net/ipv4/ipcomp.c net-2.6-mod-p2/net/ipv4/ipcomp.c
@@ -64,7 +64,6 @@ static int ipcomp_decompress(struct xfrm
 	if (err)
 		goto out;
 
-	skb->truesize += dlen - plen;
 	__skb_put(skb, dlen - plen);
 	skb_copy_to_linear_data(skb, scratch, dlen);
 out:
net/ipv6/ipcomp6.c +0 -1
diff -pruN net-2.6-mod-p1/net/ipv6/ipcomp6.c net-2.6-mod-p2/net/ipv6/ipcomp6.c
@@ -108,7 +108,6 @@ static int ipcomp6_input(struct xfrm_sta
 		goto out_put_cpu;
 	}
 
-	skb->truesize += dlen - plen;
 	__skb_put(skb, dlen - plen);
 	skb_copy_to_linear_data(skb, scratch, dlen);
 	err = nexthdr;
net/netlink/af_netlink.c +1 -2
diff -pruN net-2.6-mod-p1/net/netlink/af_netlink.c net-2.6-mod-p2/net/netlink/af_netlink.c
@@ -835,8 +835,7 @@ static inline struct sk_buff *netlink_tr
 		skb = nskb;
 	}
 
-	if (!pskb_expand_head(skb, 0, -delta, allocation))
-		skb->truesize -= delta;
+	pskb_expand_head(skb, 0, -delta, allocation);
 
 	return skb;
 }
Signature · 1 line
Hitachi Computer Products (America) Inc.

Hideo AOKI

author
#0 lore raw

cover pskb_expand_head() bugfix

Hello,

This patch set fixes a potential bug in pskb_expand_head().

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

To solve above problems, this patch set does the following:

- updating truesize in pskb_expand_head()
- removing turesize update due to pskb_expand_head() from caller functions

This patch set can be applied to net-2.6.

Comments and suggestions are welcome.

Best regards,
Hideo Aoki

Signature · 1 line
Hitachi Computer Products (America) Inc.
author
#1 lore raw

RFC PATCH 1/2 revert audit_expand()

This patch reverts commit 406a1d868001423c85a3165288e566e65f424fe6 to apply bug fix of the root cause to kernel.

I leave the fix of pskb_expand_head() call in the commit. Changing 2nd argument from skb_headroom(skb) to 0 is correct, because audit_expand() should expand only tail room as its definition.

Signed-off-by Hideo Aoki <[email protected]>

---

 audit.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)
kernel/audit.c +1 -5
diff -pruN net-2.6/kernel/audit.c net-2.6-mod-p1/kernel/audit.c
@@ -1117,17 +1117,13 @@ struct audit_buffer *audit_log_start(str
 static inline int audit_expand(struct audit_buffer *ab, int extra)
 {
 	struct sk_buff *skb = ab->skb;
-	int oldtail = skb_tailroom(skb);
 	int ret = pskb_expand_head(skb, 0, extra, ab->gfp_mask);
-	int newtail = skb_tailroom(skb);
 
 	if (ret < 0) {
 		audit_log_lost("out of memory in audit_expand");
 		return 0;
 	}
-
-	skb->truesize += newtail - oldtail;
-	return newtail;
+	return skb_tailroom(skb);
 }
 
 /*
Signature · 1 line
Hitachi Computer Products (America) Inc.
maintainer
#3 lore raw
In reply to Hideo AOKI
Quoted 11 lines · Hideo AOKI

Hello,

This patch set fixes a potential bug in pskb_expand_head().

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

Drivers usually aren't supposed to change truesize so doing this would actually create bugs.

Quoted 4 lines

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

That should be fixable by making sure that nhead + ntail is aligned.

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#4 lore raw

Hello Herbert,

Thank you for your quick response.

Quoted 6 lines · Herbert Xu
Quoted 5 lines · Hideo AOKI

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

Quoted 6 lines
Quoted 3 lines

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

Drivers usually aren't supposed to change truesize so doing this would actually create bugs.

I understood your point.

Since keeping correct truesize is important to network memory accounting, I want to fix network subsystem part at least.

I think that it is inconvenient for caller functions to need updateing truesize by themselves. How about this change to avoid the inconvenience?

- Current implementation is renamed to __pskb_expand_head().
- Drivers call __pskb_expand_head() instead of pskb_expand_head().
- New pskb_expand_head() updates truesize after calling __pskb_expand_head().

Or, should I simply add truesize calculation after pskb_expand_head() calls which change truesize?

Quoted 7 lines
Quoted 4 lines

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

That should be fixable by making sure that nhead + ntail is aligned.

I see.

Regards,
Hideo

Signature · 1 line
Hitachi Computer Products (America) Inc.
maintainer
#5 lore raw
Quoted 3 lines · Hideo AOKI

Or, should I simply add truesize calculation after pskb_expand_head() calls which change truesize?

Can you do an audit first and tell us how many sites are currently buggy?

Thanks,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#9 lore raw
In reply to Hideo AOKI

From: Hideo AOKI <[email protected]>
Date: Tue, 25 Mar 2008 14:39:04 -0400

Quoted 4 lines

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

Quoted 8 lines

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

Drivers may not update truesize, because as I explained in Tokyo a fundamental issue is the case where SKB is charged already to a socket. In such a case, skb->truesize may not be modified without corrupting socket write queue allocation state.

And at these very spots in drivers, the transmit path, the SKB is very likely to be owned by a socket.

maintainer
#7 lore raw
In reply to Hideo AOKI

From: Hideo AOKI <[email protected]>
Date: Wed, 26 Mar 2008 16:47:35 -0400

Quoted 2 lines

I think that it is inconvenient for caller functions to need updateing truesize by themselves.

Most cases what I can see are in spots where skb->truesize cannot be modified because the SKB is possibly charged to a socket.

In these limited situations where skb->truesize adjustments really are needed, and legal, it is no harm to open code things.

author
#6 lore raw
In reply to Herbert Xu

Hello,

Quoted 6 lines · Herbert Xu
Quoted 2 lines · Hideo AOKI

Or, should I simply add truesize calculation after pskb_expand_head() calls which change truesize?

Can you do an audit first and tell us how many sites are currently buggy?

Sure. I find 24 spots.

Here is the list of caller functions which don't update turesize or do update turesize without alignment. But I don't confirm yet if alignment is really needed in each case.

* macro: 1
- Missing truesize update

        linux/skbuff.h:1369:	__skb_cow()

* kernel: 1
- Missing alignment:

        audit.c:1121:		audit_expand()

* ipv4: 5
- Missing truesize update:

        ipvs/ip_vs_app.c:597:	ip_vs_skb_replace()
        netfilter/nf_nat_helper.c:119: enlarge_skb()
        netfilter.c:72: 	ip_route_me_harder()
	netfilter.c:105: 	ip_xfrm_me_harder()

- Missing alignment:

        ipcomp.c:63:		ipcomp_decompress()

* core: 5
- Missing truesize update:

        skbuff.c:741:		skb_realloc_headroom()
	skbuff.c:840: 		skb_pad()
        skbuff.c:979: 		__pskb_pull_tail()
        skbuff.c:2393: 		skb_cow_data()
        pktgen.c:2402: 		process_ipsec()

* netlink: 1
- Missing alignment:

        af_netlink.c:838:	netlink_trim()

* ipv6: 1
- Missing alignment:

    	ipcomp6.c:106:		ipcomp6_input()

* netfilter: 1
- Missing alignment:

        xt_TCPMSS.c:122:	tcpmss_mangle_packet()

* max80211: 8
- Missing truesize update:

        tx.c:1246:		ieee80211_master_start_xmit()
        tx.c:1503:		ieee80211_subif_start_xmit()
	wpa.c:103:		ieee80211_tx_h_michael_mic_add()
	wpa.c:207:		tkip_encrypt_skb()
	wpa.c:458:		ccmp_encrypt_skb()
	wep.c:99:		ieee80211_wep_add_iv()
	rx.c:146:		ieee80211_rx_monitor()
	rx.c:905:		ieee80211_rx_h_defragment()

* xfrm: 1
- Missing truesize update:

	xfrm_output.c:30:	xfrm_state_check_space()

Many thanks,
Hideo

--
Hitachi Computer Products (America) Inc.

author
#10 lore raw
In reply to David Miller

Hello David,

Quoted 16 lines · David Miller

From: Hideo AOKI <[email protected]>
Date: Tue, 25 Mar 2008 14:39:04 -0400

Quoted 4 lines

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

Quoted 8 lines

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

Quoted 8 lines

Drivers may not update truesize, because as I explained in Tokyo a fundamental issue is the case where SKB is charged already to a socket. In such a case, skb->truesize may not be modified without corrupting socket write queue allocation state.

And at these very spots in drivers, the transmit path, the SKB is very likely to be owned by a socket.

Thank you for explaining.

OK. I don't change driver code to avoid double charge.

Best regards,
Hideo

--
Hitachi Computer Products (America) Inc.

maintainer
#11 lore raw

From: Hideo AOKI <[email protected]>
Date: Fri, 28 Mar 2008 21:02:41 -0400

Quoted 25 lines

Hello David,

Quoted 22 lines · David Miller

From: Hideo AOKI <[email protected]>
Date: Tue, 25 Mar 2008 14:39:04 -0400

Quoted 4 lines

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

Quoted 8 lines

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

Drivers may not update truesize, because as I explained in Tokyo a fundamental issue is the case where SKB is charged already to a socket. In such a case, skb->truesize may not be modified without corrupting socket write queue allocation state.

Quoted 6 lines
Quoted 2 lines

And at these very spots in drivers, the transmit path, the SKB is very likely to be owned by a socket.

Thank you for explaining.

OK. I don't change driver code to avoid double charge.

This also applies to the output path, which I would say is about %95 of the "truesize buggy" functions you quoted in your previous email.

So we are back to where we started when Herbert and I started replying in this thread, in that there is one (audit) or perhaps 1 or 2 more other cases that need truesize adjustment, nothing more.

Audit is fixed, and if you can find other relevant cases they can be fixed locally.

We cannot change pskb_expand_head() to make truesize adjustments, it would break things in %95 of the places where it is called.

author
#8 lore raw
In reply to David Miller

Hello,

Quoted 6 lines · David Miller

From: Hideo AOKI <[email protected]>
Date: Wed, 26 Mar 2008 16:47:35 -0400

Quoted 2 lines

I think that it is inconvenient for caller functions to need updateing truesize by themselves.

Quoted 7 lines

Most cases what I can see are in spots where skb->truesize cannot be modified because the SKB is possibly charged to a socket.

In these limited situations where skb->truesize adjustments really are needed, and legal, it is no harm to open code things.

Thank you for the comments.

I understood that updating truesize by caller wasn't problem. And, I may misunderstand the spots where truesize can't be changed.

Then, I make patch to update truesize for each network subsystem and ask a review of the patch.

Best regards,
Hideo

--
Hitachi Computer Products (America) Inc.

author
#12 lore raw
In reply to David Miller
Quoted 40 lines · David Miller

From: Hideo AOKI <[email protected]>
Date: Fri, 28 Mar 2008 21:02:41 -0400

Quoted 24 lines

Hello David,

Quoted 21 lines · David Miller

From: Hideo AOKI <[email protected]>
Date: Tue, 25 Mar 2008 14:39:04 -0400

Quoted 4 lines

Current pskb_expand_head() doesn't change truesize, while it reallocates memory. Then, if argument nhead or ntail aren't 0, caller must update truesize.

Quoted 8 lines

We had this bug at audit_expand() in January and fixed it as commit 406a1d868001423c85a3165288e566e65f424fe6. However, some drivers and subsystems still use pskb_expand_head() without updating truesize.

In addition, there is another problem to update truesise. Since pskb_expand_head() aligns memory size before reallocation, caller functions may not update turesize correctly if they just add nhaad and ntail to turesize.

Drivers may not update truesize, because as I explained in Tokyo a fundamental issue is the case where SKB is charged already to a socket. In such a case, skb->truesize may not be modified without corrupting socket write queue allocation state.

Quoted 5 lines
Quoted 2 lines

And at these very spots in drivers, the transmit path, the SKB is very likely to be owned by a socket.

Thank you for explaining.

OK. I don't change driver code to avoid double charge.

This also applies to the output path, which I would say is about %95 of the "truesize buggy" functions you quoted in your previous email.

So we are back to where we started when Herbert and I started replying in this thread, in that there is one (audit) or perhaps 1 or 2 more other cases that need truesize adjustment, nothing more.

Quoted 5 lines

Audit is fixed, and if you can find other relevant cases they can be fixed locally.

We cannot change pskb_expand_head() to make truesize adjustments, it would break things in %95 of the places where it is called.

Thank you for you quick response.

I'll try to find the cases.

Regards,

--
Hitachi Computer Products (America) Inc.