Re: [RESEND PATCH v4] ptrace: Fix fork event messages across pid namespaces

[Andrew Morton]

On Tue, 29 Apr 2014 13:20:58 -0700 Matthew Dempsky <mdempsky@xxxxxxxxxxxx> wrote:

> When tracing a process in another pid namespace, it's important for
> fork event messages to contain the child's pid as seen from the
> tracer's pid namespace, not the parent's.  Otherwise, the tracer won't
> be able to correlate the fork event with later SIGTRAP signals it
> receives from the child.
> 
> We still risk a race condition if a ptracer from a different pid
> namespace attaches after we compute the pid_t value.  However, sending
> a bogus fork event message in this unlikely scenario is still a vast
> improvement over the status quo where we always send bogus fork event
> messages to debuggers in a different pid namespace than the forking
> process.

More Oleg review would be nice, please ;)

> @@ -129,6 +130,36 @@ static inline void ptrace_event(int event, unsigned long message)
>  }
>  
>  /**
> + * ptrace_event_pid - possibly stop for a ptrace event notification
> + * @event:	%PTRACE_EVENT_* value to report
> + * @pid:	process identifier for %PTRACE_GETEVENTMSG to return
> + *
> + * Check whether @event is enabled and, if so, report @event and @pid
> + * to the ptrace parent.  @pid is reported as the pid_t seen from the
> + * the ptrace parent's pid namespace.
> + *
> + * Called without locks.
> + */
> +static inline void ptrace_event_pid(int event, struct pid *pid)
> +{
> +	/*
> +	 * FIXME: There's a potential race if a ptracer in a different pid
> +	 * namespace than parent attaches between computing message below and
> +	 * and when we acquire tasklist_lock in ptrace_stop().
> +	 */

Well that's a scary comment.  If we're going to leave the code in this
state then please carefully describe (within this comment) the
*consequences* of the race.  Does the kernel crash?  Give away your ssh
keys?  If not then what.  

And how would userspace recognize and/or recover from the race?

> +	unsigned long message = 0;
> +	struct pid_namespace *ns;
> +
> +	rcu_read_lock();
> +	ns = task_active_pid_ns(rcu_dereference(current->parent));
> +	if (ns)
> +		message = pid_nr_ns(pid, ns);
> +	rcu_read_unlock();
> +
> +	ptrace_event(event, message);
> +}
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/