On 04/01, Matthew Dempsky wrote:
>
> @@ -1605,10 +1605,12 @@ long do_fork(unsigned long clone_flags,
> */
> if (!IS_ERR(p)) {
> struct completion vfork;
> + struct pid *pid;
>
> trace_sched_process_fork(current, p);
>
> - nr = task_pid_vnr(p);
> + pid = get_task_pid(p, PIDTYPE_PID);
So you decided to use get_pid/put_pid ;) Honestly, I'd prefer to just
calculate "pid_t trace_pid" before wake_up_new_task(), but I won't
argue. Plus this way the race window becomes really small, OK.
> + if (unlikely(trace)) {
> + /*
> + * We want to report the child's pid as seen from the
> + * tracer's pid namespace.
> + * FIXME: We still risk sending a bogus event message if
> + * debuggers from different pid namespaces detach and
> + * reattach between rcu_read_unlock() and ptrace_stop().
> + */
> + unsigned long message;
> + rcu_read_lock();
> + message = pid_nr_ns(pid,
> + task_active_pid_ns(current->parent));
> + rcu_read_unlock();
> + ptrace_event(trace, message);
> + }
>
> if (clone_flags & CLONE_VFORK) {
> - if (!wait_for_vfork_done(p, &vfork))
> - ptrace_event(PTRACE_EVENT_VFORK_DONE, nr);
> + if (!wait_for_vfork_done(p, &vfork)) {
> + /* See comment above about pid namespaces. */
> + unsigned long message;
> + rcu_read_lock();
> + message = pid_nr_ns(pid,
> + task_active_pid_ns(current->parent));
> + rcu_read_unlock();
> + ptrace_event(PTRACE_EVENT_VFORK_DONE, message);
> + }
OK, but may I suggest you to make a helper? Note that the code under
"if (trace)" and "if (CLONE_VFORK)" is the same. Even the comment above
equally applies to the CLONE_VFORK branch.
Especially because this code needs a fix. Yes, rcu_read_lock() should
be enough to ensure that ->parent and its namespace (if !NULL) can not
go away, but task_active_pid_ns() can return NULL release_task(->parent)
was already (although this race is pure theoretical). So this helper
should also check it is !NULL under rcu_read_lock(), afaics.
(Hmm... off-topic, but get_pidns looks buggy by the same reason, I'll
send a fix).
And I forgot to mention, please send v5 to akpm. We usually route ptrace
patches via -mm tree.
Oleg.
--
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/
