Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Paul Moore <paul@paul-moore.com>]

On Saturday, December 14, 2013 05:32:56 PM Oleg Nesterov wrote:
> On 12/14, Paul Moore wrote:
> > ... I'm curious about the removal of the task lock; shouldn't we keep
> > the task lock in place?
> 
> Why? It protects nothing in this case, afaics. Unless of course it
> protects cred->security somehow, but it doesn't look as if.
> 
> Probably task_lock() is here because PTRACE_ATTACH used the same lock,
> but this was changed by 4b105cbbaf7c0 in 2009 (ptrace_attach() still
> takes it for __ptrace_may_access() but this is another story).
> 
> However (iirc) PTRACE_DETACH never took this lock, so this was always
> racy and task_lock() is simply misleading and confusing, at least
> currently.

Okay, you convinced me.

> So I think the patch is fine, but I decided to send v2 without pid_alive().
> If we are going to keep ptrace_parent(), it would be better to add the
> comment into ptrace_parent() to explain that ->ptrace != 0 guarantees that
> this task is not unhashed.
> 
> IOW, I also changed my mind about this part
> 
> 	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.
> 
> in the changelog.

Seems reasonable to me.  I'll let you work on the ptrace_parent() side of 
things and I'll go ahead and merge your v2 patch; it looks fine to me and 
regardless of what happens with ptrace_parent() we should dump the task_lock() 
and add a RCU lock in its place.

Thanks for your help.

-- 
paul moore
www.paul-moore.com