assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Ken-ichirou MATSUZAWA]

[-- Attachment #1: Type: Text/Plain, Size: 438 bytes --]

 Hello.

Everytime on shutting down, the subject message is logged.  
It seems that the reason is do_one_broadcast may preempt
netlink_recvmsg. Added patch to 2.6.12-rc4 seems resolving this
problem.

But I'm not a kernel hacker. I don't understand design, detail and
the intenstion of struct netlink_broadcast_data, so that I hope
someone to do more tidy work. In addition, please check 2.4 kernel
and pfkey_broadcast_one too.

Thanks.


[-- Attachment #2: af_netlink.diff --]
[-- Type: Text/Plain, Size: 1503 bytes --]

--- net/netlink/af_netlink.c.orig	2005-05-10 21:03:11.000000000 +0900
+++ net/netlink/af_netlink.c	2005-05-10 21:10:39.000000000 +0900
@@ -721,7 +721,7 @@
 	int congested;
 	int delivered;
 	int allocation;
-	struct sk_buff *skb, *skb2;
+	struct sk_buff *skb;
 };
 
 static inline int do_one_broadcast(struct sock *sk,
@@ -729,6 +729,7 @@
 {
 	struct netlink_sock *nlk = nlk_sk(sk);
 	int val;
+	struct sk_buff *skb;
 
 	if (p->exclude_sk == sk)
 		goto out;
@@ -741,25 +742,21 @@
 		goto out;
 	}
 
-	sock_hold(sk);
-	if (p->skb2 == NULL) {
-		if (atomic_read(&p->skb->users) != 1) {
-			p->skb2 = skb_clone(p->skb, p->allocation);
-		} else {
-			p->skb2 = p->skb;
-			atomic_inc(&p->skb->users);
-		}
-	}
-	if (p->skb2 == NULL) {
+	skb = skb_clone(p->skb, p->allocation);
+	if (skb == NULL) {
 		netlink_overrun(sk);
 		/* Clone failed. Notify ALL listeners. */
 		p->failure = 1;
-	} else if ((val = netlink_broadcast_deliver(sk, p->skb2)) < 0) {
+		goto out;
+	}
+
+	sock_hold(sk);
+	if ((val = netlink_broadcast_deliver(sk, skb)) < 0) {
 		netlink_overrun(sk);
+		kfree_skb(skb);
 	} else {
 		p->congested |= val;
 		p->delivered = 1;
-		p->skb2 = NULL;
 	}
 	sock_put(sk);
 
@@ -784,7 +781,6 @@
 	info.delivered = 0;
 	info.allocation = allocation;
 	info.skb = skb;
-	info.skb2 = NULL;
 
 	/* While we sleep in clone, do not allow to change socket list */
 
@@ -795,8 +791,6 @@
 
 	netlink_unlock_table();
 
-	if (info.skb2)
-		kfree_skb(info.skb2);
 	kfree_skb(skb);
 
 	if (info.delivered) {

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Tue, May 10, 2005 at 12:43:32PM +0000, Ken-ichirou MATSUZAWA wrote:
> 
> Everytime on shutting down, the subject message is logged.  
> It seems that the reason is do_one_broadcast may preempt
> netlink_recvmsg. Added patch to 2.6.12-rc4 seems resolving this
> problem.

Thanks for the patch.  To help me understand the problem better,
could you please tell me:

1) Is this reproducible under 2.6.12-rc4 without your patch?
2) What application is holding the netlink socket?
3) Is your machine SMP, PREEMPT, or both?

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: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

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

On Wed, May 11, 2005 at 08:07:51AM +1000, herbert wrote:
> On Tue, May 10, 2005 at 12:43:32PM +0000, Ken-ichirou MATSUZAWA wrote:
> > 
> > Everytime on shutting down, the subject message is logged.  
> > It seems that the reason is do_one_broadcast may preempt
> > netlink_recvmsg. Added patch to 2.6.12-rc4 seems resolving this
> > problem.
> 
> Thanks for the patch.  To help me understand the problem better,
> could you please tell me:

I think I understand your patch now.  What's happening is that

1) The skb is sent to socket 1.
2) Someone does a recvmsg on socket 1 and drops the ref on the skb.
   Note that the rmalloc is not returned at this point since the
   skb is still referenced.
3) The same skb is now sent to socket 2.

I agree with your solution except that we should still do the skb_get
if we can.  Here is my version where we only do the skb_get at the
start.

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

Please let me know whether this works for you 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

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

--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -735,12 +735,7 @@ static inline int do_one_broadcast(struc
 
 	sock_hold(sk);
 	if (p->skb2 == NULL) {
-		if (atomic_read(&p->skb->users) != 1) {
-			p->skb2 = skb_clone(p->skb, p->allocation);
-		} else {
-			p->skb2 = p->skb;
-			atomic_inc(&p->skb->users);
-		}
+		p->skb2 = skb_clone(p->skb, p->allocation);
 	}
 	if (p->skb2 == NULL) {
 		netlink_overrun(sk);
@@ -776,7 +771,8 @@ int netlink_broadcast(struct sock *ssk, 
 	info.delivered = 0;
 	info.allocation = allocation;
 	info.skb = skb;
-	info.skb2 = NULL;
+	info.skb2 = skb;
+	skb_get(skb);
 
 	/* While we sleep in clone, do not allow to change socket list */
 

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Ken-ichirou MATSUZAWA]

 Hello.

From: Herbert Xu <herbert@gondor.apana.org.au>
Subject: Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)
Date: Wed, 11 May 2005 10:58:36 +1000

> Please let me know whether this works for you or not.

It worked, thank you.

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Tommy Christensen]

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

Herbert Xu wrote:
> I think I understand your patch now.  What's happening is that
> 
> 1) The skb is sent to socket 1.
> 2) Someone does a recvmsg on socket 1 and drops the ref on the skb.
>    Note that the rmalloc is not returned at this point since the
>    skb is still referenced.
> 3) The same skb is now sent to socket 2.

Ahh, even I get the point now.
I actually thought this was caused by another race. More on that later.

> I agree with your solution except that we should still do the skb_get
> if we can.  Here is my version where we only do the skb_get at the
> start.

What about an alternative fix, that avoids even more cloning (where
possible)? This resurrects the skb_orphan call that was moved out, last
time we had 'shared-skb troubles'. It is practically a no-op in the
common case, but still prevents the possible race with recvmsg.
(And I have a weakness for one-line-fixes). :-)


Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>

[-- Attachment #2: netlink-1.patch --]
[-- Type: text/plain, Size: 506 bytes --]

diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c
--- linux-2.6.12-rc4/net/netlink/af_netlink.c	2005-05-11 11:10:20.000000000 +0200
+++ linux-2.6.12-work/net/netlink/af_netlink.c	2005-05-12 00:08:33.634344658 +0200
@@ -697,6 +697,7 @@
 
 	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: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Thu, May 12, 2005 at 12:17:19AM +0200, Tommy Christensen wrote:
> 
> I actually thought this was caused by another race. More on that later.

Please elaborate.
 
> What about an alternative fix, that avoids even more cloning (where
> possible)? This resurrects the skb_orphan call that was moved out, last
> time we had 'shared-skb troubles'. It is practically a no-op in the
> common case, but still prevents the possible race with recvmsg.
> (And I have a weakness for one-line-fixes). :-)

Yep this is much better.  Thanks Tommy.

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

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: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Thu, May 12, 2005 at 12:17:19AM +0200, Tommy Christensen wrote:
> 
> What about an alternative fix, that avoids even more cloning (where
> possible)? This resurrects the skb_orphan call that was moved out, last
> time we had 'shared-skb troubles'. It is practically a no-op in the
> common case, but still prevents the possible race with recvmsg.
> (And I have a weakness for one-line-fixes). :-)

BTW, we could also move the skb_orphan to the skb_get path since the
cloned packets don't need the orphan call.

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: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Tommy Christensen]

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

On Thu, 2005-05-12 at 01:17, Herbert Xu wrote:
> BTW, we could also move the skb_orphan to the skb_get path since the
> cloned packets don't need the orphan call.

Yes, I like this better.  Good idea.

Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>


