Re: [PATCH 09/16] ptrace: make do_signal_stop() use ptrace_stop() if the task is being ptraced

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, Dec 23, 2010 at 01:26:34PM +0100, Oleg Nesterov wrote:
> I missed this part of the changelog. "visible via fs/proc" is not
> the only change. Another change is how the tracee reacts to SIGCONT
> after that. With this patch it can't wake the tracee up.
> 
> Consider the simplest example. The tracee is single-threaded and
> debugger == parent. Something like
> 
> 	int main(void)
> 	{
> 		int child, status;
> 
> 		child = fork();
> 		if (!child) {
> 			ptrace(PTRACE_TRACEME);
> 
> 			kill(getpid(), SIGSTOP);
> 
> 			return 0;
> 		}
> 
> 		wait(&status)
> 		// the tracee reports the signal
> 		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);
> 		// it should stop after that
> 		ptrace(PTRACE_CONT, child, SIGSTOP);
> 
> 		wait(&status);
> 		// now it is stopped
> 		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);
> 
> 		kill(child, SIGCONT);
> 
> 		wait(&status);
> 		assert(WIFSTOPPED() && WSTOPSIG() == SIGCONT);
> 
> This won't work with this patch. the last do_wait() will hang forever.
> Probably this is fine, I do not know. Please take a look and ack/nack
> explicitly.

Yes, before the change, the task would respond to SIGCONT before the
first ptrace request succeeds after attach.  To me, this doesn't seem
to be anything intentional tho.  It seems a lot of ptrace and group
stop interactions is in the grey area with only the current (quirky,
I'm afraid) behavior drawing almost arbitrary lines across different
behaviors.

We can try to preserve all those behaviors but I don't think that will
be very beneficial.  I think the best way to proceed would be
identifying the sensible and depended upon assumptions and then draw
clear lines from there stating what's guaranteed and what's undefined.
We'll have to keep some of quirkiness for sure.

Anyways, pondering and verifying all the possibly visible changes
definitely is necessary, but that said, we fortunately have rather
limited number of ptrace users and their usages don't seem to be too
wild (at least on my cursory investigation), so I think it to be
doable without breaking anything noticeably.  But yeap we definitely
need to be careful.

> However. There is something I missed previously, and this small
> detail doesn't look good to me: the behaviour of SIGCONT becomes
> a bit unpredictable. Suppose it races with do_signal_stop() and
> clears GROUP_STOP_PENDING or SIGNAL_STOP_DEQUEUED before, in this
> case in can "wakeup" the tracee.
> 
> IOW. Let's remove the 2nd wait() in the code above, the parent
> does
> 
> 		wait(&status)
> 		// the tracee reports the signal
> 		assert(WIFSTOPPED() && WSTOPSIG() == SIGSTOP);
> 		// it should stop after that
> 		ptrace(PTRACE_CONT, child, SIGSTOP);
> 
> 		kill(child, SIGCONT);
> 
> Now we can't know id this SIGCONT works or not. If the tracee
> is already parked in ptrace_stop() - it doesn't. If the parent
> wins - the tracee doesn't stop.

We can change ptrace_stop() to use TASK_STOPPED if it's stopping for
group stop to preserve the original behavior but if it doesn't disturb
current users (and I doubt it would), I think it would be far cleaner
to state that the behavior is undefined.  The current behavior - it
works if there hasn't been whichever ptrace operation inbetween - is
quite unexpected anyway, IMHO.

And, for longer term, I think it would be a good idea to separate
group stop and ptrace trap mechanisms, so that ptrace trap works
properly on per-task level and properly transparent from group stop
handling.  The intertwining between the two across different domains
of threads inhfferently involves a lot of grey areas where there is no
good intuitive behavior.

> OTOH. Looking at this patch, I can no longer understand why
> ptrace_check_attach() can silently do s/STOPPED/TRACED/. Indeed,
> as Tejun pointed out, if ptrace_stop() needs arch_ptrace_stop(),
> then ptrace_check_attach() should be arch-friendly as well.
> 
> So, the patch looks like the bugfix, but I do not understand this
> ia64/sparc magic and thus I do not know how important this fix.
> Nobody complained so far, though.

IIUC, it dumps the register window to userland memory.  ia64 has this
stacked windows of registers which gets wound up and unwound according
to function calls and those need to be dumped to userland memory so
that the debugger can PEEK and POKE them.  Not really sure why
skipping it didn't cause any problem until now tho.

Thanks.

-- 
tejun