New: possible circular locking #0: (sk_lock-AF_PACKET){--..}, at: [<c1279838>] sock_setsockopt+0x12b/0x4a4

Bugme-new Andrew Morton 5 participants 9 messages Jan 27, 2009 – Jan 30, 2009 1 message not in the archive

Subsystems (3)

Networking Network drivers Packet sockets

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author

2009-01-27 21:44 UTC #1 lore raw

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 25 lines · On Tue, 20 Jan 2009 10:16:38 -0800 (PST) [email protected] wrote:

http://bugzilla.kernel.org/show_bug.cgi?id=12515

           Summary: possible circular locking #0:  (sk_lock-AF_PACKET){--
                    ..}, at: [<c1279838>] sock_setsockopt+0x12b/0x4a4
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.29-rc1-git4
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IPV4
        AssignedTo: [email protected]
        ReportedBy: [email protected]

Latest working kernel version:
Earliest failing kernel version:
Distribution: Gentoo Linux
Hardware Environment: ASUS L3C/S laptop
Software Environment:
Problem Description:

Quoted 3 lines

Steps to reproduce: I have been reinstalling some apps while started tcpdump. It immediately hit the bug.

More info at the link. Vegard did some analysis, had a shot at fixing it, but it seems that he missed.

There has been some discussion under subject
"Re: [PATCH] net: fix setsockopt() locking errors"
and I hope it is archived somewhere via [email protected] . If not, Jarek Poplawski and Peter Zijlstra have some clues what to do and I am waiting for Vegard Nossum to give me another patch to test.

Here is some part of the discussion ,hopefully the most important part:

Quoted 28 lines · Peter Zijlstra

Quoted 28 lines · Jarek Poplawski
Quoted 11 lines
Quoted 11 lines · Martin MOKREJŠ
Quoted 3 lines
Quoted 3 lines
The patch really did not help:
http://bugzilla.kernel.org/show_bug.cgi?id=12515#c5
Martin
Actually, there is a little change: the warning triggerd in another place (sock_setsockopt() -> sk_attach_filter()). So we could go deeper with these changes, but I'm not sure this is the right way to fix.
It looks like the scenario is very old, but probably wasn't reported (maybe there is some lockdep improvement):
Yes, they likely are very old, and yes we added a lockdep annotation to copy_to/from_user() to catch these.
Quoted 11 lines
Quoted 11 lines
A) sys_mmap2() -> mm->mmap_sem -> packet_mmap() -> sk_lock
B) sock_setsockopt() -> sk_lock -> copy_from_user() -> mm->mmap_sem
packet_mmap() (net/packet/af_packet.c) seems to be the only place in net to implement mmap method, and using this lock order btw. On the other hand copy_from_user() could be more popular under sk_lock, and I'm not sure these changes are necessary.
Since I don't know enough neither sock/packet nor sys_mmap, I guess some advice would be precious. It looks like Peter Zijlstra solved similar problems in nfs, so I CC him.

Quoted 6 lines

Quoted 6 lines
The NFS/sunrpc case was special in that it did copy_to/from_kernel, that is, it never actually touched user memory -- we taught the might_fault() annotation about that.
Can't you simply do the copy_from_user() before you take the sk_lock?

Since it's really needed, and Vegard started doing it like this, I guess he will try to add the missing pieces.

Thanks again,
Jarek P.

Quoted 37 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 25 lines · On Tue, 20 Jan 2009 10:16:38 -0800 (PST) [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=12515
           Summary: possible circular locking #0:  (sk_lock-AF_PACKET){--
                    ..}, at: [<c1279838>] sock_setsockopt+0x12b/0x4a4
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.29-rc1-git4
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IPV4
        AssignedTo: [email protected]
        ReportedBy: [email protected]
Latest working kernel version:
Earliest failing kernel version:
Distribution: Gentoo Linux
Hardware Environment: ASUS L3C/S laptop
Software Environment:
Problem Description:
Quoted 3 lines
Steps to reproduce: I have been reinstalling some apps while started tcpdump. It immediately hit the bug.
More info at the link. Vegard did some analysis, had a shot at fixing it, but it seems that he missed.

Martin MOKREJŠ

2009-01-28 01:15 UTC #3 lore raw

Quoted 6 lines · Martin MOKREJŠ

There has been some discussion under subject
"Re: [PATCH] net: fix setsockopt() locking errors"
and I hope it is archived somewhere via [email protected] . If not, Jarek Poplawski and Peter Zijlstra have some clues what to do and I am waiting for Vegard Nossum to give me another patch to test.

