Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
From: Herbert Xu <[EMAIL PROTECTED]> 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 :) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
On Wed, 2006-11-29 at 18:49 +1100, Herbert Xu wrote:
> Peter Zijlstra <[EMAIL PROTECTED]> 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: []
> > 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
On 29-11-2006 08:49, Herbert Xu wrote:
> Peter Zijlstra <[EMAIL PROTECTED]> 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: []
>> 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
Peter Zijlstra <[EMAIL PROTECTED]> 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: []
> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] lockdep: fix sk->sk_callback_lock locking
=
[ 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: []
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:
[] show_trace_log_lvl+0x26/0x40
[] show_trace+0x1b/0x20
[] dump_stack+0x24/0x30
[] print_irq_inversion_bug+0x10c/0x130
[] check_usage_backwards+0x42/0x50
[] mark_lock+0x312/0x620
[] __lock_acquire+0x4c5/0xcb0
[] lock_acquire+0x69/0x90
[] _read_lock+0x39/0x50
[] sock_def_wakeup+0x1c/0x60
[] inet_shutdown+0x93/0xf0
[] sys_shutdown+0x32/0x60
[] sys_socketcall+0x1d4/0x260
[] syscall_call+0x7/0xb
===
stack backtrace:
[] show_trace_log_lvl+0x26/0x40
[] show_trace+0x1b/0x20
[] dump_stack+0x24/0x30
[] print_irq_inversion_bug+0x10c/0x130
[] check_usage_backwards+0x42/0x50
[] mark_lock+0x312/0x620
[] __lock_acquire+0x4c5/0xcb0
[] lock_acquire+0x69/0x90
[] _read_lock+0x39/0x50
[] sock_def_error_report+0x1f/0x90
[] tcp_disconnect+0x318/0x490
[] inet_stream_connect+0x220/0x260
[] sys_connect+0x6b/0x90
[] sys_socketcall+0x9f/0x260
[] 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 <[EMAIL PROTECTED]>
CC: Martin Josefsson <[EMAIL PROTECTED]>
---
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.c2006-11-27 14:41:51.0
+0100
+++ linux-2.6-netdev/net/ipv4/af_inet.c 2006-11-28 07:06:23.0 +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.c2006-11-28 07:06:16.0
+0100
+++ linux-2.6-netdev/net/ipv4/tcp.c 2006-11-28 07:06:20.0 +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;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
