Messages in this thread | | | Date | Wed, 13 Jul 2011 20:33:22 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/1] ptrace: fix ptrace_signal() && STOP_DEQUEUED interaction |
| |
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.
| |