Re: [PATCH v2] Make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Oleg Nesterov <oleg@redhat.com>]

Hello,

On 09/11, Tejun Heo wrote:
>
> On Thu, Sep 08, 2011 at 08:17:47PM +0200, Oleg Nesterov wrote:
> > > There are places which assume ->ptrace is protected by siglock.
> >
> > Really? Once again, I agree. But _afaics_ currently this is not strictly
> > needed. PT_PTRACED/PT_SEIZED should not go away under ->siglock, yes, but
> > it seems that it is fine to set them.
>
> Hmmm.... I haven't checked each direction.  Maybe we don't strictly
> need it on setting it but I definitely was assuming that ->ptrace was
> protected by siglock while coding recent ptrace changes.  Can't the
> following happen?
>
> * ptracer seizes child, sets PT_PTRACED and then OR PT_SEIZED.
>
> * ptracee enters signal delivery path with group stop scheduled.
>   PT_PTRACED is visible and group stop is transformed into
>   JOBCTL_TRAP_STOP.
>
> * ptracee enters do_jobct_trap().  PT_SEIZED is still not visible and
>   it takes the path for the old behavior.
>
> * ptracer SEIZE'd and expects PTRACE_EVENT_STOP but it gets the old
>   no-siginfo trap.

Heh ;) Please look at http://marc.info/?l=linux-kernel&m=131541614232539

	> @@ -263,7 +267,7 @@ static int ptrace_attach(struct task_struct *task, long request,
	>  	if (task->ptrace)
	>  		goto unlock_tasklist;
	>
	> -	task->ptrace = PT_PTRACED;
	> +	task->ptrace = PT_PTRACED | (flags << PT_OPT_FLAG_SHIFT);
	>  	if (seize)
	>  		task->ptrace |= PT_SEIZED;

	Hmm. Tejun, Denys, this doesn't look exactly right.

	I already thought about this before, but somehow I convinced myself
	this is fine.

	I think we should set both PT_PTRACED | PT_SEIZED "atomically", at
	once. Otherwise, say, the tracee can do do_jobctl_trap() in between,
	no? Nothing really bad can happen, but we shouldn't lose EVENT_STOP
	code.

Yes, we need to set them both at once.

And yes, I agree, it is better to do this under ->siglock even if currently
this is not strictly necessary.

> > > and linking are protected by siglock
> >
> > Hmm. Could you explain this? Why do want __ptrace_link under ->siglock ?
>
> Because it's much simpler to assume that w/ siglock locked, everything
> including ->parent is set up properly w.r.t. ->ptrace.

Well, but then we shouldn't rely on tracee's ->siglock. The tracee simply
do not care about its ->ptrace_entry, only the tracer does.

We need to rework the locking, yes. But we need the lock which protects
the parent's list_head (currently we rely on tasklist). Yes, a single
lock can't help. We already use ->cred_guard_mutex though.

This needs more thinking, but imho child->siglock is pointless here.

Oleg.