I have tested the patch below in bugzilla and cannot reproduce anymore. dmesg(1) output in bugzilla.

Martin

------- Comment #8 from [email protected] 2009-01-27 14:38 ------- Created an attachment (id=20018)
--> (http://bugzilla.kernel.org/attachment.cgi?id=20018&action=view)
[PATCH] net: fix setsockopt() locking errors #2

Andrew, I just missed a case because the actual call to copy_from_user was in a different file altogether.

There might be more, but I didn't really have the time to look for everything yet.

In the meantime, I give you this patch, which includes the SO_ATTACH_FILTER socket option, which is what was reported to fail with the first patch applied.

I also don't REALLY think this is a regression, it's just lockdep that got smarter (or the insertion of a might_fault() something something made it more likely to show up -- I think this was info from Peter, but I don't remember accurately).

Quoted 85 lines

Here is some part of the discussion ,hopefully the most important part:
Quoted 25 lines · Peter Zijlstra
Quoted 25 lines · Jarek Poplawski
Quoted 10 lines
Quoted 10 lines · Martin MOKREJŠ
Quoted 3 lines
Quoted 3 lines
The patch really did not help:
http://bugzilla.kernel.org/show_bug.cgi?id=12515#c5
Martin
Actually, there is a little change: the warning triggerd in another place (sock_setsockopt() -> sk_attach_filter()). So we could go deeper with these changes, but I'm not sure this is the right way to fix.
It looks like the scenario is very old, but probably wasn't reported (maybe there is some lockdep improvement):
Yes, they likely are very old, and yes we added a lockdep annotation to copy_to/from_user() to catch these.
Quoted 11 lines
Quoted 11 lines
A) sys_mmap2() -> mm->mmap_sem -> packet_mmap() -> sk_lock
B) sock_setsockopt() -> sk_lock -> copy_from_user() -> mm->mmap_sem
packet_mmap() (net/packet/af_packet.c) seems to be the only place in net to implement mmap method, and using this lock order btw. On the other hand copy_from_user() could be more popular under sk_lock, and I'm not sure these changes are necessary.
Since I don't know enough neither sock/packet nor sys_mmap, I guess some advice would be precious. It looks like Peter Zijlstra solved similar problems in nfs, so I CC him.
Quoted 6 lines
Quoted 6 lines
The NFS/sunrpc case was special in that it did copy_to/from_kernel, that is, it never actually touched user memory -- we taught the might_fault() annotation about that.
Can't you simply do the copy_from_user() before you take the sk_lock?
Since it's really needed, and Vegard started doing it like this, I guess he will try to add the missing pieces.
Thanks again,
Jarek P.
Quoted 36 lines · Andrew Morton
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 25 lines · On Tue, 20 Jan 2009 10:16:38 -0800 (PST) [email protected] wrote:
http://bugzilla.kernel.org/show_bug.cgi?id=12515
           Summary: possible circular locking #0:  (sk_lock-AF_PACKET){--
                    ..}, at: [<c1279838>] sock_setsockopt+0x12b/0x4a4
           Product: Networking
           Version: 2.5
     KernelVersion: 2.6.29-rc1-git4
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IPV4
        AssignedTo: [email protected]
        ReportedBy: [email protected]
Latest working kernel version:
Earliest failing kernel version:
Distribution: Gentoo Linux
Hardware Environment: ASUS L3C/S laptop
Software Environment:
Problem Description:
Quoted 3 lines
Steps to reproduce: I have been reinstalling some apps while started tcpdump. It immediately hit the bug.
More info at the link. Vegard did some analysis, had a shot at fixing it, but it seems that he missed.

Herbert Xu

2009-01-30 12:49 UTC #4 lore raw

Quoted 11 lines · Herbert Xu

Well, doing the copy under sk_lock is pretty common through all protocols. So I think it'd be safer to change the other path, which is doing the odd thing here, i.e., ->mmap() grabbing the socket lock while holding mmap_sem.
In fact, it would appear that we don't really need the socket lock in ->mmap() since it only needs to ensure that pg_vec* doesn't get yanked or changed. So this patch should work:
packet: Avoid lock_sock in mmap handler

Dave pointed out that a spin lock is illegal for this purpose as vm_insert_page can do a GFP_KERNEL allocation. So I've added a mutex for this.

I've also widened the critical section in packet_set_ring since we need the mapped check to be within it.

packet: Avoid lock_sock in mmap handler

As the mmap handler gets called under mmap_sem, and we may grab mmap_sem elsewhere under the socket lock to access user data, we should avoid grabbing the socket lock in the mmap handler.

Since the only thing we care about in the mmap handler is for pg_vec* to be invariant, i.e., to exclude packet_set_ring, we can achieve this by simply using sk_receive_queue.lock.

I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.

Signed-off-by Herbert Xu <[email protected]>

net/packet/af_packet.c +7 -2

index 5f94db2..9454d4a 100644
@@ -77,6 +77,7 @@
 #include <linux/poll.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/mutex.h>
 
 #ifdef CONFIG_INET
 #include <net/inet_common.h>

@@ -175,6 +176,7 @@ struct packet_sock {
 #endif
 	struct packet_type	prot_hook;
 	spinlock_t		bind_lock;
+	struct mutex		pg_vec_lock;
 	unsigned int		running:1,	/* prot_hook is attached*/
 				auxdata:1,
 				origdev:1;

@@ -1069,6 +1071,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
 	 */
 
 	spin_lock_init(&po->bind_lock);
+	mutex_init(&po->pg_vec_lock);
 	po->prot_hook.func = packet_rcv;
 
 	if (sock->type == SOCK_PACKET)

@@ -1865,6 +1868,7 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req, int closing
 	synchronize_net();
 
 	err = -EBUSY;
+	mutex_lock(&po->pg_vec_lock);
 	if (closing || atomic_read(&po->mapped) == 0) {
 		err = 0;
 #define XC(a, b) ({ __typeof__ ((a)) __t; __t = (a); (a) = (b); __t; })

@@ -1886,6 +1890,7 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req, int closing
 		if (atomic_read(&po->mapped))
 			printk(KERN_DEBUG "packet_mmap: vma is busy: %d\n", atomic_read(&po->mapped));
 	}
+	mutex_unlock(&po->pg_vec_lock);
 
 	spin_lock(&po->bind_lock);
 	if (was_running && !po->running) {

@@ -1918,7 +1923,7 @@ static int packet_mmap(struct file *file, struct socket *sock, struct vm_area_st
 
 	size = vma->vm_end - vma->vm_start;
 
-	lock_sock(sk);
+	mutex_lock(&po->pg_vec_lock);
 	if (po->pg_vec == NULL)
 		goto out;
 	if (size != po->pg_vec_len*po->pg_vec_pages*PAGE_SIZE)

@@ -1941,7 +1946,7 @@ static int packet_mmap(struct file *file, struct socket *sock, struct vm_area_st
 	err = 0;
 
 out:
-	release_sock(sk);
+	mutex_unlock(&po->pg_vec_lock);
 	return err;
 }
 #endif

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

Jarek Poplawski

2009-01-30 13:56 UTC #5 lore raw

On Fri, Jan 30, 2009 at 11:49:47PM +1100, Herbert Xu wrote:
...

Quoted 3 lines

Dave pointed out that a spin lock is illegal for this purpose as vm_insert_page can do a GFP_KERNEL allocation. So I've added a mutex for this.

...

Quoted 2 lines

I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.

So, Dave is stronger than the temptation...

Jarek P.

Herbert Xu

2009-01-30 22:01 UTC #6 lore raw

Quoted 5 lines · Jarek Poplawski

Quoted 3 lines
I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.
So, Dave is stronger than the temptation...

Heh, that sentence should be removed from the description.

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

David Miller

maintainer

2009-01-30 22:05 UTC #7 lore raw

From: Herbert Xu <[email protected]>
Date: Sat, 31 Jan 2009 09:01:28 +1100

Quoted 8 lines · Jarek Poplawski

Quoted 5 lines
Quoted 3 lines
I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.
So, Dave is stronger than the temptation...
Heh, that sentence should be removed from the description.

I might leave it in there for laughs :-)

Martin MOKREJŠ

2009-01-30 17:17 UTC #8 lore raw

In reply to Herbert Xu

This patch works for me.
Thanks
M.

Quoted 96 lines · Herbert Xu

Quoted 10 lines · Herbert Xu
Well, doing the copy under sk_lock is pretty common through all protocols. So I think it'd be safer to change the other path, which is doing the odd thing here, i.e., ->mmap() grabbing the socket lock while holding mmap_sem.
In fact, it would appear that we don't really need the socket lock in ->mmap() since it only needs to ensure that pg_vec* doesn't get yanked or changed. So this patch should work:
packet: Avoid lock_sock in mmap handler
Dave pointed out that a spin lock is illegal for this purpose as vm_insert_page can do a GFP_KERNEL allocation. So I've added a mutex for this.
I've also widened the critical section in packet_set_ring since we need the mapped check to be within it.
packet: Avoid lock_sock in mmap handler
As the mmap handler gets called under mmap_sem, and we may grab mmap_sem elsewhere under the socket lock to access user data, we should avoid grabbing the socket lock in the mmap handler.
Since the only thing we care about in the mmap handler is for pg_vec* to be invariant, i.e., to exclude packet_set_ring, we can achieve this by simply using sk_receive_queue.lock.
I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.
Signed-off-by Herbert Xu <[email protected]>
net/packet/af_packet.c +7 -2
index 5f94db2..9454d4a 100644
@@ -77,6 +77,7 @@
 #include <linux/poll.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/mutex.h>
 
 #ifdef CONFIG_INET
 #include <net/inet_common.h>
@@ -175,6 +176,7 @@ struct packet_sock {
 #endif
 	struct packet_type	prot_hook;
 	spinlock_t		bind_lock;
+	struct mutex		pg_vec_lock;
 	unsigned int		running:1,	/* prot_hook is attached*/
 				auxdata:1,
 				origdev:1;
@@ -1069,6 +1071,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol)
 	 */
 
 	spin_lock_init(&po->bind_lock);
+	mutex_init(&po->pg_vec_lock);
 	po->prot_hook.func = packet_rcv;
 
 	if (sock->type == SOCK_PACKET)
@@ -1865,6 +1868,7 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req, int closing
 	synchronize_net();
 
 	err = -EBUSY;
+	mutex_lock(&po->pg_vec_lock);
 	if (closing || atomic_read(&po->mapped) == 0) {
 		err = 0;
 #define XC(a, b) ({ __typeof__ ((a)) __t; __t = (a); (a) = (b); __t; })
@@ -1886,6 +1890,7 @@ static int packet_set_ring(struct sock *sk, struct tpacket_req *req, int closing
 		if (atomic_read(&po->mapped))
 			printk(KERN_DEBUG "packet_mmap: vma is busy: %d\n", atomic_read(&po->mapped));
 	}
+	mutex_unlock(&po->pg_vec_lock);
 
 	spin_lock(&po->bind_lock);
 	if (was_running && !po->running) {
@@ -1918,7 +1923,7 @@ static int packet_mmap(struct file *file, struct socket *sock, struct vm_area_st
 
 	size = vma->vm_end - vma->vm_start;
 
-	lock_sock(sk);
+	mutex_lock(&po->pg_vec_lock);
 	if (po->pg_vec == NULL)
 		goto out;
 	if (size != po->pg_vec_len*po->pg_vec_pages*PAGE_SIZE)
@@ -1941,7 +1946,7 @@ static int packet_mmap(struct file *file, struct socket *sock, struct vm_area_st
 	err = 0;
 
 out:
-	release_sock(sk);
+	mutex_unlock(&po->pg_vec_lock);
 	return err;
 }
 #endif

David Miller

maintainer

2009-01-30 22:13 UTC #9 lore raw

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Fri, 30 Jan 2009 23:49:47 +1100

Quoted 6 lines

packet: Avoid lock_sock in mmap handler
As the mmap handler gets called under mmap_sem, and we may grab mmap_sem elsewhere under the socket lock to access user data, we should avoid grabbing the socket lock in the mmap handler.

Quoted 8 lines

Since the only thing we care about in the mmap handler is for pg_vec* to be invariant, i.e., to exclude packet_set_ring, we can achieve this by simply using sk_receive_queue.lock.
I resisted the temptation to create a new spin lock because the mmap path isn't exactly common.
Signed-off-by Herbert Xu <[email protected]>

Looks good, applied.

I rewrote the commit message as follows so that it actually matches what happens in the fix ;-)

--------------------
packet: Avoid lock_sock in mmap handler

As the mmap handler gets called under mmap_sem, and we may grab mmap_sem elsewhere under the socket lock to access user data, we should avoid grabbing the socket lock in the mmap handler.

Since the only thing we care about in the mmap handler is for pg_vec* to be invariant, i.e., to exclude packet_set_ring, we can achieve this by simply using a new mutex.

Signed-off-by Herbert Xu <[email protected]>

Signed-off-by David S. Miller <[email protected]>