lkml.org 
[lkml]   [2008]   [Mar]   [27]   [last100]   RSS-feed
Views: [more markup]  [less markup]  [headers]  [forward] 
 
Messages in this thread
Complete thread (EXPERIMENTAL)
/
DateThu, 27 Mar 2008 16:44:54 +0300
FromOleg Nesterov <>
SubjectRe: [PATCH] Discard notification signals when a tracer exits
Digg This
On 03/27, Petr Tesarik wrote:

On Wed, 2008-03-26 at 21:17 +0300, Oleg Nesterov wrote:
[...]
If we really need this, _perhaps_ it is better to change do_syscall_trace(), so that the tracee checks ->ptrace before sending the signal to itself.

You're missing the point. The child _is_ traced before sending the signal. It leaves the notification code in ->exit_code, so that the tracer can fetch it with a call to wait4(). Later, the same field is used to tell the tracee which signal the tracer delivered to it. However, if the tracer dies before it reads (and resets) the value in ->exit_code, the tracee interprets the notification code as the signal to be delivered.

I see! That is why I suggested to re-check ->ptrace, and if we are not ptraced any longer - discard the notification. Even better, we can change ptrace_stop() as Roland pointed out.

That's not what you want. It's totally OK if the tracer resumes the tracee with a signal and immediately exits before the tracee returns from schedule(), so this approach won't work. Sorry.

You misunderstood. I didn't claim this approach is right, but I think it is better if we desperately need to fix this problem. Yes the tracee can lost the right SIGTRAP, this is obvious. But this can happen with your patch as well, the kernel just can't know what should we do with SIGTRAP in ->exit_code.

In short, I (roughly) meant

ptrace_stop:
...
		// return path
		spin_lock_irq(->siglock);
		current->last_siginfo = NULL;
		// can be false positive
		if (!->ptrace && (->exit_code & 7f) == SIGTRAP)
			->exit_code = 0;
...

Yes, this is not right too. (and yes, ptrace_stop() is much better than do_syscall_trace() I suggested originally).

Anyway, I have a very real bug report from a paying customer, so whatever their use case, I'm bound to solve it for them.

Just curious: what is the bug report? Why do they want to SIGKILL strace?
And I always
thought that pushing a fix upstreams is considered "the right thing" (TM).

Once again: of course I agree it would be nice to fix the problem if we had a clean fix.

Yes I think this problem is _relatively_ minor, and I don't really think it is BUG. But I am not maintainer or expert, just my personal opinion. I jumped into discussion only because I don't agree with the patch, not because I think we should not fix this.

(btw, I think that maintainer has already give a good summary ;)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1628,6 +1628,8 @@ ptrace_stop(int exit_code, int c do_notify_parent_cldstop(current, CLD_TRAPPED);
read_unlock(&tasklist_lock);
schedule();
+ if (current->flags & PF_PTRACEORPHAN & clear_code) + current->exit_code = 0;
} else {
/*
* By the time we got the lock, our tracer went away.
And then replace p->exit_code = 0 in my original patch with something like p->flags |= PF_PTRACEORPHAN. Better?

This is racy, and we can't modify p->flags, and I don't really understand how this can help.

Why is it racy? It's ugly, but where's the race condition? The tracee cannot get out of schedule() until the tracer lets it go. And it doesn't let it go, because there can only be one tracer for any given task and that's the task which is exiting. So AFAICS doing (almost) anything to the tracee is safe.

SIGKILL can wake up the tracee, it could be TASK_RUNNING when the tracer plays with its flag, this is wrong.

But there are other problems. It is racy because TASK_TRACED doesn't necessary mean the tracee sleeps in TASK_TRACED state, it is possible that the tracee is running and waits for tasklist_lock() in ptrace_stop. As a very minimum, we should clear PF_PTRACEORPHAN.
More. Suppose that we set PF_PTRACEORPHAN, and then ptrace_untrace() changes TASK_TRACED to TASK_STOPPED. Another tracer attaches to the poor tracee, ptrace_check_attach() changes TASK_STOPPED to TASK_TRACED. When the new tracer wakes up the tracee, it will see PF_PTRACEORPHAN and clear ->exit_code.

Oleg.




\
ISP Services
Valid XHTML 1.0!\ /
Valid CSS! Last update: 2008-03-27 14:47    [W:0.226 / U:0.350 seconds]
©2003-2005 Jasper Spaans