Fwd: [2.6] ethertap and af_inet.c assertion failures

Fwd: [2.6] ethertap and af_inet.c assertion failures

[Simon Roscic]

(i forgot to cc this mail to this list, sorry)

----------  Forwarded Message  ----------

Subject: [2.6] ethertap and af_inet.c assertion failures
Date: Wednesday 22 December 2004 19:53
From: Simon Roscic <simon.roscic@chello.at>
To: linux-kernel@vger.kernel.org

(please cc me, as i'm not subscribed to lkml - thanks)

hi,

today i upgraded my kernel from 2.6.9-rc2 to 2.6.10-rc3-bk12, now i get the
following assertion failures while using the (closed source) phion vpn
client, the vpn client uses ethertap, there are no closed source kernel
modules or the like:

KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc)) failed at
net/ipv4/af_inet.c (150)

when the kernel prints out the above message the connection for the program
using the vpn gets stuck - it happens very often if i use rdesktop, but it
also happens when i just use ssh, so the bug may be triggered more often when
there is more traffic over the vpn tunnel.

i tried with some other 2.6 kernel releases:

up to and including 2.6.9-rc2: no problem
2.6.9-rc3      does not boot on my machine
2.6.9-rc4      assertion failed as explained above
2.6.9        assertion failed as explained above

so it seems ethertap got broken somewhere post 2.6.9-rc2.
any ideas what got changed post 2.6.9-rc2 wich might cause this?

thanks for looking at this problem, if i can provide more information, just
contact me.

bye,
 simon.

-------------------------------------------------------

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Tommy Christensen]

[-- Attachment #1: Type: text/plain, Size: 521 bytes --]

Simon Roscic wrote:
> hi,
> 
> today i upgraded my kernel from 2.6.9-rc2 to 2.6.10-rc3-bk12, now i get the
> following assertion failures while using the (closed source) phion vpn
> client, the vpn client uses ethertap, there are no closed source kernel
> modules or the like:
> 
> KERNEL: assertion (!atomic_read(&sk->sk_wmem_alloc)) failed at
> net/ipv4/af_inet.c (150)

netlink has messed up the send buffer accounting, when trimming a skb.

Patch below should fix this for ethertap. Could you give it a try?

-Tommy


[-- Attachment #2: ethertap.c.patch --]
[-- Type: text/plain, Size: 388 bytes --]

--- linux-2.6.10-bk14/drivers/net/ethertap.c	2004-12-24 22:34:26.000000000 +0100
+++ linux-2.6.10-work/drivers/net/ethertap.c	2005-01-11 22:18:19.113295324 +0100
@@ -207,8 +207,8 @@
 			return 0;
 		}
 	  	dev_kfree_skb(skb2);
-	}
-	/* ... but do not orphan it here, netlink does it in any case. */
+	} else
+		skb_orphan(skb);
 
 	lp->stats.tx_bytes+=skb->len;
 	lp->stats.tx_packets++;

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Simon Roscic]

Tommy Christensen wrote:
> netlink has messed up the send buffer accounting, when trimming a skb.
>
> Patch below should fix this for ethertap. Could you give it a try?
>
> -Tommy

Thanks a lot! Your patch fixed the bug.

bye,
 simon.

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Herbert Xu]

Tommy Christensen <tommy.christensen@tpack.net> wrote:
> 
> netlink has messed up the send buffer accounting, when trimming a skb.

You are spot on.  Dave, you were right too about checking for charged
skb's in netlink_trim.

Since the skb will be orphaned on its way to the destination anyway,
let's simply do that before the trimming.  This shouldn't lead to
skb leakage since the skb will either be charged to the destination
socket or freed.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Simon, please check if this fixes your problem or not.

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
--
===== net/netlink/af_netlink.c 1.66 vs edited =====
--- 1.66/net/netlink/af_netlink.c	2005-01-14 15:41:06 +11:00
+++ edited/net/netlink/af_netlink.c	2005-01-16 00:24:20 +11:00
@@ -629,7 +629,6 @@
 		}
 		return 1;
 	}
-	skb_orphan(skb);
 	skb_set_owner_r(skb, sk);
 	return 0;
 }
@@ -663,14 +662,11 @@
 
 static inline void netlink_trim(struct sk_buff *skb, int allocation)
 {
-	int delta = skb->end - skb->tail;
+	int delta;
 
-	/* If the packet is charged to a socket, the modification
-	 * of truesize below is illegal and will corrupt socket
-	 * buffer accounting state.
-	 */
-	BUG_ON(skb->list != NULL);
+	skb_orphan(skb);
 
+	delta = skb->end - skb->tail;
 	if (delta * 2 < skb->truesize)
 		return;
 	if (pskb_expand_head(skb, 0, -delta, allocation))
@@ -707,14 +703,12 @@
 	struct netlink_opt *nlk = nlk_sk(sk);
 #ifdef NL_EMULATE_DEV
 	if (nlk->handler) {
-		skb_orphan(skb);
 		nlk->handler(sk->sk_protocol, skb);
 		return 0;
 	} else
 #endif
 	if (atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf &&
 	    !test_bit(0, &nlk->state)) {
-                skb_orphan(skb);
 		skb_set_owner_r(skb, sk);
 		skb_queue_tail(&sk->sk_receive_queue, skb);
 		sk->sk_data_ready(sk, skb->len);

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Tommy Christensen]

Herbert Xu wrote:
> Since the skb will be orphaned on its way to the destination anyway,
> let's simply do that before the trimming.  This shouldn't lead to
> skb leakage since the skb will either be charged to the destination
> socket or freed.

Looks fine. This will solve the problem for other callers as well.

Shouldn't there be a check for skb_shared as well? Or are the
callers of netlink_unicast/broadcast supposed to avoid this.

-Tommy

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Simon Roscic]

