[PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[Oleg Nesterov]

Both ptrace_stop() and do_signal_stop() pathes always take ->siglock and
do recalc_sigpending() after wakeup.

This means that if the tracer sees task_is_traced(child) == T (perhaps it
it was actually TASK_STOPPED before ptrace_check_attach) under ->siglock,
it can use the plain wake_up_process() instead of signal_wake_up().

(and note that ptrace_resume() does wake_up_process() too).

We also have sys_clone(CLONE_STOPPED), but a) it must set TIF_SIGPENDING
by hand anyway, and b) it is deprecated.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/ptrace.c~2_KILL_SIGWAKE	2009-02-08 05:11:52.000000000 +0100
+++ 6.29-rc3/kernel/ptrace.c	2009-02-08 06:22:26.000000000 +0100
@@ -60,11 +60,10 @@ static void ptrace_untrace(struct task_s
 {
 	spin_lock(&child->sighand->siglock);
 	if (task_is_traced(child)) {
-		if (child->signal->flags & SIGNAL_STOP_STOPPED) {
+		if (child->signal->flags & SIGNAL_STOP_STOPPED)
 			__set_task_state(child, TASK_STOPPED);
-		} else {
-			signal_wake_up(child, 1);
-		}
+		else
+			wake_up_process(child);
 	}
 	spin_unlock(&child->sighand->siglock);
 }

Re: [PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[Roland McGrath]

> Both ptrace_stop() and do_signal_stop() pathes always take ->siglock and
> do recalc_sigpending() after wakeup.

Yes, that's true.  But so what?  Why is this a reason to introduce yet
another unconditional (i.e. wrong) wake_up_process?  signal_wake_up does
the job fine, i.e. it calls wake_up_state the right way.  For exactly the
reasons you cited, setting TIF_SIGPENDING is both superfluous and
harmless--its effects will happen upon resume whether it was set or not.
So what's wrong with signal_wake_up?


Thanks,
Roland

Re: [PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[Oleg Nesterov]

On 02/08, Roland McGrath wrote:
>
> > Both ptrace_stop() and do_signal_stop() pathes always take ->siglock and
> > do recalc_sigpending() after wakeup.
>
> Yes, that's true.  But so what?  Why is this a reason to introduce yet
> another unconditional (i.e. wrong) wake_up_process?  signal_wake_up does
> the job fine, i.e. it calls wake_up_state the right way.

We are holding ->siglock, and task->state is TASK_TRACED. We can not do
the "wrong" wakeup, afaics.

And even if we forget about the unneeded TIF_SIGPENDING/kick_process,
personally I can't agree that signal_wake_up() uses wake_up_state()
more "correctly". This doesn't matter of course, but TASK_INTERRUPTIBLE
(and to some extent TASK_WAKEKILL) has nothing to do here, imho.

Of course, the tracee can already spin for ->siglock in TASK_RUNNING
when we do wake_up_process(), but this is true for signal_wake_up()
as well, and this is fine.

> For exactly the
> reasons you cited, setting TIF_SIGPENDING is both superfluous and
> harmless--its effects will happen upon resume whether it was set or not.
> So what's wrong with signal_wake_up?

Because it complicates the understanding of this code. I spent a lot
of time trying to understand this signal_wake_up().

Perhaps this is just me. But when you see the code which does something,
it is always good to understand the reason, otherwise the code at least
looks wrong.

Or, at least we can add the comment

		/*
		 * there are no reasons for signal_wake_up(), we could
		 * use wake_up_state() or wake_up_process() instead.
		 */
		signal_wake_up(child, 1);

Oleg.

Re: [PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[Roland McGrath]

> We are holding ->siglock, and task->state is TASK_TRACED. We can not do
> the "wrong" wakeup, afaics.

I guess that's true with the siglock.  But you'd be wrong to think that
this sort of detailed thinking is why wake_up_process() appears *anywhere
at all* in ptrace-related code.  I'm really quite sure that it's the
aforementioned (ancient) lack of detailed thinking about it that led to
wake_up_process() appearing originally--so any use of it reminds us of that
dubious past.

> Because it complicates the understanding of this code. I spent a lot
> of time trying to understand this signal_wake_up().
> 
> Perhaps this is just me. But when you see the code which does something,
> it is always good to understand the reason, otherwise the code at least
> looks wrong.

I had presumed most people interpret it the way I do: it does
signal_wake_up(,1), meaning "whatever it is that works right for SIGKILL or
SIGCONT", which it seems intuitive to think is right for this case too.
You don't have to think about exactly what is always exactly right for this
case, because it makes sense to think that this is like the wakeup that
SIGCONT would do.  To me, it takes much more thought to be convinced that
wake_up_process() without other considerations is correct here--because it
looks like such a scary, unconditional thing, whereas normally that is
wrapped up inside calls that handle appropriate bookkeeping--signal_wake_up()
being one of those.


Thanks,
Roland

Re: [PATCH 2/3] ptrace_untrace: use wake_up_process() instead of bogus signal_wake_up()

[Oleg Nesterov]

I have already asked Andrew to ignore this series.

But since I am a bore...

On 02/08, Roland McGrath wrote:
> > We are holding ->siglock, and task->state is TASK_TRACED. We can not do
> > the "wrong" wakeup, afaics.
>
> I guess that's true with the siglock.  But you'd be wrong to think that
> this sort of detailed thinking is why wake_up_process() appears *anywhere
> at all* in ptrace-related code.  I'm really quite sure that it's the
> aforementioned (ancient) lack of detailed thinking about it that led to
> wake_up_process() appearing originally--so any use of it reminds us of that
> dubious past.

Yes, wake_up_process() is not always right. But without ->siglock
signal_wake_up() is not "safe" too if we want to wake up the
TRACED/STOPPED task.

If we change ptrace_resume() to use signal_wake_up(), we won't fix
this minor problem. Fortunately, this is really minor, if the task
is not TASK_TRACED any longer - it must be dying.

> > Because it complicates the understanding of this code. I spent a lot
> > of time trying to understand this signal_wake_up().
> >
> > Perhaps this is just me. But when you see the code which does something,
> > it is always good to understand the reason, otherwise the code at least
> > looks wrong.
>
> I had presumed most people interpret it the way I do: it does
> signal_wake_up(,1), meaning "whatever it is that works right for SIGKILL or
> SIGCONT", which it seems intuitive to think is right for this case too.
> You don't have to think about exactly what is always exactly right for this
> case, because it makes sense to think that this is like the wakeup that
> SIGCONT would do.

No, SIGCONT uses wake_up_state(), not signal_wake_up(). Because unless
we have a handler for SIGCONT, we don't need to set TIF_SIGPENDING.
And I think this is right, and this is also right for ptrace_untrace().

So, in fact SIGCONT votes for this patch ;)

As for SIGKILL. Of course we should set TIF_SIGPENDING when we send
the signal, SIGKILL or any other. Yes, complete_signal() checks
sig == SIGKILL to figure out the correct mask. But ptrace_untrace()
already knows it.

> To me, it takes much more thought to be convinced that
> wake_up_process() without other considerations is correct here--because it
> looks like such a scary, unconditional thing, whereas normally that is
> wrapped up inside calls that handle appropriate bookkeeping--signal_wake_up()
> being one of those.

We have the rule: if we see the task in TASK_TRACED/STOPPED state
under ->siglock, it can do nothing except sleep or spin for ->siglock.
IOW, it can't "escape" from TRACED/STOPPED state even if it is already
TASK_RUNNING. do_wait() depends on this.

To me, signal_wake_up() means: we do have a reason for TIF_SIGPENDING,
otherwise the task can wrongly return to userspace without noticing
a signal (or pseudo signal). (and we call it with resume == 1, because
we can't pass the mask which is "exactly correct").


I strongly believe the code should be simplified as much as possible,
to simplify the understanding of the details. I spent more than 2 days
trying to understand whats going on with the bug-report which I wasn't
able to reproduce (it was actually the problem in glibc). It looked as
if the ptraced task can miss SIGKILL if it races with detach. When
I read this code I was really puzzled by this signal_wake_up().
I thought that perhaps it adresses some signal related problem which
I am not aware of, and perhaps the lost SIGKILL actually "belongs"
to this problem. It took me a lot of time to convince myself this
signal_wake_up() is just unneeded, and I should dig somewhere else.


That said. I agree this is harmless, and the matter of taste.
I don't think I can convince you, let's forget this patch.

Oleg.