Re: [PATCH] signal: cleanup the .si_signo checks in dequeue_synchronous_signal()

[Oleg Nesterov <oleg@redhat.com>]

Christian,

This is on top of Avi's
[PATCH v2] signal: respect the blocked mask in dequeue_synchronous_signal()

If you are going to update your tree and take V2 + my cleanup, I will not
mind if you fold my patch into the fix from Avi.

And! I just noticed the sashiko's review of Avi's patch:
https://sashiko.dev/#/patchset/20261022061015.3737924-1-thnkslprpt%40gmail.com

It noticed the problem with UB too. But, unlike me, it noticed another problem
with the "still_pending" logic. The described scenario is not possible after my
change, but we have the same problem with posix timers.

This needs another change. I'll try to think a bit to avoid a "brute force" fix.

Oleg.

On 10/23, Oleg Nesterov wrote:
>
> - Use SI_FROMKERNEL() instead of open-coded check
> 
> - Use rt_sigmask() instead of sigmask() to avoid UB on 32-bit if
>   .si_signo > 32
> 
> - Pre-calculate the unblocked synchronous signal "mask" to not check
>   .si_signo twice.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/signal.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7386b9552dc3..64259219bbdc 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -676,21 +676,17 @@ static int dequeue_synchronous_signal(kernel_siginfo_t *info)
>  	struct task_struct *tsk = current;
>  	struct sigpending *pending = &tsk->pending;
>  	struct sigqueue *q, *sync = NULL;
> +	unsigned long mask;
>  
> -	/*
> -	 * Might a synchronous signal be in the queue?
> -	 */
> -	if (!((pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK))
> +	/* Might a synchronous signal be in the queue? */
> +	mask = (pending->signal.sig[0] & ~tsk->blocked.sig[0]) & SYNCHRONOUS_MASK;
> +	if (!mask)
>  		return 0;
>  
> -	/*
> -	 * Return the first synchronous signal in the queue.
> -	 */
> +	/* Return the first 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) &&
> -		    !sigismember(&tsk->blocked, q->info.si_signo)) {
> +		if (SI_FROMKERNEL(&q->info) &&
> +		    (rt_sigmask(q->info.si_signo) & mask)) {
>  			sync = q;
>  			goto next;
>  		}
> -- 
> 2.55.0
>