[PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Oleg Nesterov]

No functional changes.

- Fold ptrace_exit() into forget_original_parent(), it is trivial
  now. More importantly, this makes the code more symmetrical with
  reparent_thread().

- The same for ptrace_exit_finish(), and "ptrace_" is not correct
  any longer.

- "ptrace_dead" doesn't match the reality, rename to "dead_list".

- swap the reparent_thread()'s arguments, just to make it more
  symmetrical with __ptrace_detach().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/exit.c~8_CLEANUP	2009-01-29 08:03:18.000000000 +0100
+++ 6.29-rc3/kernel/exit.c	2009-01-29 08:54:14.000000000 +0100
@@ -770,42 +770,8 @@ int __ptrace_detach(struct task_struct *
 	return 1;
 }
 
-/*
- * Detach all tasks we were using ptrace on.
- * Any that need to be release_task'd are put on the @dead list.
- *
- * Called with write_lock(&tasklist_lock) held.
- */
-static void ptrace_exit(struct task_struct *parent, struct list_head *dead)
-{
-	struct task_struct *p, *n;
-
-	list_for_each_entry_safe(p, n, &parent->ptraced, ptrace_entry) {
-		if (__ptrace_detach(parent, p))
-			list_add(&p->ptrace_entry, dead);
-	}
-}
-
-/*
- * Finish up exit-time ptrace cleanup.
- *
- * Called without locks.
- */
-static void ptrace_exit_finish(struct task_struct *parent,
-			       struct list_head *dead)
-{
-	struct task_struct *p, *n;
-
-	BUG_ON(!list_empty(&parent->ptraced));
-
-	list_for_each_entry_safe(p, n, dead, ptrace_entry) {
-		list_del_init(&p->ptrace_entry);
-		release_task(p);
-	}
-}
-
 /* Returns nonzero if the child should be released. */
-static int reparent_thread(struct task_struct *p, struct task_struct *father)
+static int reparent_thread(struct task_struct *father, struct task_struct *p)
 {
 	int dead;
 
@@ -886,14 +852,17 @@ static struct task_struct *find_new_reap
 static void forget_original_parent(struct task_struct *father)
 {
 	struct task_struct *p, *n, *reaper;
-	LIST_HEAD(ptrace_dead);
+	LIST_HEAD(dead_list);
 
 	write_lock_irq(&tasklist_lock);
 	reaper = find_new_reaper(father);
 	/*
 	 * First clean up ptrace if we were using it.
 	 */
-	ptrace_exit(father, &ptrace_dead);
+	list_for_each_entry_safe(p, n, &father->ptraced, ptrace_entry) {
+		if (__ptrace_detach(father, p))
+			list_add(&p->ptrace_entry, &dead_list);
+	}
 
 	list_for_each_entry_safe(p, n, &father->children, sibling) {
 		p->real_parent = reaper;
@@ -901,14 +870,18 @@ static void forget_original_parent(struc
 			BUG_ON(p->ptrace);
 			p->parent = p->real_parent;
 		}
-		if (reparent_thread(p, father))
-			list_add(&p->ptrace_entry, &ptrace_dead);;
+		if (reparent_thread(father, p))
+			list_add(&p->ptrace_entry, &dead_list);;
 	}
-
 	write_unlock_irq(&tasklist_lock);
+
 	BUG_ON(!list_empty(&father->children));
+	BUG_ON(!list_empty(&father->ptraced));
 
-	ptrace_exit_finish(father, &ptrace_dead);
+	list_for_each_entry_safe(p, n, &dead_list, ptrace_entry) {
+		list_del_init(&p->ptrace_entry);
+		release_task(p);
+	}
 }
 
 /*

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Roland McGrath]

> - Fold ptrace_exit() into forget_original_parent(), it is trivial
>   now. More importantly, this makes the code more symmetrical with
>   reparent_thread().

Please don't do this.  The ptrace functions are separated not because they
are large, but because they are the ptrace innards.  We have been moving
away from the core task handling innards having intimate ptrace magic
knowledge directly intertwined, and we don't want to move back the other
way.  In later cleanups, we will eventually separate ptrace linkage from
the tasklist_lock'd parent/child linkage more thoroughly.

> - The same for ptrace_exit_finish(), and "ptrace_" is not correct
>   any longer.
> 
> - "ptrace_dead" doesn't match the reality, rename to "dead_list".

For the same reasons, I am not entirely sanguine about overloading
ptrace_entry for any case not related to ptrace.  We want to be able to
clean up the ptrace data structures in the future such that there may well
not be any such list_head allocated in an untraced task_struct.

This case is sufficiently obscure, I can't see that anyone would really
care about the marginal performance hit for just dropping the lock after
reparent_thread returns true, call release_task(), re-lock and restart the
loop on remaining children.  (And I don't see any atomicity problem with
this unlock/relock.)

> - swap the reparent_thread()'s arguments, just to make it more
>   symmetrical with __ptrace_detach().

No problem there.


Thanks,
Roland

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Oleg Nesterov]

On 02/04, Roland McGrath wrote:
>
> > - Fold ptrace_exit() into forget_original_parent(), it is trivial
> >   now. More importantly, this makes the code more symmetrical with
> >   reparent_thread().
>
> Please don't do this.

Roland, I don't know how to reply ;) Because you didn't comment the
previois patch

	[PATCH 3/4] reparent_thread: fix a zombie leak if /sbin/init ignores SIGCHLD
	http://marc.info/?l=linux-kernel&m=123321657429797

which hopefully fixes the ancient (but yes, minor) bug.


But I must admit, I disagree.

> The ptrace functions are separated not because they
> are large, but because they are the ptrace innards.  We have been moving
> away from the core task handling innards having intimate ptrace magic
> knowledge directly intertwined, and we don't want to move back the other
> way.

Yes. But The code becomes much more readable for the new readers,
it is immediately obvious waht forget_original_parent() does.

And we don't mix things, imho. The "ptrace" logic is lives in
the __ptrace_deatch(), so it stays separated.

> In later cleanups, we will eventually separate ptrace linkage from
> the tasklist_lock'd parent/child linkage more thoroughly.

Well, who knows how the code will involve ;) And when. But yes, sure,
in that case we will need a lot of changes, including the new helper.

> > - The same for ptrace_exit_finish(), and "ptrace_" is not correct
> >   any longer.
> >
> > - "ptrace_dead" doesn't match the reality, rename to "dead_list".
>
> For the same reasons, I am not entirely sanguine about overloading
> ptrace_entry for any case not related to ptrace.

Yes. But I don't see a better soulution for now.

But if reparent_thread returns true, we know the task is not traced
and detached, so it must be safe to use ->ptrace_entry. And in fact
this is not much worse than ptrace_exit() currently does.

And I think this is actually another reason to do all this work in
forget_original_parent(), this way we keep this hack "local".

> We want to be able to
> clean up the ptrace data structures in the future such that there may well
> not be any such list_head allocated in an untraced task_struct.

Yes, perhaps. But again, in that case we need a lot of other changes.


And. After the "[PATCH 3/4]" the code looks really bad. At least we
should rename ptrace_exit_finish and ptrace_dead. But if you think
this patch is buggy or you see the better fix, then of course this
patch should be dropped too.

> This case is sufficiently obscure, I can't see that anyone would really
> care about the marginal performance hit for just dropping the lock after
> reparent_thread returns true, call release_task(), re-lock and restart the
> loop on remaining children.  (And I don't see any atomicity problem with
> this unlock/relock.)

Oh. We can do this right now. But I don't think this makes the code
better. We should restart the list_for_each_entry_safe() loop again
and againg until we can't find the task to reap.

And we have the small problem with atomicity, we should call
find_new_reaper() every time we re-lock tasklist.

Oleg.

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Roland McGrath]

> Roland, I don't know how to reply ;) Because you didn't comment the
> previois patch

I think my remarks about using ->ptrace_entry covered my sentiments.

> Yes. But The code becomes much more readable for the new readers,
> it is immediately obvious waht forget_original_parent() does.

How so?  It's already "immediately obvious" that it cleans up ptrace stuff
and then does the father->children loop.

> And we don't mix things, imho. The "ptrace" logic is lives in
> the __ptrace_deatch(), so it stays separated.

The fact that task_struct.ptraced exists at all, much less that you need to
iterate on it, is purely part of the "ptrace logic".

> > In later cleanups, we will eventually separate ptrace linkage from
> > the tasklist_lock'd parent/child linkage more thoroughly.
> 
> Well, who knows how the code will involve ;) And when. But yes, sure,
> in that case we will need a lot of changes, including the new helper.

I think we know pretty much how it will evolve, because it will be us doing it.

> > For the same reasons, I am not entirely sanguine about overloading
> > ptrace_entry for any case not related to ptrace.
> 
> Yes. But I don't see a better soulution for now.

The clunky solution below works fine right now.  It perhaps theoretically
performs worse--when we actually hit this utterly obscure case, if that
ever actually happens in real life at all.

> But if reparent_thread returns true, we know the task is not traced
> and detached, so it must be safe to use ->ptrace_entry. And in fact
> this is not much worse than ptrace_exit() currently does.

Sure, we "know" it's safe because this code "knows" every intimate thing
about ptrace innards.  Great.  More of that instead of less is exactly what
we need, don't you think?  Or perhaps all these stages of clean-up had a
point, and it was to avoid exactly things like this.  ptrace_exit is itself
part of the ptrace innards to begin with; that's why it's called that!

> Yes, perhaps. But again, in that case we need a lot of other changes.

Not really so many.  I have thought about plan this from the beginning
when I did the cleanups creating ptrace_exit et al.  Because of the past
cleanups, pretty much all those changes are in places now called ptrace_*.
That's why those were cleanups: they made it easier to isolate such later
changes from the rest of the code.

> And. After the "[PATCH 3/4]" the code looks really bad. At least we
> should rename ptrace_exit_finish and ptrace_dead. But if you think
> this patch is buggy or you see the better fix, then of course this
> patch should be dropped too.

"Better" is obviously subjective.  I think you are only talking about
"better" in a micro-optimizing sense that here is only actually relevant to
optimizing rare cases that don't need it.  I think the "slow kludge" here
is "better" than overloading ptrace innards in any way because the "better"
comes from a sanity, readability and maintenance perspective that puts a
high value on isolating ptrace crud fully from core code, and because the
specific local cost in "slow" and "kludge" forms of "worse" is IMHO quite
marginal in this particular spot.

> Oh. We can do this right now. But I don't think this makes the code
> better. We should restart the list_for_each_entry_safe() loop again
> and againg until we can't find the task to reap.

Sure, but "restarting" doesn't mean walking any part of the list again--the
parts you've walked earlier are already gone.  It just means the pure
overhead of the restart (unlock/relock et al).

> And we have the small problem with atomicity, we should call
> find_new_reaper() every time we re-lock tasklist.

Ok.  That's right.  I don't think it hurts anything.


Thanks,
Roland

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Oleg Nesterov]

On 02/08, Roland McGrath wrote:
>
> "Better" is obviously subjective.

Yes. I understand.

> > Oh. We can do this right now. But I don't think this makes the code
> > better. We should restart the list_for_each_entry_safe() loop again
> > and againg until we can't find the task to reap.
>
> Sure, but "restarting" doesn't mean walking any part of the list again--the
> parts you've walked earlier are already gone.  It just means the pure
> overhead of the restart (unlock/relock et al).

Yes.

> > And we have the small problem with atomicity, we should call
> > find_new_reaper() every time we re-lock tasklist.
>
> Ok.  That's right.  I don't think it hurts anything.

It doesn't really hurt, but a bit ugly. Imho.

How about below? Modulo comments and some other cleanups. For example,
I think it is better to move the changing of ->real_parent into
reparent_thread().

Oleg.

kernel/ptrace.c:

	// also used by ptrace_detach()
	int __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
	{
		__ptrace_unlink(p);

		if (p->exit_state != EXIT_ZOMBIE)
			return 0;
		/*
		 * If it's a zombie, our attachedness prevented normal
		 * parent notification or self-reaping.  Do notification
		 * now if it would have happened earlier.  If it should
		 * reap itself we return true.
		 *
		 * If it's our own child, there is no notification to do.
		 * But if our normal children self-reap, then this child
		 * was prevented by ptrace and we must reap it now.
		 */
		if (!task_detached(p) && thread_group_empty(p)) {
			if (!same_thread_group(p->real_parent, tracer))
				do_notify_parent(p, p->exit_signal);
			else if (ignoring_children(tracer->sighand))
				p->exit_signal = -1;
		}

		if (!task_detached(p))
			return 0;

		/* Mark it as in the process of being reaped. */
		p->exit_state = EXIT_DEAD;
		return 1;
	}

	void ptrace_xxx(struct task_struct *tracer)
	{
		struct task_struct *p, *n;
		LIST_HEAD(ptrace_dead);

		write_lock_irq(&tasklist_lock);
		list_for_each_entry_safe(p, n, &tracer->ptraced, ptrace_entry) {
			if (__ptrace_detach(tracer, p))
				list_add(&p->ptrace_entry, &ptrace_dead);
		}
		write_unlock_irq(&tasklist_lock);

		list_for_each_entry_safe(p, n, &ptrace_dead, ptrace_entry) {
			list_del_init(&p->ptrace_entry);
			release_task(p);
		}
	}

