lkml.org 
[lkml]   [2012]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: ptrace fixes for 3.2
Hi Tejun,

On 01/03, Tejun Heo wrote:
>
> On Tue, Jan 03, 2012 at 04:44:04PM +0100, Oleg Nesterov wrote:
> > It fails because ->real_parent sees its child in EXIT_DEAD state
> > while the tracer is going to change the state back to EXIT_ZOMBIE
> > in wait_task_zombie().
>
> Argh.... EXIT_ZOMBIE -> DEAD -> ZOMBIE dancing in wait_task_zombie()
> is just nasty. Didn't realize it was doing that. :(

We both missed this ;)

> > The offending commit is 823b018e which moved the EXIT_DEAD check,
> > but in fact we should not blame it. The original code was not
> > correct as well because it didn't take ptrace_reparented() into
> > account and because we can't really trust ->ptrace.
> >
> > This patch adds the additional check to close this particular
> > race but it doesn't solve the whole problem. We simply can't
> > rely on ->ptrace in this case, it can be cleared if the tracer
> > is multithreaded by the exiting ->parent.
>
> I'm not following this part. Can you please explain it in a bit more
> detail?

Before 823b018e the code was:

if (!ptrace && p->ptrace) {
wo->notask_error = 0;
return 0;
}
if (p->exit_state == EXIT_DEAD)
return 0;
There are 2 problems:

1. it is not correct to clear ->notask_error unless
this child is ptrace_reparented(). Nobody will
wakeup us if EXIT_DEAD was set by our sub-thread.

2. We can not rely on ->ptrace to detect this case.

Suppose that the tracer is multithreaded, it has
two threads T1 and T2, T1 traces our child.

- T2 does do_wait(WEXITED), sets EXIT_DEAD, drops
tasklist_lock.

- T1 exits and does __ptrace_detach(), this means
__ptrace_unlink() and nothing more.

- Now, real_parent does do_wait() and sees the
EXIT_DEAD child but ->ptrace = 0.

- finally T2 sets EXIT_DEAD but it is too late,

The patch doesn't solve the 2nd (btw very old) problem. Fortunately
this race is very unlikely.

> > I think we should kill EXIT_DEAD altogether, we should always
> > remove the soon-to-be-reaped child from ->children or at least
> > we should never do the DEAD->ZOMBIE transition. But this is too
> > complex for 3.2.
>
> Agreed. Removing the reverse transition shouldn't be too difficult
> and can be done without affecting fast non-ptrace path. ie. if the
> child is ptraced, drop readlock, grab writelock, recheck, buffer
> states to copy out to userland, detach and transit to DEAD if
> necessary.

Yes.

> > Also, I think wait_consider_task() needs more fixes. I do not
> > think we should clear ->notask_error without WEXITED in this
> > case, but this is what we do in the EXIT_ZOMBIE case.
>
> Hmmm... I'm not sure about that. Why do you think so?

I am not sure too. But why do_wait() should sleep if it is called
without WEXITED (lets ignore WCONTINUED) and the child is ZOMBIE?
I think it should return -ECHILD, like it does if the child is not
traced.

IOW. Suppose we have a single EXIT_ZOMBIE child. If it is not traced,
do_wait(WSTOPPED) returns -ECHILD. If the child is traced (by another
process) do_wait() sleeps until detach just to return the same error.
This looks a bit strange.

> Acked-by: Tejun Heo <tj@kernel.org>

Great, thanks.

Oleg.



\
 
 \ /
  Last update: 2012-01-03 18:17    [from the cache]
©2003-2011 Jasper Spaans