Re: [PATCH 2/3] wait_task_stopped: tidy up the noreap case

[Oleg Nesterov <oleg@tv-sign.ru>]

On 11/16, Roland McGrath wrote:
>
> This is good, but not quite enough.  The original intent behind having the
> test was never to return mismatched stale/fresh data.  (Not that it ever
> really worked as intended.)  That is, it's fine if the task has woken up
> and done other things while WNOWAIT reports it as stopped--that's stale
> data, but it just means the waitid call happened "before" the resumption.
> However, it should not report anything that could not possibly have been
> true before the resumption.  i.e. a changed exit_code, which now means an
> normal termination status or a death signal, not the stop signal.  This
> also applies to the uid, in case the thread called setuid upon resuming
> (and even to ptracedness, not that that one really matters).  (It doesn't
> matter for rusage, since that's not really an exact change of state with
> reliable ordering anyway.)
>
> So the setting of uid and why should also move before read_unlock.

Yes I agree, and I also realized this. In fact, I already tried to do this
a long ago: http://marc.info/?l=linux-kernel&m=112809846204068, please note
that !noreap branch should be changed as well.

This time I'am trying to cleanup (remove) the games with ->exit_state first.
I am mostly concerned about 3/3 patch, what do you think about it?

And. Please note that 3/3 removes the "It must also be done with the write
lock held to prevent a race with the EXIT_ZOMBIE case" comment. Afaics, we
don't need write_lock(tasklist) any longer, we can simplify things further
and remove the EGAIN case completely.

However, wait_task_stopped does:

	/* move to end of parent's list to avoid starvation */
	remove_parent(p);
	add_parent(p);

That is why we need write_lock(). Is this really so important? Yes, the next
do_wait() can find another "interesting" task a bit faster, but only a little
bit. wait_task_continued() could be optimized in a same manner...

Also. I think the locking is not complete. {read,write}_lock(tasklist) can't
really pin the task in TRACED/STOPPED state. We need ->siglock to ensure that
the child can't escape from get_signal_to_deliver() at least, so it can't do
exit/setuid/etc. I was going to try to do this later, because this needs nasty
changes...

Oh well. OK, we can ignore patches 2-3 for now. I'd like to know your opinion
before going further, perhaps I missed something else.

> While you're at it, you could fix the status argument to wait_noreap_copyout.
> It should be just exit_code, not the WIFSTOPPED bit format it does now.

OK, unless Scott is going to do this.

Oleg.