kernel/exit.c:

	static int reparent_thread(struct task_struct *father, struct task_struct *p)
	{
		int dead;

		if (p->pdeath_signal)
			/* We already hold the tasklist_lock here.  */
			group_send_sig_info(p->pdeath_signal, SEND_SIG_NOINFO, p);

		if (task_detached(p))
			return 0;
		/* If this is a threaded reparent there is no need to
		 * notify anyone anything has happened.
		 */
		if (same_thread_group(p->real_parent, father))
			return 0;

		/* We don't want people slaying init.  */
		p->exit_signal = SIGCHLD;

		/* If we'd notified the old parent about this child's death,
		 * also notify the new parent.
		 */
		dead = 0;
		if (!p->ptrace &&
		    p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {
			do_notify_parent(p, p->exit_signal);
			if (task_detached(p)) {
				p->exit_state = EXIT_DEAD;
				dead = 1;
			}
		}

		kill_orphaned_pgrp(p, father);

		return dead;
	}

	static void forget_original_parent(struct task_struct *father)
	{
		struct task_struct *p, *n, *reaper;
		struct list_head *xxx;
		LIST_HEAD(child_dead);

		ptrace_xxx(father);

		write_lock_irq(&tasklist_lock);
		reaper = find_new_reaper(father);

		list_for_each_entry_safe(p, n, &father->children, sibling) {
			p->real_parent = reaper;
			if (p->parent == father) {
				BUG_ON(p->ptrace);
				p->parent = p->real_parent;
			}

			xxx = &p->real_parent->children;
			if (reparent_thread(father, p))
				xxx = &child_dead;
			list_move_tail(&p->sibling, xxx);;
		}
		write_unlock_irq(&tasklist_lock);

		list_for_each_entry_safe(p, n, &child_dead, sibling) {
			list_del_init(&p->sibling);
			release_task(p);
		}
	}

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Roland McGrath]

> It doesn't really hurt, but a bit ugly. Imho.

Agreed.

> How about below? Modulo comments and some other cleanups. For example,
> I think it is better to move the changing of ->real_parent into
> reparent_thread().

The exact split between reparent_thread and forget_original_parent (and
their names) never made much sense to me.

If ptrace_exit does its own lock/unlock, then it could move much earlier.
I'd be inclined to do it right before exit_signals().  But it should at
least short-circuit and not lock for list_empty(->ptraced), so we're not
adding a whole lock_irq/unlock_irq to the common case of no ptrace use.

> 			xxx = &p->real_parent->children;
> 			if (reparent_thread(father, p))
> 				xxx = &child_dead;
> 			list_move_tail(&p->sibling, xxx);;

I'd thought of this before.  But I didn't mention it because I was afraid
to wonder what might care about the use of ->sibling.  It really looks like
nothing does.  This is clearly the clean and nice way to go if there is no
problem with it.  

This change and moving ptrace_exit around should probably be separate patches.


Thanks,
Roland

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Oleg Nesterov]

On 02/10, Roland McGrath wrote:
>
> > How about below? Modulo comments and some other cleanups. For example,
> > I think it is better to move the changing of ->real_parent into
> > reparent_thread().
>
> The exact split between reparent_thread and forget_original_parent (and
> their names) never made much sense to me.
>
> If ptrace_exit does its own lock/unlock, then it could move much earlier.
> I'd be inclined to do it right before exit_signals().

