Re: [PATCH] signal: skip blocked signals in dequeue_synchronous_signal()

["Eric W. Biederman" <ebiederm@xmission.com>]

Avi Weiss <thnkslprpt@gmail.com> writes:

> dequeue_synchronous_signal() checks whether an unblocked
> synchronous signal is pending, but its queue scan can select a
> different blocked entry with a positive si_code. This can deliver
> a signal while it is still blocked.
>
> Check the blocked mask for each queued entry. Forced faults
> are unblocked when necessary by force_sig_info_to_task(), and the
> existing pending bookkeeping is unchanged.

I am going to quibble a little with your description.

What you describe above as a forced fault, is what the code calls a
synchronous signal.  As such synchronous signals by definition can not
be blocked.  Unfortunately the code does not currently have a way to
distinguish between such a signal number used synchronously, and such a
signal number used normally.

Given the poor state of delivering these things I think the code change
is technically correct.

The comments change is wrong as there is no such thing as a blocked
synchronous signal.

Eric

> Fixes: 7146db3317c6 ("signal: Better detection of synchronous signals")
> Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
> ---
>
> Reproduced by blocking SIGSEGV, queuing a positive-code SIGSEGV
> with rt_tgsigqueueinfo(), then triggering an unblocked SIGTRAP.
> Without the fix, SIGSEGV was delivered while still blocked.
>
> Patched version behaved as expected.
>
>  kernel/signal.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index e433de93b430..4dcc47a7d30d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -684,12 +684,13 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
>  		return 0;
>  
>  	/*
> -	 * Return the first synchronous signal in the queue.
> +	 * Return the first unblocked synchronous signal in the queue.
>  	 */
>  	list_for_each_entry(q, &pending->list, list) {
>  		/* Synchronous signals have a positive si_code */
>  		if ((q->info.si_code > SI_USER) &&
> -		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK)) {
> +		    (sigmask(q->info.si_signo) & SYNCHRONOUS_MASK) &&
> +		    !sigismember(&tsk->blocked, q->info.si_signo)) {
>  			sync = q;
>  			goto next;
>  		}
>
> base-commit: f47d223627f80cb7a1eb14c585fe6cb35de5e984