* suspicious rcu_dereference_check in security/selinux/netnode.c
@ 2011-04-20 18:35 Dave Jones
2011-04-20 18:42 ` Eric Paris
0 siblings, 1 reply; 4+ messages in thread
From: Dave Jones @ 2011-04-20 18:35 UTC (permalink / raw)
To: sds; +Cc: jmorris, eparis, Linux Kernel
===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
security/selinux/netnode.c:193 invoked rcu_dereference_check() without protection!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
1 lock held by a.out/2018:
#0: (sel_netnode_lock){+.....}, at: [<ffffffff81212ab7>] sel_netnode_sid+0x9e/0x267
stack backtrace:
Pid: 2018, comm: a.out Not tainted 2.6.39-rc4+ #3
Call Trace:
[<ffffffff81084908>] lockdep_rcu_dereference+0xa8/0xb0
[<ffffffff81212c0d>] sel_netnode_sid+0x1f4/0x267
[<ffffffff81212a19>] ? sel_netnode_find+0xe3/0xe3
[<ffffffff8120d564>] selinux_socket_bind+0x1cf/0x26f
[<ffffffff81086c08>] ? lock_release+0x181/0x18e
[<ffffffff81100db8>] ? might_fault+0xa5/0xac
[<ffffffff81100d6f>] ? might_fault+0x5c/0xac
[<ffffffff812073f1>] security_socket_bind+0x16/0x18
[<ffffffff813ee0e9>] sys_bind+0x73/0xcf
[<ffffffff814c5d7a>] ? sysret_check+0x2e/0x69
[<ffffffff810870cf>] ? trace_hardirqs_on_caller+0x10b/0x12f
[<ffffffff810a9efb>] ? audit_syscall_entry+0x11c/0x148
[<ffffffff81255e2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
[<ffffffff814c5d42>] system_call_fastpath+0x16/0x1b
something like this perhaps ?
Dave
diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
index 65ebfe9..d0c38ba 100644
--- a/security/selinux/netnode.c
+++ b/security/selinux/netnode.c
@@ -188,9 +188,11 @@ static void sel_netnode_insert(struct sel_netnode *node)
list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) {
struct sel_netnode *tail;
+ rcu_read_lock();
tail = list_entry(
rcu_dereference(sel_netnode_hash[idx].list.prev),
struct sel_netnode, list);
+ rcu_read_unlock();
list_del_rcu(&tail->list);
call_rcu(&tail->rcu, sel_netnode_free);
} else
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: suspicious rcu_dereference_check in security/selinux/netnode.c 2011-04-20 18:35 suspicious rcu_dereference_check in security/selinux/netnode.c Dave Jones @ 2011-04-20 18:42 ` Eric Paris 2011-04-20 19:29 ` Paul Moore 0 siblings, 1 reply; 4+ messages in thread From: Eric Paris @ 2011-04-20 18:42 UTC (permalink / raw) To: Dave Jones; +Cc: sds, jmorris, eparis, Linux Kernel, paul.moore, selinux [added paul] EOM On Wed, 2011-04-20 at 14:35 -0400, Dave Jones wrote: > =================================================== > [ INFO: suspicious rcu_dereference_check() usage. ] > --------------------------------------------------- > security/selinux/netnode.c:193 invoked rcu_dereference_check() without protection! > > other info that might help us debug this: > > rcu_scheduler_active = 1, debug_locks = 0 > 1 lock held by a.out/2018: > #0: (sel_netnode_lock){+.....}, at: [<ffffffff81212ab7>] sel_netnode_sid+0x9e/0x267 > > stack backtrace: > Pid: 2018, comm: a.out Not tainted 2.6.39-rc4+ #3 > Call Trace: > [<ffffffff81084908>] lockdep_rcu_dereference+0xa8/0xb0 > [<ffffffff81212c0d>] sel_netnode_sid+0x1f4/0x267 > [<ffffffff81212a19>] ? sel_netnode_find+0xe3/0xe3 > [<ffffffff8120d564>] selinux_socket_bind+0x1cf/0x26f > [<ffffffff81086c08>] ? lock_release+0x181/0x18e > [<ffffffff81100db8>] ? might_fault+0xa5/0xac > [<ffffffff81100d6f>] ? might_fault+0x5c/0xac > [<ffffffff812073f1>] security_socket_bind+0x16/0x18 > [<ffffffff813ee0e9>] sys_bind+0x73/0xcf > [<ffffffff814c5d7a>] ? sysret_check+0x2e/0x69 > [<ffffffff810870cf>] ? trace_hardirqs_on_caller+0x10b/0x12f > [<ffffffff810a9efb>] ? audit_syscall_entry+0x11c/0x148 > [<ffffffff81255e2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f > [<ffffffff814c5d42>] system_call_fastpath+0x16/0x1b > > > something like this perhaps ? > > Dave > > diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c > index 65ebfe9..d0c38ba 100644 > --- a/security/selinux/netnode.c > +++ b/security/selinux/netnode.c > @@ -188,9 +188,11 @@ static void sel_netnode_insert(struct sel_netnode *node) > list_add_rcu(&node->list, &sel_netnode_hash[idx].list); > if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) { > struct sel_netnode *tail; > + rcu_read_lock(); > tail = list_entry( > rcu_dereference(sel_netnode_hash[idx].list.prev), > struct sel_netnode, list); > + rcu_read_unlock(); > list_del_rcu(&tail->list); > call_rcu(&tail->rcu, sel_netnode_free); > } else ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: suspicious rcu_dereference_check in security/selinux/netnode.c 2011-04-20 18:42 ` Eric Paris @ 2011-04-20 19:29 ` Paul Moore 2011-04-21 19:54 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Paul Moore @ 2011-04-20 19:29 UTC (permalink / raw) To: Eric Paris; +Cc: Dave Jones, sds, jmorris, eparis, Linux Kernel, selinux On Wednesday, April 20, 2011 2:42:04 PM Eric Paris wrote: > [added paul] EOM > > On Wed, 2011-04-20 at 14:35 -0400, Dave Jones wrote: > > =================================================== > > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > --------------------------------------------------- > > security/selinux/netnode.c:193 invoked rcu_dereference_check() without > > protection! > > > > other info that might help us debug this: > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > > 1 lock held by a.out/2018: > > #0: (sel_netnode_lock){+.....}, at: [<ffffffff81212ab7>] > > sel_netnode_sid+0x9e/0x267 > > > > stack backtrace: > > Pid: 2018, comm: a.out Not tainted 2.6.39-rc4+ #3 > > > > Call Trace: > > [<ffffffff81084908>] lockdep_rcu_dereference+0xa8/0xb0 > > [<ffffffff81212c0d>] sel_netnode_sid+0x1f4/0x267 > > [<ffffffff81212a19>] ? sel_netnode_find+0xe3/0xe3 > > [<ffffffff8120d564>] selinux_socket_bind+0x1cf/0x26f > > [<ffffffff81086c08>] ? lock_release+0x181/0x18e > > [<ffffffff81100db8>] ? might_fault+0xa5/0xac > > [<ffffffff81100d6f>] ? might_fault+0x5c/0xac > > [<ffffffff812073f1>] security_socket_bind+0x16/0x18 > > [<ffffffff813ee0e9>] sys_bind+0x73/0xcf > > [<ffffffff814c5d7a>] ? sysret_check+0x2e/0x69 > > [<ffffffff810870cf>] ? trace_hardirqs_on_caller+0x10b/0x12f > > [<ffffffff810a9efb>] ? audit_syscall_entry+0x11c/0x148 > > [<ffffffff81255e2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f > > [<ffffffff814c5d42>] system_call_fastpath+0x16/0x1b > > > > something like this perhaps ? > > > > Dave > > > > diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c > > index 65ebfe9..d0c38ba 100644 > > --- a/security/selinux/netnode.c > > +++ b/security/selinux/netnode.c > > @@ -188,9 +188,11 @@ static void sel_netnode_insert(struct sel_netnode > > *node) > > > > list_add_rcu(&node->list, &sel_netnode_hash[idx].list); > > if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) { > > > > struct sel_netnode *tail; > > > > + rcu_read_lock(); > > > > tail = list_entry( > > > > rcu_dereference(sel_netnode_hash[idx].list.prev), > > struct sel_netnode, list); > > > > + rcu_read_unlock(); > > > > list_del_rcu(&tail->list); > > call_rcu(&tail->rcu, sel_netnode_free); > > > > } else [Ooops, forgot to hit reply-all on the first attempt] Hmm, I think the correct fix might be to just remove the rcu_dereference() call since this is protected by a spin lock (see sel_netnode_sid_slow()). I may be wrong, but I thought rcu locks/derefs were not needed when a spin lock was held, yes? Regardless of the fix, the same thing should probably be done to the sel_netport_* versions of these functions. -- paul moore linux @ hp ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: suspicious rcu_dereference_check in security/selinux/netnode.c 2011-04-20 19:29 ` Paul Moore @ 2011-04-21 19:54 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2011-04-21 19:54 UTC (permalink / raw) To: Paul Moore Cc: Eric Paris, Dave Jones, sds, jmorris, eparis, Linux Kernel, selinux On Wed, Apr 20, 2011 at 03:29:59PM -0400, Paul Moore wrote: > On Wednesday, April 20, 2011 2:42:04 PM Eric Paris wrote: > > [added paul] EOM > > > > On Wed, 2011-04-20 at 14:35 -0400, Dave Jones wrote: > > > =================================================== > > > > > > [ INFO: suspicious rcu_dereference_check() usage. ] > > > --------------------------------------------------- > > > security/selinux/netnode.c:193 invoked rcu_dereference_check() without > > > protection! > > > > > > other info that might help us debug this: > > > > > > rcu_scheduler_active = 1, debug_locks = 0 > > > > > > 1 lock held by a.out/2018: > > > #0: (sel_netnode_lock){+.....}, at: [<ffffffff81212ab7>] > > > sel_netnode_sid+0x9e/0x267 > > > > > > stack backtrace: > > > Pid: 2018, comm: a.out Not tainted 2.6.39-rc4+ #3 > > > > > > Call Trace: > > > [<ffffffff81084908>] lockdep_rcu_dereference+0xa8/0xb0 > > > [<ffffffff81212c0d>] sel_netnode_sid+0x1f4/0x267 > > > [<ffffffff81212a19>] ? sel_netnode_find+0xe3/0xe3 > > > [<ffffffff8120d564>] selinux_socket_bind+0x1cf/0x26f > > > [<ffffffff81086c08>] ? lock_release+0x181/0x18e > > > [<ffffffff81100db8>] ? might_fault+0xa5/0xac > > > [<ffffffff81100d6f>] ? might_fault+0x5c/0xac > > > [<ffffffff812073f1>] security_socket_bind+0x16/0x18 > > > [<ffffffff813ee0e9>] sys_bind+0x73/0xcf > > > [<ffffffff814c5d7a>] ? sysret_check+0x2e/0x69 > > > [<ffffffff810870cf>] ? trace_hardirqs_on_caller+0x10b/0x12f > > > [<ffffffff810a9efb>] ? audit_syscall_entry+0x11c/0x148 > > > [<ffffffff81255e2e>] ? trace_hardirqs_on_thunk+0x3a/0x3f > > > [<ffffffff814c5d42>] system_call_fastpath+0x16/0x1b > > > > > > something like this perhaps ? > > > > > > Dave > > > > > > diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c > > > index 65ebfe9..d0c38ba 100644 > > > --- a/security/selinux/netnode.c > > > +++ b/security/selinux/netnode.c > > > @@ -188,9 +188,11 @@ static void sel_netnode_insert(struct sel_netnode > > > *node) > > > > > > list_add_rcu(&node->list, &sel_netnode_hash[idx].list); > > > if (sel_netnode_hash[idx].size == SEL_NETNODE_HASH_BKT_LIMIT) { > > > > > > struct sel_netnode *tail; > > > > > > + rcu_read_lock(); > > > > > > tail = list_entry( > > > > > > rcu_dereference(sel_netnode_hash[idx].list.prev), > > > struct sel_netnode, list); > > > > > > + rcu_read_unlock(); > > > > > > list_del_rcu(&tail->list); > > > call_rcu(&tail->rcu, sel_netnode_free); > > > > > > } else > > [Ooops, forgot to hit reply-all on the first attempt] > > Hmm, I think the correct fix might be to just remove the rcu_dereference() > call since this is protected by a spin lock (see sel_netnode_sid_slow()). I > may be wrong, but I thought rcu locks/derefs were not needed when a spin lock > was held, yes? > > Regardless of the fix, the same thing should probably be done to the > sel_netport_* versions of these functions. The lock is sel_netnode_lock, correct? Then the best approach is as follows: tail = list_entry( rcu_dereference_protected(sel_netnode_hash[idx].list.prev, lockdep_is_held(&sel_netnode_lock)), struct sel_netnode, list); Give or take long lines, anyway... :-( This way, if someone mistakenly calls this function without holding the lock, CONFIG_PROVE_RCU will know to complain. And Paul Moore is quite correct when he says that rcu_read_lock() is not needed in this case. Thanx, Paul ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-22 5:59 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-20 18:35 suspicious rcu_dereference_check in security/selinux/netnode.c Dave Jones 2011-04-20 18:42 ` Eric Paris 2011-04-20 19:29 ` Paul Moore 2011-04-21 19:54 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox