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

[Oleg Nesterov <oleg@redhat.com>]

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);
		}
	}