[PATCH] netlink: defer socket destruction a bit

[PATCH] netlink: defer socket destruction a bit

[Tommy Christensen]

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

In netlink_broadcast() we're sending shared skb's to netlink listeners
when possible (saves some copying). This is OK, since we hold the only
other reference to the skb.

However, this implies that we must drop our reference on the skb, before
allowing a receiving socket to disappear. Otherwise, the socket buffer
accounting is disrupted.


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

[-- Attachment #2: netlink-2.patch --]
[-- Type: text/plain, Size: 568 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:11:08.990990172 +0200
@@ -785,11 +785,12 @@
 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
 		do_one_broadcast(sk, &info);
 
+	kfree_skb(skb);
+
 	netlink_unlock_table();
 
 	if (info.skb2)
 		kfree_skb(info.skb2);
-	kfree_skb(skb);
 
 	if (info.delivered) {
 		if (info.congested && (allocation & __GFP_WAIT))

Re: [PATCH] netlink: defer socket destruction a bit

[Herbert Xu]

On Thu, May 12, 2005 at 12:19:02AM +0200, Tommy Christensen wrote:
>
> 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:11:08.990990172 +0200
> @@ -785,11 +785,12 @@
>  	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
>  		do_one_broadcast(sk, &info);
>  
> +	kfree_skb(skb);
> +
>  	netlink_unlock_table();
>  
>  	if (info.skb2)
>  		kfree_skb(info.skb2);
> -	kfree_skb(skb);

Good catch.  But doesn't this affect skb2 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: [PATCH] netlink: defer socket destruction a bit

[Tommy Christensen]

Herbert Xu wrote:
> On Thu, May 12, 2005 at 12:19:02AM +0200, Tommy Christensen wrote:
> 
>>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:11:08.990990172 +0200
>>@@ -785,11 +785,12 @@
>> 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
>> 		do_one_broadcast(sk, &info);
>> 
>>+	kfree_skb(skb);
>>+
>> 	netlink_unlock_table();
>> 
>> 	if (info.skb2)
>> 		kfree_skb(info.skb2);
>>-	kfree_skb(skb);
> 
> 
> Good catch.  But doesn't this affect skb2 as well?

No, skb2 cannot be shared with a listening socket. As I read the code,
it can only be non-null when delivery has failed.

-Tommy

Re: [PATCH] netlink: defer socket destruction a bit

[Herbert Xu]

On Thu, May 12, 2005 at 12:35:06AM +0200, Tommy Christensen wrote:
> 
> No, skb2 cannot be shared with a listening socket. As I read the code,
> it can only be non-null when delivery has failed.

What about this code path:

1) skb2 = skb, refcnt++.
2) Devliered to socket 1.
3) Socket 1 frees skb through recvmsg.
4) skb2 = skb, refcnt++.
5) Delivery fails.

Now skb2 is identical to skb and they both refer to socket 1.

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: [PATCH] netlink: defer socket destruction a bit

[Tommy Christensen]

On Thu, 2005-05-12 at 01:03, Herbert Xu wrote:
> On Thu, May 12, 2005 at 12:35:06AM +0200, Tommy Christensen wrote:
> > 
> > No, skb2 cannot be shared with a listening socket. As I read the code,
> > it can only be non-null when delivery has failed.
> 
> What about this code path:
> 
> 1) skb2 = skb, refcnt++.
> 2) Devliered to socket 1.
> 3) Socket 1 frees skb through recvmsg.
> 4) skb2 = skb, refcnt++.
> 5) Delivery fails.
> 
> Now skb2 is identical to skb and they both refer to socket 1.

You're right, Herbert.  The interception by recvmsg has to be
considered in this case also.  Tricky.

I moved the call to skb_orphan in the other patch, as you
suggested.  I think that also makes this patch safe as it is.

Right?

-Tommy

Re: [PATCH] netlink: defer socket destruction a bit

[Herbert Xu]

On Thu, May 12, 2005 at 11:57:01AM +0200, Tommy Christensen wrote:
> 
> I moved the call to skb_orphan in the other patch, as you
> suggested.  I think that also makes this patch safe as it is.
> 
> Right?

Indeed it is.  This also means that we don't hold onto the skb's
share of rmalloc quota for an excessive amount of time if the
number of broadcast sockets is large.

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: [PATCH] netlink: defer socket destruction a bit

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 12 May 2005 20:36:39 +1000

> On Thu, May 12, 2005 at 11:57:01AM +0200, Tommy Christensen wrote:
> > 
> > I moved the call to skb_orphan in the other patch, as you
> > suggested.  I think that also makes this patch safe as it is.
> > 
> > Right?
> 
> Indeed it is.  This also means that we don't hold onto the skb's
> share of rmalloc quota for an excessive amount of time if the
> number of broadcast sockets is large.

Ok, I think I got all the patches straight.  All of Tommy's
patches combined together look like this diff in my tree.
Please double check it.

Thanks.

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -735,11 +735,15 @@ static inline int do_one_broadcast(struc
 
 	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) {
@@ -785,11 +789,12 @@ int netlink_broadcast(struct sock *ssk, 
 	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
 		do_one_broadcast(sk, &info);
 
+	kfree_skb(skb);
+
 	netlink_unlock_table();
 
 	if (info.skb2)
 		kfree_skb(info.skb2);
-	kfree_skb(skb);
 
 	if (info.delivered) {
 		if (info.congested && (allocation & __GFP_WAIT))

Re: [PATCH] netlink: defer socket destruction a bit

[Herbert Xu]

On Thu, May 19, 2005 at 01:08:59PM -0700, David S. Miller wrote:
> 
> Ok, I think I got all the patches straight.  All of Tommy's
> patches combined together look like this diff in my tree.
> Please double check it.

Yes this is the one.

> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -735,11 +735,15 @@ static inline int do_one_broadcast(struc
>  
>  	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) {
> @@ -785,11 +789,12 @@ int netlink_broadcast(struct sock *ssk, 
>  	sk_for_each_bound(sk, node, &nl_table[ssk->sk_protocol].mc_list)
>  		do_one_broadcast(sk, &info);
>  
> +	kfree_skb(skb);
> +
>  	netlink_unlock_table();
>  
>  	if (info.skb2)
>  		kfree_skb(info.skb2);
> -	kfree_skb(skb);
>  
>  	if (info.delivered) {
>  		if (info.congested && (allocation & __GFP_WAIT))
-- 
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: [PATCH] netlink: defer socket destruction a bit

[Tommy Christensen]

Herbert Xu wrote:
> On Thu, May 19, 2005 at 01:08:59PM -0700, David S. Miller wrote:
> 
>>Ok, I think I got all the patches straight.  All of Tommy's
>>patches combined together look like this diff in my tree.
>>Please double check it.
> 
> Yes this is the one.

Yup. This combination fixes both problems.

Thanks,
Tommy

Re: [PATCH] netlink: defer socket destruction a bit

[David S. Miller]

From: Tommy Christensen <tommy.christensen@tpack.net>
Subject: Re: [PATCH] netlink: defer socket destruction a bit
Date: Fri, 20 May 2005 01:15:37 +0200

> Herbert Xu wrote:
> > On Thu, May 19, 2005 at 01:08:59PM -0700, David S. Miller wrote:
> > 
> >>Ok, I think I got all the patches straight.  All of Tommy's
> >>patches combined together look like this diff in my tree.
> >>Please double check it.
> > 
> > Yes this is the one.
> 
> Yup. This combination fixes both problems.

Thanks a lot for double checking, both of you :-)