Date:	Thu, 19 Feb 2009 18:27:46 -0800 (PST)
From:	Roland McGrath <roland@...hat.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"Metzger, Markus T" <markus.t.metzger@...el.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] forget_original_parent: split out the un-ptrace part

> +static inline int task_detached(struct task_struct *p)

Maybe take the opportunity to make it bool?
Clearly trivial, but a bit of implicit documentation that doesn't hurt.

> @@ -911,7 +825,10 @@ static void forget_original_parent(struc
>  	write_unlock_irq(&tasklist_lock);
>  	BUG_ON(!list_empty(&father->children));
>  
> -	ptrace_exit_finish(father, &ptrace_dead);
> +	list_for_each_entry_safe(p, n, &ptrace_dead, ptrace_entry) {
> +		list_del_init(&p->ptrace_entry);
> +		release_task(p);
> +	}
>  }

I know it's removed by the next patch anyway.  But I wouldn't mind a comment
in this patch saying "kludge overload of ptrace_entry should go away soon".

> +/*
> + * Called with irqs disabled, returns true if childs should reap themselves.

s/childs/children/

Make this bool too.

> +/*
> + * Detach all tasks we were using ptrace on.
> + */
> +void exit_ptrace(struct task_struct *tracer)
> +{
> +	struct task_struct *p, *n;
> +	LIST_HEAD(ptrace_dead);

I think this can do a short-circuit for the common case and avoid the lock:

	if (list_empty(&tracer->ptraced))
		return;

It might even be worthwhile making it an inline that does the short-circuit
check before calling out to ptrace.c at all.

I see your patch 4/4 on this.  In fact, I think the short-circuit
optimization of these two cases should be two separate patches.  They are
for the same motivation and the same kind of optimization, but the details
of what the race issues might be are quite distinct between the two.

Moreover, the exit_ptrace optimization is removing the new lock overhead we
introduce in this patch.  The real-child optimization is just a new
optimization beyond the status quo.  It can really be considered wholly
after this whole series (and probably just punted because it gets so hairy).

For the ptrace case, I started out thinking it was trivially clear that no
false positives are possible here.  The tracer can't do ptrace_attach() or
do_fork(), it's exiting.  But ptrace_traceme() can sneak in here.  (I don't
see how release_task() could be a problem at all.  Going from nonempty to
empty could only cause false negatives--which are always harmless--not false
positives.)  

You didn't mention ptrace_traceme() in your 4/4 message.  Off hand I don't
see what keeps it safe when we go to dropping the tasklist_lock between
exit_ptrace and changing children's real_parent links.  In fact, that seems
like a new hole, period--without the short-circuit optimization.  
Am I overlooking something here?

That seems addressed by e.g.:

--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -534,7 +534,7 @@ repeat:
 		 * Set the ptrace bit in the process ptrace flags.
 		 * Then link us on our parent's ptraced list.
 		 */
-		if (!ret) {
+		if (!ret && !(current->real_parent->flags & PF_EXITING)) {
 			current->ptrace |= PT_PTRACED;
 			__ptrace_link(current, current->real_parent);
 		}



Thanks,
Roland
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists