Re: [PATCH 02/11] ptrace: implement PTRACE_SEIZE

[Oleg Nesterov <oleg@redhat.com>]

On 05/10, Tejun Heo wrote:
>
> On Mon, May 09, 2011 at 06:18:38PM +0200, Oleg Nesterov wrote:
>
> > > @@ -247,6 +272,14 @@ static int ptrace_attach(struct task_struct *task)
> > >  	if (task_is_stopped(task)) {
> > >  		task->jobctl |= JOBCTL_STOP_PENDING | JOBCTL_TRAPPING;
> > >  		signal_wake_up(task, 1);
> > > +	} else if (seize) {
> > > +		/*
> > > +		 * Otherwise, SEIZE uses jobctl trap to put tracee into
> > > +		 * TASK_TRACED, which doesn't have the nasty side effects
> > > +		 * of sending SIGSTOP.
> > > +		 */
> > > +		task->jobctl |= JOBCTL_TRAP_SEIZE;
> > > +		signal_wake_up(task, 0);
> >
> > OK... I am a bit worried we can set JOBCTL_TRAP_SEIZE even if the tracee
> > was already killed, and if it is killed later JOBCTL_TRAP_SEIZE won't be
> > cleared. Probably this is fine, ptrace_stop()->schedule() won't sleep in
> > this case.
>
> Hmmm... if killed, the tracee would go through __ptrace_unlink() which
> always clears JOBCTL_TRAP_MASK which includes JOBCTL_TRAP_SEIZE.  What
> am I missing?

No. If killed, the tracee becomes a zombie (but see below). __ptrace_unlink()
won't be called until the tracee does wait().

> > > @@ -1752,12 +1752,13 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
> > >  	set_current_state(TASK_TRACED);
> > >
> > >  	/*
> > > -	 * We're committing to trapping.  Clearing JOBCTL_TRAPPING and
> > > -	 * transition to TASK_TRACED should be atomic with respect to
> > > -	 * siglock.  This should be done after the arch hook as siglock is
> > > -	 * released and regrabbed across it.
> > > +	 * We're committing to trapping.  Adjust ->jobctl.  Updates to
> > > +	 * these flags and transition to TASK_TRACED should be atomic with
> > > +	 * respect to siglock.  This should be done after the arch hook as
> > > +	 * siglock may be released and regrabbed across it.
> > >  	 */
> > >  	task_clear_jobctl_trapping(current);
> > > +	current->jobctl &= ~JOBCTL_TRAP_SEIZE;
> >
> > Yes. But, it seems, this is too late.
> >
> > Suppose that the JOBCTL_TRAP_SEIZE tracee was SIGKILLED before it reports
> > PTRACE_EVENT_INTERRUPT. Now, if arch_ptrace_stop_needed() == T, ptrace_stop()
> > returns without clearing JOBCTL_TRAP_SEIZE/TIF_SIGPENDING. This means
> > get_signal_to_deliver() will loop forever.
>
> Argh... right it has an early exit path there.

Yes, and thus the tracee will loop forever until the tracer detaches, so
it can't even become a zombie. And the tracer can't detach via ptrace().

> > I never understood why ptrace_stop()->sigkill_pending() logic, I think
> > we should check fatal_signal_pending() unconditionally.
>
> Probably the best way to do it is adding fatal_signal_pending() into
> may_ptrace_stop() so that the failure path shares most of the code
> path instead of doing quick dirty exit?  It's just nasty to have early
> exit above there and walking through the normal code path wouldn't
> hurt SIGKILL either.

Yes, probably. But, apart from the user-visible change which needs another
discussion, there are more problems. fatal_signal_pending() can "wrongly"
return false if the caller is tracehook_report_exit(). OTOH, it can correctly
return true (exec), but we probably want to stop. Let's discuss this later,
I think this has nothing to do with your patches. The main problem is not
fatal_signal_pending(), we need the better definition of explicit SIGKILL
behaviour. Not for ptrace only, there are other issues.

Oleg.