From: Thomas Gleixner <tglx@linutronix.de>

There is no reason for handing the *resched pointer argument through
several functions just to check whether the signal is related to a self
rearming posix timer.

SI_TIMER is only used by the posix timer code and cannot be queued from
user space. The only extra check in collect_signal() to verify whether the
queued signal is preallocated is not really useful. Some other places
already check purely the SI_TIMER type.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

---
 kernel/signal.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)
---
diff --git a/kernel/signal.c b/kernel/signal.c
index 7706cd304785..3d2e087283ab 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -526,8 +526,7 @@ bool unhandled_signal(struct task_struct *tsk, int sig)
 	return !tsk->ptrace;
 }
 
-static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info,
-			   bool *resched_timer)
+static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info)
 {
 	struct sigqueue *q, *first = NULL;
 
@@ -549,12 +548,6 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
 still_pending:
 		list_del_init(&first->list);
 		copy_siginfo(info, &first->info);
-
-		*resched_timer =
-			(first->flags & SIGQUEUE_PREALLOC) &&
-			(info->si_code == SI_TIMER) &&
-			(info->si_sys_private);
-
 		__sigqueue_free(first);
 	} else {
 		/*
@@ -571,13 +564,12 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
 	}
 }
 
-static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
-			kernel_siginfo_t *info, bool *resched_timer)
+static int __dequeue_signal(struct sigpending *pending, sigset_t *mask, kernel_siginfo_t *info)
 {
 	int sig = next_signal(pending, mask);
 
 	if (sig)
-		collect_signal(sig, pending, info, resched_timer);
+		collect_signal(sig, pending, info);
 	return sig;
 }
 
@@ -589,17 +581,15 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
 int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
 {
 	struct task_struct *tsk = current;
-	bool resched_timer = false;
 	int signr;
 
 	lockdep_assert_held(&tsk->sighand->siglock);
 
 	*type = PIDTYPE_PID;
-	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
+	signr = __dequeue_signal(&tsk->pending, mask, info);
 	if (!signr) {
 		*type = PIDTYPE_TGID;
-		signr = __dequeue_signal(&tsk->signal->shared_pending,
-					 mask, info, &resched_timer);
+		signr = __dequeue_signal(&tsk->signal->shared_pending, mask, info);
 
 		if (unlikely(signr == SIGALRM))
 			posixtimer_rearm_itimer(tsk);
@@ -626,7 +616,7 @@ int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
 	}
 
 	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
-		if (unlikely(resched_timer))
+		if (unlikely(info->si_code == SI_TIMER && info->si_sys_private))
 			posixtimer_rearm(info);
 	}
 
@@ -1011,6 +1001,9 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
 
 	lockdep_assert_held(&t->sighand->siglock);
 
+	if (WARN_ON_ONCE(!is_si_special(info) && info->si_code == SI_TIMER))
+		return 0;
+
 	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, force))
 		goto ret;

Thomas Gleixner <tglx@linutronix.de> writes:

> From: Thomas Gleixner <tglx@linutronix.de>
>
> There is no reason for handing the *resched pointer argument through
> several functions just to check whether the signal is related to a self
> rearming posix timer.
>
> SI_TIMER is only used by the posix timer code and cannot be queued from
> user space.

Huh???  We have rt_sigqueueinfo.  You just touched the code that
copies the queued signal from userspace.

> The only extra check in collect_signal() to verify whether the
> queued signal is preallocated is not really useful. Some other places
> already check purely the SI_TIMER type.

The check to see if the signal was preallocated prevents shenanigans
with setting si_sys_private.

That is today you can queue a signal with rt_sigqueueinfo and set
si_sys_private and it will make it to userspace.  I don't know how
much we care but that is the case.

Which means that WARN_ON you added in __send_signal_locked can
most definitely be triggered by userspace.

Eric


> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> ---
>  kernel/signal.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> ---
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 7706cd304785..3d2e087283ab 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -526,8 +526,7 @@ bool unhandled_signal(struct task_struct *tsk, int sig)
>  	return !tsk->ptrace;
>  }
>  
> -static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info,
> -			   bool *resched_timer)
> +static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info)
>  {
>  	struct sigqueue *q, *first = NULL;
>  
> @@ -549,12 +548,6 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
>  still_pending:
>  		list_del_init(&first->list);
>  		copy_siginfo(info, &first->info);
> -
> -		*resched_timer =
> -			(first->flags & SIGQUEUE_PREALLOC) &&
> -			(info->si_code == SI_TIMER) &&
> -			(info->si_sys_private);
> -
>  		__sigqueue_free(first);
>  	} else {
>  		/*
> @@ -571,13 +564,12 @@ static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *i
>  	}
>  }
>  
> -static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
> -			kernel_siginfo_t *info, bool *resched_timer)
> +static int __dequeue_signal(struct sigpending *pending, sigset_t *mask, kernel_siginfo_t *info)
>  {
>  	int sig = next_signal(pending, mask);
>  
>  	if (sig)
> -		collect_signal(sig, pending, info, resched_timer);
> +		collect_signal(sig, pending, info);
>  	return sig;
>  }
>  
> @@ -589,17 +581,15 @@ static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
>  int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
>  {
>  	struct task_struct *tsk = current;
> -	bool resched_timer = false;
>  	int signr;
>  
>  	lockdep_assert_held(&tsk->sighand->siglock);
>  
>  	*type = PIDTYPE_PID;
> -	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
> +	signr = __dequeue_signal(&tsk->pending, mask, info);
>  	if (!signr) {
>  		*type = PIDTYPE_TGID;
> -		signr = __dequeue_signal(&tsk->signal->shared_pending,
> -					 mask, info, &resched_timer);
> +		signr = __dequeue_signal(&tsk->signal->shared_pending, mask, info);
>  
>  		if (unlikely(signr == SIGALRM))
>  			posixtimer_rearm_itimer(tsk);
> @@ -626,7 +616,7 @@ int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
>  	}
>  
>  	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
> -		if (unlikely(resched_timer))
> +		if (unlikely(info->si_code == SI_TIMER && info->si_sys_private))
>  			posixtimer_rearm(info);
>  	}
>  
> @@ -1011,6 +1001,9 @@ static int __send_signal_locked(int sig, struct kernel_siginfo *info,
>  
>  	lockdep_assert_held(&t->sighand->siglock);
>  
> +	if (WARN_ON_ONCE(!is_si_special(info) && info->si_code == SI_TIMER))
> +		return 0;
> +
>  	result = TRACE_SIGNAL_IGNORED;
>  	if (!prepare_signal(sig, t, force))
>  		goto ret;

Le Fri, Sep 27, 2024 at 10:48:42AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> There is no reason for handing the *resched pointer argument through
> several functions just to check whether the signal is related to a self
> rearming posix timer.
> 
> SI_TIMER is only used by the posix timer code and cannot be queued from
> user space. The only extra check in collect_signal() to verify whether the
> queued signal is preallocated is not really useful. Some other places
> already check purely the SI_TIMER type.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Acked-by: Frederic Weisbecker <frederic@kernel.org>