[-- Attachment #2: Type: text/x-patch, Size: 687 bytes --]

diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c
--- linux-2.6.12-rc4/net/netlink/af_netlink.c	2005-05-11 11:10:20.000000000 +0200
+++ linux-2.6.12-work/net/netlink/af_netlink.c	2005-05-12 11:51:57.427312857 +0200
@@ -735,11 +735,13 @@
 
 	sock_hold(sk);
 	if (p->skb2 == NULL) {
-		if (atomic_read(&p->skb->users) != 1) {
+		if (skb_shared(p->skb)) {
 			p->skb2 = skb_clone(p->skb, p->allocation);
 		} else {
-			p->skb2 = p->skb;
-			atomic_inc(&p->skb->users);
+			p->skb2 = skb_get(p->skb);
+			/* skb ownership may have been set when
+			   delivered to a previous socket. */
+			skb_orphan(p->skb2);
 		}
 	}
 	if (p->skb2 == NULL) {

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Thu, May 12, 2005 at 11:58:07AM +0200, Tommy Christensen wrote:
> 
> Yes, I like this better.  Good idea.

I see that you've given up on the minimalism :)

> Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>

It looks good to me too except for that comment of yours :)

> diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c
> --- linux-2.6.12-rc4/net/netlink/af_netlink.c	2005-05-11 11:10:20.000000000 +0200
> +++ linux-2.6.12-work/net/netlink/af_netlink.c	2005-05-12 11:51:57.427312857 +0200
> @@ -735,11 +735,13 @@
>  
>  	sock_hold(sk);
>  	if (p->skb2 == NULL) {
> -		if (atomic_read(&p->skb->users) != 1) {
> +		if (skb_shared(p->skb)) {
>  			p->skb2 = skb_clone(p->skb, p->allocation);
>  		} else {
> -			p->skb2 = p->skb;
> -			atomic_inc(&p->skb->users);
> +			p->skb2 = skb_get(p->skb);
> +			/* skb ownership may have been set when
> +			   delivered to a previous socket. */

For new comments please use the following style which is more standard
in the kernel these days:

/*
 * blah blah blah ....
 * for comments that don't fit on a single line.
 */

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: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Tommy Christensen]

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

On Thu, 2005-05-12 at 12:42, Herbert Xu wrote:
> For new comments please use the following style which is more standard
> in the kernel these days:

Sure, no problem.

Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>


[-- Attachment #2: Type: text/x-patch, Size: 699 bytes --]

diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c
--- linux-2.6.12-rc4/net/netlink/af_netlink.c	2005-05-11 11:10:20.000000000 +0200
+++ linux-2.6.12-work/net/netlink/af_netlink.c	2005-05-12 12:55:32.374430076 +0200
@@ -735,11 +735,15 @@
 
 	sock_hold(sk);
 	if (p->skb2 == NULL) {
-		if (atomic_read(&p->skb->users) != 1) {
+		if (skb_shared(p->skb)) {
 			p->skb2 = skb_clone(p->skb, p->allocation);
 		} else {
-			p->skb2 = p->skb;
-			atomic_inc(&p->skb->users);
+			p->skb2 = skb_get(p->skb);
+			/*
+			 * skb ownership may have been set when
+			 * delivered to a previous socket.
+			 */
+			skb_orphan(p->skb2);
 		}
 	}
 	if (p->skb2 == NULL) {

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Thu, May 12, 2005 at 12:58:03PM +0200, Tommy Christensen wrote:
> 
> Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>

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

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 12 May 2005 08:22:10 +1000

> > What about an alternative fix, that avoids even more cloning (where
> > possible)? This resurrects the skb_orphan call that was moved out, last
> > time we had 'shared-skb troubles'. It is practically a no-op in the
> > common case, but still prevents the possible race with recvmsg.
> > (And I have a weakness for one-line-fixes). :-)
> 
> Yep this is much better.  Thanks Tommy.
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks a lot guys.

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 12 May 2005 21:07:38 +1000

> On Thu, May 12, 2005 at 12:58:03PM +0200, Tommy Christensen wrote:
> > 
> > Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Hmmm, should I undo Tommy's one-liner skb_orphan() change
then add this one instead?

Re: assertion (!atomic_read(&sk->sk_rmem_alloc)) failed at net/netlink/af_netlink.c (122)

[Herbert Xu]

On Thu, May 19, 2005 at 12:49:21PM -0700, David S. Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Thu, 12 May 2005 21:07:38 +1000
> 
> > On Thu, May 12, 2005 at 12:58:03PM +0200, Tommy Christensen wrote:
> > > 
> > > Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>
> > 
> > Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Hmmm, should I undo Tommy's one-liner skb_orphan() change
> then add this one instead?

Please do since the first one had a subtle bug.

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