[PATCH] lockdep: fix sk->sk_callback_lock locking

[PATCH] lockdep: fix sk->sk_callback_lock locking

[Peter Zijlstra]

=========================================================
[ INFO: possible irq lock inversion dependency detected ]
2.6.19-rc6 #4
---------------------------------------------------------
nc/1854 just changed the state of lock:
 (af_callback_keys + sk->sk_family#2){-.-?}, at: [<c0268a7f>] sock_def_error_report+0x1f/0x90
but this lock was taken by another, soft-irq-safe lock in the past:
 (slock-AF_INET){-+..}

and interrupts could create inverse lock ordering between them.

stack backtrace:
 [<c0103f36>] show_trace_log_lvl+0x26/0x40
 [<c010406b>] show_trace+0x1b/0x20
 [<c01049e4>] dump_stack+0x24/0x30
 [<c013738c>] print_irq_inversion_bug+0x10c/0x130
 [<c01374f2>] check_usage_backwards+0x42/0x50
 [<c0137912>] mark_lock+0x312/0x620
 [<c0138925>] __lock_acquire+0x4c5/0xcb0
 [<c0139499>] lock_acquire+0x69/0x90
 [<c02ddc49>] _read_lock+0x39/0x50
 [<c026778c>] sock_def_wakeup+0x1c/0x60
 [<c02b7d73>] inet_shutdown+0x93/0xf0
 [<c02648d2>] sys_shutdown+0x32/0x60
 [<c0266284>] sys_socketcall+0x1d4/0x260
 [<c01031f3>] syscall_call+0x7/0xb
 =======================

stack backtrace:
 [<c0103f36>] show_trace_log_lvl+0x26/0x40
 [<c010406b>] show_trace+0x1b/0x20
 [<c01049e4>] dump_stack+0x24/0x30
 [<c013738c>] print_irq_inversion_bug+0x10c/0x130
 [<c01374f2>] check_usage_backwards+0x42/0x50
 [<c0137912>] mark_lock+0x312/0x620
 [<c0138925>] __lock_acquire+0x4c5/0xcb0
 [<c0139499>] lock_acquire+0x69/0x90
 [<c02ddc59>] _read_lock+0x39/0x50
 [<c0268a7f>] sock_def_error_report+0x1f/0x90
 [<c029b968>] tcp_disconnect+0x318/0x490
 [<c02b9110>] inet_stream_connect+0x220/0x260
 [<c0264adb>] sys_connect+0x6b/0x90
 [<c026614f>] sys_socketcall+0x9f/0x260
 [<c01031f3>] syscall_call+0x7/0xb
 =======================

sk->sk_callback_lock is usually only read locked from softirq context
however it seems lockdep found two spots that are reachable from process
context, thus creating the possibility of a deadlock.

For now fix these two call sites with manual disabling of softirqs; if
more of these sites are found we might consider changing the read_lock() to
read_lock_bh().

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
CC: Martin Josefsson <gandalf@wlug.westbo.se>
---
 net/ipv4/af_inet.c |    2 ++
 net/ipv4/tcp.c     |    2 ++
 2 files changed, 4 insertions(+)

Index: linux-2.6-netdev/net/ipv4/af_inet.c
===================================================================
--- linux-2.6-netdev.orig/net/ipv4/af_inet.c	2006-11-27 14:41:51.000000000 +0100
+++ linux-2.6-netdev/net/ipv4/af_inet.c	2006-11-28 07:06:23.000000000 +0100
@@ -731,7 +731,9 @@ int inet_shutdown(struct socket *sock, i
 	}
 
 	/* Wake up anyone sleeping in poll. */
+	local_bh_disable();
 	sk->sk_state_change(sk);
+	local_bh_enable();
 	release_sock(sk);
 	return err;
 }
Index: linux-2.6-netdev/net/ipv4/tcp.c
===================================================================
--- linux-2.6-netdev.orig/net/ipv4/tcp.c	2006-11-28 07:06:16.000000000 +0100
+++ linux-2.6-netdev/net/ipv4/tcp.c	2006-11-28 07:06:20.000000000 +0100
@@ -1765,7 +1765,9 @@ int tcp_disconnect(struct sock *sk, int 
 
 	BUG_TRAP(!inet->num || icsk->icsk_bind_hash);
 
+	local_bh_disable();
 	sk->sk_error_report(sk);
+	local_bh_enable();
 	return err;
 }
 

Re: [PATCH] lockdep: fix sk->sk_callback_lock locking

