suspicious RCU usage in security/selinux/netnode.c

DISCUSSION Dave Jones 4 participants 8 messages May 15, 2012

lore

Share & tools

lore

Dave Jones

author

2012-05-15 04:41 UTC #0 lore raw

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

Paul E. McKenney

2012-05-15 05:16 UTC #1 lore raw

Quoted 1 line · Dave Jones

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

Quoted 32 lines

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

Eric Paris

2012-05-15 14:24 UTC #2 lore raw

Quoted 5 lines · Paul E. McKenney

Quoted 1 line · Dave Jones
I just triggered this on Linus' current tree.
This is a bare:

Quoted 7 lines

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

Paul E. McKenney

2012-05-15 14:47 UTC #3 lore raw

Quoted 12 lines · Eric Paris

Quoted 10 lines · Paul E. McKenney
Quoted 1 line · Dave Jones
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:

Quoted 8 lines

Quoted 2 lines
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.

Quoted 3 lines

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

Eric Paris

2012-05-15 14:52 UTC #4 lore raw

Quoted 22 lines · Paul E. McKenney

Quoted 12 lines · Eric Paris
Quoted 10 lines · Paul E. McKenney
Quoted 1 line · Dave Jones
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:
Quoted 8 lines
Quoted 2 lines
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.

Quoted 4 lines

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

Paul Moore

2012-05-15 15:12 UTC #5 lore raw

Quoted 31 lines · Eric Paris

On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
Quoted 23 lines · <[email protected]> wrote:
Quoted 13 lines · Eric Paris
On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
Quoted 10 lines · <[email protected]> wrote:
Quoted 1 line · Dave Jones
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:
Quoted 8 lines
Quoted 2 lines
       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.
Quoted 4 lines
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.

Quoted 4 lines

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.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul E. McKenney

2012-05-15 15:39 UTC #6 lore raw

Quoted 37 lines · Paul Moore

Quoted 31 lines · Eric Paris
On Tue, May 15, 2012 at 10:46 AM, Paul E. McKenney
Quoted 23 lines · <[email protected]> wrote:
Quoted 13 lines · Eric Paris
On Tue, May 15, 2012 at 1:16 AM, Paul E. McKenney
Quoted 10 lines · <[email protected]> wrote:
Quoted 1 line · Dave Jones
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:
Quoted 8 lines
Quoted 2 lines
       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.
Quoted 4 lines
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.
Quoted 4 lines
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

Quoted 8 lines

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

Quoted 12 lines

I'll throw a patch together but wanted to clear this up first.

Signature · 8 lines

paul moore
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/

Paul Moore

2012-05-15 18:35 UTC #7 lore raw

Quoted 11 lines · Paul E. McKenney

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.

Signature · 2 lines

paul moore
www.paul-moore.com