Re: [PATCH 3/3] signal, ptrace: Fix delayed CONTINUED notification when ptraced

[Tejun Heo <tj@kernel.org>]

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