[patch V2 25/50] signal: Confine POSIX_TIMERS properly

Thomas Gleixner posted 50 patches 1 year, 8 months ago
There is a newer version of this series
[patch V2 25/50] signal: Confine POSIX_TIMERS properly
Posted by Thomas Gleixner 1 year, 8 months ago
Move the itimer rearming out of the signal code and consolidate all posix
timer related functions in the signal code under one ifdef.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/posix-timers.h |    5 +
 kernel/signal.c              |  125 +++++++++++++++----------------------------
 kernel/time/itimer.c         |   22 +++++++
 kernel/time/posix-timers.c   |   15 ++++-
 4 files changed, 82 insertions(+), 85 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -100,6 +100,8 @@ static inline void posix_cputimers_rt_wa
 {
 	pct->bases[CPUCLOCK_SCHED].nextevt = runtime;
 }
+void posixtimer_rearm_itimer(struct task_struct *p);
+void posixtimer_rearm(struct kernel_siginfo *info);
 
 /* Init task static initializer */
 #define INIT_CPU_TIMERBASE(b) {						\
@@ -122,6 +124,8 @@ struct cpu_timer { };
 static inline void posix_cputimers_init(struct posix_cputimers *pct) { }
 static inline void posix_cputimers_group_init(struct posix_cputimers *pct,
 					      u64 cpu_limit) { }
+static inline void posixtimer_rearm_itimer(struct task_struct *p) { }
+static inline void posixtimer_rearm(struct kernel_siginfo *info) { }
 #endif
 
 #ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
@@ -196,5 +200,4 @@ void set_process_cpu_timer(struct task_s
 
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
-void posixtimer_rearm(struct kernel_siginfo *info);
 #endif
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -478,42 +478,6 @@ void flush_signals(struct task_struct *t
 }
 EXPORT_SYMBOL(flush_signals);
 
-#ifdef CONFIG_POSIX_TIMERS
-static void __flush_itimer_signals(struct sigpending *pending)
-{
-	sigset_t signal, retain;
-	struct sigqueue *q, *n;
-
-	signal = pending->signal;
-	sigemptyset(&retain);
-
-	list_for_each_entry_safe(q, n, &pending->list, list) {
-		int sig = q->info.si_signo;
-
-		if (likely(q->info.si_code != SI_TIMER)) {
-			sigaddset(&retain, sig);
-		} else {
-			sigdelset(&signal, sig);
-			list_del_init(&q->list);
-			__sigqueue_free(q);
-		}
-	}
-
-	sigorsets(&pending->signal, &signal, &retain);
-}
-
-void flush_itimer_signals(void)
-{
-	struct task_struct *tsk = current;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tsk->sighand->siglock, flags);
-	__flush_itimer_signals(&tsk->pending);
-	__flush_itimer_signals(&tsk->signal->shared_pending);
-	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
-}
-#endif
-
 void ignore_signals(struct task_struct *t)
 {
 	int i;
@@ -636,31 +600,9 @@ int dequeue_signal(sigset_t *mask, kerne
 		*type = PIDTYPE_TGID;
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
 					 mask, info, &resched_timer);
-#ifdef CONFIG_POSIX_TIMERS
-		/*
-		 * itimer signal ?
-		 *
-		 * itimers are process shared and we restart periodic
-		 * itimers in the signal delivery path to prevent DoS
-		 * attacks in the high resolution timer case. This is
-		 * compliant with the old way of self-restarting
-		 * itimers, as the SIGALRM is a legacy signal and only
-		 * queued once. Changing the restart behaviour to
-		 * restart the timer in the signal dequeue path is
-		 * reducing the timer noise on heavy loaded !highres
-		 * systems too.
-		 */
-		if (unlikely(signr == SIGALRM)) {
-			struct hrtimer *tmr = &tsk->signal->real_timer;
 
-			if (!hrtimer_is_queued(tmr) &&
-			    tsk->signal->it_real_incr != 0) {
-				hrtimer_forward(tmr, tmr->base->get_time(),
-						tsk->signal->it_real_incr);
-				hrtimer_restart(tmr);
-			}
-		}
-#endif
+		if (unlikely(signr == SIGALRM))
+			posixtimer_rearm_itimer(tsk);
 	}
 
 	recalc_sigpending();
@@ -682,22 +624,12 @@ int dequeue_signal(sigset_t *mask, kerne
 		 */
 		current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	}
-#ifdef CONFIG_POSIX_TIMERS
-	if (resched_timer) {
-		/*
-		 * Release the siglock to ensure proper locking order
-		 * of timer locks outside of siglocks.  Note, we leave
-		 * irqs disabled here, since the posix-timers code is
-		 * about to disable them again anyway.
-		 */
-		spin_unlock(&tsk->sighand->siglock);
-		posixtimer_rearm(info);
-		spin_lock(&tsk->sighand->siglock);
 
-		/* Don't expose the si_sys_private value to userspace */
-		info->si_sys_private = 0;
+	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
+		if (unlikely(resched_timer))
+			posixtimer_rearm(info);
 	}
