selinux_setprocattr() does ptrace_parent(p) under task_lock(p), but task_struct->alloc_lock doesn't pin ->parent or ->ptrace, this looks confusing and triggers the "suspicious RCU usage" warning because ptrace_parent() does rcu_dereference_check().
And in theory this is wrong, spin_lock()->preempt_disable() doesn't necessarily imply rcu_read_lock() we need to access the ->parent. The patch also checks pid_alive(p) before ptrace_parent(p) to ensure that this task can't be dead even before rcu_read_lock(), in this case its ->parent points to nowhere. This is not really needed "in practice", task->ptrace must be already cleared in this case but we should not rely on this. Note: perhaps we should simply kill ptrace_parent(), it buys almost nothing and it is obviously racy. Or perhaps we should change it to ensure it can't wrongly return the natural parent if it races with ptrace_detach. Reported-by: Evan McNabb <[email protected]> Signed-off-by: Oleg Nesterov <[email protected]> --- security/selinux/hooks.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 794c3ca..2adfd7a 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5503,11 +5503,14 @@ static int selinux_setprocattr(struct task_struct *p, /* Check for ptracing, and update the task SID if ok. Otherwise, leave SID unchanged and fail. */ ptsid = 0; - task_lock(p); - tracer = ptrace_parent(p); - if (tracer) - ptsid = task_sid(tracer); - task_unlock(p); + tracer = NULL; + rcu_read_lock(); + if (pid_alive(p)) { + tracer = ptrace_parent(p); + if (tracer) + ptsid = task_sid(tracer); + } + rcu_read_unlock(); if (tracer) { error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS, -- 1.5.5.1 -- 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/
