I just triggered this on Linus' current tree.
Dave
===============================
[ INFO: suspicious RCU usage. ]
3.4.0-rc7+ #93 Not tainted
-------------------------------
security/selinux/netnode.c:178 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/25132:
#0: (sel_netnode_lock){+.....}, at: [<ffffffff812db738>] sel_netnode_sid+0x148/0x3c0
stack backtrace:
Pid: 25132, comm: trinity Not tainted 3.4.0-rc7+ #93
Call Trace:
[<ffffffff810cc7cd>] lockdep_rcu_suspicious+0xfd/0x130
[<ffffffff812db981>] sel_netnode_sid+0x391/0x3c0
[<ffffffff812db5f0>] ? sel_netnode_find+0x1a0/0x1a0
[<ffffffff812d4a84>] selinux_socket_bind+0x104/0x350
[<ffffffff810a6648>] ? sched_clock_cpu+0xb8/0x130
[<ffffffff816a47b9>] ? sub_preempt_count+0xa9/0xe0
[<ffffffff812cb3e6>] security_socket_bind+0x16/0x20
[<ffffffff815468aa>] sys_bind+0x7a/0x100
[<ffffffff816a8795>] ? sysret_check+0x22/0x5d
[<ffffffff810d149d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
[<ffffffff810fc18c>] ? __audit_syscall_entry+0xcc/0x310
[<ffffffff8133839e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff816a8769>] system_call_fastpath+0x16/0x1b
On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
> I just triggered this on Linus' current tree.
This is a bare:
rcu_dereference(sel_netnode_hash[idx].list.prev)
which needs to be in an RCU read-side critical section. Alternatively,
the above should instead be something like:
rcu_dereference_check(sel_netnode_hash[idx].list.prev,
lockdep_is_held(&sel_netnode_lock));
This second approach assumes that all modifications to the hash table
are protected by sel_netnode_lock. Paul Moore, thoughts?
Thanx, Paul
> Dave
>
> ===============================
> [ INFO: suspicious RCU usage. ]
> 3.4.0-rc7+ #93 Not tainted
> -------------------------------
> security/selinux/netnode.c:178 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/25132:
> #0: (sel_netnode_lock){+.....}, at: [<ffffffff812db738>] sel_netnode_sid+0x148/0x3c0
>
> stack backtrace:
> Pid: 25132, comm: trinity Not tainted 3.4.0-rc7+ #93
> Call Trace:
> [<ffffffff810cc7cd>] lockdep_rcu_suspicious+0xfd/0x130
> [<ffffffff812db981>] sel_netnode_sid+0x391/0x3c0
> [<ffffffff812db5f0>] ? sel_netnode_find+0x1a0/0x1a0
> [<ffffffff812d4a84>] selinux_socket_bind+0x104/0x350
> [<ffffffff810a6648>] ? sched_clock_cpu+0xb8/0x130
> [<ffffffff816a47b9>] ? sub_preempt_count+0xa9/0xe0
> [<ffffffff812cb3e6>] security_socket_bind+0x16/0x20
> [<ffffffff815468aa>] sys_bind+0x7a/0x100
> [<ffffffff816a8795>] ? sysret_check+0x22/0x5d
> [<ffffffff810d149d>] ? trace_hardirqs_on_caller+0x10d/0x1a0
> [<ffffffff810fc18c>] ? __audit_syscall_entry+0xcc/0x310
> [<ffffffff8133839e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> [<ffffffff816a8769>] system_call_fastpath+0x16/0x1b
>
On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
<[email protected]> wrote:
> On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
>> I just triggered this on Linus' current tree.
>
> This is a bare:
>
> ? ? ? ?rcu_dereference(sel_netnode_hash[idx].list.prev)
>
> which needs to be in an RCU read-side critical section. ?Alternatively,
> the above should instead be something like:
>
> ? ? ? ?rcu_dereference_check(sel_netnode_hash[idx].list.prev,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?lockdep_is_held(&sel_netnode_lock));
Right, but that 'bare' dereference comes from
list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
see how to easily annotate with the lock. Nor do I think it's within
my brain power (or my willingness to maintain such in the future) to
want to open code that logic.
Should we just take the rcu_read_lock() where we take the spinlock?
Is that a perf hit and figuring out how to do the annotation correctly
is the better idea?
-Eric
On Tue, May 15, 2012 at 10:24:23AM -0400, Eric Paris wrote:
> On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
> <[email protected]> wrote:
> > On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
> >> I just triggered this on Linus' current tree.
> >
> > This is a bare:
> >
> > ? ? ? ?rcu_dereference(sel_netnode_hash[idx].list.prev)
> >
> > which needs to be in an RCU read-side critical section. ?Alternatively,
> > the above should instead be something like:
> >
> > ? ? ? ?rcu_dereference_check(sel_netnode_hash[idx].list.prev,
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?lockdep_is_held(&sel_netnode_lock));
>
> Right, but that 'bare' dereference comes from
> list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
> see how to easily annotate with the lock. Nor do I think it's within
> my brain power (or my willingness to maintain such in the future) to
> want to open code that logic.
You lost me on this one. The lockdep splat called out the
rcu_dereference() above, not a list_for_each_entry_rcu(). Besides which,
the list_for_each_entry_rcu() does not do the checking -- at the time,
I was not willing to explode the API that much.
> Should we just take the rcu_read_lock() where we take the spinlock?
> Is that a perf hit and figuring out how to do the annotation correctly
> is the better idea?
If the spinlock is protecting the data, then just add the spinlock
to the rcu_dereference_check() as shown above.
Thanx, Paul
On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
<[email protected]> wrote:
> On Tue, May 15, 2012 at 10:24:23AM -0400, Eric Paris wrote:
>> On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
>> <[email protected]> wrote:
>> > On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
>> >> I just triggered this on Linus' current tree.
>> >
>> > This is a bare:
>> >
>> > ? ? ? ?rcu_dereference(sel_netnode_hash[idx].list.prev)
>> >
>> > which needs to be in an RCU read-side critical section. ?Alternatively,
>> > the above should instead be something like:
>> >
>> > ? ? ? ?rcu_dereference_check(sel_netnode_hash[idx].list.prev,
>> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?lockdep_is_held(&sel_netnode_lock));
>>
>> Right, but that 'bare' dereference comes from
>> list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
>> see how to easily annotate with the lock. ?Nor do I think it's within
>> my brain power (or my willingness to maintain such in the future) to
>> want to open code that logic.
>
> You lost me on this one. ?The lockdep splat called out the
> rcu_dereference() above, not a list_for_each_entry_rcu(). ?Besides which,
> the list_for_each_entry_rcu() does not do the checking -- at the time,
> I was not willing to explode the API that much.
Ohhhh, ok. I assumed we needed to annotate list_for_each_entry_rcu()
under the spinlock as well as the bare dereference in the insert code.
Ok, should be very easy to fix, although the list running code is
still going to be un-annotated in any way. Thanks
-Eric
On Tuesday, May 15, 2012 10:52:07 AM Eric Paris wrote:
> On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
>
> <[email protected]> wrote:
> > On Tue, May 15, 2012 at 10:24:23AM -0400, Eric Paris wrote:
> >> On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
> >>
> >> <[email protected]> wrote:
> >> > On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
> >> >> I just triggered this on Linus' current tree.
> >> >
> >> > This is a bare:
> >> >
> >> > rcu_dereference(sel_netnode_hash[idx].list.prev)
> >> >
> >> > which needs to be in an RCU read-side critical section. Alternatively,
> >> > the above should instead be something like:
> >> >
> >> > rcu_dereference_check(sel_netnode_hash[idx].list.prev,
> >> > lockdep_is_held(&sel_netnode_lock));
> >>
> >> Right, but that 'bare' dereference comes from
> >> list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
> >> see how to easily annotate with the lock. Nor do I think it's within
> >> my brain power (or my willingness to maintain such in the future) to
> >> want to open code that logic.
> >
> > You lost me on this one. The lockdep splat called out the
> > rcu_dereference() above, not a list_for_each_entry_rcu(). Besides which,
> > the list_for_each_entry_rcu() does not do the checking -- at the time,
> > I was not willing to explode the API that much.
>
> Ohhhh, ok. I assumed we needed to annotate list_for_each_entry_rcu()
> under the spinlock as well as the bare dereference in the insert code.
> Ok, should be very easy to fix, although the list running code is
> still going to be un-annotated in any way. Thanks
Sorry, email filters went awry and I lost this thread until Eric pointed it
out to me ...
Despite a common first name, the other Paul is the RCU expert, no I
unfortunately. Can someone explain the difference between
rcu_dereference_check() and rcu_dereference_protected()? We use
rcu_dereference_protected() for a very similar reason in
selinux/netport.c:sel_netport_insert() and it seems like a better choice ... ?
I'll throw a patch together but wanted to clear this up first.
--
paul moore
http://www.paul-moore.com
On Tue, May 15, 2012 at 11:12:27AM -0400, Paul Moore wrote:
> On Tuesday, May 15, 2012 10:52:07 AM Eric Paris wrote:
> > On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
> >
> > <[email protected]> wrote:
> > > On Tue, May 15, 2012 at 10:24:23AM -0400, Eric Paris wrote:
> > >> On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
> > >>
> > >> <[email protected]> wrote:
> > >> > On Tue, May 15, 2012 at 12:41:45AM -0400, Dave Jones wrote:
> > >> >> I just triggered this on Linus' current tree.
> > >> >
> > >> > This is a bare:
> > >> >
> > >> > rcu_dereference(sel_netnode_hash[idx].list.prev)
> > >> >
> > >> > which needs to be in an RCU read-side critical section. Alternatively,
> > >> > the above should instead be something like:
> > >> >
> > >> > rcu_dereference_check(sel_netnode_hash[idx].list.prev,
> > >> > lockdep_is_held(&sel_netnode_lock));
> > >>
> > >> Right, but that 'bare' dereference comes from
> > >> list_for_each_entry_rcu(), [from sel_netnode_sid_slow()] which I don't
> > >> see how to easily annotate with the lock. Nor do I think it's within
> > >> my brain power (or my willingness to maintain such in the future) to
> > >> want to open code that logic.
> > >
> > > You lost me on this one. The lockdep splat called out the
> > > rcu_dereference() above, not a list_for_each_entry_rcu(). Besides which,
> > > the list_for_each_entry_rcu() does not do the checking -- at the time,
> > > I was not willing to explode the API that much.
> >
> > Ohhhh, ok. I assumed we needed to annotate list_for_each_entry_rcu()
> > under the spinlock as well as the bare dereference in the insert code.
> > Ok, should be very easy to fix, although the list running code is
> > still going to be un-annotated in any way. Thanks
>
> Sorry, email filters went awry and I lost this thread until Eric pointed it
> out to me ...
>
> Despite a common first name, the other Paul is the RCU expert, no I
> unfortunately. Can someone explain the difference between
> rcu_dereference_check() and rcu_dereference_protected()? We use
> rcu_dereference_protected() for a very similar reason in
> selinux/netport.c:sel_netport_insert() and it seems like a better choice ... ?
Here you go:
o rcu_dereference_check() is for code shared between readers and
writers, so that it might be protected by either rcu_read_lock()
or some lock.
o rcu_dereference_protected() is for code that is only used by
updaters and never by readers. So rcu_dereference_protected()
is (slightly) lighter weight than rcu_dereference_check()
because it does not need to protect against concurrent
updates.
Thanx, Paul
> I'll throw a patch together but wanted to clear this up first.
>
> --
> paul moore
> http://www.paul-moore.com
>
> --
> 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/
>
On Tue, May 15, 2012 at 11:37 AM, Paul E. McKenney
<[email protected]> wrote:
> Here you go:
>
> o ? ? ? rcu_dereference_check() is for code shared between readers and
> ? ? ? ?writers, so that it might be protected by either rcu_read_lock()
> ? ? ? ?or some lock.
>
> o ? ? ? rcu_dereference_protected() is for code that is only used by
> ? ? ? ?updaters and never by readers. ?So rcu_dereference_protected()
> ? ? ? ?is (slightly) lighter weight than rcu_dereference_check()
> ? ? ? ?because it does not need to protect against concurrent
> ? ? ? ?updates.
Perfect, thank you. Looks like rcu_dereference_protected() is the
right choice here. Patch coming tomorrow if not later today.
--
paul moore
http://www.paul-moore.com