[Herbert Xu]

Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> =========================================================
> [ INFO: possible irq lock inversion dependency detected ]
> 2.6.19-rc6 #4
> ---------------------------------------------------------
> nc/1854 just changed the state of lock:
> (af_callback_keys + sk->sk_family#2){-.-?}, at: [<c0268a7f>] sock_def_error_report+0x1f/0x90
> but this lock was taken by another, soft-irq-safe lock in the past:
> (slock-AF_INET){-+..}
> 
> and interrupts could create inverse lock ordering between them.

I think this is bogus.  The slock is not a standard lock.  When we
hold it in process context we don't actually hold the spin lock part
of it.  However, it does prevent the softirq path from running in
critical sections which also prevents any attempt to grab the
callback lock from softirq context.

If you still think there is a problem, please show an actual scenario
where it dead locks.

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] lockdep: fix sk->sk_callback_lock locking

[Jarek Poplawski]

On 29-11-2006 08:49, Herbert Xu wrote:
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
>> =========================================================
>> [ INFO: possible irq lock inversion dependency detected ]
>> 2.6.19-rc6 #4
>> ---------------------------------------------------------
>> nc/1854 just changed the state of lock:
>> (af_callback_keys + sk->sk_family#2){-.-?}, at: [<c0268a7f>] sock_def_error_report+0x1f/0x90
>> but this lock was taken by another, soft-irq-safe lock in the past:
>> (slock-AF_INET){-+..}
>>
>> and interrupts could create inverse lock ordering between them.
> 
> I think this is bogus.  The slock is not a standard lock.  When we
> hold it in process context we don't actually hold the spin lock part
> of it.  However, it does prevent the softirq path from running in
> critical sections which also prevents any attempt to grab the
> callback lock from softirq context.
> 
> If you still think there is a problem, please show an actual scenario
> where it dead locks.

It would be nice to have a look at other parts of stack
backtraces probably with softirq part, which took that
lock: sk->sk_family#2){-.-?}

Jarek P. 

Re: [PATCH] lockdep: fix sk->sk_callback_lock locking

[Peter Zijlstra]

On Wed, 2006-11-29 at 18:49 +1100, Herbert Xu wrote:
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > =========================================================
> > [ INFO: possible irq lock inversion dependency detected ]
> > 2.6.19-rc6 #4
> > ---------------------------------------------------------
> > nc/1854 just changed the state of lock:
> > (af_callback_keys + sk->sk_family#2){-.-?}, at: [<c0268a7f>] sock_def_error_report+0x1f/0x90
> > but this lock was taken by another, soft-irq-safe lock in the past:
> > (slock-AF_INET){-+..}
> > 
> > and interrupts could create inverse lock ordering between them.
> 
> I think this is bogus.  The slock is not a standard lock.  When we
> hold it in process context we don't actually hold the spin lock part
> of it.  However, it does prevent the softirq path from running in
> critical sections which also prevents any attempt to grab the
> callback lock from softirq context.
> 
> If you still think there is a problem, please show an actual scenario
> where it dead locks.

process context does lock_sock(sk) which is basically a sleeping lock
and sets an owner field when acquired.

BH context does bh_lock_sock(sk); which spins on the spinlock protecting
the owner field; and checks for an owner under this lock. When an owner
is found it will stick the skb on a queue for later processing.

This scheme does indeed seem to avoid the reported deadlock scenario -
although I didn't audit all code paths.

However I'm not quite sure yet how to teach lockdep about this. The
proposed patch will shut it up though.

Re: [PATCH] lockdep: fix sk->sk_callback_lock locking

[Herbert Xu]

On Wed, Nov 29, 2006 at 12:42:24PM +0100, Peter Zijlstra wrote:
> 
> However I'm not quite sure yet how to teach lockdep about this. The
> proposed patch will shut it up though.

As a rule I think we should never make semantic changes to shut up
lockdep.

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] lockdep: fix sk->sk_callback_lock locking

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Nov 2006 23:07:09 +1100

> On Wed, Nov 29, 2006 at 12:42:24PM +0100, Peter Zijlstra wrote:
> > 
> > However I'm not quite sure yet how to teach lockdep about this. The
> > proposed patch will shut it up though.
> 
> As a rule I think we should never make semantic changes to shut up
> lockdep.

Especially ones which are costly, as this proposed change is in
that it disables software interrupts in a place where that
is completely unnecessary.

Let's not even consider this patch :)