Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction

[Oleg Nesterov <oleg@redhat.com>]

Hi Tejun,

On 07/13, Tejun Heo wrote:
>
> Sorry about the long delay.

No, no, you shouldn't use this mantra.  I hold the copyright!

> On Thu, Jul 07, 2011 at 09:03:24PM +0200, Oleg Nesterov wrote:
> > Without the patch it hangs. After the patch SIGSTOP "injected" by the
> > tracer is not ignored and stops the tracee.
>
> I always felt the ability to 'inject' different signal there is rather
> useless and prone to induce weird issues.  It would be better if
> ptrace_signal() is part of signal delivery action after all the checks
> so that the ptracer says whether to proceed with the action or not but
> no more.  Well...

Oh, probably. If the tracer wants a different signr, it can simply do
tkill() + PTRACE_CONT(0). I agree. Although perhaps this is needed for
gdb, I dunno. But we can't change this.

And, I'd like to clarify... It is not that I think it is that important
to ensure PTRACE_CONT(SIGSTOP) will actually stop the tracee if the tracer
changes the original signal. I simply do not know if it is used this way.

The only important thing, imho, the behaviour shouldn't depend on
/dev/random, with this patch it is at least clearly defined.

> > So lets add STOP_DEQUEUED _before_ we report the signal. It has no effect
> > unless sig_kernel_stop() == T after the tracer resumes us, and in the
> > latter case the pending STOP_DEQUEUED means no SIGCONT in between, we
> > should stop.
>
> Anyways, yes, this seems to be a nice improvement but it looks very
> weird (and difficult to comprehend) to be setting STOP_DEQUEUED
> unconditionally in ptrace_signal().

Yeeees, agreed. I even added the comment to explain this weirdness.

> Wouldn't it be better to flip the
> flag so that we have CONT_RECEIVED before doing this?

May be. You know, I thought about this when I did ee77f075
"signal: Turn SIGNAL_STOP_DEQUEUED into GROUP_STOP_DEQUEUED".

Or may be we can simply rename it into STOP_ALLOWED. In this case
we can even set it unconditionally before dequeue_signal().


Anyway, whatever we do, this patch doesn't complicate the
CONT_RECEIVED/STOP_ALLOWED change. Can't we do this later?

Oleg.