Yes. But since I am paranoid, can we move the callsite later? I mean,
I'd prefer to make a separate (trivial) patch which moves it.

> But it should at
> least short-circuit and not lock for list_empty(->ptraced), so we're not
> adding a whole lock_irq/unlock_irq to the common case of no ptrace use.

Agreed, and probably forget_original_parent() can check empty(->children)
too.

> > 			xxx = &p->real_parent->children;
> > 			if (reparent_thread(father, p))
> > 				xxx = &child_dead;
> > 			list_move_tail(&p->sibling, xxx);;
>
> I'd thought of this before.  But I didn't mention it because I was afraid
> to wonder what might care about the use of ->sibling.  It really looks like
> nothing does.

Yes, nobody should at least. Nobody can find this task on its own list.

> This is clearly the clean and nice way to go if there is no
> problem with it.

OK, will try to send the patches soon.

> This change and moving ptrace_exit around should probably be separate patches.

Yes, yes, sure.


If you don't mind, I'd prefer to make these changes on top of [PATCH 3/4],
reparent_thread-fix-a-zombie-leak-if-sbin-init-ignores-sigchld.patch
(and this one should be dropped).

Because that patch fixes the bug and changes the behaviour, while the
discussed changes are cleanups.

Oleg.

Re: [PATCH 4/4] forget_original_parent: cleanup ptrace pathes

[Roland McGrath]

> Yes. But since I am paranoid, can we move the callsite later? I mean,
> I'd prefer to make a separate (trivial) patch which moves it.

Absolutely.

> Agreed, and probably forget_original_parent() can check empty(->children) too.

Yes, that might optimize (vs what we've always done) a case so common that
it actually makes a bit of difference in the grand scale. :-)

> Yes, nobody should at least. Nobody can find this task on its own list.

I was more worried about presumptions of all the linkage being complete
until release_task.  But I don't see any actual thing to worry about.

> If you don't mind, I'd prefer to make these changes on top of [PATCH 3/4],
> reparent_thread-fix-a-zombie-leak-if-sbin-init-ignores-sigchld.patch
> (and this one should be dropped).
> 
> Because that patch fixes the bug and changes the behaviour, while the
> discussed changes are cleanups.

I don't object to that patch first (and it might be fine for -stable even)
as long as these cleanups are really going in soon so that ->ptrace_entry
abuse disappears quickly.


Thanks,
Roland