suspicious RCU usage in security/selinux/netnode.c

suspicious RCU usage in security/selinux/netnode.c

[Dave Jones]

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

Re: suspicious RCU usage in security/selinux/netnode.c

[Paul E. McKenney]

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
> 

Re: suspicious RCU usage in security/selinux/netnode.c

[Eric Paris]

On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> 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

Re: suspicious RCU usage in security/selinux/netnode.c

[Paul E. McKenney]

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
> <paulmck@linux.vnet.ibm.com> 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

Re: suspicious RCU usage in security/selinux/netnode.c

[Eric Paris]

On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> 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
>> <paulmck@linux.vnet.ibm.com> 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

Re: suspicious RCU usage in security/selinux/netnode.c

[Paul Moore]

On Tuesday, May 15, 2012 10:52:07 AM Eric Paris wrote:
> On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
> 
> <paulmck@linux.vnet.ibm.com> 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
> >> 
> >> <paulmck@linux.vnet.ibm.com> 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
www.paul-moore.com

Re: suspicious RCU usage in security/selinux/netnode.c

[Paul E. McKenney]

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
> > 
> > <paulmck@linux.vnet.ibm.com> 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
> > >> 
> > >> <paulmck@linux.vnet.ibm.com> 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
> www.paul-moore.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: suspicious RCU usage in security/selinux/netnode.c

[Paul Moore]

On Tue, May 15, 2012 at 11:37 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> 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
www.paul-moore.com