-#endif
+
 	return signr;
 }
 EXPORT_SYMBOL_GPL(dequeue_signal);
@@ -1924,15 +1856,45 @@ int kill_pid(struct pid *pid, int sig, i
 }
 EXPORT_SYMBOL(kill_pid);
 
+#ifdef CONFIG_POSIX_TIMERS
 /*
- * These functions support sending signals using preallocated sigqueue
- * structures.  This is needed "because realtime applications cannot
- * afford to lose notifications of asynchronous events, like timer
- * expirations or I/O completions".  In the case of POSIX Timers
- * we allocate the sigqueue structure from the timer_create.  If this
- * allocation fails we are able to report the failure to the application
- * with an EAGAIN error.
+ * These functions handle POSIX timer signals. POSIX timers use
+ * preallocated sigqueue structs for sending signals.
  */
+static void __flush_itimer_signals(struct sigpending *pending)
+{
+	sigset_t signal, retain;
+	struct sigqueue *q, *n;
+
+	signal = pending->signal;
+	sigemptyset(&retain);
+
+	list_for_each_entry_safe(q, n, &pending->list, list) {
+		int sig = q->info.si_signo;
+
+		if (likely(q->info.si_code != SI_TIMER)) {
+			sigaddset(&retain, sig);
+		} else {
+			sigdelset(&signal, sig);
+			list_del_init(&q->list);
+			__sigqueue_free(q);
+		}
+	}
+
+	sigorsets(&pending->signal, &signal, &retain);
+}
+
+void flush_itimer_signals(void)
+{
+	struct task_struct *tsk = current;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
+	__flush_itimer_signals(&tsk->pending);
+	__flush_itimer_signals(&tsk->signal->shared_pending);
+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+}
+
 struct sigqueue *sigqueue_alloc(void)
 {
 	return __sigqueue_alloc(-1, current, GFP_KERNEL, 0, SIGQUEUE_PREALLOC);
@@ -2029,6 +1991,7 @@ int send_sigqueue(struct sigqueue *q, st
 	rcu_read_unlock();
 	return ret;
 }
+#endif /* CONFIG_POSIX_TIMERS */
 
 void do_notify_pidfd(struct task_struct *task)
 {
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -151,7 +151,27 @@ COMPAT_SYSCALL_DEFINE2(getitimer, int, w
 #endif
 
 /*
- * The timer is automagically restarted, when interval != 0
+ * Invoked from dequeue_signal() when SIG_ALRM is delivered.
+ *
+ * Restart the ITIMER_REAL timer if it is armed as periodic timer.  Doing
+ * this in the signal delivery path instead of self rearming prevents a DoS
+ * with small increments in the high reolution timer case and reduces timer
+ * noise in general.
+ */
+void posixtimer_rearm_itimer(struct task_struct *tsk)
+{
+	struct hrtimer *tmr = &tsk->signal->real_timer;
+
+	if (!hrtimer_is_queued(tmr) && tsk->signal->it_real_incr != 0) {
+		hrtimer_forward(tmr, tmr->base->get_time(),
+				tsk->signal->it_real_incr);
+		hrtimer_restart(tmr);
+	}
+}
+
+/*
+ * Interval timers are restarted in the signal delivery path.  See
+ * posixtimer_rearm_itimer().
  */
 enum hrtimer_restart it_real_fn(struct hrtimer *timer)
 {
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -251,7 +251,7 @@ static void common_hrtimer_rearm(struct
 
 /*
  * This function is called from the signal delivery code if
- * info->si_sys_private is not zero, which indicates that the timer has to
+ * info::si_sys_private is not zero, which indicates that the timer has to
  * be rearmed. Restart the timer and update info::si_overrun.
  */
 void posixtimer_rearm(struct kernel_siginfo *info)
@@ -259,9 +259,15 @@ void posixtimer_rearm(struct kernel_sigi
 	struct k_itimer *timr;
 	unsigned long flags;
 
+	/*
+	 * Release siglock to ensure proper locking order versus
+	 * timr::it_lock. Keep interrupts disabled.
+	 */
+	spin_unlock(&current->sighand->siglock);
+
 	timr = lock_timer(info->si_tid, &flags);
 	if (!timr)
-		return;
+		goto out;
 
 	if (timr->it_interval && timr->it_requeue_pending == info->si_sys_private) {
 		timr->kclock->timer_rearm(timr);
@@ -275,6 +281,11 @@ void posixtimer_rearm(struct kernel_sigi
 	}
 
 	unlock_timer(timr, flags);
+out:
+	spin_lock(&current->sighand->siglock);
+
+	/* Don't expose the si_sys_private value to userspace */
+	info->si_sys_private = 0;
 }
 
 int posix_timer_queue_signal(struct k_itimer *timr)
Re: [patch V2 25/50] signal: Confine POSIX_TIMERS properly
Posted by Oleg Nesterov 1 year, 8 months ago
On 04/11, Thomas Gleixner wrote:
>
> Move the itimer rearming out of the signal code and consolidate all posix
> timer related functions in the signal code under one ifdef.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/posix-timers.h |    5 +
>  kernel/signal.c              |  125 +++++++++++++++----------------------------
>  kernel/time/itimer.c         |   22 +++++++
>  kernel/time/posix-timers.c   |   15 ++++-
>  4 files changed, 82 insertions(+), 85 deletions(-)

Reviewed-by: Oleg Nesterov <oleg@redhat.com>


A minor nit below...

> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
...
> +static inline void posixtimer_rearm_itimer(struct task_struct *p) { }
> +static inline void posixtimer_rearm(struct kernel_siginfo *info) { }

Do we really need these 2 nops ? please see below.

...

> +		if (unlikely(signr == SIGALRM))
> +			posixtimer_rearm_itimer(tsk);

...

> +	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
> +		if (unlikely(resched_timer))
> +			posixtimer_rearm(info);
>  	}

This looks a bit inconsistent to me.

Can't we change the callsite of posixtimer_rearm_itimer() to check
IS_ENABLED(CONFIG_POSIX_TIMERS) too,

		if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
			if (unlikely(signr == SIGALRM))
				posixtimer_rearm_itimer(tsk);
		}
?

This will make the code more symmetrical, and we can avoid the dumb
definitions of posixtimer_rearm_itimer/posixtimer_rearm.

Oleg.
Re: [patch V2 25/50] signal: Confine POSIX_TIMERS properly
Posted by Thomas Gleixner 1 year, 8 months ago
On Thu, Apr 18 2024 at 17:23, Oleg Nesterov wrote:
> On 04/11, Thomas Gleixner wrote:
>> +static inline void posixtimer_rearm_itimer(struct task_struct *p) { }
>> +static inline void posixtimer_rearm(struct kernel_siginfo *info) { }
>
> Do we really need these 2 nops ? please see below.

>> +		if (unlikely(signr == SIGALRM))
>> +			posixtimer_rearm_itimer(tsk);
>
> ...
>
>> +	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
>> +		if (unlikely(resched_timer))
>> +			posixtimer_rearm(info);
>>  	}
>
> This looks a bit inconsistent to me.
>
> Can't we change the callsite of posixtimer_rearm_itimer() to check
> IS_ENABLED(CONFIG_POSIX_TIMERS) too,
>
> 		if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
> 			if (unlikely(signr == SIGALRM))
> 				posixtimer_rearm_itimer(tsk);
> 		}
> ?
>
> This will make the code more symmetrical, and we can avoid the dumb
> definitions of posixtimer_rearm_itimer/posixtimer_rearm.

Yes, we just need to expose the actual function prototypes
unconditionally. Let me fix that.

Thanks,

        tglx
Re: [patch V2 25/50] signal: Confine POSIX_TIMERS properly
Posted by Anna-Maria Behnsen 1 year, 8 months ago
Thomas Gleixner <tglx@linutronix.de> writes:

> Move the itimer rearming out of the signal code and consolidate all posix
> timer related functions in the signal code under one ifdef.

It would be easier to read, when it is splitted. But I made it :)

With the typo fix below feel free to add:

Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>


> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  include/linux/posix-timers.h |    5 +
>  kernel/signal.c              |  125 +++++++++++++++----------------------------
>  kernel/time/itimer.c         |   22 +++++++
>  kernel/time/posix-timers.c   |   15 ++++-
>  4 files changed, 82 insertions(+), 85 deletions(-)
>
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h

[...]

> @@ -151,7 +151,27 @@ COMPAT_SYSCALL_DEFINE2(getitimer, int, w
>  #endif
>  
>  /*
> - * The timer is automagically restarted, when interval != 0
> + * Invoked from dequeue_signal() when SIG_ALRM is delivered.

s/SIG_ALRM/SIGALRM


> + *
> + * Restart the ITIMER_REAL timer if it is armed as periodic timer.  Doing
> + * this in the signal delivery path instead of self rearming prevents a DoS
> + * with small increments in the high reolution timer case and reduces timer
> + * noise in general.
> + */
> +void posixtimer_rearm_itimer(struct task_struct *tsk)
> +{
> +	struct hrtimer *tmr = &tsk->signal->real_timer;
> +
> +	if (!hrtimer_is_queued(tmr) && tsk->signal->it_real_incr != 0) {
> +		hrtimer_forward(tmr, tmr->base->get_time(),
> +				tsk->signal->it_real_incr);
> +		hrtimer_restart(tmr);
> +	}
> +}
> +
> +/*
> + * Interval timers are restarted in the signal delivery path.  See
> + * posixtimer_rearm_itimer().
>   */
>  enum hrtimer_restart it_real_fn(struct hrtimer *timer)
>  {