* [PATCH] net: fix lockdep issue in __neigh_event_send
@ 2011-09-19 19:12 Maciej Żenczykowski
2011-09-19 19:42 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Maciej Żenczykowski @ 2011-09-19 19:12 UTC (permalink / raw)
To: Maciej Żenczykowski, David S. Miller
Cc: netdev, Maciej Żenczykowski, MuraliRaja Muniraju
From: Maciej Żenczykowski <maze@google.com>
skb's should be freed once neigh->lock is no longer held.
Google-Bug-Id: 4561441
Signed-off-by: Maciej Żenczykowski <maze@google.com>
CC: MuraliRaja Muniraju <muralira@google.com>
---
net/core/neighbour.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 4002261..53d034a 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -949,6 +949,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
{
int rc;
bool immediate_probe = false;
+ struct sk_buff *buff_to_free = NULL;
write_lock_bh(&neigh->lock);
@@ -969,10 +970,10 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
} else {
neigh->nud_state = NUD_FAILED;
neigh->updated = jiffies;
- write_unlock_bh(&neigh->lock);
- kfree_skb(skb);
- return 1;
+ buff_to_free = skb;
+ rc = 1;
+ goto out_unlock_bh;
}
} else if (neigh->nud_state & NUD_STALE) {
NEIGH_PRINTK2("neigh %p is delayed.\n", neigh);
@@ -986,9 +987,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
if (skb) {
if (skb_queue_len(&neigh->arp_queue) >=
neigh->parms->queue_len) {
- struct sk_buff *buff;
- buff = __skb_dequeue(&neigh->arp_queue);
- kfree_skb(buff);
+ buff_to_free = __skb_dequeue(&neigh->arp_queue);
NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);
}
skb_dst_force(skb);
@@ -1002,6 +1001,7 @@ out_unlock_bh:
else
write_unlock(&neigh->lock);
local_bh_enable();
+ kfree_skb(buff_to_free);
return rc;
}
EXPORT_SYMBOL(__neigh_event_send);
--
1.7.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: fix lockdep issue in __neigh_event_send
2011-09-19 19:12 [PATCH] net: fix lockdep issue in __neigh_event_send Maciej Żenczykowski
@ 2011-09-19 19:42 ` Eric Dumazet
2011-09-19 20:39 ` Maciej Żenczykowski
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-09-19 19:42 UTC (permalink / raw)
To: Maciej Żenczykowski
Cc: Maciej Żenczykowski, David S. Miller, netdev,
MuraliRaja Muniraju
Le lundi 19 septembre 2011 à 12:12 -0700, Maciej Żenczykowski a écrit :
> From: Maciej Żenczykowski <maze@google.com>
>
> skb's should be freed once neigh->lock is no longer held.
>
> Google-Bug-Id: 4561441
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> CC: MuraliRaja Muniraju <muralira@google.com>
> ---
> net/core/neighbour.c net/core/neighbour.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 4002261..53d034a 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -949,6 +949,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> {
> int rc;
> bool immediate_probe = false;
> + struct sk_buff *buff_to_free = NULL;
>
> write_lock_bh(&neigh->lock);
>
> @@ -969,10 +970,10 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> } else {
> neigh->nud_state = NUD_FAILED;
> neigh->updated = jiffies;
> - write_unlock_bh(&neigh->lock);
>
> - kfree_skb(skb);
I dont understand, here we call kfree_skb() while no lock is held.
> - return 1;
> + buff_to_free = skb;
> + rc = 1;
> + goto out_unlock_bh;
> }
> } else if (neigh->nud_state & NUD_STALE) {
> NEIGH_PRINTK2("neigh %p is delayed.\n", neigh);
> @@ -986,9 +987,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> if (skb) {
> if (skb_queue_len(&neigh->arp_queue) >=
> neigh->parms->queue_len) {
> - struct sk_buff *buff;
> - buff = __skb_dequeue(&neigh->arp_queue);
> - kfree_skb(buff);
> + buff_to_free = __skb_dequeue(&neigh->arp_queue);
> NEIGH_CACHE_STAT_INC(neigh->tbl, unres_discards);
> }
> skb_dst_force(skb);
> @@ -1002,6 +1001,7 @@ out_unlock_bh:
> else
> write_unlock(&neigh->lock);
> local_bh_enable();
> + kfree_skb(buff_to_free);
> return rc;
> }
> EXPORT_SYMBOL(__neigh_event_send);
Could you give us the lockdep report please ?
Thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: fix lockdep issue in __neigh_event_send
2011-09-19 19:42 ` Eric Dumazet
@ 2011-09-19 20:39 ` Maciej Żenczykowski
2011-09-19 20:46 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Maciej Żenczykowski @ 2011-09-19 20:39 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, MuraliRaja Muniraju
The first hunk was for consistency - to always free in exit path. The
second hunk is where the lockdep bug is.
LOCKDEP gives the following (it's pretty rare):
[ 90.543085] =======================================================
[ 90.544012] [ INFO: possible circular locking dependency detected ]
[ 90.544012] 2.6.34-smp-DEV #70
[ 90.544012] -------------------------------------------------------
[ 90.544012] swapper/0 is trying to acquire lock:
[ 90.544012] (&tbl->lock){++--..}, at: [<ffffffff813c106d>]
neigh_lookup+0x42/0xbf
[ 90.544012]
[ 90.544012] but task is already holding lock:
[ 90.544012] (&(&rt_hash_locks[i])->rlock){+.-...}, at:
[<ffffffff813db663>] rt_intern_hash+0xf2/0x3ad
[ 90.544012]
[ 90.544012] which lock already depends on the new lock.
[ 90.544012]
[ 90.544012]
[ 90.544012] the existing dependency chain (in reverse order) is:
[ 90.544012]
[ 90.544012] -> #2 (&(&rt_hash_locks[i])->rlock){+.-...}:
[ 90.544012] [<ffffffff810a4910>] __lock_acquire+0xb3b/0xcda
[ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
[ 90.544012] [<ffffffff81430d66>] _raw_spin_lock_bh+0x36/0x45
[ 90.544012] [<ffffffff813d9659>] rt_dst_release+0x4a/0x8d
[ 90.544012] [<ffffffff813bee34>] dst_release+0x5d/0x66
[ 90.544012] [<ffffffff813af24e>] skb_release_head_state+0x1f/0xa5
[ 90.544012] [<ffffffff813aefec>] __kfree_skb+0x16/0x87
[ 90.544012] [<ffffffff813af101>] kfree_skb+0x72/0x7a
[ 90.544012] [<ffffffff813c06f9>] __neigh_event_send+0x126/0x176
[ 90.544012] [<ffffffff813c2c1b>] neigh_resolve_output+0x7f/0x267
[ 90.544012] [<ffffffff813e3057>] ip_finish_output2+0x1f2/0x221
[ 90.544012] [<ffffffff813e30ea>] ip_finish_output+0x64/0x66
[ 90.544012] [<ffffffff813e3200>] NF_HOOK_COND.clone.1+0x41/0x45
[ 90.544012] [<ffffffff813e3277>] ip_output+0x73/0x75
[ 90.544012] [<ffffffff813e1524>] dst_output+0x10/0x12
[ 90.544012] [<ffffffff813e349f>] ip_local_out+0x23/0x27
[ 90.544012] [<ffffffff813e378a>] ip_push_pending_frames+0x2e7/0x348
[ 90.544012] [<ffffffff81401a13>] udp_push_pending_frames+0x2dc/0x35c
[ 90.544012] [<ffffffff8140314e>] udp_sendmsg+0x628/0x763
[ 90.544012] [<ffffffff81409933>] inet_sendmsg+0x58/0x61
[ 90.544012] [<ffffffff813a726f>] __sock_sendmsg+0x5e/0x69
[ 90.544012] [<ffffffff813a7737>] sock_sendmsg+0xa8/0xc1
[ 90.544012] [<ffffffff813a79d9>] sys_sendmsg+0x1da/0x241
[ 90.544012] [<ffffffff813cb482>] compat_sys_sendmsg+0x14/0x16
[ 90.544012] [<ffffffff813cc081>] compat_sys_socketcall+0x13b/0x16c
[ 90.544012] [<ffffffff81059687>] sysenter_dispatch+0x7/0x2c
[ 90.544012]
[ 90.544012] -> #1 (&n->lock){++--..}:
[ 90.544012] [<ffffffff810a4910>] __lock_acquire+0xb3b/0xcda
[ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
[ 90.544012] [<ffffffff81430e81>] _raw_write_lock+0x31/0x40
[ 90.544012] [<ffffffff813c16fb>] neigh_periodic_work+0xae/0x196
[ 90.544012] [<ffffffff8108a8ca>] worker_thread+0x1fb/0x2f1
[ 90.544012] [<ffffffff8108e099>] kthread+0x7f/0x87
[ 90.544012] [<ffffffff81032a94>] kernel_thread_helper+0x4/0x10
[ 90.544012]
[ 90.544012] -> #0 (&tbl->lock){++--..}:
[ 90.544012] [<ffffffff810a47ba>] __lock_acquire+0x9e5/0xcda
[ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
[ 90.544012] [<ffffffff81430ffa>] _raw_read_lock_bh+0x39/0x48
[ 90.544012] [<ffffffff813c106d>] neigh_lookup+0x42/0xbf
[ 90.544012] [<ffffffff81405067>]
__neigh_lookup_errno.clone.0+0x24/0x40
[ 90.544012] [<ffffffff814054b2>] arp_bind_neighbour+0x4c/0x65
[ 90.544012] [<ffffffff813db815>] rt_intern_hash+0x2a4/0x3ad
[ 90.544012] [<ffffffff813dc054>] ip_route_output_slow+0x6ec/0x739
[ 90.544012] [<ffffffff813dc574>] __ip_route_output_key+0x1bd/0x1c9
[ 90.544012] [<ffffffff81406f45>] icmp_send+0x2bf/0x3ca
[ 90.544012] [<ffffffff813d9739>] ipv4_link_failure+0x22/0x5c
[ 90.544012] [<ffffffff81404acd>] arp_error_report+0x31/0x3d
[ 90.544012] [<ffffffff813c1172>] neigh_invalidate+0x88/0xa3
[ 90.544012] [<ffffffff813c2a1b>] neigh_timer_handler+0x1a8/0x27f
[ 90.544012] [<ffffffff8108087a>] run_timer_softirq+0x219/0x2e3
[ 90.544012] [<ffffffff8107910e>] __do_softirq+0xf7/0x1cb
[ 90.544012] [<ffffffff81032b8c>] call_softirq+0x1c/0x3e
[ 90.544012] [<ffffffff81034615>] do_softirq+0x3d/0x85
[ 90.544012] [<ffffffff81078d49>] irq_exit+0x4a/0x8c
[ 90.544012] [<ffffffff8143708c>] smp_apic_timer_interrupt+0x99/0xa7
[ 90.544012] [<ffffffff81032653>] apic_timer_interrupt+0x13/0x20
[ 90.544012] [<ffffffff81030d92>] cpu_idle+0x5c/0x79
[ 90.544012] [<ffffffff8142213f>] rest_init+0xc3/0xca
[ 90.544012] [<ffffffff81b47d77>] start_kernel+0x3e2/0x3ed
[ 90.544012] [<ffffffff81b4729f>] x86_64_start_reservations+0xaf/0xb3
[ 90.544012] [<ffffffff81b47393>] x86_64_start_kernel+0xf0/0xf7
[ 90.544012]
[ 90.544012] other info that might help us debug this:
[ 90.544012]
[ 90.544012] 3 locks held by swapper/0:
[ 90.544012] #0: (&n->timer){+.-...}, at: [<ffffffff810807e8>]
run_timer_softirq+0x187/0x2e3
[ 90.544012] #1: (slock-AF_INET){+.-...}, at: [<ffffffff814063d3>]
icmp_xmit_lock+0x35/0x47
[ 90.544012] #2: (&(&rt_hash_locks[i])->rlock){+.-...}, at:
[<ffffffff813db663>] rt_intern_hash+0xf2/0x3ad
[ 90.544012]
[ 90.544012] stack backtrace:
[ 90.544012] Pid: 0, comm: swapper Not tainted 2.6.34-smp-DEV #70
[ 90.544012] Call Trace:
[ 90.544012] <IRQ> [<ffffffff810a39c0>] print_circular_bug+0xa8/0xb7
[ 90.544012] [<ffffffff810a47ba>] __lock_acquire+0x9e5/0xcda
[ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
[ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
[ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
[ 90.544012] [<ffffffff81430ffa>] _raw_read_lock_bh+0x39/0x48
[ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
[ 90.544012] [<ffffffff813c106d>] neigh_lookup+0x42/0xbf
[ 90.544012] [<ffffffff81405067>] __neigh_lookup_errno.clone.0+0x24/0x40
[ 90.544012] [<ffffffff814054b2>] arp_bind_neighbour+0x4c/0x65
[ 90.544012] [<ffffffff813db663>] ? rt_intern_hash+0xf2/0x3ad
[ 90.544012] [<ffffffff813db815>] rt_intern_hash+0x2a4/0x3ad
[ 90.544012] [<ffffffff813dc054>] ip_route_output_slow+0x6ec/0x739
[ 90.544012] [<ffffffff81078f49>] ? _local_bh_enable_ip+0xc1/0xc7
[ 90.544012] [<ffffffff813dc574>] __ip_route_output_key+0x1bd/0x1c9
[ 90.544012] [<ffffffff81406f45>] icmp_send+0x2bf/0x3ca
[ 90.544012] [<ffffffff813c2800>] ? neigh_event_ns+0x2f/0xa2
[ 90.544012] [<ffffffff813d9739>] ipv4_link_failure+0x22/0x5c
[ 90.544012] [<ffffffff81404acd>] arp_error_report+0x31/0x3d
[ 90.544012] [<ffffffff813c1172>] neigh_invalidate+0x88/0xa3
[ 90.544012] [<ffffffff813c2a1b>] neigh_timer_handler+0x1a8/0x27f
[ 90.544012] [<ffffffff8108087a>] run_timer_softirq+0x219/0x2e3
[ 90.544012] [<ffffffff810807e8>] ? run_timer_softirq+0x187/0x2e3
[ 90.544012] [<ffffffff8109158d>] ? __run_hrtimer+0xf4/0x132
[ 90.544012] [<ffffffff813c2873>] ? neigh_timer_handler+0x0/0x27f
[ 90.544012] [<ffffffff8107908d>] ? __do_softirq+0x76/0x1cb
[ 90.544012] [<ffffffff8107910e>] __do_softirq+0xf7/0x1cb
[ 90.544012] [<ffffffff8109f74a>] ? tick_program_event+0x2a/0x2c
[ 90.544012] [<ffffffff81032b8c>] call_softirq+0x1c/0x3e
[ 90.544012] [<ffffffff81034615>] do_softirq+0x3d/0x85
[ 90.544012] [<ffffffff81078d49>] irq_exit+0x4a/0x8c
[ 90.544012] [<ffffffff8143708c>] smp_apic_timer_interrupt+0x99/0xa7
[ 90.544012] [<ffffffff81032653>] apic_timer_interrupt+0x13/0x20
[ 90.544012] <EOI> [<ffffffff81039700>] ? mwait_idle+0x85/0x92
[ 90.544012] [<ffffffff810396f7>] ? mwait_idle+0x7c/0x92
[ 90.544012] [<ffffffff81030d92>] cpu_idle+0x5c/0x79
[ 90.544012] [<ffffffff8142213f>] rest_init+0xc3/0xca
[ 90.544012] [<ffffffff8142207c>] ? rest_init+0x0/0xca
[ 90.544012] [<ffffffff81b47d77>] start_kernel+0x3e2/0x3ed
[ 90.544012] [<ffffffff81b4729f>] x86_64_start_reservations+0xaf/0xb3
[ 90.544012] [<ffffffff81b47393>] x86_64_start_kernel+0xf0/0xf7
At some point in the past of the lifetime of this kernel we grabbed
exactly the following 3 locks in the following order (where the first
2 were still held while we grabbed the 3rd):
(&tbl->lock){++--..} => (&n->lock){++--..} =>
(&(&rt_hash_locks[i])->rlock){+.-...}
Now, we have grabbed the first three (are still holding them and are
trying to grab the fourth) of these locks in this order:
(&n->timer){+.-...} => (slock-AF_INET){+.-...} =>
(&(&rt_hash_locks[i])->rlock){+.-...} => (&tbl->lock){++--..}
We're complaining that this 4th lock is being grabbed after the 3rd,
when previously it was the 1st before the 3rd - we've apparently
reversed lock ordering...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: fix lockdep issue in __neigh_event_send
2011-09-19 20:39 ` Maciej Żenczykowski
@ 2011-09-19 20:46 ` Eric Dumazet
2011-09-19 21:03 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-09-19 20:46 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: David S. Miller, netdev, MuraliRaja Muniraju
Le lundi 19 septembre 2011 à 13:39 -0700, Maciej Żenczykowski a écrit :
> The first hunk was for consistency - to always free in exit path. The
> second hunk is where the lockdep bug is.
>
> LOCKDEP gives the following (it's pretty rare):
>
> [ 90.543085] =======================================================
> [ 90.544012] [ INFO: possible circular locking dependency detected ]
> [ 90.544012] 2.6.34-smp-DEV #70
> [ 90.544012] -------------------------------------------------------
> [ 90.544012] swapper/0 is trying to acquire lock:
> [ 90.544012] (&tbl->lock){++--..}, at: [<ffffffff813c106d>]
> neigh_lookup+0x42/0xbf
> [ 90.544012]
> [ 90.544012] but task is already holding lock:
> [ 90.544012] (&(&rt_hash_locks[i])->rlock){+.-...}, at:
> [<ffffffff813db663>] rt_intern_hash+0xf2/0x3ad
> [ 90.544012]
> [ 90.544012] which lock already depends on the new lock.
> [ 90.544012]
> [ 90.544012]
> [ 90.544012] the existing dependency chain (in reverse order) is:
> [ 90.544012]
> [ 90.544012] -> #2 (&(&rt_hash_locks[i])->rlock){+.-...}:
> [ 90.544012] [<ffffffff810a4910>] __lock_acquire+0xb3b/0xcda
> [ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
> [ 90.544012] [<ffffffff81430d66>] _raw_spin_lock_bh+0x36/0x45
> [ 90.544012] [<ffffffff813d9659>] rt_dst_release+0x4a/0x8d
> [ 90.544012] [<ffffffff813bee34>] dst_release+0x5d/0x66
> [ 90.544012] [<ffffffff813af24e>] skb_release_head_state+0x1f/0xa5
> [ 90.544012] [<ffffffff813aefec>] __kfree_skb+0x16/0x87
> [ 90.544012] [<ffffffff813af101>] kfree_skb+0x72/0x7a
> [ 90.544012] [<ffffffff813c06f9>] __neigh_event_send+0x126/0x176
> [ 90.544012] [<ffffffff813c2c1b>] neigh_resolve_output+0x7f/0x267
> [ 90.544012] [<ffffffff813e3057>] ip_finish_output2+0x1f2/0x221
> [ 90.544012] [<ffffffff813e30ea>] ip_finish_output+0x64/0x66
> [ 90.544012] [<ffffffff813e3200>] NF_HOOK_COND.clone.1+0x41/0x45
> [ 90.544012] [<ffffffff813e3277>] ip_output+0x73/0x75
> [ 90.544012] [<ffffffff813e1524>] dst_output+0x10/0x12
> [ 90.544012] [<ffffffff813e349f>] ip_local_out+0x23/0x27
> [ 90.544012] [<ffffffff813e378a>] ip_push_pending_frames+0x2e7/0x348
> [ 90.544012] [<ffffffff81401a13>] udp_push_pending_frames+0x2dc/0x35c
> [ 90.544012] [<ffffffff8140314e>] udp_sendmsg+0x628/0x763
> [ 90.544012] [<ffffffff81409933>] inet_sendmsg+0x58/0x61
> [ 90.544012] [<ffffffff813a726f>] __sock_sendmsg+0x5e/0x69
> [ 90.544012] [<ffffffff813a7737>] sock_sendmsg+0xa8/0xc1
> [ 90.544012] [<ffffffff813a79d9>] sys_sendmsg+0x1da/0x241
> [ 90.544012] [<ffffffff813cb482>] compat_sys_sendmsg+0x14/0x16
> [ 90.544012] [<ffffffff813cc081>] compat_sys_socketcall+0x13b/0x16c
> [ 90.544012] [<ffffffff81059687>] sysenter_dispatch+0x7/0x2c
> [ 90.544012]
> [ 90.544012] -> #1 (&n->lock){++--..}:
> [ 90.544012] [<ffffffff810a4910>] __lock_acquire+0xb3b/0xcda
> [ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
> [ 90.544012] [<ffffffff81430e81>] _raw_write_lock+0x31/0x40
> [ 90.544012] [<ffffffff813c16fb>] neigh_periodic_work+0xae/0x196
> [ 90.544012] [<ffffffff8108a8ca>] worker_thread+0x1fb/0x2f1
> [ 90.544012] [<ffffffff8108e099>] kthread+0x7f/0x87
> [ 90.544012] [<ffffffff81032a94>] kernel_thread_helper+0x4/0x10
> [ 90.544012]
> [ 90.544012] -> #0 (&tbl->lock){++--..}:
> [ 90.544012] [<ffffffff810a47ba>] __lock_acquire+0x9e5/0xcda
> [ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
> [ 90.544012] [<ffffffff81430ffa>] _raw_read_lock_bh+0x39/0x48
> [ 90.544012] [<ffffffff813c106d>] neigh_lookup+0x42/0xbf
> [ 90.544012] [<ffffffff81405067>]
> __neigh_lookup_errno.clone.0+0x24/0x40
> [ 90.544012] [<ffffffff814054b2>] arp_bind_neighbour+0x4c/0x65
> [ 90.544012] [<ffffffff813db815>] rt_intern_hash+0x2a4/0x3ad
> [ 90.544012] [<ffffffff813dc054>] ip_route_output_slow+0x6ec/0x739
> [ 90.544012] [<ffffffff813dc574>] __ip_route_output_key+0x1bd/0x1c9
> [ 90.544012] [<ffffffff81406f45>] icmp_send+0x2bf/0x3ca
> [ 90.544012] [<ffffffff813d9739>] ipv4_link_failure+0x22/0x5c
> [ 90.544012] [<ffffffff81404acd>] arp_error_report+0x31/0x3d
> [ 90.544012] [<ffffffff813c1172>] neigh_invalidate+0x88/0xa3
> [ 90.544012] [<ffffffff813c2a1b>] neigh_timer_handler+0x1a8/0x27f
> [ 90.544012] [<ffffffff8108087a>] run_timer_softirq+0x219/0x2e3
> [ 90.544012] [<ffffffff8107910e>] __do_softirq+0xf7/0x1cb
> [ 90.544012] [<ffffffff81032b8c>] call_softirq+0x1c/0x3e
> [ 90.544012] [<ffffffff81034615>] do_softirq+0x3d/0x85
> [ 90.544012] [<ffffffff81078d49>] irq_exit+0x4a/0x8c
> [ 90.544012] [<ffffffff8143708c>] smp_apic_timer_interrupt+0x99/0xa7
> [ 90.544012] [<ffffffff81032653>] apic_timer_interrupt+0x13/0x20
> [ 90.544012] [<ffffffff81030d92>] cpu_idle+0x5c/0x79
> [ 90.544012] [<ffffffff8142213f>] rest_init+0xc3/0xca
> [ 90.544012] [<ffffffff81b47d77>] start_kernel+0x3e2/0x3ed
> [ 90.544012] [<ffffffff81b4729f>] x86_64_start_reservations+0xaf/0xb3
> [ 90.544012] [<ffffffff81b47393>] x86_64_start_kernel+0xf0/0xf7
> [ 90.544012]
> [ 90.544012] other info that might help us debug this:
> [ 90.544012]
> [ 90.544012] 3 locks held by swapper/0:
> [ 90.544012] #0: (&n->timer){+.-...}, at: [<ffffffff810807e8>]
> run_timer_softirq+0x187/0x2e3
> [ 90.544012] #1: (slock-AF_INET){+.-...}, at: [<ffffffff814063d3>]
> icmp_xmit_lock+0x35/0x47
> [ 90.544012] #2: (&(&rt_hash_locks[i])->rlock){+.-...}, at:
> [<ffffffff813db663>] rt_intern_hash+0xf2/0x3ad
> [ 90.544012]
> [ 90.544012] stack backtrace:
> [ 90.544012] Pid: 0, comm: swapper Not tainted 2.6.34-smp-DEV #70
> [ 90.544012] Call Trace:
> [ 90.544012] <IRQ> [<ffffffff810a39c0>] print_circular_bug+0xa8/0xb7
> [ 90.544012] [<ffffffff810a47ba>] __lock_acquire+0x9e5/0xcda
> [ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
> [ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
> [ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
> [ 90.544012] [<ffffffff81430ffa>] _raw_read_lock_bh+0x39/0x48
> [ 90.544012] [<ffffffff813c106d>] ? neigh_lookup+0x42/0xbf
> [ 90.544012] [<ffffffff813c106d>] neigh_lookup+0x42/0xbf
> [ 90.544012] [<ffffffff81405067>] __neigh_lookup_errno.clone.0+0x24/0x40
> [ 90.544012] [<ffffffff814054b2>] arp_bind_neighbour+0x4c/0x65
> [ 90.544012] [<ffffffff813db663>] ? rt_intern_hash+0xf2/0x3ad
> [ 90.544012] [<ffffffff813db815>] rt_intern_hash+0x2a4/0x3ad
> [ 90.544012] [<ffffffff813dc054>] ip_route_output_slow+0x6ec/0x739
> [ 90.544012] [<ffffffff81078f49>] ? _local_bh_enable_ip+0xc1/0xc7
> [ 90.544012] [<ffffffff813dc574>] __ip_route_output_key+0x1bd/0x1c9
> [ 90.544012] [<ffffffff81406f45>] icmp_send+0x2bf/0x3ca
> [ 90.544012] [<ffffffff813c2800>] ? neigh_event_ns+0x2f/0xa2
> [ 90.544012] [<ffffffff813d9739>] ipv4_link_failure+0x22/0x5c
> [ 90.544012] [<ffffffff81404acd>] arp_error_report+0x31/0x3d
> [ 90.544012] [<ffffffff813c1172>] neigh_invalidate+0x88/0xa3
> [ 90.544012] [<ffffffff813c2a1b>] neigh_timer_handler+0x1a8/0x27f
> [ 90.544012] [<ffffffff8108087a>] run_timer_softirq+0x219/0x2e3
> [ 90.544012] [<ffffffff810807e8>] ? run_timer_softirq+0x187/0x2e3
> [ 90.544012] [<ffffffff8109158d>] ? __run_hrtimer+0xf4/0x132
> [ 90.544012] [<ffffffff813c2873>] ? neigh_timer_handler+0x0/0x27f
> [ 90.544012] [<ffffffff8107908d>] ? __do_softirq+0x76/0x1cb
> [ 90.544012] [<ffffffff8107910e>] __do_softirq+0xf7/0x1cb
> [ 90.544012] [<ffffffff8109f74a>] ? tick_program_event+0x2a/0x2c
> [ 90.544012] [<ffffffff81032b8c>] call_softirq+0x1c/0x3e
> [ 90.544012] [<ffffffff81034615>] do_softirq+0x3d/0x85
> [ 90.544012] [<ffffffff81078d49>] irq_exit+0x4a/0x8c
> [ 90.544012] [<ffffffff8143708c>] smp_apic_timer_interrupt+0x99/0xa7
> [ 90.544012] [<ffffffff81032653>] apic_timer_interrupt+0x13/0x20
> [ 90.544012] <EOI> [<ffffffff81039700>] ? mwait_idle+0x85/0x92
> [ 90.544012] [<ffffffff810396f7>] ? mwait_idle+0x7c/0x92
> [ 90.544012] [<ffffffff81030d92>] cpu_idle+0x5c/0x79
> [ 90.544012] [<ffffffff8142213f>] rest_init+0xc3/0xca
> [ 90.544012] [<ffffffff8142207c>] ? rest_init+0x0/0xca
> [ 90.544012] [<ffffffff81b47d77>] start_kernel+0x3e2/0x3ed
> [ 90.544012] [<ffffffff81b4729f>] x86_64_start_reservations+0xaf/0xb3
> [ 90.544012] [<ffffffff81b47393>] x86_64_start_kernel+0xf0/0xf7
>
> At some point in the past of the lifetime of this kernel we grabbed
> exactly the following 3 locks in the following order (where the first
> 2 were still held while we grabbed the 3rd):
> (&tbl->lock){++--..} => (&n->lock){++--..} =>
> (&(&rt_hash_locks[i])->rlock){+.-...}
>
> Now, we have grabbed the first three (are still holding them and are
> trying to grab the fourth) of these locks in this order:
> (&n->timer){+.-...} => (slock-AF_INET){+.-...} =>
> (&(&rt_hash_locks[i])->rlock){+.-...} => (&tbl->lock){++--..}
>
> We're complaining that this 4th lock is being grabbed after the 3rd,
> when previously it was the 1st before the 3rd - we've apparently
> reversed lock ordering...
Hmm
What exact kernel version is it ?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: fix lockdep issue in __neigh_event_send
2011-09-19 20:46 ` Eric Dumazet
@ 2011-09-19 21:03 ` Eric Dumazet
2011-09-20 0:01 ` Maciej Żenczykowski
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-09-19 21:03 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: David S. Miller, netdev, MuraliRaja Muniraju
Le lundi 19 septembre 2011 à 22:46 +0200, Eric Dumazet a écrit :
> Le lundi 19 septembre 2011 à 13:39 -0700, Maciej Żenczykowski a écrit :
> > [ 90.544012] the existing dependency chain (in reverse order) is:
> > [ 90.544012]
> > [ 90.544012] -> #2 (&(&rt_hash_locks[i])->rlock){+.-...}:
> > [ 90.544012] [<ffffffff810a4910>] __lock_acquire+0xb3b/0xcda
> > [ 90.544012] [<ffffffff810a4b72>] lock_acquire+0xc3/0xe9
> > [ 90.544012] [<ffffffff81430d66>] _raw_spin_lock_bh+0x36/0x45
> > [ 90.544012] [<ffffffff813d9659>] rt_dst_release+0x4a/0x8d
rt_dst_release() again...
> > [ 90.544012] [<ffffffff813bee34>] dst_release+0x5d/0x66
> > [ 90.544012] [<ffffffff813af24e>] skb_release_head_state+0x1f/0xa5
> > [ 90.544012] [<ffffffff813aefec>] __kfree_skb+0x16/0x87
> > [ 90.544012] [<ffffffff813af101>] kfree_skb+0x72/0x7a
> > [ 90.544012] [<ffffffff813c06f9>] __neigh_event_send+0x126/0x176
> >
> Hmm
>
> What exact kernel version is it ?
>
>
Just to say what I already said to Murali raja Muniraju one week ago :
To my knowledge, no linux pristine linux kernel has this bug.
dst_release() is lockless.
So could you explain why its so important we add this 'bugfix' if it
only applies to a Google kernel ?
Anything I missed from my previous analysis ?
Thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: fix lockdep issue in __neigh_event_send
2011-09-19 21:03 ` Eric Dumazet
@ 2011-09-20 0:01 ` Maciej Żenczykowski
0 siblings, 0 replies; 6+ messages in thread
From: Maciej Żenczykowski @ 2011-09-20 0:01 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, MuraliRaja Muniraju
> To my knowledge, no linux pristine linux kernel has this bug.
>
> dst_release() is lockless.
You are probably right there, I missed that this was caused by some
other old crappy code we've been trying to get rid of.
> So could you explain why its so important we add this 'bugfix' if it
> only applies to a Google kernel ?
Freeing the skb later seemed to be pretty painless...
> Anything I missed from my previous analysis ?
>
> Thanks !
Probably not.
Now that the dst cache is going away this probably doesn't really
matter either way.
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-20 0:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-19 19:12 [PATCH] net: fix lockdep issue in __neigh_event_send Maciej Żenczykowski
2011-09-19 19:42 ` Eric Dumazet
2011-09-19 20:39 ` Maciej Żenczykowski
2011-09-19 20:46 ` Eric Dumazet
2011-09-19 21:03 ` Eric Dumazet
2011-09-20 0:01 ` Maciej Żenczykowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox