Skip to contentSkip to conversation

netlink: defer socket destruction a bit

PATCH In review Tommy Christensen 3 participants 10 messages May 11, 2005 – May 19, 2005

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Tommy Christensen

author
#0 lore raw

PATCH 1/1 netlink: defer socket destruction a bit

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 <[email protected]>
net/netlink/af_netlink.c +2 -1
diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c
@@ -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))
#1 lore raw
Quoted 5 lines · Tommy Christensen

diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c

net/netlink/af_netlink.c line 792

Quoted 10 lines
net/netlink/af_netlink.c +2 -1
@@ -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,

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
#2 lore raw
Quoted 13 lines · Herbert Xu
Quoted 11 lines · Tommy Christensen

diff -ru linux-2.6.12-rc4/net/netlink/af_netlink.c linux-2.6.12-work/net/netlink/af_netlink.c

net/netlink/af_netlink.c +2 -0
@@ -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();
Quoted 6 lines

net/netlink/af_netlink.c line 792

Quoted 3 lines
net/netlink/af_netlink.c +0 -1
 	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

#3 lore raw
Quoted 3 lines · Tommy Christensen

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,

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
Quoted 7 lines · Herbert Xu
Quoted 3 lines · Tommy Christensen

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:

Quoted 7 lines

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

#5 lore raw
Quoted 5 lines · Tommy Christensen

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,

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
#6 lore raw

From: Herbert Xu <[email protected]>
Date: Thu, 12 May 2005 20:36:39 +1000

Quoted 10 lines · Tommy Christensen
Quoted 5 lines

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.

net/netlink/af_netlink.c +9 -4
@@ -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))
#7 lore raw
Quoted 4 lines · David S. Miller

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.

Quoted 36 lines
net/netlink/af_netlink.c +9 -4
@@ -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))
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
#8 lore raw
Quoted 7 lines · Herbert Xu
Quoted 3 lines · David S. Miller

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

maintainer
#9 lore raw

From: Tommy Christensen <[email protected]>
Subject: Re: [PATCH] netlink: defer socket destruction a bit
Date: Fri, 20 May 2005 01:15:37 +0200

Quoted 10 lines
Quoted 7 lines · Herbert Xu
Quoted 3 lines · David S. Miller

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 :-)