Re: [PATCH] arch_ptrace_stop

[Oleg Nesterov <oleg@tv-sign.ru>]

Roland, I am sorry for delay,

On 12/13, Roland McGrath wrote:
>
> > Currently ptrace_stop() schedules in TASK_TRACED state even if we have a
> > pending SIGKILL. With this patch this is still possible, but unless
> > arch_ptrace_stop_needed() is true and thus we will check sigkill_pending().
>
> Currently the siglock is always held throughout.  The case this change
> addresses is when no SIGKILL was already pending before we took the lock.
> Currently, a new SIGKILL cannot come in until we've released the lock,
> which is after we've set TASK_TRACED.  The signal's sender will hold the
> lock while checking each thread's state, waking up any in TASK_TRACED.
>
> When arch_ptrace_stop_needed() is true, we release the siglock for an
> unknown period (might block, etc).  If a SIGKILL is sent there, it becomes
> pending while we are in TASK_RUNNING or a normal blocked state.  Next we
> finish arch_ptrace_stop() and reacquire the siglock.

Yes, yes, I see.

> Now entering
> TASK_TRACED would leave us unkillable because SIGKILL is already pending
> and nothing else (except PTRACE_CONT et al) will try to wake us up.

But this doesn't differ from the case when SIGKILL was already pending when
we enter ptrace_stop, and arch_ptrace_stop_needed() == false, that was my
point.

Yes, arch_ptrace_stop() can take a long time, might block, etc. But what
about TIF_SYSCALL_TRACE ? The task can recieve SIGKILL while executing the
syscall which can also block and so on, but do_syscall_trace(entryexit == 1)
doesn't check the pending signal.

I should clarify my question. What I can't understand is the subtle dependency
on the result of arch_ptrace_stop_needed(). This means that it is hard to
predict the behaviour.

IOW, can't we

	- ignore the pending SIGKILL (current behaviour)
		-- OR --
	- always check it unconditionally, before setting TASK_TRACED

? This looks a bit more consistent to me.

Please also note "before setting TASK_TRACED" above. With this patch we set
TASK_TRACED under ->siglock, and then change the ->state to TASK_RUNNING if
killed == 1. Minor, but this doesn't look correct, we can fool the tracer
which does ptrace_check_attach().

Oleg.