Re: [PATCH] Fix for ptrace breakage

[OGAWA Hirofumi]

Daniel Jacobowitz <[e-mail]> writes:

> > > Some comments.  First of all, you said you fixed a race on
> > > current->ptrace and some other bugs - would you mind saying where they
> > > were?  It's definitely cleaner after your patch but I'd like to
> > > understand where you found bugs, since I think you're introducing more.
> > 
> > It's the following
> > 
> > 		task_t *trace_task = p->parent;
> > 		int ptrace_flag = p->ptrace;
> > 		BUG_ON (ptrace_flag == 0);
> > 
> > 		__ptrace_unlink(p);
> > 		p->ptrace = ptrace_flag;
> > 		__ptrace_link(p, trace_task);
> 
> We have the tasklist lock.  How can there be a race here?  The parent
> can't detach while we're holding the tasklist lock.  If there is a race
> with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.

No. If the real parent don't change ->ptrace, it doesn't need
lock.

> > > So you reparent children on the ptrace_list right here.  But they still
> > > need to go through zap_thread!  You're right, the do_notify_parent in
> > > zap_thread isn't necessary; it'll be taken care of in sys_wait4.  The
> > > orphaned pgrp check is still relevant though.
> > 
> > ??? You forget tasklist_lock?
> 
> Huh?
> 
> The problem I am describing is when a child - which will become an
> orphaned pgrp when ``father'' dies - is being ptraced at the moment of
> ``father''s death.  With your patch it will be moved to
> reaper->ptrace_children (or child_reaper->ptrace_children) but never
> orphaned properly.  It'll miss a signal.
>
> > > If you're going to remove the if, you need to maintain its effect! 
> > > See:
> > > > -		if (p->parent != father) {
> > > > -			BUG_ON(p->parent != p->real_parent);
> > > > -			return;
> > > > -		}
> > > 
> > > This is the case where we were tracing something.  The ptrace_unlink
> > > returned it to its original parent.  It doesn't need the
> > > remove_parent/add_parent (though they are harmless); it does need to
> > > avoid the orphaned pgrp check.  It may need the do_notify_parent check,
> > > which was a bug in the previous code.
> > 
> > What is the basis which you think it is bug?
> 
> The death of a tracing process should not have any effect on the traced
> process except to untrace it.  It should not go through the orphaning
> checks.  The orphaning checks assume that the exiting process is the
> real parent, and will orphan the pgrp if it is not in the same
> session... as its tracer!  That's a bug.

Ah, ok. I think, it's longtime (odd) behavior. And you think, it's
a bug. Right?

And, both of your and old code has odd behavior. yes?
--
OGAWA Hirofumi <[e-mail]>