[RFC] [NET] [0/2] pskb_expand_head() bugfix

[RFC] [NET] [0/2] pskb_expand_head() bugfix

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

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

-- 
Hitachi Computer Products (America) Inc.

[RFC PATCH] [NET] [1/2] revert audit_expand()

[Hideo AOKI]

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 <haoki@redhat.com>
---

 audit.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff -pruN net-2.6/kernel/audit.c net-2.6-mod-p1/kernel/audit.c
--- net-2.6/kernel/audit.c	2008-03-24 09:58:26.000000000 -0400
+++ net-2.6-mod-p1/kernel/audit.c	2008-03-24 14:22:18.000000000 -0400
@@ -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);
 }
 
 /*
-- 
Hitachi Computer Products (America) Inc.

[RFC PATCH] [NET] [2/2] pskb_expand_head() updates truesize

[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 <haoki@redhat.com>
---

 core/skbuff.c        |    6 ++++++
 ipv4/ipcomp.c        |    1 -
 ipv6/ipcomp6.c       |    1 -
 netlink/af_netlink.c |    3 +--
 4 files changed, 7 insertions(+), 4 deletions(-)

diff -pruN net-2.6-mod-p1/net/core/skbuff.c net-2.6-mod-p2/net/core/skbuff.c
--- net-2.6-mod-p1/net/core/skbuff.c	2008-02-25 09:05:46.000000000 -0500
+++ net-2.6-mod-p2/net/core/skbuff.c	2008-03-24 14:22:55.000000000 -0400
@@ -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;
diff -pruN net-2.6-mod-p1/net/ipv4/ipcomp.c net-2.6-mod-p2/net/ipv4/ipcomp.c
--- net-2.6-mod-p1/net/ipv4/ipcomp.c	2008-03-05 05:12:30.000000000 -0500
+++ net-2.6-mod-p2/net/ipv4/ipcomp.c	2008-03-24 14:25:47.000000000 -0400
@@ -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:
diff -pruN net-2.6-mod-p1/net/ipv6/ipcomp6.c net-2.6-mod-p2/net/ipv6/ipcomp6.c
--- net-2.6-mod-p1/net/ipv6/ipcomp6.c	2008-03-05 05:12:30.000000000 -0500
+++ net-2.6-mod-p2/net/ipv6/ipcomp6.c	2008-03-24 14:22:55.000000000 -0400
@@ -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;
diff -pruN net-2.6-mod-p1/net/netlink/af_netlink.c net-2.6-mod-p2/net/netlink/af_netlink.c
--- net-2.6-mod-p1/net/netlink/af_netlink.c	2008-02-05 12:00:28.000000000 -0500
+++ net-2.6-mod-p2/net/netlink/af_netlink.c	2008-03-24 14:22:55.000000000 -0400
@@ -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;
 }
-- 
Hitachi Computer Products (America) Inc.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Herbert Xu]

On Tue, Mar 25, 2008 at 02:39:04PM -0400, Hideo AOKI wrote:
> 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.

> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Hideo AOKI]

Hello Herbert,

Thank you for your quick response.

Herbert Xu wrote:
> On Tue, Mar 25, 2008 at 02:39:04PM -0400, Hideo AOKI wrote:
>>
>> 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.

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

-- 
Hitachi Computer Products (America) Inc.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Herbert Xu]

On Wed, Mar 26, 2008 at 04:47:35PM -0400, Hideo AOKI wrote:
>
> 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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Hideo AOKI]

Hello,

Herbert Xu wrote:
> On Wed, Mar 26, 2008 at 04:47:35PM -0400, Hideo AOKI wrote:
>> 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.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[David Miller]

From: Hideo AOKI <haoki@redhat.com>
Date: Wed, 26 Mar 2008 16:47:35 -0400

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

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Hideo AOKI]

Hello,

David Miller wrote:
> From: Hideo AOKI <haoki@redhat.com>
> Date: Wed, 26 Mar 2008 16:47:35 -0400
> 
>> 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.

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.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[David Miller]

From: Hideo AOKI <haoki@redhat.com>
Date: Tue, 25 Mar 2008 14:39:04 -0400

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

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.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Hideo AOKI]

Hello David,

David Miller wrote:
> From: Hideo AOKI <haoki@redhat.com>
> Date: Tue, 25 Mar 2008 14:39:04 -0400
> 
>> 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.
> 
> 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.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[David Miller]

From: Hideo AOKI <haoki@redhat.com>
Date: Fri, 28 Mar 2008 21:02:41 -0400

> Hello David,
> 
> David Miller wrote:
> > From: Hideo AOKI <haoki@redhat.com>
> > Date: Tue, 25 Mar 2008 14:39:04 -0400
> > 
> >> 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.
> > 
> > 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.

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.

Re: [RFC] [NET] [0/2] pskb_expand_head() bugfix

[Hideo AOKI]

David Miller wrote:
> From: Hideo AOKI <haoki@redhat.com>
> Date: Fri, 28 Mar 2008 21:02:41 -0400
> 
>> Hello David,
>>
>> David Miller wrote:
>>> From: Hideo AOKI <haoki@redhat.com>
>>> Date: Tue, 25 Mar 2008 14:39:04 -0400
>>>
>>>> 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.
>>> 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.
> 
> 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.

Thank you for you quick response.

I'll try to find the cases.

Regards,

--
Hitachi Computer Products (America) Inc.