Herbert Xu wrote:
> Simon, please check if this fixes your problem or not.

Yup, your patch fixes the problem. Thanks!

bye,
 simon.

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Herbert Xu]

On Sat, Jan 15, 2005 at 05:19:59PM +0100, Tommy Christensen wrote:
> 
> Shouldn't there be a check for skb_shared as well? Or are the
> callers of netlink_unicast/broadcast supposed to avoid this.

Had anyone been using shared skb's here before they would've got into
trouble a long time ago with calls such as skb_orphan in the path.

Even if they managed to do that and not notice then the pskb_expand_head
call in netlink_trim would've likely caught it as well.

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: Fwd: [2.6] ethertap and af_inet.c assertion failures

[Tommy Christensen]

Herbert Xu wrote:
> On Sat, Jan 15, 2005 at 05:19:59PM +0100, Tommy Christensen wrote:
> 
>>Shouldn't there be a check for skb_shared as well? Or are the
>>callers of netlink_unicast/broadcast supposed to avoid this.
> 
> 
> Had anyone been using shared skb's here before they would've got into
> trouble a long time ago with calls such as skb_orphan in the path.
> 
> Even if they managed to do that and not notice then the pskb_expand_head
> call in netlink_trim would've likely caught it as well.

Well, pskb_expand_head was added recently and isn't even always called.

Have a look at audit_log_drain() in kernel/audit.c. This code does:
   skb_get
   netlink_unicast
   ...
   kfree_skb

This is working just fine - until one day an over-sized skb comes along
and hits the BUG in pskb_expand_head ...

That particular example can easily be solved by re-arranging the code a
bit (I'll send a patch to someone).

Still, regarding netlink, I think the safe approach is to either handle
or disallow shared skb's. Anything in-between could appear to be working,
and then suddenly blow up on your production system. </paranoia>

-Tommy

[NETLINK] Do not netlink_unicast shared packet in kernel/audit.c

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Sat, Jan 15, 2005 at 09:20:42PM +0100, Tommy Christensen wrote:
> 
> Well, pskb_expand_head was added recently and isn't even always called.
> 
> Have a look at audit_log_drain() in kernel/audit.c. This code does:
>   skb_get
>   netlink_unicast
>   ...
>   kfree_skb
> 
> This is working just fine - until one day an over-sized skb comes along
> and hits the BUG in pskb_expand_head ...

You're always right :)
 
> Still, regarding netlink, I think the safe approach is to either handle
> or disallow shared skb's. Anything in-between could appear to be working,
> and then suddenly blow up on your production system. </paranoia>

kernel/audit.c seems to be the only caller that does this.  Strictly
speaking shared skb's are always illegal in netlink_unicast/netlink_broadcast
since we call functions such as skb_orphan which modifies the sk_buff
structure.  However, for callers such as kernel/audit.c that don't care
about those fields, it actually turns out to be harmless.

I'm hesitant to put an outright ban on it since it will force callers
such as kernel/audit.c to always do an skb_clone.

So here is a patch to handle shared skb's by cloning them if we're
going to do a pskb_expand_head.  This patch depends on the previous
one that moves skb_orphan into netlink_trim.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1275 bytes --]

--- linux-2.6/net/netlink/af_netlink.c.orig	2005-01-16 17:46:49.000000000 +1100
+++ linux-2.6/net/netlink/af_netlink.c	2005-01-16 19:00:51.000000000 +1100
@@ -660,7 +660,7 @@
 	sock_put(sk);
 }
 
-static inline void netlink_trim(struct sk_buff *skb, int allocation)
+static inline struct sk_buff *netlink_trim(struct sk_buff *skb, int allocation)
 {
 	int delta;
 
@@ -668,10 +668,20 @@
 
 	delta = skb->end - skb->tail;
 	if (delta * 2 < skb->truesize)
-		return;
-	if (pskb_expand_head(skb, 0, -delta, allocation))
-		return;
-	skb->truesize -= delta;
+		return skb;
+
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, allocation);
+		if (!nskb)
+			return skb;
+		kfree_skb(skb);
+		skb = nskb;
+	}
+
+	if (!pskb_expand_head(skb, 0, -delta, allocation))
+		skb->truesize -= delta;
+
+	return skb;
 }
 
 int netlink_unicast(struct sock *ssk, struct sk_buff *skb, u32 pid, int nonblock)
@@ -680,7 +690,7 @@
 	int err;
 	long timeo;
 
