* suspicious rcu_dereference_check in sctp_v6_get_dst
@ 2015-12-06 0:25 Dave Jones
2015-12-06 1:13 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Dave Jones @ 2015-12-06 0:25 UTC (permalink / raw)
To: netdev
===============================
[ INFO: suspicious RCU usage. ]
4.4.0-rc3-think+ #8 Tainted: G W
-------------------------------
net/sctp/ipv6.c:331 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
1 lock held by trinity-c2/15441:
#0: (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffffc065fc01>] sctp_sendmsg+0x501/0x16a0 [sctp]
stack backtrace:
CPU: 2 PID: 15441 Comm: trinity-c2 Tainted: G W 4.4.0-rc3-think+ #8
ffffffffffffff9b ffff880458d677c8 ffffffffab530f11 ffff88045f1db700
ffff880458d677f8 ffffffffab12d248 ffff880452b13f80 ffff880452b13f60
0000000000000000 ffff88045ad45280 ffff880458d67970 ffffffffc0673a7c
Call Trace:
[<ffffffffab530f11>] dump_stack+0x4e/0x7d
[<ffffffffab12d248>] lockdep_rcu_suspicious+0xf8/0x110
[<ffffffffc0673a7c>] sctp_v6_get_dst+0xacc/0xb30 [sctp]
[<ffffffffc0673666>] ? sctp_v6_get_dst+0x6b6/0xb30 [sctp]
[<ffffffffc0672fb0>] ? sctp_inet6_send_verify+0x180/0x180 [sctp]
[<ffffffffab6b1c69>] ? get_random_bytes+0x69/0x150
[<ffffffffab6b0950>] ? extract_buf+0x370/0x370
[<ffffffffab12afa2>] ? __lock_is_held+0x92/0xd0
[<ffffffffc0647390>] ? sctp_transport_new+0x2f0/0x320 [sctp]
[<ffffffffc0647786>] sctp_transport_route+0x66/0x1c0 [sctp]
[<ffffffffc0644d02>] sctp_assoc_add_peer+0x242/0x680 [sctp]
[<ffffffffc06603d1>] sctp_sendmsg+0xcd1/0x16a0 [sctp]
[<ffffffffab12f64f>] ? mark_lock+0x6f/0x8a0
[<ffffffffc065f700>] ? sctp_id2assoc+0x140/0x140 [sctp]
[<ffffffffab130620>] ? debug_check_no_locks_freed+0x1b0/0x1b0
[<ffffffffab12f64f>] ? mark_lock+0x6f/0x8a0
[<ffffffffab015269>] ? native_sched_clock+0x69/0x160
[<ffffffffab5606e7>] ? debug_smp_processor_id+0x17/0x20
[<ffffffffab0eb1e1>] ? preempt_count_sub+0xc1/0x120
[<ffffffffabbb3d0e>] inet_sendmsg+0x18e/0x270
[<ffffffffabbb3b85>] ? inet_sendmsg+0x5/0x270
[<ffffffffabaa4ee8>] SYSC_sendto+0x1d8/0x2c0
[<ffffffffabaa4d10>] ? sock_create_kern+0x20/0x20
[<ffffffffab12af35>] ? __lock_is_held+0x25/0xd0
[<ffffffffab1300c6>] ? trace_hardirqs_on_caller+0x186/0x280
[<ffffffffab1301cd>] ? trace_hardirqs_on+0xd/0x10
[<ffffffffab253a5d>] ? context_tracking_exit+0x1d/0x20
[<ffffffffab002fdf>] ? enter_from_user_mode+0x1f/0x50
[<ffffffffab0031b2>] ? syscall_trace_enter_phase1+0x1a2/0x240
[<ffffffffab003010>] ? enter_from_user_mode+0x50/0x50
[<ffffffffabcb735a>] ? int_ret_from_sys_call+0x52/0x9f
[<ffffffffab1300c6>] ? trace_hardirqs_on_caller+0x186/0x280
[<ffffffffab002017>] ? trace_hardirqs_on_thunk+0x17/0x19
[<ffffffffabaa5aee>] SyS_sendto+0xe/0x10
[<ffffffffabcb71d7>] entry_SYSCALL_64_fastpath+0x12/0x6b
This maybe ?
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index acb45b8c2a9d..7081183f4d9f 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -328,7 +328,9 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
if (baddr) {
fl6->saddr = baddr->v6.sin6_addr;
fl6->fl6_sport = baddr->v6.sin6_port;
+ rcu_read_lock();
final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
+ rcu_read_unlock();
dst = ip6_dst_lookup_flow(sk, fl6, final_p);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: suspicious rcu_dereference_check in sctp_v6_get_dst
2015-12-06 0:25 suspicious rcu_dereference_check in sctp_v6_get_dst Dave Jones
@ 2015-12-06 1:13 ` Eric Dumazet
2015-12-06 1:37 ` Dave Jones
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2015-12-06 1:13 UTC (permalink / raw)
To: Dave Jones; +Cc: netdev
On Sat, 2015-12-05 at 19:25 -0500, Dave Jones wrote:
> ===============================
> [ INFO: suspicious RCU usage. ]
> 4.4.0-rc3-think+ #8 Tainted: G W
> -------------------------------
> net/sctp/ipv6.c:331 suspicious rcu_dereference_check() usage!
>
> other info that might help us debug this:
>
>
> rcu_scheduler_active = 1, debug_locks = 0
> 1 lock held by trinity-c2/15441:
> #0: (sk_lock-AF_INET6){+.+.+.}, at: [<ffffffffc065fc01>] sctp_sendmsg+0x501/0x16a0 [sctp]
>
> stack backtrace:
> CPU: 2 PID: 15441 Comm: trinity-c2 Tainted: G W 4.4.0-rc3-think+ #8
> ffffffffffffff9b ffff880458d677c8 ffffffffab530f11 ffff88045f1db700
> ffff880458d677f8 ffffffffab12d248 ffff880452b13f80 ffff880452b13f60
> 0000000000000000 ffff88045ad45280 ffff880458d67970 ffffffffc0673a7c
> Call Trace:
> [<ffffffffab530f11>] dump_stack+0x4e/0x7d
> [<ffffffffab12d248>] lockdep_rcu_suspicious+0xf8/0x110
> [<ffffffffc0673a7c>] sctp_v6_get_dst+0xacc/0xb30 [sctp]
> [<ffffffffc0673666>] ? sctp_v6_get_dst+0x6b6/0xb30 [sctp]
> [<ffffffffc0672fb0>] ? sctp_inet6_send_verify+0x180/0x180 [sctp]
> [<ffffffffab6b1c69>] ? get_random_bytes+0x69/0x150
> [<ffffffffab6b0950>] ? extract_buf+0x370/0x370
> [<ffffffffab12afa2>] ? __lock_is_held+0x92/0xd0
> [<ffffffffc0647390>] ? sctp_transport_new+0x2f0/0x320 [sctp]
> [<ffffffffc0647786>] sctp_transport_route+0x66/0x1c0 [sctp]
> [<ffffffffc0644d02>] sctp_assoc_add_peer+0x242/0x680 [sctp]
> [<ffffffffc06603d1>] sctp_sendmsg+0xcd1/0x16a0 [sctp]
> [<ffffffffab12f64f>] ? mark_lock+0x6f/0x8a0
> [<ffffffffc065f700>] ? sctp_id2assoc+0x140/0x140 [sctp]
> [<ffffffffab130620>] ? debug_check_no_locks_freed+0x1b0/0x1b0
> [<ffffffffab12f64f>] ? mark_lock+0x6f/0x8a0
> [<ffffffffab015269>] ? native_sched_clock+0x69/0x160
> [<ffffffffab5606e7>] ? debug_smp_processor_id+0x17/0x20
> [<ffffffffab0eb1e1>] ? preempt_count_sub+0xc1/0x120
> [<ffffffffabbb3d0e>] inet_sendmsg+0x18e/0x270
> [<ffffffffabbb3b85>] ? inet_sendmsg+0x5/0x270
> [<ffffffffabaa4ee8>] SYSC_sendto+0x1d8/0x2c0
> [<ffffffffabaa4d10>] ? sock_create_kern+0x20/0x20
> [<ffffffffab12af35>] ? __lock_is_held+0x25/0xd0
> [<ffffffffab1300c6>] ? trace_hardirqs_on_caller+0x186/0x280
> [<ffffffffab1301cd>] ? trace_hardirqs_on+0xd/0x10
> [<ffffffffab253a5d>] ? context_tracking_exit+0x1d/0x20
> [<ffffffffab002fdf>] ? enter_from_user_mode+0x1f/0x50
> [<ffffffffab0031b2>] ? syscall_trace_enter_phase1+0x1a2/0x240
> [<ffffffffab003010>] ? enter_from_user_mode+0x50/0x50
> [<ffffffffabcb735a>] ? int_ret_from_sys_call+0x52/0x9f
> [<ffffffffab1300c6>] ? trace_hardirqs_on_caller+0x186/0x280
> [<ffffffffab002017>] ? trace_hardirqs_on_thunk+0x17/0x19
> [<ffffffffabaa5aee>] SyS_sendto+0xe/0x10
> [<ffffffffabcb71d7>] entry_SYSCALL_64_fastpath+0x12/0x6b
>
> This maybe ?
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index acb45b8c2a9d..7081183f4d9f 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -328,7 +328,9 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> if (baddr) {
> fl6->saddr = baddr->v6.sin6_addr;
> fl6->fl6_sport = baddr->v6.sin6_port;
> + rcu_read_lock();
> final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> + rcu_read_unlock();
> dst = ip6_dst_lookup_flow(sk, fl6, final_p);
> }
>
Hmm, better use :
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index acb45b8c2a9d..d28c0b4c9128 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -323,14 +323,13 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
}
}
}
- rcu_read_unlock();
-
if (baddr) {
fl6->saddr = baddr->v6.sin6_addr;
fl6->fl6_sport = baddr->v6.sin6_port;
final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
dst = ip6_dst_lookup_flow(sk, fl6, final_p);
}
+ rcu_read_unlock();
out:
if (!IS_ERR_OR_NULL(dst)) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: suspicious rcu_dereference_check in sctp_v6_get_dst
2015-12-06 1:13 ` Eric Dumazet
@ 2015-12-06 1:37 ` Dave Jones
2015-12-06 1:56 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Dave Jones @ 2015-12-06 1:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
On Sat, Dec 05, 2015 at 05:13:06PM -0800, Eric Dumazet wrote:
> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> > index acb45b8c2a9d..7081183f4d9f 100644
> > --- a/net/sctp/ipv6.c
> > +++ b/net/sctp/ipv6.c
> > @@ -328,7 +328,9 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> > if (baddr) {
> > fl6->saddr = baddr->v6.sin6_addr;
> > fl6->fl6_sport = baddr->v6.sin6_port;
> > + rcu_read_lock();
> > final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> > + rcu_read_unlock();
> > dst = ip6_dst_lookup_flow(sk, fl6, final_p);
> > }
> >
>
> Hmm, better use :
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index acb45b8c2a9d..d28c0b4c9128 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -323,14 +323,13 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> }
> }
> }
> - rcu_read_unlock();
> -
> if (baddr) {
> fl6->saddr = baddr->v6.sin6_addr;
> fl6->fl6_sport = baddr->v6.sin6_port;
> final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> dst = ip6_dst_lookup_flow(sk, fl6, final_p);
> }
> + rcu_read_unlock();
>
> out:
> if (!IS_ERR_OR_NULL(dst)) {
I looked at that option first, but decided to mirror the other use of fl6_update_dst.
It looks like your solution would work too, so I'm not against it, but..
For my own understanding, why is this better? Just to cut down on the
number of repeated lock/unlocks in the same function? Or is there some
semantic I'm missing in the earlier lock/unlock section that's somehow
related to the np->opt ?
thanks,
Dave
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: suspicious rcu_dereference_check in sctp_v6_get_dst
2015-12-06 1:37 ` Dave Jones
@ 2015-12-06 1:56 ` Eric Dumazet
2015-12-07 16:25 ` [PATCH net] ipv6: sctp: fix lockdep splat in sctp_v6_get_dst() Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2015-12-06 1:56 UTC (permalink / raw)
To: Dave Jones; +Cc: netdev
On Sat, 2015-12-05 at 20:37 -0500, Dave Jones wrote:
> On Sat, Dec 05, 2015 at 05:13:06PM -0800, Eric Dumazet wrote:
>
> > > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> > > index acb45b8c2a9d..7081183f4d9f 100644
> > > --- a/net/sctp/ipv6.c
> > > +++ b/net/sctp/ipv6.c
> > > @@ -328,7 +328,9 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> > > if (baddr) {
> > > fl6->saddr = baddr->v6.sin6_addr;
> > > fl6->fl6_sport = baddr->v6.sin6_port;
> > > + rcu_read_lock();
> > > final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> > > + rcu_read_unlock();
> > > dst = ip6_dst_lookup_flow(sk, fl6, final_p);
> > > }
> > >
> >
> > Hmm, better use :
> >
> > diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> > index acb45b8c2a9d..d28c0b4c9128 100644
> > --- a/net/sctp/ipv6.c
> > +++ b/net/sctp/ipv6.c
> > @@ -323,14 +323,13 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> > }
> > }
> > }
> > - rcu_read_unlock();
> > -
> > if (baddr) {
> > fl6->saddr = baddr->v6.sin6_addr;
> > fl6->fl6_sport = baddr->v6.sin6_port;
> > final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> > dst = ip6_dst_lookup_flow(sk, fl6, final_p);
> > }
> > + rcu_read_unlock();
> >
> > out:
> > if (!IS_ERR_OR_NULL(dst)) {
>
> I looked at that option first, but decided to mirror the other use of fl6_update_dst.
>
> It looks like your solution would work too, so I'm not against it, but..
> For my own understanding, why is this better? Just to cut down on the
> number of repeated lock/unlocks in the same function? Or is there some
> semantic I'm missing in the earlier lock/unlock section that's somehow
> related to the np->opt ?
This was my intent when cooking commit c836a8ba93869d6a0
I have no idea how I missed to move the rcu_read_lock()
Yes, there is no need to have too many rcu_read_lock()/unlock() all
around the places. Extending the existing section is good enough.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net] ipv6: sctp: fix lockdep splat in sctp_v6_get_dst()
2015-12-06 1:56 ` Eric Dumazet
@ 2015-12-07 16:25 ` Eric Dumazet
2015-12-07 22:07 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2015-12-07 16:25 UTC (permalink / raw)
To: Dave Jones, David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
While cooking the sctp np->opt rcu fixes, I forgot to move
one rcu_read_unlock() after the added rcu_dereference() in
sctp_v6_get_dst()
This gave lockdep warnings reported by Dave Jones.
Fixes: c836a8ba9386 ("ipv6: sctp: add rcu protection around np->opt")
Reported-by: Dave Jones <davej@codemonkey.org.uk>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/sctp/ipv6.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index acb45b8c2a9d..d28c0b4c9128 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -323,14 +323,13 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
}
}
}
- rcu_read_unlock();
-
if (baddr) {
fl6->saddr = baddr->v6.sin6_addr;
fl6->fl6_sport = baddr->v6.sin6_port;
final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
dst = ip6_dst_lookup_flow(sk, fl6, final_p);
}
+ rcu_read_unlock();
out:
if (!IS_ERR_OR_NULL(dst)) {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] ipv6: sctp: fix lockdep splat in sctp_v6_get_dst()
2015-12-07 16:25 ` [PATCH net] ipv6: sctp: fix lockdep splat in sctp_v6_get_dst() Eric Dumazet
@ 2015-12-07 22:07 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2015-12-07 22:07 UTC (permalink / raw)
To: eric.dumazet; +Cc: davej, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Dec 2015 08:25:21 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> While cooking the sctp np->opt rcu fixes, I forgot to move
> one rcu_read_unlock() after the added rcu_dereference() in
> sctp_v6_get_dst()
>
> This gave lockdep warnings reported by Dave Jones.
>
> Fixes: c836a8ba9386 ("ipv6: sctp: add rcu protection around np->opt")
> Reported-by: Dave Jones <davej@codemonkey.org.uk>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-07 22:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-06 0:25 suspicious rcu_dereference_check in sctp_v6_get_dst Dave Jones
2015-12-06 1:13 ` Eric Dumazet
2015-12-06 1:37 ` Dave Jones
2015-12-06 1:56 ` Eric Dumazet
2015-12-07 16:25 ` [PATCH net] ipv6: sctp: fix lockdep splat in sctp_v6_get_dst() Eric Dumazet
2015-12-07 22:07 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).