lkml.org 
[lkml]   [2011]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/11] ptrace: implement PTRACE_SEIZE
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.



\
 
 \ /
  Last update: 2011-05-10 15:23    [W:0.665 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site