-	netlink_trim(skb, gfp_any());
+	skb = netlink_trim(skb, gfp_any());
 
 	timeo = sock_sndtimeo(ssk, nonblock);
 retry:
@@ -788,7 +798,7 @@
 	info.skb = skb;
 	info.skb2 = NULL;
 
-	netlink_trim(skb, allocation);
+	skb = netlink_trim(skb, allocation);
 
 	/* While we sleep in clone, do not allow to change socket list */
 

Re: [NETLINK] Do not netlink_unicast shared packet in kernel/audit.c

[Tommy Christensen]

Herbert Xu wrote:
> I'm hesitant to put an outright ban on it since it will force callers
> such as kernel/audit.c to always do an skb_clone.
> 
> So here is a patch to handle shared skb's by cloning them if we're
> going to do a pskb_expand_head.  This patch depends on the previous
> one that moves skb_orphan into netlink_trim.

Very nice, Herbert. This should be fine for everybody.

-Tommy

Re: [NETLINK] Do not netlink_unicast shared packet in kernel/audit.c

[David S. Miller]

On Sun, 16 Jan 2005 19:02:54 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> So here is a patch to handle shared skb's by cloning them if we're
> going to do a pskb_expand_head.  This patch depends on the previous
> one that moves skb_orphan into netlink_trim.

I think this patch is buggy.  At least in the final hunk, we
do this:

	info.skb = skb;
	...
	skb = netlink_trim( ... );

Now info.skb can be pointing to a freed up skb.

I'll apply the first patch though.

Re: Fwd: [2.6] ethertap and af_inet.c assertion failures

[David S. Miller]

On Sun, 16 Jan 2005 00:31:46 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Tommy Christensen <tommy.christensen@tpack.net> wrote:
> > 
> > netlink has messed up the send buffer accounting, when trimming a skb.
> 
> You are spot on.  Dave, you were right too about checking for charged
> skb's in netlink_trim.

It's good to know that I'm legitimately paranoid from time to
time :-)

> Since the skb will be orphaned on its way to the destination anyway,
> let's simply do that before the trimming.  This shouldn't lead to
> skb leakage since the skb will either be charged to the destination
> socket or freed.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Looks good, I'll apply this.

Thanks Herbert.

Re: [NETLINK] Do not netlink_unicast shared packet in kernel/audit.c

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Mon, Jan 17, 2005 at 01:36:40PM -0800, David S. Miller wrote:
> 
> I think this patch is buggy.  At least in the final hunk, we
> do this:
> 
> 	info.skb = skb;
> 	...
> 	skb = netlink_trim( ... );
> 
> Now info.skb can be pointing to a freed up skb.

Doh! Here is the corrected version.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1494 bytes --]

--- /home/gondolin/herbert/src/bk/linux-2.6/net/netlink/af_netlink.c.orig	2005-01-18 10:06:45.000000000 +1100
+++ /home/gondolin/herbert/src/bk/linux-2.6/net/netlink/af_netlink.c	2005-01-18 10:09:54.000000000 +1100
@@ -660,7 +660,7 @@
 	sock_put(sk);
 }
 
-static inline void netlink_trim(struct sk_buff *skb, int allocation)
+static inline struct sk_buff *netlink_trim(struct sk_buff *skb, int allocation)
 {
 	int delta;
 
@@ -668,10 +668,20 @@
 
 	delta = skb->end - skb->tail;
 	if (delta * 2 < skb->truesize)
-		return;
-	if (pskb_expand_head(skb, 0, -delta, allocation))
-		return;
-	skb->truesize -= delta;
+		return skb;
+
+	if (skb_shared(skb)) {
+		struct sk_buff *nskb = skb_clone(skb, allocation);
+		if (!nskb)
+			return skb;
+		kfree_skb(skb);
+		skb = nskb;
+	}
+
+	if (!pskb_expand_head(skb, 0, -delta, allocation))
+		skb->truesize -= delta;
+
+	return skb;
 }
 
 int netlink_unicast(struct sock *ssk, struct sk_buff *skb, u32 pid, int nonblock)
@@ -680,7 +690,7 @@
 	int err;
 	long timeo;
 
-	netlink_trim(skb, gfp_any());
+	skb = netlink_trim(skb, gfp_any());
 
 	timeo = sock_sndtimeo(ssk, nonblock);
 retry:
@@ -778,6 +788,8 @@
 	struct hlist_node *node;
 	struct sock *sk;
 
+	skb = netlink_trim(skb, allocation);
+
 	info.exclude_sk = ssk;
 	info.pid = pid;
 	info.group = group;
@@ -788,8 +800,6 @@
 	info.skb = skb;
 	info.skb2 = NULL;
 
-	netlink_trim(skb, allocation);
-
 	/* While we sleep in clone, do not allow to change socket list */
 
 	netlink_lock_table();

Re: [NETLINK] Do not netlink_unicast shared packet in kernel/audit.c

[David S. Miller]

On Tue, 18 Jan 2005 10:11:28 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Doh! Here is the corrected version.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Looks great, applied.  Thanks Herbert.