Re: [PATCH 3/3] signal, ptrace: Fix delayed CONTINUED notificationwhen ptraced
From: Tejun Heo
Date: Thu Mar 31 2011 - 12:34:52 EST
Hello,
On Thu, Mar 31, 2011 at 05:15:49PM +0200, Oleg Nesterov wrote:
> No, I think prepare_signal(SIGCONT) should do this, but only if the
> thread is ptraced.
>
> My main objection was, signal_wake_up() shouldn't play with
> sig_user_defined/blocked at all. And, we shouldn't blindly set
> TIF_SIGPENDING as the current code does.
Hmmm... okay.
> prepare_signal(SIGCONT) should never set TIF_SIGPENDING or wake up
> the TASK_INTERRUPTIBLE threads. We are going to call complete_signal()
> which should pick the right thread correctly. All we need is to wake
> up the TASK_STOPPED threads.
>
> If the task was stopped, it can't return to usermode without taking
> ->siglock. Otherwise we don't care, and the spurious TIF_SIGPENDING
> can't be useful.
>
> The comment says:
>
> * If there is a handler for SIGCONT, we must make
> * sure that no thread returns to user mode before
> * we post the signal
I interpreted it as "when there's only single thread, it should not
return to userland before executing the signal handler".
> It is not clear what this means. But in any case, even if this SIGCONT
> is not private, only one thread can dequeue SIGCONT, other threads can
> happily return to user mode before before that thread handles this signal.
>
> Note also that wake_up_state(t, __TASK_STOPPED) can't race with the
> task which changes its state, TASK_STOPPED state is protected by
> ->siglock as well.
>
> --- x/kernel/signal.c
> +++ x/kernel/signal.c
> @@ -726,34 +726,13 @@ static int prepare_signal(int sig, struc
> } else if (sig == SIGCONT) {
> unsigned int why;
> /*
> - * Remove all stop signals from all queues,
> - * and wake all threads.
> + * Remove all stop signals from all queues, wake all threads.
> */
> rm_from_queue(SIG_KERNEL_STOP_MASK, &signal->shared_pending);
> t = p;
> do {
> - unsigned int state;
> rm_from_queue(SIG_KERNEL_STOP_MASK, &t->pending);
> - /*
> - * If there is a handler for SIGCONT, we must make
> - * sure that no thread returns to user mode before
> - * we post the signal, in case it was the only
> - * thread eligible to run the signal handler--then
> - * it must not do anything between resuming and
> - * running the handler. With the TIF_SIGPENDING
> - * flag set, the thread will pause and acquire the
> - * siglock that we hold now and until we've queued
> - * the pending signal.
> - *
> - * Wake up the stopped thread _after_ setting
> - * TIF_SIGPENDING
> - */
> - state = __TASK_STOPPED;
> - if (sig_user_defined(t, SIGCONT) && !sigismember(&t->blocked, SIGCONT)) {
> - set_tsk_thread_flag(t, TIF_SIGPENDING);
> - state |= TASK_INTERRUPTIBLE;
> - }
> - wake_up_state(t, state);
> + wake_up_state(t, __TASK_STOPPED);
> } while_each_thread(p, t);
This is awesome and, at the first glance, yeah, this seems to be the
right thing to do. That part is pure signal delivery after all.
* As wants_signal() doesn't take uninterruptible sleeps into
consideration, the signal might get delivered later with the change
but I don't think it's problematic in any way.
* Interruptible sleeps won't be disturbed on SIGCONT generation, which
is a visible behavior change, but, I agree, this is more of a bug
fix.
I'll mull over it a bit more but please go ahead and create a proper
patch. I'll apply it to the ptrace branch with the previous two
patches. (Can I add your Acked-by's there?)
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/