[patch V5 00/26] posix-timers: Cure the SIG_IGN mess

[patch V5 00/26] posix-timers: Cure the SIG_IGN mess

[Thomas Gleixner]

This are the remaining bits to cure the SIG_IGN mess. Version 4 can be found
here:

   https://lore.kernel.org/lkml/20240927083900.989915582@linutronix.de

Last year I reread a 15 years old comment about the SIG_IGN problem:

 "FIXME: What we really want, is to stop this timer completely and restart
  it in case the SIG_IGN is removed. This is a non trivial change which
  involves sighand locking (sigh !), which we don't want to do late in the
  release cycle.  ...  A more complex fix which solves also another related
  inconsistency is already in the pipeline."

The embarrasing part was that I put that comment in back then. So I went
back and rumaged through old notes as I completely had forgotten why our
attempts to fix this back then failed.

It turned out that the comment is about right: sighand locking and life
time issues. So I sat down with the old notes and started to wrap my head
around this again.

The problem to solve:

Posix interval timers are not rearmed automatically by the kernel for
various reasons:

   1) To prevent DoS by extremly short intervals.
   2) To avoid timer overhead when a signal is pending and has not
      yet been delivered.

This is achieved by queueing the signal at timer expiry and rearming the
timer at signal delivery to user space. This puts the rearming basically
under scheduler control and the work happens in context of the task which
asked for the signal.

There is a problem with that vs. SIG_IGN. If a signal has SIG_IGN installed
as handler, the related signals are discarded. So in case of posix interval
timers this means that such a timer is never rearmed even when SIG_IGN is
replaced later with a real handler (including SIG_DFL).

To work around that the kernel self rearms those timers and throttles them
when the interval is smaller than a tick to prevent a DoS.

That just keeps timers ticking, which obviously has effects on power and
just creates work for nothing.

So ideally these timers should be stopped and rearmed when SIG_IGN is
replaced, which aligns with the regular handling of posix timers.

Sounds trivial, but isn't:

  1) Lock ordering.

     The timer lock cannot be taken with sighand lock held which is
     problematic vs. the atomicity of sigaction().

  2) Life time rules

     The timer and the sigqueue are separate entities which requires a
     lookup of the timer ID in the signal rearm code. This can be handled,
     but the separate life time rules are not necessarily robust.

  3) Finding the relevant timers

     Obviosly it is possible to walk the posix timer list under sighand
     lock and handle it from there. That can be expensive especially in the
     case that there are no affected timers as the walk would just end up
     doing nothing.

The following series is a new and this time actually working attempt to
solve this. It addresses it by:

  1) Embedding the preallocated sigqueue into struct k_itimer, which makes
     the life time rules way simpler and just needs a trivial reference
     count.

  2) Having a separate list in task::signal on which ignored timers are
     queued.

     This avoids walking a potentially large timer list for nothing on a
     SIG_IGN to handler transition.

  3) Requeueing the timers signal in the relevant signal queue so the timer
     is rearmed when the signal is actually delivered

     That turned out to be the least complicated way to address the sighand
     lock vs. timer lock ordering issue.

With that timers which have their signal ignored are not longer self
rearmed and the relevant workarounds including throttling for DoS
prevention are removed.

The series is also available from git:

    git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git posixt-v5

Changes vs. V4:

    - Remove the si_sys_private restrictions - Eric

    - Hand down the pointer to the preallocated sigqueue instead of relying
      on si_sys_priv* magic - Eric
   
Thanks,

	tglx
---
 drivers/power/supply/charger-manager.c |    3 
 fs/proc/base.c                         |    4 
 fs/timerfd.c                           |    4 
 include/linux/alarmtimer.h             |   10 
 include/linux/posix-timers.h           |   70 ++++
 include/linux/sched/signal.h           |    4 
 init/init_task.c                       |    5 
 kernel/fork.c                          |    1 
 kernel/signal.c                        |  465 +++++++++++++++++++--------------
 kernel/time/alarmtimer.c               |   87 ------
 kernel/time/itimer.c                   |   22 +
 kernel/time/posix-cpu-timers.c         |   38 +-
 kernel/time/posix-timers.c             |  222 +++++++--------
 kernel/time/posix-timers.h             |    8 
 net/netfilter/xt_IDLETIMER.c           |    4 
 15 files changed, 502 insertions(+), 445 deletions(-)

[patch V5 01/26] signal: Confine POSIX_TIMERS properly

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

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>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |   5 +-
 kernel/signal.c              | 125 +++++++++++++++-----------------------------
 kernel/time/itimer.c         |  22 +++++++-
 kernel/time/posix-timers.c   |  15 ++++-
 4 files changed, 81 insertions(+), 86 deletions(-)
---
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 453691710839..670bf03a56ef 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -100,6 +100,8 @@ static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
 {
 	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_struct *task, unsigned int clock_idx,
 
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
-void posixtimer_rearm(struct kernel_siginfo *info);
 #endif
diff --git a/kernel/signal.c b/kernel/signal.c
index 6f3a5aa39b09..a83ea99f9389 100644
--- 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, kernel_siginfo_t *info, enum pid_type *type)
 		*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, kernel_siginfo_t *info, enum pid_type *type)
 		 */
 		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);
@@ -1922,15 +1854,43 @@ int kill_pid(struct pid *pid, int sig, int priv)
 }
 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;
+
+	guard(spinlock_irqsave)(&tsk->sighand->siglock);
+	__flush_itimer_signals(&tsk->pending);
+	__flush_itimer_signals(&tsk->signal->shared_pending);
+}
+
 struct sigqueue *sigqueue_alloc(void)
 {
 	return __sigqueue_alloc(-1, current, GFP_KERNEL, 0, SIGQUEUE_PREALLOC);
@@ -2027,6 +1987,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	rcu_read_unlock();
 	return ret;
 }
+#endif /* CONFIG_POSIX_TIMERS */
 
 void do_notify_pidfd(struct task_struct *task)
 {
diff --git a/kernel/time/itimer.c b/kernel/time/itimer.c
index 00629e658ca1..876d389b2e21 100644
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -151,7 +151,27 @@ COMPAT_SYSCALL_DEFINE2(getitimer, int, which,
 #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)
 {
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 1cc830ef93a7..bcd5e56412e7 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -251,7 +251,7 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
 
 /*
  * 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_siginfo *info)
 	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_siginfo *info)
 	}
 
 	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 V5 01/26] signal: Confine POSIX_TIMERS properly

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:00AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> 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>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I assume nothing has changed on this one? If so I reiterate:

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

Re: [patch V5 01/26] signal: Confine POSIX_TIMERS properly

[Thomas Gleixner]

On Fri, Oct 18 2024 at 14:50, Frederic Weisbecker wrote:

> Le Tue, Oct 01, 2024 at 10:42:00AM +0200, Thomas Gleixner a écrit :
>> From: Thomas Gleixner <tglx@linutronix.de>
>> 
>> 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>
>> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> I assume nothing has changed on this one? If so I reiterate:
>
> Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Sorry, I somehow missed to pick up the tags.

[tip: timers/core] signal: Confine POSIX_TIMERS properly

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     68f99be287a59d50a9ad231d523f7e578f8bd28a
Gitweb:        https://git.kernel.org/tip/68f99be287a59d50a9ad231d523f7e578f8bd28a
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:00 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:18 +01:00

signal: Confine POSIX_TIMERS properly

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>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.314100569@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, 81 insertions(+), 86 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 4536917..670bf03 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -100,6 +100,8 @@ static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
 {
 	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_struct *task, unsigned int clock_idx,
 
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
-void posixtimer_rearm(struct kernel_siginfo *info);
 #endif
diff --git a/kernel/signal.c b/kernel/signal.c
index 4344860..b65cc18 100644
--- 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, kernel_siginfo_t *info, enum pid_type *type)
 		*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, kernel_siginfo_t *info, enum pid_type *type)
 		 */
 		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);
@@ -1922,15 +1854,43 @@ int kill_pid(struct pid *pid, int sig, int priv)
 }
 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;
+
+	guard(spinlock_irqsave)(&tsk->sighand->siglock);
+	__flush_itimer_signals(&tsk->pending);
+	__flush_itimer_signals(&tsk->signal->shared_pending);
+}
+
 struct sigqueue *sigqueue_alloc(void)
 {
 	return __sigqueue_alloc(-1, current, GFP_KERNEL, 0, SIGQUEUE_PREALLOC);
@@ -2027,6 +1987,7 @@ ret:
 	rcu_read_unlock();
 	return ret;
 }
+#endif /* CONFIG_POSIX_TIMERS */
 
 void do_notify_pidfd(struct task_struct *task)
 {
diff --git a/kernel/time/itimer.c b/kernel/time/itimer.c
index 00629e6..876d389 100644
--- a/kernel/time/itimer.c
+++ b/kernel/time/itimer.c
@@ -151,7 +151,27 @@ COMPAT_SYSCALL_DEFINE2(getitimer, int, which,
 #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)
 {
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index fc40dac..d461a32 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -251,7 +251,7 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
 
 /*
  * 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_siginfo *info)
 	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_siginfo *info)
 	}
 
 	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)

[patch V5 02/26] signal: Cleanup flush_sigqueue_mask()

[Thomas Gleixner]

Mop up the stale return value comment and add a lockdep check instead of
commenting on the locking requirement.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/signal.c |   27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -704,17 +704,14 @@ void signal_wake_up_state(struct task_st
 		kick_process(t);
 }
 
-/*
- * Remove signals in mask from the pending set and queue.
- * Returns 1 if any signals were found.
- *
- * All callers must be holding the siglock.
- */
-static void flush_sigqueue_mask(sigset_t *mask, struct sigpending *s)
+/* Remove signals in mask from the pending set and queue. */
+static void flush_sigqueue_mask(struct task_struct *p, sigset_t *mask, struct sigpending *s)
 {
 	struct sigqueue *q, *n;
 	sigset_t m;
 
+	lockdep_assert_held(&p->sighand->siglock);
+
 	sigandsets(&m, mask, &s->signal);
 	if (sigisemptyset(&m))
 		return;
@@ -848,18 +845,18 @@ static bool prepare_signal(int sig, stru
 		 * This is a stop signal.  Remove SIGCONT from all queues.
 		 */
 		siginitset(&flush, sigmask(SIGCONT));
-		flush_sigqueue_mask(&flush, &signal->shared_pending);
+		flush_sigqueue_mask(p, &flush, &signal->shared_pending);
 		for_each_thread(p, t)
-			flush_sigqueue_mask(&flush, &t->pending);
+			flush_sigqueue_mask(p, &flush, &t->pending);
 	} else if (sig == SIGCONT) {
 		unsigned int why;
 		/*
 		 * Remove all stop signals from all queues, wake all threads.
 		 */
 		siginitset(&flush, SIG_KERNEL_STOP_MASK);
-		flush_sigqueue_mask(&flush, &signal->shared_pending);
+		flush_sigqueue_mask(p, &flush, &signal->shared_pending);
 		for_each_thread(p, t) {
-			flush_sigqueue_mask(&flush, &t->pending);
+			flush_sigqueue_mask(p, &flush, &t->pending);
 			task_clear_jobctl_pending(t, JOBCTL_STOP_PENDING);
 			if (likely(!(t->ptrace & PT_SEIZED))) {
 				t->jobctl &= ~JOBCTL_STOPPED;
@@ -4114,8 +4111,8 @@ void kernel_sigaction(int sig, __sighand
 		sigemptyset(&mask);
 		sigaddset(&mask, sig);
 
-		flush_sigqueue_mask(&mask, &current->signal->shared_pending);
-		flush_sigqueue_mask(&mask, &current->pending);
+		flush_sigqueue_mask(current, &mask, &current->signal->shared_pending);
+		flush_sigqueue_mask(current, &mask, &current->pending);
 		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
@@ -4182,9 +4179,9 @@ int do_sigaction(int sig, struct k_sigac
 		if (sig_handler_ignored(sig_handler(p, sig), sig)) {
 			sigemptyset(&mask);
 			sigaddset(&mask, sig);
-			flush_sigqueue_mask(&mask, &p->signal->shared_pending);
+			flush_sigqueue_mask(p, &mask, &p->signal->shared_pending);
 			for_each_thread(p, t)
-				flush_sigqueue_mask(&mask, &t->pending);
+				flush_sigqueue_mask(p, &mask, &t->pending);
 		}
 	}
 

Re: [patch V5 02/26] signal: Cleanup flush_sigqueue_mask()

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:02AM +0200, Thomas Gleixner a écrit :
> Mop up the stale return value comment and add a lockdep check instead of
> commenting on the locking requirement.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

[tip: timers/core] signal: Cleanup flush_sigqueue_mask()

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     a76e1bbe879cf39952ec4b43ed653b0905635f24
Gitweb:        https://git.kernel.org/tip/a76e1bbe879cf39952ec4b43ed653b0905635f24
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:02 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:18 +01:00

signal: Cleanup flush_sigqueue_mask()

Mop up the stale return value comment and add a lockdep check instead of
commenting on the locking requirement.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Link: https://lore.kernel.org/all/20241001083835.374933959@linutronix.de

---
 kernel/signal.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index b65cc18..f420c43 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -704,17 +704,14 @@ void signal_wake_up_state(struct task_struct *t, unsigned int state)
 		kick_process(t);
 }
 
-/*
- * Remove signals in mask from the pending set and queue.
- * Returns 1 if any signals were found.
- *
- * All callers must be holding the siglock.
- */
-static void flush_sigqueue_mask(sigset_t *mask, struct sigpending *s)
+/* Remove signals in mask from the pending set and queue. */
+static void flush_sigqueue_mask(struct task_struct *p, sigset_t *mask, struct sigpending *s)
 {
 	struct sigqueue *q, *n;
 	sigset_t m;
 
+	lockdep_assert_held(&p->sighand->siglock);
+
 	sigandsets(&m, mask, &s->signal);
 	if (sigisemptyset(&m))
 		return;
@@ -848,18 +845,18 @@ static bool prepare_signal(int sig, struct task_struct *p, bool force)
 		 * This is a stop signal.  Remove SIGCONT from all queues.
 		 */
 		siginitset(&flush, sigmask(SIGCONT));
-		flush_sigqueue_mask(&flush, &signal->shared_pending);
+		flush_sigqueue_mask(p, &flush, &signal->shared_pending);
 		for_each_thread(p, t)
-			flush_sigqueue_mask(&flush, &t->pending);
+			flush_sigqueue_mask(p, &flush, &t->pending);
 	} else if (sig == SIGCONT) {
 		unsigned int why;
 		/*
 		 * Remove all stop signals from all queues, wake all threads.
 		 */
 		siginitset(&flush, SIG_KERNEL_STOP_MASK);
-		flush_sigqueue_mask(&flush, &signal->shared_pending);
+		flush_sigqueue_mask(p, &flush, &signal->shared_pending);
 		for_each_thread(p, t) {
-			flush_sigqueue_mask(&flush, &t->pending);
+			flush_sigqueue_mask(p, &flush, &t->pending);
 			task_clear_jobctl_pending(t, JOBCTL_STOP_PENDING);
 			if (likely(!(t->ptrace & PT_SEIZED))) {
 				t->jobctl &= ~JOBCTL_STOPPED;
@@ -4114,8 +4111,8 @@ void kernel_sigaction(int sig, __sighandler_t action)
 		sigemptyset(&mask);
 		sigaddset(&mask, sig);
 
-		flush_sigqueue_mask(&mask, &current->signal->shared_pending);
-		flush_sigqueue_mask(&mask, &current->pending);
+		flush_sigqueue_mask(current, &mask, &current->signal->shared_pending);
+		flush_sigqueue_mask(current, &mask, &current->pending);
 		recalc_sigpending();
 	}
 	spin_unlock_irq(&current->sighand->siglock);
@@ -4182,9 +4179,9 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
 		if (sig_handler_ignored(sig_handler(p, sig), sig)) {
 			sigemptyset(&mask);
 			sigaddset(&mask, sig);
-			flush_sigqueue_mask(&mask, &p->signal->shared_pending);
+			flush_sigqueue_mask(p, &mask, &p->signal->shared_pending);
 			for_each_thread(p, t)
-				flush_sigqueue_mask(&mask, &t->pending);
+				flush_sigqueue_mask(p, &mask, &t->pending);
 		}
 	}
 

[patch V5 03/26] posix-timers: Cure si_sys_private race

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

The si_sys_private member of the siginfo which is embedded in the
preallocated sigqueue is used by the posix timer code to decide whether a
timer must be reprogrammed on signal delivery.

The handling of this is racy as a long standing comment in that code
documents. It is modified with the timer lock held, but without sighand
lock being held. The actual signal delivery code checks for it under
sighand lock without holding the timer lock.

Hand the new value to send_sigqueue() as argument and store it with sighand
lock held. This is an intermediate change to address this issue.

The arguments to this function will be cleanup in subsequent changes.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
V5: Document in the change log that this is intermediate.
---
 include/linux/sched/signal.h |    2 +-
 kernel/signal.c              |   10 +++++++++-
 kernel/time/posix-timers.c   |   15 +--------------
 3 files changed, 11 insertions(+), 16 deletions(-)
---
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -340,7 +340,7 @@ extern int send_sig(int, struct task_str
 extern int zap_other_threads(struct task_struct *p);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
-extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type);
+extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type, int si_private);
 extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
 
 static inline void clear_notify_signal(void)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1919,7 +1919,7 @@ void sigqueue_free(struct sigqueue *q)
 		__sigqueue_free(q);
 }
 
-int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
+int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type, int si_private)
 {
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
@@ -1954,6 +1954,14 @@ int send_sigqueue(struct sigqueue *q, st
 	if (!likely(lock_task_sighand(t, &flags)))
 		goto ret;
 
+	/*
+	 * Update @q::info::si_sys_private for posix timer signals with
+	 * sighand locked to prevent a race against dequeue_signal() which
+	 * decides based on si_sys_private whether to invoke
+	 * posixtimer_rearm() or not.
+	 */
+	q->info.si_sys_private = si_private;
+
 	ret = 1; /* the signal is ignored */
 	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, false))
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -299,21 +299,8 @@ int posix_timer_queue_signal(struct k_it
 	if (timr->it_interval)
 		si_private = ++timr->it_requeue_pending;
 
-	/*
-	 * FIXME: if ->sigq is queued we can race with
-	 * dequeue_signal()->posixtimer_rearm().
-	 *
-	 * If dequeue_signal() sees the "right" value of
-	 * si_sys_private it calls posixtimer_rearm().
-	 * We re-queue ->sigq and drop ->it_lock().
-	 * posixtimer_rearm() locks the timer
-	 * and re-schedules it while ->sigq is pending.
-	 * Not really bad, but not that we want.
-	 */
-	timr->sigq->info.si_sys_private = si_private;
-
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
-	ret = send_sigqueue(timr->sigq, timr->it_pid, type);
+	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
 	/* If we failed to send the signal the timer stops. */
 	return ret > 0;
 }

Re: [patch V5 03/26] posix-timers: Cure si_sys_private race

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:03AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The si_sys_private member of the siginfo which is embedded in the
> preallocated sigqueue is used by the posix timer code to decide whether a
> timer must be reprogrammed on signal delivery.
> 
> The handling of this is racy as a long standing comment in that code
> documents. It is modified with the timer lock held, but without sighand
> lock being held. The actual signal delivery code checks for it under
> sighand lock without holding the timer lock.
> 
> Hand the new value to send_sigqueue() as argument and store it with sighand
> lock held. This is an intermediate change to address this issue.
> 
> The arguments to this function will be cleanup in subsequent changes.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[tip: timers/core] posix-timers: Cure si_sys_private race

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     4febce44cfebcb490b196d5d10ae9f403ca4c956
Gitweb:        https://git.kernel.org/tip/4febce44cfebcb490b196d5d10ae9f403ca4c956
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:03 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:18 +01:00

posix-timers: Cure si_sys_private race

The si_sys_private member of the siginfo which is embedded in the
preallocated sigqueue is used by the posix timer code to decide whether a
timer must be reprogrammed on signal delivery.

The handling of this is racy as a long standing comment in that code
documents. It is modified with the timer lock held, but without sighand
lock being held. The actual signal delivery code checks for it under
sighand lock without holding the timer lock.

Hand the new value to send_sigqueue() as argument and store it with sighand
lock held. This is an intermediate change to address this issue.

The arguments to this function will be cleanup in subsequent changes.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.434338954@linutronix.de

---
 include/linux/sched/signal.h |  2 +-
 kernel/signal.c              | 10 +++++++++-
 kernel/time/posix-timers.c   | 15 +--------------
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index c8ed09a..bd9f569 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -340,7 +340,7 @@ extern int send_sig(int, struct task_struct *, int);
 extern int zap_other_threads(struct task_struct *p);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
-extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type);
+extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type, int si_private);
 extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
 
 static inline void clear_notify_signal(void)
diff --git a/kernel/signal.c b/kernel/signal.c
index f420c43..1563c83 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1919,7 +1919,7 @@ void sigqueue_free(struct sigqueue *q)
 		__sigqueue_free(q);
 }
 
-int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
+int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type, int si_private)
 {
 	int sig = q->info.si_signo;
 	struct sigpending *pending;
@@ -1954,6 +1954,14 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	if (!likely(lock_task_sighand(t, &flags)))
 		goto ret;
 
+	/*
+	 * Update @q::info::si_sys_private for posix timer signals with
+	 * sighand locked to prevent a race against dequeue_signal() which
+	 * decides based on si_sys_private whether to invoke
+	 * posixtimer_rearm() or not.
+	 */
+	q->info.si_sys_private = si_private;
+
 	ret = 1; /* the signal is ignored */
 	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, false))
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index d461a32..05af074 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -299,21 +299,8 @@ int posix_timer_queue_signal(struct k_itimer *timr)
 	if (timr->it_interval)
 		si_private = ++timr->it_requeue_pending;
 
-	/*
-	 * FIXME: if ->sigq is queued we can race with
-	 * dequeue_signal()->posixtimer_rearm().
-	 *
-	 * If dequeue_signal() sees the "right" value of
-	 * si_sys_private it calls posixtimer_rearm().
-	 * We re-queue ->sigq and drop ->it_lock().
-	 * posixtimer_rearm() locks the timer
-	 * and re-schedules it while ->sigq is pending.
-	 * Not really bad, but not that we want.
-	 */
-	timr->sigq->info.si_sys_private = si_private;
-
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
-	ret = send_sigqueue(timr->sigq, timr->it_pid, type);
+	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
 	/* If we failed to send the signal the timer stops. */
 	return ret > 0;
 }

[patch V5 04/26] signal: Allow POSIX timer signals to be dropped

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

In case that a timer was reprogrammed or deleted an already pending signal
is obsolete. Right now such signals are kept around and eventually
delivered. While POSIX is blury about this:

 - "The effect of disarming or resetting a timer with pending expiration
    notifications is unspecified."

 - "The disposition of pending signals for the deleted timer is
    unspecified."

it is reasonable in both cases to expect that pending signals are discarded
as they have no meaning anymore.

Prepare the signal code to allow dropping posix timer signals.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |    5 +++--
 kernel/signal.c              |    7 ++++---
 kernel/time/posix-timers.c   |    3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -100,8 +100,9 @@ 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);
+bool posixtimer_deliver_signal(struct kernel_siginfo *info);
 
 /* Init task static initializer */
 #define INIT_CPU_TIMERBASE(b) {						\
@@ -125,7 +126,7 @@ static inline void posix_cputimers_init(
 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) { }
+static inline bool posixtimer_deliver_signal(struct kernel_siginfo *info) { return false; }
 #endif
 
 #ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -594,6 +594,7 @@ int dequeue_signal(sigset_t *mask, kerne
 
 	lockdep_assert_held(&tsk->sighand->siglock);
 
+again:
 	*type = PIDTYPE_PID;
 	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
 	if (!signr) {
@@ -625,9 +626,9 @@ int dequeue_signal(sigset_t *mask, kerne
 		current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	}
 
-	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
-		if (unlikely(resched_timer))
-			posixtimer_rearm(info);
+	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(resched_timer)) {
+		if (!posixtimer_deliver_signal(info))
+			goto again;
 	}
 
 	return signr;
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -254,7 +254,7 @@ static void common_hrtimer_rearm(struct
  * 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)
+bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 {
 	struct k_itimer *timr;
 	unsigned long flags;
@@ -286,6 +286,7 @@ void posixtimer_rearm(struct kernel_sigi
 
 	/* Don't expose the si_sys_private value to userspace */
 	info->si_sys_private = 0;
+	return true;
 }
 
 int posix_timer_queue_signal(struct k_itimer *timr)

Re: [patch V5 04/26] signal: Allow POSIX timer signals to be dropped

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:04AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> In case that a timer was reprogrammed or deleted an already pending signal
> is obsolete. Right now such signals are kept around and eventually
> delivered. While POSIX is blury about this:
> 
>  - "The effect of disarming or resetting a timer with pending expiration
>     notifications is unspecified."
> 
>  - "The disposition of pending signals for the deleted timer is
>     unspecified."
> 
> it is reasonable in both cases to expect that pending signals are discarded
> as they have no meaning anymore.
> 
> Prepare the signal code to allow dropping posix timer signals.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[tip: timers/core] signal: Allow POSIX timer signals to be dropped

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     c775ea28d4e23f5e58b6953645ef90c1b27a8e83
Gitweb:        https://git.kernel.org/tip/c775ea28d4e23f5e58b6953645ef90c1b27a8e83
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:04 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:19 +01:00

signal: Allow POSIX timer signals to be dropped

In case that a timer was reprogrammed or deleted an already pending signal
is obsolete. Right now such signals are kept around and eventually
delivered. While POSIX is blury about this:

 - "The effect of disarming or resetting a timer with pending expiration
    notifications is unspecified."

 - "The disposition of pending signals for the deleted timer is
    unspecified."

it is reasonable in both cases to expect that pending signals are discarded
as they have no meaning anymore.

Prepare the signal code to allow dropping posix timer signals.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.494416923@linutronix.de

---
 include/linux/posix-timers.h | 5 +++--
 kernel/signal.c              | 7 ++++---
 kernel/time/posix-timers.c   | 3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 670bf03..4ab49e5 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -100,8 +100,9 @@ static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
 {
 	pct->bases[CPUCLOCK_SCHED].nextevt = runtime;
 }
+
 void posixtimer_rearm_itimer(struct task_struct *p);
-void posixtimer_rearm(struct kernel_siginfo *info);
+bool posixtimer_deliver_signal(struct kernel_siginfo *info);
 
 /* Init task static initializer */
 #define INIT_CPU_TIMERBASE(b) {						\
@@ -125,7 +126,7 @@ 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) { }
+static inline bool posixtimer_deliver_signal(struct kernel_siginfo *info) { return false; }
 #endif
 
 #ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
diff --git a/kernel/signal.c b/kernel/signal.c
index 1563c83..df34aa4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -594,6 +594,7 @@ int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
 
 	lockdep_assert_held(&tsk->sighand->siglock);
 
+again:
 	*type = PIDTYPE_PID;
 	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
 	if (!signr) {
@@ -625,9 +626,9 @@ int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
 		current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	}
 
-	if (IS_ENABLED(CONFIG_POSIX_TIMERS)) {
-		if (unlikely(resched_timer))
-			posixtimer_rearm(info);
+	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(resched_timer)) {
+		if (!posixtimer_deliver_signal(info))
+			goto again;
 	}
 
 	return signr;
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 05af074..dd0b1df 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -254,7 +254,7 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
  * 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)
+bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 {
 	struct k_itimer *timr;
 	unsigned long flags;
@@ -286,6 +286,7 @@ out:
 
 	/* Don't expose the si_sys_private value to userspace */
 	info->si_sys_private = 0;
+	return true;
 }
 
 int posix_timer_queue_signal(struct k_itimer *timr)

[patch V5 05/26] posix-timers: Drop signal if timer has been deleted or reprogrammed

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

No point in delivering a signal from the past. POSIX does not specify the
behaviour here:

 - "The effect of disarming or resetting a timer with pending expiration
    notifications is unspecified."

 - "The disposition of pending signals for the deleted timer is unspecified."

In both cases it is reasonable to expect that pending signals are
discarded. Especially in the reprogramming case it does not make sense to
account for previous overruns or to deliver a signal for a timer which has
been disarmed.

Drop the signal as that is conistent and understandable behaviour.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/time/posix-timers.c |  9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
---
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index d7ed7542f803..b5d7e71c10f2 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -250,14 +250,14 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
 }
 
 /*
- * This function is called from the signal delivery code if
- * info::si_sys_private is not zero, which indicates that the timer has to
- * be rearmed. Restart the timer and update info::si_overrun.
+ * This function is called from the signal delivery code. It decides
+ * whether the signal should be dropped and rearms interval timers.
  */
 bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 {
 	struct k_itimer *timr;
 	unsigned long flags;
+	bool ret = false;
 
 	/*
 	 * Release siglock to ensure proper locking order versus
@@ -279,6 +279,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 
 		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
 	}
+	ret = true;
 
 	unlock_timer(timr, flags);
 out:
@@ -286,7 +287,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 
 	/* Don't expose the si_sys_private value to userspace */
 	info->si_sys_private = 0;
-	return true;
+	return ret;
 }
 
 int posix_timer_queue_signal(struct k_itimer *timr)

Re: [patch V5 05/26] posix-timers: Drop signal if timer has been deleted or reprogrammed

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:06AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> No point in delivering a signal from the past. POSIX does not specify the
> behaviour here:
> 
>  - "The effect of disarming or resetting a timer with pending expiration
>     notifications is unspecified."
> 
>  - "The disposition of pending signals for the deleted timer is unspecified."
> 
> In both cases it is reasonable to expect that pending signals are
> discarded. Especially in the reprogramming case it does not make sense to
> account for previous overruns or to deliver a signal for a timer which has
> been disarmed.

The change below indeed checks if the timer has been deleted but not if
it has been reprogrammed/disarmed/reset.

Or am I missing something?

Thanks.

> 
> Drop the signal as that is conistent and understandable behaviour.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/time/posix-timers.c |  9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> ---
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index d7ed7542f803..b5d7e71c10f2 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -250,14 +250,14 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
>  }
>  
>  /*
> - * This function is called from the signal delivery code if
> - * info::si_sys_private is not zero, which indicates that the timer has to
> - * be rearmed. Restart the timer and update info::si_overrun.
> + * This function is called from the signal delivery code. It decides
> + * whether the signal should be dropped and rearms interval timers.
>   */
>  bool posixtimer_deliver_signal(struct kernel_siginfo *info)
>  {
>  	struct k_itimer *timr;
>  	unsigned long flags;
> +	bool ret = false;
>  
>  	/*
>  	 * Release siglock to ensure proper locking order versus
> @@ -279,6 +279,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
>  
>  		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
>  	}
> +	ret = true;
>  
>  	unlock_timer(timr, flags);
>  out:
> @@ -286,7 +287,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
>  
>  	/* Don't expose the si_sys_private value to userspace */
>  	info->si_sys_private = 0;
> -	return true;
> +	return ret;
>  }
>  
>  int posix_timer_queue_signal(struct k_itimer *timr)
> 

Re: [patch V5 05/26] posix-timers: Drop signal if timer has been deleted or reprogrammed

[Thomas Gleixner]

On Mon, Oct 21 2024 at 14:29, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:06AM +0200, Thomas Gleixner a écrit :
>> No point in delivering a signal from the past. POSIX does not specify the
>> behaviour here:
>> 
>>  - "The effect of disarming or resetting a timer with pending expiration
>>     notifications is unspecified."
>> 
>>  - "The disposition of pending signals for the deleted timer is unspecified."
>> 
>> In both cases it is reasonable to expect that pending signals are
>> discarded. Especially in the reprogramming case it does not make sense to
>> account for previous overruns or to deliver a signal for a timer which has
>> been disarmed.
>
> The change below indeed checks if the timer has been deleted but not if
> it has been reprogrammed/disarmed/reset.
>
> Or am I missing something?

No. You are right. This is a change log left over from a previous
version. This rearm/disarm part is handled later in the series.

Thanks,

        tglx

Re: [patch V5 05/26] posix-timers: Drop signal if timer has been deleted or reprogrammed

[Frederic Weisbecker]

Le Thu, Oct 24, 2024 at 10:40:08AM +0200, Thomas Gleixner a écrit :
> On Mon, Oct 21 2024 at 14:29, Frederic Weisbecker wrote:
> > Le Tue, Oct 01, 2024 at 10:42:06AM +0200, Thomas Gleixner a écrit :
> >> No point in delivering a signal from the past. POSIX does not specify the
> >> behaviour here:
> >> 
> >>  - "The effect of disarming or resetting a timer with pending expiration
> >>     notifications is unspecified."
> >> 
> >>  - "The disposition of pending signals for the deleted timer is unspecified."
> >> 
> >> In both cases it is reasonable to expect that pending signals are
> >> discarded. Especially in the reprogramming case it does not make sense to
> >> account for previous overruns or to deliver a signal for a timer which has
> >> been disarmed.
> >
> > The change below indeed checks if the timer has been deleted but not if
> > it has been reprogrammed/disarmed/reset.
> >
> > Or am I missing something?
> 
> No. You are right. This is a change log left over from a previous
> version. This rearm/disarm part is handled later in the series.

Right, I realized that after. Anyway:

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


> 
> Thanks,
> 
>         tglx

[tip: timers/core] posix-timers: Drop signal if timer has been deleted or reprogrammed

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     2860d4d315dc01f001dfd328adaf2ab440c47dd3
Gitweb:        https://git.kernel.org/tip/2860d4d315dc01f001dfd328adaf2ab440c47dd3
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:06 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:19 +01:00

posix-timers: Drop signal if timer has been deleted or reprogrammed

No point in delivering a signal from the past. POSIX does not specify the
behaviour here:

 - "The effect of disarming or resetting a timer with pending expiration
    notifications is unspecified."

 - "The disposition of pending signals for the deleted timer is unspecified."

In both cases it is reasonable to expect that pending signals are
discarded. Especially in the reprogramming case it does not make sense to
account for previous overruns or to deliver a signal for a timer which has
been disarmed.

Drop the signal as that is conistent and understandable behaviour.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.553646280@linutronix.de

---
 kernel/time/posix-timers.c |  9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index dd0b1df..22e1d6b 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -250,14 +250,14 @@ static void common_hrtimer_rearm(struct k_itimer *timr)
 }
 
 /*
- * This function is called from the signal delivery code if
- * info::si_sys_private is not zero, which indicates that the timer has to
- * be rearmed. Restart the timer and update info::si_overrun.
+ * This function is called from the signal delivery code. It decides
+ * whether the signal should be dropped and rearms interval timers.
  */
 bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 {
 	struct k_itimer *timr;
 	unsigned long flags;
+	bool ret = false;
 
 	/*
 	 * Release siglock to ensure proper locking order versus
@@ -279,6 +279,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 
 		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
 	}
+	ret = true;
 
 	unlock_timer(timr, flags);
 out:
@@ -286,7 +287,7 @@ out:
 
 	/* Don't expose the si_sys_private value to userspace */
 	info->si_sys_private = 0;
-	return true;
+	return ret;
 }
 
 int posix_timer_queue_signal(struct k_itimer *timr)

[patch V5 06/26] posix-timers: Rename k_itimer::it_requeue_pending

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Prepare for using this struct member to do a proper reprogramming and
deletion accounting so that stale signals can be dropped.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h   |  5 ++---
 kernel/time/alarmtimer.c       |  2 +-
 kernel/time/posix-cpu-timers.c |  4 ++--
 kernel/time/posix-timers.c     | 12 ++++++------
 4 files changed, 11 insertions(+), 12 deletions(-)
---
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 4ab49e5c42af..253d106fac2c 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -150,8 +150,7 @@ static inline void posix_cputimers_init_work(void) { }
  * @it_active:		Marker that timer is active
  * @it_overrun:		The overrun counter for pending signals
  * @it_overrun_last:	The overrun at the time of the last delivered signal
- * @it_requeue_pending:	Indicator that timer waits for being requeued on
- *			signal delivery
+ * @it_signal_seq:	Sequence count to control signal delivery
  * @it_sigev_notify:	The notify word of sigevent struct for signal delivery
  * @it_interval:	The interval for periodic timers
  * @it_signal:		Pointer to the creators signal struct
@@ -172,7 +171,7 @@ struct k_itimer {
 	int			it_active;
 	s64			it_overrun;
 	s64			it_overrun_last;
-	int			it_requeue_pending;
+	unsigned int		it_signal_seq;
 	int			it_sigev_notify;
 	ktime_t			it_interval;
 	struct signal_struct	*it_signal;
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 76bd4fda3472..22d5145dd9a7 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -584,7 +584,7 @@ static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm,
 		 * small intervals cannot starve the system.
 		 */
 		ptr->it_overrun += __alarm_forward_now(alarm, ptr->it_interval, true);
-		++ptr->it_requeue_pending;
+		++ptr->it_signal_seq;
 		ptr->it_active = 1;
 		result = ALARMTIMER_RESTART;
 	}
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 6bcee4704059..993243b5be98 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -608,7 +608,7 @@ static void cpu_timer_fire(struct k_itimer *timer)
 		 * ticking in case the signal is deliverable next time.
 		 */
 		posix_cpu_timer_rearm(timer);
-		++timer->it_requeue_pending;
+		++timer->it_signal_seq;
 	}
 }
 
@@ -745,7 +745,7 @@ static void __posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *i
 	 *  - Timers which expired, but the signal has not yet been
 	 *    delivered
 	 */
-	if (iv && ((timer->it_requeue_pending & REQUEUE_PENDING) || sigev_none))
+	if (iv && ((timer->it_signal_seq & REQUEUE_PENDING) || sigev_none))
 		expires = bump_cpu_timer(timer, now);
 	else
 		expires = cpu_timer_getexpires(&timer->it.cpu);
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index b5d7e71c10f2..26243d38d27d 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -269,13 +269,13 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 	if (!timr)
 		goto out;
 
-	if (timr->it_interval && timr->it_requeue_pending == info->si_sys_private) {
+	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
 		timr->kclock->timer_rearm(timr);
 
 		timr->it_active = 1;
 		timr->it_overrun_last = timr->it_overrun;
 		timr->it_overrun = -1LL;
-		++timr->it_requeue_pending;
+		++timr->it_signal_seq;
 
 		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
 	}
@@ -299,7 +299,7 @@ int posix_timer_queue_signal(struct k_itimer *timr)
 
 	timr->it_active = 0;
 	if (timr->it_interval)
-		si_private = ++timr->it_requeue_pending;
+		si_private = ++timr->it_signal_seq;
 
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
 	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
@@ -366,7 +366,7 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 
 			timr->it_overrun += hrtimer_forward(timer, now, timr->it_interval);
 			ret = HRTIMER_RESTART;
-			++timr->it_requeue_pending;
+			++timr->it_signal_seq;
 			timr->it_active = 1;
 		}
 	}
@@ -667,7 +667,7 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	 * is a SIGEV_NONE timer move the expiry time forward by intervals,
 	 * so expiry is > now.
 	 */
-	if (iv && (timr->it_requeue_pending & REQUEUE_PENDING || sig_none))
+	if (iv && (timr->it_signal_seq & REQUEUE_PENDING || sig_none))
 		timr->it_overrun += kc->timer_forward(timr, now);
 
 	remaining = kc->timer_remaining(timr, now);
@@ -868,7 +868,7 @@ void posix_timer_set_common(struct k_itimer *timer, struct itimerspec64 *new_set
 		timer->it_interval = 0;
 
 	/* Prevent reloading in case there is a signal pending */
-	timer->it_requeue_pending = (timer->it_requeue_pending + 2) & ~REQUEUE_PENDING;
+	timer->it_signal_seq = (timer->it_signal_seq + 2) & ~REQUEUE_PENDING;
 	/* Reset overrun accounting */
 	timer->it_overrun_last = 0;
 	timer->it_overrun = -1LL;

Re: [patch V5 06/26] posix-timers: Rename k_itimer::it_requeue_pending

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:07AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Prepare for using this struct member to do a proper reprogramming and
> deletion accounting so that stale signals can be dropped.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[tip: timers/core] posix-timers: Rename k_itimer:: It_requeue_pending

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     cd1e93aedab7f749760a33e9e094381973b1120e
Gitweb:        https://git.kernel.org/tip/cd1e93aedab7f749760a33e9e094381973b1120e
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:07 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:19 +01:00

posix-timers: Rename k_itimer:: It_requeue_pending

Prepare for using this struct member to do a proper reprogramming and
deletion accounting so that stale signals can be dropped.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.611997737@linutronix.de

---
 include/linux/posix-timers.h   |  5 ++---
 kernel/time/alarmtimer.c       |  2 +-
 kernel/time/posix-cpu-timers.c |  4 ++--
 kernel/time/posix-timers.c     | 12 ++++++------
 4 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 4ab49e5..253d106 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -150,8 +150,7 @@ static inline void posix_cputimers_init_work(void) { }
  * @it_active:		Marker that timer is active
  * @it_overrun:		The overrun counter for pending signals
  * @it_overrun_last:	The overrun at the time of the last delivered signal
- * @it_requeue_pending:	Indicator that timer waits for being requeued on
- *			signal delivery
+ * @it_signal_seq:	Sequence count to control signal delivery
  * @it_sigev_notify:	The notify word of sigevent struct for signal delivery
  * @it_interval:	The interval for periodic timers
  * @it_signal:		Pointer to the creators signal struct
@@ -172,7 +171,7 @@ struct k_itimer {
 	int			it_active;
 	s64			it_overrun;
 	s64			it_overrun_last;
-	int			it_requeue_pending;
+	unsigned int		it_signal_seq;
 	int			it_sigev_notify;
 	ktime_t			it_interval;
 	struct signal_struct	*it_signal;
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 8bf8886..75f8443 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -584,7 +584,7 @@ static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm,
 		 * small intervals cannot starve the system.
 		 */
 		ptr->it_overrun += __alarm_forward_now(alarm, ptr->it_interval, true);
-		++ptr->it_requeue_pending;
+		++ptr->it_signal_seq;
 		ptr->it_active = 1;
 		result = ALARMTIMER_RESTART;
 	}
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 6bcee47..993243b 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -608,7 +608,7 @@ static void cpu_timer_fire(struct k_itimer *timer)
 		 * ticking in case the signal is deliverable next time.
 		 */
 		posix_cpu_timer_rearm(timer);
-		++timer->it_requeue_pending;
+		++timer->it_signal_seq;
 	}
 }
 
@@ -745,7 +745,7 @@ static void __posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *i
 	 *  - Timers which expired, but the signal has not yet been
 	 *    delivered
 	 */
-	if (iv && ((timer->it_requeue_pending & REQUEUE_PENDING) || sigev_none))
+	if (iv && ((timer->it_signal_seq & REQUEUE_PENDING) || sigev_none))
 		expires = bump_cpu_timer(timer, now);
 	else
 		expires = cpu_timer_getexpires(&timer->it.cpu);
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 22e1d6b..fd321fc 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -269,13 +269,13 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 	if (!timr)
 		goto out;
 
-	if (timr->it_interval && timr->it_requeue_pending == info->si_sys_private) {
+	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
 		timr->kclock->timer_rearm(timr);
 
 		timr->it_active = 1;
 		timr->it_overrun_last = timr->it_overrun;
 		timr->it_overrun = -1LL;
-		++timr->it_requeue_pending;
+		++timr->it_signal_seq;
 
 		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
 	}
@@ -299,7 +299,7 @@ int posix_timer_queue_signal(struct k_itimer *timr)
 
 	timr->it_active = 0;
 	if (timr->it_interval)
-		si_private = ++timr->it_requeue_pending;
+		si_private = ++timr->it_signal_seq;
 
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
 	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
@@ -366,7 +366,7 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 
 			timr->it_overrun += hrtimer_forward(timer, now, timr->it_interval);
 			ret = HRTIMER_RESTART;
-			++timr->it_requeue_pending;
+			++timr->it_signal_seq;
 			timr->it_active = 1;
 		}
 	}
@@ -660,7 +660,7 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	 * is a SIGEV_NONE timer move the expiry time forward by intervals,
 	 * so expiry is > now.
 	 */
-	if (iv && (timr->it_requeue_pending & REQUEUE_PENDING || sig_none))
+	if (iv && (timr->it_signal_seq & REQUEUE_PENDING || sig_none))
 		timr->it_overrun += kc->timer_forward(timr, now);
 
 	remaining = kc->timer_remaining(timr, now);
@@ -861,7 +861,7 @@ void posix_timer_set_common(struct k_itimer *timer, struct itimerspec64 *new_set
 		timer->it_interval = 0;
 
 	/* Prevent reloading in case there is a signal pending */
-	timer->it_requeue_pending = (timer->it_requeue_pending + 2) & ~REQUEUE_PENDING;
+	timer->it_signal_seq = (timer->it_signal_seq + 2) & ~REQUEUE_PENDING;
 	/* Reset overrun accounting */
 	timer->it_overrun_last = 0;
 	timer->it_overrun = -1LL;

[patch V5 07/26] posix-timers: Add proper state tracking

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Right now the state tracking is done by two struct members:

 - it_active:
     A boolean which tracks armed/disarmed state

 - it_signal_seq:
     A sequence counter which is used to invalidate settings
     and prevent rearming

Replace it_active with it_status and keep properly track about the states
in one place.

This allows to reuse it_signal_seq to track reprogramming, disarm and
delete operations in order to drop signals which are related to the state
previous of those operations.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h   |  4 ++--
 kernel/time/alarmtimer.c       |  2 +-
 kernel/time/posix-cpu-timers.c | 15 ++++++++-------
 kernel/time/posix-timers.c     | 22 +++++++++++++---------
 kernel/time/posix-timers.h     |  6 ++++++
 5 files changed, 30 insertions(+), 19 deletions(-)
---
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 253d106fac2c..02afbb4da7f7 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -147,7 +147,7 @@ static inline void posix_cputimers_init_work(void) { }
  * @kclock:		Pointer to the k_clock struct handling this timer
  * @it_clock:		The posix timer clock id
  * @it_id:		The posix timer id for identifying the timer
- * @it_active:		Marker that timer is active
+ * @it_status:		The status of the timer
  * @it_overrun:		The overrun counter for pending signals
  * @it_overrun_last:	The overrun at the time of the last delivered signal
  * @it_signal_seq:	Sequence count to control signal delivery
@@ -168,7 +168,7 @@ struct k_itimer {
 	const struct k_clock	*kclock;
 	clockid_t		it_clock;
 	timer_t			it_id;
-	int			it_active;
+	int			it_status;
 	s64			it_overrun;
 	s64			it_overrun_last;
 	unsigned int		it_signal_seq;
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 22d5145dd9a7..71360f8072d4 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -585,7 +585,7 @@ static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm,
 		 */
 		ptr->it_overrun += __alarm_forward_now(alarm, ptr->it_interval, true);
 		++ptr->it_signal_seq;
-		ptr->it_active = 1;
+		ptr->it_status = POSIX_TIMER_ARMED;
 		result = ALARMTIMER_RESTART;
 	}
 	spin_unlock_irqrestore(&ptr->it_lock, flags);
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 993243b5be98..12f828d704b1 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -453,7 +453,6 @@ static void disarm_timer(struct k_itimer *timer, struct task_struct *p)
 	struct cpu_timer *ctmr = &timer->it.cpu;
 	struct posix_cputimer_base *base;
 
-	timer->it_active = 0;
 	if (!cpu_timer_dequeue(ctmr))
 		return;
 
@@ -494,11 +493,12 @@ static int posix_cpu_timer_del(struct k_itimer *timer)
 		 */
 		WARN_ON_ONCE(ctmr->head || timerqueue_node_queued(&ctmr->node));
 	} else {
-		if (timer->it.cpu.firing)
+		if (timer->it.cpu.firing) {
 			ret = TIMER_RETRY;
-		else
+		} else {
 			disarm_timer(timer, p);
-
+			timer->it_status = POSIX_TIMER_DISARMED;
+		}
 		unlock_task_sighand(p, &flags);
 	}
 
@@ -560,7 +560,7 @@ static void arm_timer(struct k_itimer *timer, struct task_struct *p)
 	struct cpu_timer *ctmr = &timer->it.cpu;
 	u64 newexp = cpu_timer_getexpires(ctmr);
 
-	timer->it_active = 1;
+	timer->it_status = POSIX_TIMER_ARMED;
 	if (!cpu_timer_enqueue(&base->tqhead, ctmr))
 		return;
 
@@ -586,7 +586,8 @@ static void cpu_timer_fire(struct k_itimer *timer)
 {
 	struct cpu_timer *ctmr = &timer->it.cpu;
 
-	timer->it_active = 0;
+	timer->it_status = POSIX_TIMER_DISARMED;
+
 	if (unlikely(timer->sigq == NULL)) {
 		/*
 		 * This a special case for clock_nanosleep,
@@ -671,7 +672,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,
 		ret = TIMER_RETRY;
 	} else {
 		cpu_timer_dequeue(ctmr);
-		timer->it_active = 0;
+		timer->it_status = POSIX_TIMER_DISARMED;
 	}
 
 	/*
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 26243d38d27d..6f0dacec25e0 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -272,7 +272,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
 		timr->kclock->timer_rearm(timr);
 
-		timr->it_active = 1;
+		timr->it_status = POSIX_TIMER_ARMED;
 		timr->it_overrun_last = timr->it_overrun;
 		timr->it_overrun = -1LL;
 		++timr->it_signal_seq;
@@ -292,14 +292,17 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 
 int posix_timer_queue_signal(struct k_itimer *timr)
 {
+	enum posix_timer_state state = POSIX_TIMER_DISARMED;
 	int ret, si_private = 0;
 	enum pid_type type;
 
 	lockdep_assert_held(&timr->it_lock);
 
-	timr->it_active = 0;
-	if (timr->it_interval)
+	if (timr->it_interval) {
+		state = POSIX_TIMER_REQUEUE_PENDING;
 		si_private = ++timr->it_signal_seq;
+	}
+	timr->it_status = state;
 
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
 	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
@@ -367,7 +370,7 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 			timr->it_overrun += hrtimer_forward(timer, now, timr->it_interval);
 			ret = HRTIMER_RESTART;
 			++timr->it_signal_seq;
-			timr->it_active = 1;
+			timr->it_status = POSIX_TIMER_ARMED;
 		}
 	}
 
@@ -647,10 +650,10 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	/* interval timer ? */
 	if (iv) {
 		cur_setting->it_interval = ktime_to_timespec64(iv);
-	} else if (!timr->it_active) {
+	} else if (timr->it_status == POSIX_TIMER_DISARMED) {
 		/*
 		 * SIGEV_NONE oneshot timers are never queued and therefore
-		 * timr->it_active is always false. The check below
+		 * timr->it_status is always DISARMED. The check below
 		 * vs. remaining time will handle this case.
 		 *
 		 * For all other timers there is nothing to update here, so
@@ -895,7 +898,7 @@ int common_timer_set(struct k_itimer *timr, int flags,
 	if (kc->timer_try_to_cancel(timr) < 0)
 		return TIMER_RETRY;
 
-	timr->it_active = 0;
+	timr->it_status = POSIX_TIMER_DISARMED;
 	posix_timer_set_common(timr, new_setting);
 
 	/* Keep timer disarmed when it_value is zero */
@@ -908,7 +911,8 @@ int common_timer_set(struct k_itimer *timr, int flags,
 	sigev_none = timr->it_sigev_notify == SIGEV_NONE;
 
 	kc->timer_arm(timr, expires, flags & TIMER_ABSTIME, sigev_none);
-	timr->it_active = !sigev_none;
+	if (!sigev_none)
+		timr->it_status = POSIX_TIMER_ARMED;
 	return 0;
 }
 
@@ -1007,7 +1011,7 @@ int common_timer_del(struct k_itimer *timer)
 	timer->it_interval = 0;
 	if (kc->timer_try_to_cancel(timer) < 0)
 		return TIMER_RETRY;
-	timer->it_active = 0;
+	timer->it_status = POSIX_TIMER_DISARMED;
 	return 0;
 }
 
diff --git a/kernel/time/posix-timers.h b/kernel/time/posix-timers.h
index 4784ea65f685..4d09677e584e 100644
--- a/kernel/time/posix-timers.h
+++ b/kernel/time/posix-timers.h
@@ -1,6 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #define TIMER_RETRY 1
 
+enum posix_timer_state {
+	POSIX_TIMER_DISARMED,
+	POSIX_TIMER_ARMED,
+	POSIX_TIMER_REQUEUE_PENDING,
+};
+
 struct k_clock {
 	int	(*clock_getres)(const clockid_t which_clock,
 				struct timespec64 *tp);

Re: [patch V5 07/26] posix-timers: Add proper state tracking

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:09AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Right now the state tracking is done by two struct members:
> 
>  - it_active:
>      A boolean which tracks armed/disarmed state
> 
>  - it_signal_seq:
>      A sequence counter which is used to invalidate settings
>      and prevent rearming
> 
> Replace it_active with it_status and keep properly track about the states
> in one place.
> 
> This allows to reuse it_signal_seq to track reprogramming, disarm and

Did you mean it_status here?

> delete operations in order to drop signals which are related to the state
> previous of those operations.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Anyway:

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

Re: [patch V5 07/26] posix-timers: Add proper state tracking

[Thomas Gleixner]

On Mon, Oct 21 2024 at 15:19, Frederic Weisbecker wrote:

> Le Tue, Oct 01, 2024 at 10:42:09AM +0200, Thomas Gleixner a écrit :
>> From: Thomas Gleixner <tglx@linutronix.de>
>> 
>> Right now the state tracking is done by two struct members:
>> 
>>  - it_active:
>>      A boolean which tracks armed/disarmed state
>> 
>>  - it_signal_seq:
>>      A sequence counter which is used to invalidate settings
>>      and prevent rearming
>> 
>> Replace it_active with it_status and keep properly track about the states
>> in one place.
>> 
>> This allows to reuse it_signal_seq to track reprogramming, disarm and
>
> Did you mean it_status here?

No. it_signal_seq is used to validate whether a queued, but not yet
delivered signal, is valid or not. The sequence is stored in the signal
when the signal is queued and compared to the sequence on delivery.

Thanks,

        tglx

Re: [patch V5 07/26] posix-timers: Add proper state tracking

[Frederic Weisbecker]

Le Thu, Oct 24, 2024 at 10:45:24AM +0200, Thomas Gleixner a écrit :
> On Mon, Oct 21 2024 at 15:19, Frederic Weisbecker wrote:
> 
> > Le Tue, Oct 01, 2024 at 10:42:09AM +0200, Thomas Gleixner a écrit :
> >> From: Thomas Gleixner <tglx@linutronix.de>
> >> 
> >> Right now the state tracking is done by two struct members:
> >> 
> >>  - it_active:
> >>      A boolean which tracks armed/disarmed state
> >> 
> >>  - it_signal_seq:
> >>      A sequence counter which is used to invalidate settings
> >>      and prevent rearming
> >> 
> >> Replace it_active with it_status and keep properly track about the states
> >> in one place.
> >> 
> >> This allows to reuse it_signal_seq to track reprogramming, disarm and
> >
> > Did you mean it_status here?
> 
> No. it_signal_seq is used to validate whether a queued, but not yet
> delivered signal, is valid or not. The sequence is stored in the signal
> when the signal is queued and compared to the sequence on delivery.

Yeah I missed the following patches that increment it_signal_seq on delete
and reset. Makes more sense now.

Thanks!

> 
> Thanks,
> 
>         tglx

[tip: timers/core] posix-timers: Add proper state tracking

[tip-bot2 for Thomas Gleixner]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     1550dde8a537b35dbf066c7f9cfe5f9b360bce0d
Gitweb:        https://git.kernel.org/tip/1550dde8a537b35dbf066c7f9cfe5f9b360bce0d
Author:        Thomas Gleixner <tglx@linutronix.de>
AuthorDate:    Tue, 01 Oct 2024 10:42:09 +02:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Tue, 29 Oct 2024 11:43:19 +01:00

posix-timers: Add proper state tracking

Right now the state tracking is done by two struct members:

 - it_active:
     A boolean which tracks armed/disarmed state

 - it_signal_seq:
     A sequence counter which is used to invalidate settings
     and prevent rearming

Replace it_active with it_status and keep properly track about the states
in one place.

This allows to reuse it_signal_seq to track reprogramming, disarm and
delete operations in order to drop signals which are related to the state
previous of those operations.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lore.kernel.org/all/20241001083835.670337048@linutronix.de

---
 include/linux/posix-timers.h   |  4 ++--
 kernel/time/alarmtimer.c       |  2 +-
 kernel/time/posix-cpu-timers.c | 15 ++++++++-------
 kernel/time/posix-timers.c     | 22 +++++++++++++---------
 kernel/time/posix-timers.h     |  6 ++++++
 5 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 253d106..02afbb4 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -147,7 +147,7 @@ static inline void posix_cputimers_init_work(void) { }
  * @kclock:		Pointer to the k_clock struct handling this timer
  * @it_clock:		The posix timer clock id
  * @it_id:		The posix timer id for identifying the timer
- * @it_active:		Marker that timer is active
+ * @it_status:		The status of the timer
  * @it_overrun:		The overrun counter for pending signals
  * @it_overrun_last:	The overrun at the time of the last delivered signal
  * @it_signal_seq:	Sequence count to control signal delivery
@@ -168,7 +168,7 @@ struct k_itimer {
 	const struct k_clock	*kclock;
 	clockid_t		it_clock;
 	timer_t			it_id;
-	int			it_active;
+	int			it_status;
 	s64			it_overrun;
 	s64			it_overrun_last;
 	unsigned int		it_signal_seq;
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 75f8443..452d8aa 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -585,7 +585,7 @@ static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm,
 		 */
 		ptr->it_overrun += __alarm_forward_now(alarm, ptr->it_interval, true);
 		++ptr->it_signal_seq;
-		ptr->it_active = 1;
+		ptr->it_status = POSIX_TIMER_ARMED;
 		result = ALARMTIMER_RESTART;
 	}
 	spin_unlock_irqrestore(&ptr->it_lock, flags);
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 993243b..12f828d 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -453,7 +453,6 @@ static void disarm_timer(struct k_itimer *timer, struct task_struct *p)
 	struct cpu_timer *ctmr = &timer->it.cpu;
 	struct posix_cputimer_base *base;
 
-	timer->it_active = 0;
 	if (!cpu_timer_dequeue(ctmr))
 		return;
 
@@ -494,11 +493,12 @@ static int posix_cpu_timer_del(struct k_itimer *timer)
 		 */
 		WARN_ON_ONCE(ctmr->head || timerqueue_node_queued(&ctmr->node));
 	} else {
-		if (timer->it.cpu.firing)
+		if (timer->it.cpu.firing) {
 			ret = TIMER_RETRY;
-		else
+		} else {
 			disarm_timer(timer, p);
-
+			timer->it_status = POSIX_TIMER_DISARMED;
+		}
 		unlock_task_sighand(p, &flags);
 	}
 
@@ -560,7 +560,7 @@ static void arm_timer(struct k_itimer *timer, struct task_struct *p)
 	struct cpu_timer *ctmr = &timer->it.cpu;
 	u64 newexp = cpu_timer_getexpires(ctmr);
 
-	timer->it_active = 1;
+	timer->it_status = POSIX_TIMER_ARMED;
 	if (!cpu_timer_enqueue(&base->tqhead, ctmr))
 		return;
 
@@ -586,7 +586,8 @@ static void cpu_timer_fire(struct k_itimer *timer)
 {
 	struct cpu_timer *ctmr = &timer->it.cpu;
 
-	timer->it_active = 0;
+	timer->it_status = POSIX_TIMER_DISARMED;
+
 	if (unlikely(timer->sigq == NULL)) {
 		/*
 		 * This a special case for clock_nanosleep,
@@ -671,7 +672,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,
 		ret = TIMER_RETRY;
 	} else {
 		cpu_timer_dequeue(ctmr);
-		timer->it_active = 0;
+		timer->it_status = POSIX_TIMER_DISARMED;
 	}
 
 	/*
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index fd321fc..dd72b8e 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -272,7 +272,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
 		timr->kclock->timer_rearm(timr);
 
-		timr->it_active = 1;
+		timr->it_status = POSIX_TIMER_ARMED;
 		timr->it_overrun_last = timr->it_overrun;
 		timr->it_overrun = -1LL;
 		++timr->it_signal_seq;
@@ -292,14 +292,17 @@ out:
 
 int posix_timer_queue_signal(struct k_itimer *timr)
 {
+	enum posix_timer_state state = POSIX_TIMER_DISARMED;
 	int ret, si_private = 0;
 	enum pid_type type;
 
 	lockdep_assert_held(&timr->it_lock);
 
-	timr->it_active = 0;
-	if (timr->it_interval)
+	if (timr->it_interval) {
+		state = POSIX_TIMER_REQUEUE_PENDING;
 		si_private = ++timr->it_signal_seq;
+	}
+	timr->it_status = state;
 
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
 	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
@@ -367,7 +370,7 @@ static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 			timr->it_overrun += hrtimer_forward(timer, now, timr->it_interval);
 			ret = HRTIMER_RESTART;
 			++timr->it_signal_seq;
-			timr->it_active = 1;
+			timr->it_status = POSIX_TIMER_ARMED;
 		}
 	}
 
@@ -640,10 +643,10 @@ void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting)
 	/* interval timer ? */
 	if (iv) {
 		cur_setting->it_interval = ktime_to_timespec64(iv);
-	} else if (!timr->it_active) {
+	} else if (timr->it_status == POSIX_TIMER_DISARMED) {
 		/*
 		 * SIGEV_NONE oneshot timers are never queued and therefore
-		 * timr->it_active is always false. The check below
+		 * timr->it_status is always DISARMED. The check below
 		 * vs. remaining time will handle this case.
 		 *
 		 * For all other timers there is nothing to update here, so
@@ -888,7 +891,7 @@ int common_timer_set(struct k_itimer *timr, int flags,
 	if (kc->timer_try_to_cancel(timr) < 0)
 		return TIMER_RETRY;
 
-	timr->it_active = 0;
+	timr->it_status = POSIX_TIMER_DISARMED;
 	posix_timer_set_common(timr, new_setting);
 
 	/* Keep timer disarmed when it_value is zero */
@@ -901,7 +904,8 @@ int common_timer_set(struct k_itimer *timr, int flags,
 	sigev_none = timr->it_sigev_notify == SIGEV_NONE;
 
 	kc->timer_arm(timr, expires, flags & TIMER_ABSTIME, sigev_none);
-	timr->it_active = !sigev_none;
+	if (!sigev_none)
+		timr->it_status = POSIX_TIMER_ARMED;
 	return 0;
 }
 
@@ -1000,7 +1004,7 @@ int common_timer_del(struct k_itimer *timer)
 	timer->it_interval = 0;
 	if (kc->timer_try_to_cancel(timer) < 0)
 		return TIMER_RETRY;
-	timer->it_active = 0;
+	timer->it_status = POSIX_TIMER_DISARMED;
 	return 0;
 }
 
diff --git a/kernel/time/posix-timers.h b/kernel/time/posix-timers.h
index 4784ea6..4d09677 100644
--- a/kernel/time/posix-timers.h
+++ b/kernel/time/posix-timers.h
@@ -1,6 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #define TIMER_RETRY 1
 
+enum posix_timer_state {
+	POSIX_TIMER_DISARMED,
+	POSIX_TIMER_ARMED,
+	POSIX_TIMER_REQUEUE_PENDING,
+};
+
 struct k_clock {
 	int	(*clock_getres)(const clockid_t which_clock,
 				struct timespec64 *tp);

[patch V5 08/26] posix-timers: Make signal delivery consistent

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Signals of timers which are reprogammed, disarmed or deleted can deliver
signals related to the past. The POSIX spec is blury about this:

 - "The effect of disarming or resetting a timer with pending expiration
   notifications is unspecified."

 - "The disposition of pending signals for the deleted timer is
    unspecified."

In both cases it is reasonable to expect that pending signals are
discarded. Especially in the reprogramming case it does not make sense to
account for previous overruns or to deliver a signal for a timer which has
been disarmed. This makes the behaviour consistent and understandable.

Remove the si_sys_private check from the signal delivery code and invoke
posix_timer_deliver_signal() unconditionally for posix timer related
signals.

Change posix_timer_deliver_signal() so it controls the actual signal delivery via the
return value. It now instructs the signal code to drop the signal when:

  1) The timer does not longer exist in the hash table

  2) The timer signal_seq value is not the same as the si_sys_private value
     which was set when the signal was queued.

This is also a preparatory change to embed the sigqueue into the k_itimer
structure, which in turn allows to remove the si_sys_private magic.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/posix-timers.h   |    2 --
 kernel/signal.c                |    6 ++----
 kernel/time/posix-cpu-timers.c |    2 +-
 kernel/time/posix-timers.c     |   25 +++++++++++++++----------
 4 files changed, 18 insertions(+), 17 deletions(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -137,8 +137,6 @@ static inline void clear_posix_cputimers
 static inline void posix_cputimers_init_work(void) { }
 #endif
 
-#define REQUEUE_PENDING 1
-
 /**
  * struct k_itimer - POSIX.1b interval timer structure.
  * @list:		List head for binding the timer to signals->posix_timers
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -550,10 +550,8 @@ static void collect_signal(int sig, stru
 		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);
+		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
+				 (info->si_code == SI_TIMER);
 
 		__sigqueue_free(first);
 	} else {
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -746,7 +746,7 @@ static void __posix_cpu_timer_get(struct
 	 *  - Timers which expired, but the signal has not yet been
 	 *    delivered
 	 */
-	if (iv && ((timer->it_signal_seq & REQUEUE_PENDING) || sigev_none))
+	if (iv && timer->it_status != POSIX_TIMER_ARMED)
 		expires = bump_cpu_timer(timer, now);
 	else
 		expires = cpu_timer_getexpires(&timer->it.cpu);
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -269,7 +269,10 @@ bool posixtimer_deliver_signal(struct ke
 	if (!timr)
 		goto out;
 
-	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
+	if (timr->it_signal_seq != info->si_sys_private)
+		goto out_unlock;
+
+	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
 		timr->kclock->timer_rearm(timr);
 
 		timr->it_status = POSIX_TIMER_ARMED;
@@ -281,6 +284,7 @@ bool posixtimer_deliver_signal(struct ke
 	}
 	ret = true;
 
+out_unlock:
 	unlock_timer(timr, flags);
 out:
 	spin_lock(&current->sighand->siglock);
@@ -293,19 +297,19 @@ bool posixtimer_deliver_signal(struct ke
 int posix_timer_queue_signal(struct k_itimer *timr)
 {
 	enum posix_timer_state state = POSIX_TIMER_DISARMED;
-	int ret, si_private = 0;
 	enum pid_type type;
+	int ret;
 
 	lockdep_assert_held(&timr->it_lock);
 
 	if (timr->it_interval) {
+		timr->it_signal_seq++;
 		state = POSIX_TIMER_REQUEUE_PENDING;
-		si_private = ++timr->it_signal_seq;
 	}
 	timr->it_status = state;
 
 	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
-	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
+	ret = send_sigqueue(timr->sigq, timr->it_pid, type, timr->it_signal_seq);
 	/* If we failed to send the signal the timer stops. */
 	return ret > 0;
 }
@@ -670,7 +674,7 @@ void common_timer_get(struct k_itimer *t
 	 * is a SIGEV_NONE timer move the expiry time forward by intervals,
 	 * so expiry is > now.
 	 */
-	if (iv && (timr->it_signal_seq & REQUEUE_PENDING || sig_none))
+	if (iv && timr->it_status != POSIX_TIMER_ARMED)
 		timr->it_overrun += kc->timer_forward(timr, now);
 
 	remaining = kc->timer_remaining(timr, now);
@@ -870,8 +874,6 @@ void posix_timer_set_common(struct k_iti
 	else
 		timer->it_interval = 0;
 
-	/* Prevent reloading in case there is a signal pending */
-	timer->it_signal_seq = (timer->it_signal_seq + 2) & ~REQUEUE_PENDING;
 	/* Reset overrun accounting */
 	timer->it_overrun_last = 0;
 	timer->it_overrun = -1LL;
@@ -889,8 +891,6 @@ int common_timer_set(struct k_itimer *ti
 	if (old_setting)
 		common_timer_get(timr, old_setting);
 
-	/* Prevent rearming by clearing the interval */
-	timr->it_interval = 0;
 	/*
 	 * Careful here. On SMP systems the timer expiry function could be
 	 * active and spinning on timr->it_lock.
@@ -940,6 +940,9 @@ static int do_timer_settime(timer_t time
 	if (old_spec64)
 		old_spec64->it_interval = ktime_to_timespec64(timr->it_interval);
 
+	/* Prevent signal delivery and rearming. */
+	timr->it_signal_seq++;
+
 	kc = timr->kclock;
 	if (WARN_ON_ONCE(!kc || !kc->timer_set))
 		error = -EINVAL;
@@ -1008,7 +1011,6 @@ int common_timer_del(struct k_itimer *ti
 {
 	const struct k_clock *kc = timer->kclock;
 
-	timer->it_interval = 0;
 	if (kc->timer_try_to_cancel(timer) < 0)
 		return TIMER_RETRY;
 	timer->it_status = POSIX_TIMER_DISARMED;
@@ -1036,6 +1038,9 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
 	if (!timer)
 		return -EINVAL;
 
+	/* Prevent signal delivery and rearming. */
+	timer->it_signal_seq++;
+
 	if (unlikely(timer_delete_hook(timer) == TIMER_RETRY)) {
 		/* Unlocks and relocks the timer if it still exists */
 		timer = timer_wait_running(timer, &flags);

Re: [patch V5 08/26] posix-timers: Make signal delivery consistent

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:10AM +0200, Thomas Gleixner a écrit :
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -269,7 +269,10 @@ bool posixtimer_deliver_signal(struct ke
>  	if (!timr)
>  		goto out;
>  
> -	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
> +	if (timr->it_signal_seq != info->si_sys_private)
> +		goto out_unlock;
> +
> +	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {

Can it be something else than POSIX_TIMER_REQUEUE_PENDING actually?
And if not, should it be a WARN_ON() ?

>  		timr->kclock->timer_rearm(timr);
>  
>  		timr->it_status = POSIX_TIMER_ARMED;
> @@ -281,6 +284,7 @@ bool posixtimer_deliver_signal(struct ke
>  	}
>  	ret = true;
>  
> +out_unlock:
>  	unlock_timer(timr, flags);
>  out:
>  	spin_lock(&current->sighand->siglock);
> @@ -293,19 +297,19 @@ bool posixtimer_deliver_signal(struct ke
>  int posix_timer_queue_signal(struct k_itimer *timr)
>  {
>  	enum posix_timer_state state = POSIX_TIMER_DISARMED;
> -	int ret, si_private = 0;
>  	enum pid_type type;
> +	int ret;
>  
>  	lockdep_assert_held(&timr->it_lock);
>  
>  	if (timr->it_interval) {
> +		timr->it_signal_seq++;

Is the increment here is still needed then, since it's done
from del and set?

Thanks.

>  		state = POSIX_TIMER_REQUEUE_PENDING;
> -		si_private = ++timr->it_signal_seq;
>  	}
>  	timr->it_status = state;
>  
>  	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
> -	ret = send_sigqueue(timr->sigq, timr->it_pid, type, si_private);
> +	ret = send_sigqueue(timr->sigq, timr->it_pid, type, timr->it_signal_seq);
>  	/* If we failed to send the signal the timer stops. */
>  	return ret > 0;
>  }

Re: [patch V5 08/26] posix-timers: Make signal delivery consistent

[Thomas Gleixner]

On Mon, Oct 21 2024 at 16:40, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:10AM +0200, Thomas Gleixner a écrit :
>> --- a/kernel/time/posix-timers.c
>> +++ b/kernel/time/posix-timers.c
>> @@ -269,7 +269,10 @@ bool posixtimer_deliver_signal(struct ke
>>  	if (!timr)
>>  		goto out;
>>  
>> -	if (timr->it_interval && timr->it_signal_seq == info->si_sys_private) {
>> +	if (timr->it_signal_seq != info->si_sys_private)
>> +		goto out_unlock;
>> +
>> +	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
>
> Can it be something else than POSIX_TIMER_REQUEUE_PENDING actually?
> And if not, should it be a WARN_ON() ?

Good point. It should not be anything else than pending.

>>  		timr->kclock->timer_rearm(timr);
>>  
>>  		timr->it_status = POSIX_TIMER_ARMED;
>> @@ -281,6 +284,7 @@ bool posixtimer_deliver_signal(struct ke
>>  	}
>>  	ret = true;
>>  
>> +out_unlock:
>>  	unlock_timer(timr, flags);
>>  out:
>>  	spin_lock(&current->sighand->siglock);
>> @@ -293,19 +297,19 @@ bool posixtimer_deliver_signal(struct ke
>>  int posix_timer_queue_signal(struct k_itimer *timr)
>>  {
>>  	enum posix_timer_state state = POSIX_TIMER_DISARMED;
>> -	int ret, si_private = 0;
>>  	enum pid_type type;
>> +	int ret;
>>  
>>  	lockdep_assert_held(&timr->it_lock);
>>  
>>  	if (timr->it_interval) {
>> +		timr->it_signal_seq++;
>
> Is the increment here is still needed then, since it's done
> from del and set?

Probably not. Let me stare at it.

Thanks,

        tglx

[patch V5 09/26] posix-timers: Make signal overrun accounting sensible

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

The handling of the timer overrun in the signal code is inconsistent as it
takes previous overruns into account. This is just wrong as after the
reprogramming of a timer the overrun count starts over from a clean state,
i.e. 0.

Make the accounting in send_sigqueue() consistent with that.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/signal.c |   34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1961,6 +1961,34 @@ int send_sigqueue(struct sigqueue *q, st
 	 */
 	q->info.si_sys_private = si_private;
 
+	/*
+	 * Set the overrun count to zero unconditionally. The posix timer
+	 * code does not self rearm periodic timers. They are rearmed from
+	 * dequeue_signal().
+	 *
+	 * But there is a situation where @q is already enqueued:
+	 *
+	 * 1) timer_settime()
+	 *      arm_timer()
+	 * 2) timer_expires()
+	 *      send_sigqueue(@q)
+	 *        enqueue(@q)
+	 * 3) timer_settime()
+	 *      arm_timer()
+	 * 4) timer_expires()
+	 *      send_sigqueue(@q) <- Observes @q already queued
+	 *
+	 * In this case incrementing si_overrun does not make sense because
+	 * there is no relationship between timer_settime() #1 and #2.
+	 *
+	 * The POSIX specification is useful as always: "The effect of
+	 * disarming or resetting a timer with pending expiration
+	 * notifications is unspecified."
+	 *
+	 * Just do the sensible thing and reset the overrun.
+	 */
+	q->info.si_overrun = 0;
+
 	ret = 1; /* the signal is ignored */
 	result = TRACE_SIGNAL_IGNORED;
 	if (!prepare_signal(sig, t, false))
@@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
 
 	ret = 0;
 	if (unlikely(!list_empty(&q->list))) {
-		/*
-		 * If an SI_TIMER entry is already queue just increment
-		 * the overrun count.
-		 */
-		q->info.si_overrun++;
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
-	q->info.si_overrun = 0;
 
 	signalfd_notify(t, sig);
 	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;

Re: [patch V5 09/26] posix-timers: Make signal overrun accounting sensible

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:12AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The handling of the timer overrun in the signal code is inconsistent as it
> takes previous overruns into account. This is just wrong as after the
> reprogramming of a timer the overrun count starts over from a clean state,
> i.e. 0.
> 
> Make the accounting in send_sigqueue() consistent with that.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/signal.c |   34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> ---
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1961,6 +1961,34 @@ int send_sigqueue(struct sigqueue *q, st
>  	 */
>  	q->info.si_sys_private = si_private;
>  
> +	/*
> +	 * Set the overrun count to zero unconditionally. The posix timer
> +	 * code does not self rearm periodic timers. They are rearmed from
> +	 * dequeue_signal().
> +	 *
> +	 * But there is a situation where @q is already enqueued:
> +	 *
> +	 * 1) timer_settime()
> +	 *      arm_timer()
> +	 * 2) timer_expires()
> +	 *      send_sigqueue(@q)
> +	 *        enqueue(@q)
> +	 * 3) timer_settime()
> +	 *      arm_timer()
> +	 * 4) timer_expires()
> +	 *      send_sigqueue(@q) <- Observes @q already queued
> +	 *
> +	 * In this case incrementing si_overrun does not make sense because
> +	 * there is no relationship between timer_settime() #1 and #2.
> +	 *
> +	 * The POSIX specification is useful as always: "The effect of
> +	 * disarming or resetting a timer with pending expiration
> +	 * notifications is unspecified."
> +	 *
> +	 * Just do the sensible thing and reset the overrun.
> +	 */
> +	q->info.si_overrun = 0;

So this means that in the above example case, no signal at all is going to be
delivered (because the seq will be impaired on the previously queued
signal) and no overrun count will be incremented either?

> +
>  	ret = 1; /* the signal is ignored */
>  	result = TRACE_SIGNAL_IGNORED;
>  	if (!prepare_signal(sig, t, false))
> @@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
>  
>  	ret = 0;
>  	if (unlikely(!list_empty(&q->list))) {
> -		/*
> -		 * If an SI_TIMER entry is already queue just increment
> -		 * the overrun count.
> -		 */
> -		q->info.si_overrun++;

Who is ever incrementing this after that? I'm a bit confused between the
timer overrun and the sigqueue overrun. Those seem to be two different
things without any link...

Thanks.

>  		result = TRACE_SIGNAL_ALREADY_PENDING;
>  		goto out;
>  	}
> -	q->info.si_overrun = 0;
>  
>  	signalfd_notify(t, sig);
>  	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
> 

Re: [patch V5 09/26] posix-timers: Make signal overrun accounting sensible

[Thomas Gleixner]

On Mon, Oct 21 2024 at 23:54, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:12AM +0200, Thomas Gleixner a écrit :
>> +	/*
>> +	 * Set the overrun count to zero unconditionally. The posix timer
>> +	 * code does not self rearm periodic timers. They are rearmed from
>> +	 * dequeue_signal().
>> +	 *
>> +	 * But there is a situation where @q is already enqueued:
>> +	 *
>> +	 * 1) timer_settime()
>> +	 *      arm_timer()
>> +	 * 2) timer_expires()
>> +	 *      send_sigqueue(@q)
>> +	 *        enqueue(@q)
>> +	 * 3) timer_settime()
>> +	 *      arm_timer()
>> +	 * 4) timer_expires()
>> +	 *      send_sigqueue(@q) <- Observes @q already queued
>> +	 *
>> +	 * In this case incrementing si_overrun does not make sense because
>> +	 * there is no relationship between timer_settime() #1 and #2.
>> +	 *
>> +	 * The POSIX specification is useful as always: "The effect of
>> +	 * disarming or resetting a timer with pending expiration
>> +	 * notifications is unspecified."
>> +	 *
>> +	 * Just do the sensible thing and reset the overrun.
>> +	 */
>> +	q->info.si_overrun = 0;
>
> So this means that in the above example case, no signal at all is going to be
> delivered (because the seq will be impaired on the previously queued
> signal) and no overrun count will be incremented either?

So #2 queues the signal, but before delivery the timer is rearmed, which
invalidates the signal via the sequence count. So #4 has to set the
overrun counter which might be set already.

>> +
>>  	ret = 1; /* the signal is ignored */
>>  	result = TRACE_SIGNAL_IGNORED;
>>  	if (!prepare_signal(sig, t, false))
>> @@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
>>  
>>  	ret = 0;
>>  	if (unlikely(!list_empty(&q->list))) {
>> -		/*
>> -		 * If an SI_TIMER entry is already queue just increment
>> -		 * the overrun count.
>> -		 */
>> -		q->info.si_overrun++;
>
> Who is ever incrementing this after that? I'm a bit confused between the
> timer overrun and the sigqueue overrun. Those seem to be two different
> things without any link...

Hmm. You're right. This should now never happen. Let me stare at it some
moar.

Thanks,

        tglx

Re: [patch V5 09/26] posix-timers: Make signal overrun accounting sensible

[Thomas Gleixner]

On Thu, Oct 24 2024 at 10:57, Thomas Gleixner wrote:
> On Mon, Oct 21 2024 at 23:54, Frederic Weisbecker wrote:
>> Le Tue, Oct 01, 2024 at 10:42:12AM +0200, Thomas Gleixner a écrit :
>>> +	/*
>>> +	 * Set the overrun count to zero unconditionally. The posix timer
>>> +	 * code does not self rearm periodic timers. They are rearmed from
>>> +	 * dequeue_signal().
>>> +	 *
>>> +	 * But there is a situation where @q is already enqueued:
>>> +	 *
>>> +	 * 1) timer_settime()
>>> +	 *      arm_timer()
>>> +	 * 2) timer_expires()
>>> +	 *      send_sigqueue(@q)
>>> +	 *        enqueue(@q)
>>> +	 * 3) timer_settime()
>>> +	 *      arm_timer()
>>> +	 * 4) timer_expires()
>>> +	 *      send_sigqueue(@q) <- Observes @q already queued
>>> +	 *
>>> +	 * In this case incrementing si_overrun does not make sense because
>>> +	 * there is no relationship between timer_settime() #1 and #2.
>>> +	 *
>>> +	 * The POSIX specification is useful as always: "The effect of
>>> +	 * disarming or resetting a timer with pending expiration
>>> +	 * notifications is unspecified."
>>> +	 *
>>> +	 * Just do the sensible thing and reset the overrun.
>>> +	 */
>>> +	q->info.si_overrun = 0;
>>
>> So this means that in the above example case, no signal at all is going to be
>> delivered (because the seq will be impaired on the previously queued
>> signal) and no overrun count will be incremented either?
>
> So #2 queues the signal, but before delivery the timer is rearmed, which
> invalidates the signal via the sequence count. So #4 has to set the
> overrun counter which might be set already.

The signal will be delivered. It's either already marked as pending, but
not yet delivered or it is masked and will be marked pending on unmask.

>>> +
>>>  	ret = 1; /* the signal is ignored */
>>>  	result = TRACE_SIGNAL_IGNORED;
>>>  	if (!prepare_signal(sig, t, false))
>>> @@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
>>>  
>>>  	ret = 0;
>>>  	if (unlikely(!list_empty(&q->list))) {
>>> -		/*
>>> -		 * If an SI_TIMER entry is already queue just increment
>>> -		 * the overrun count.
>>> -		 */
>>> -		q->info.si_overrun++;
>>
>> Who is ever incrementing this after that? I'm a bit confused between the
>> timer overrun and the sigqueue overrun. Those seem to be two different
>> things without any link...
>
> Hmm. You're right. This should now never happen. Let me stare at it some
> moar.

Of course it can happen that the signal is queued. See above scenario :)

But q->info.si_overrun should never be anything else than 0. The timer code
increments the overrun counter and updates si_overrun on signal
delivery, but that's done on the siginfo which is a copy of q->info.

This seems to be a leftover from early posix-timer days... So that
change can be reduced to removing the increment.

Thanks,

        tglx

Re: [patch V5 09/26] posix-timers: Make signal overrun accounting sensible

[Frederic Weisbecker]

Le Thu, Oct 24, 2024 at 11:10:09AM +0200, Thomas Gleixner a écrit :
> On Thu, Oct 24 2024 at 10:57, Thomas Gleixner wrote:
> > On Mon, Oct 21 2024 at 23:54, Frederic Weisbecker wrote:
> >> Le Tue, Oct 01, 2024 at 10:42:12AM +0200, Thomas Gleixner a écrit :
> >>> +	/*
> >>> +	 * Set the overrun count to zero unconditionally. The posix timer
> >>> +	 * code does not self rearm periodic timers. They are rearmed from
> >>> +	 * dequeue_signal().
> >>> +	 *
> >>> +	 * But there is a situation where @q is already enqueued:
> >>> +	 *
> >>> +	 * 1) timer_settime()
> >>> +	 *      arm_timer()
> >>> +	 * 2) timer_expires()
> >>> +	 *      send_sigqueue(@q)
> >>> +	 *        enqueue(@q)
> >>> +	 * 3) timer_settime()
> >>> +	 *      arm_timer()
> >>> +	 * 4) timer_expires()
> >>> +	 *      send_sigqueue(@q) <- Observes @q already queued
> >>> +	 *
> >>> +	 * In this case incrementing si_overrun does not make sense because
> >>> +	 * there is no relationship between timer_settime() #1 and #2.
> >>> +	 *
> >>> +	 * The POSIX specification is useful as always: "The effect of
> >>> +	 * disarming or resetting a timer with pending expiration
> >>> +	 * notifications is unspecified."
> >>> +	 *
> >>> +	 * Just do the sensible thing and reset the overrun.
> >>> +	 */
> >>> +	q->info.si_overrun = 0;
> >>
> >> So this means that in the above example case, no signal at all is going to be
> >> delivered (because the seq will be impaired on the previously queued
> >> signal) and no overrun count will be incremented either?
> >
> > So #2 queues the signal, but before delivery the timer is rearmed, which
> > invalidates the signal via the sequence count. So #4 has to set the
> > overrun counter which might be set already.
> 
> The signal will be delivered. It's either already marked as pending, but
> not yet delivered or it is masked and will be marked pending on unmask.

Oh I see now, si_sys_private is still overwritten with the new seq value
even if the signal is already queued. So yes the old queued signal will
remain valid and apply for the latest event. Phew!

> 
> >>> +
> >>>  	ret = 1; /* the signal is ignored */
> >>>  	result = TRACE_SIGNAL_IGNORED;
> >>>  	if (!prepare_signal(sig, t, false))
> >>> @@ -1968,15 +1996,9 @@ int send_sigqueue(struct sigqueue *q, st
> >>>  
> >>>  	ret = 0;
> >>>  	if (unlikely(!list_empty(&q->list))) {
> >>> -		/*
> >>> -		 * If an SI_TIMER entry is already queue just increment
> >>> -		 * the overrun count.
> >>> -		 */
> >>> -		q->info.si_overrun++;
> >>
> >> Who is ever incrementing this after that? I'm a bit confused between the
> >> timer overrun and the sigqueue overrun. Those seem to be two different
> >> things without any link...
> >
> > Hmm. You're right. This should now never happen. Let me stare at it some
> > moar.
> 
> Of course it can happen that the signal is queued. See above scenario :)
> 
> But q->info.si_overrun should never be anything else than 0. The timer code
> increments the overrun counter and updates si_overrun on signal
> delivery, but that's done on the siginfo which is a copy of q->info.

Bah right of course. But then, there is an undocumented side effect
to this patch in that it also resets the value when a queued signal
is ignored.

> 
> This seems to be a leftover from early posix-timer days... So that
> change can be reduced to removing the increment.

Ok. One thing you can add to this patch is the following, since now si_overrun
is expected to be 0 upon delivery (nothing can change it between the
enqueue and the delivery)

diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index ff7970030b17..0d44e3d87cd3 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -233,11 +233,12 @@ __initcall(init_posix_timers);
  * The siginfo si_overrun field and the return value of timer_getoverrun(2)
  * are of type int. Clamp the overrun value to INT_MAX
  */
-static inline int timer_overrun_to_int(struct k_itimer *timr, int baseval)
+static inline int timer_overrun_to_int(struct k_itimer *timr)
 {
-	s64 sum = timr->it_overrun_last + (s64)baseval;
+	if (timr->it_overrun_last > (s64)INT_MAX)
+		return INT_MAX;
 
-	return sum > (s64)INT_MAX ? INT_MAX : (int)sum;
+	return (int)timr->it_overrun_last;
 }
 
 static void common_hrtimer_rearm(struct k_itimer *timr)
@@ -280,7 +281,7 @@ bool posixtimer_deliver_signal(struct kernel_siginfo *info)
 		timr->it_overrun = -1LL;
 		++timr->it_signal_seq;
 
-		info->si_overrun = timer_overrun_to_int(timr, info->si_overrun);
+		info->si_overrun = timer_overrun_to_int(timr);
 	}
 	ret = true;
 
@@ -783,7 +784,7 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id)
 	if (!timr)
 		return -EINVAL;
 
-	overrun = timer_overrun_to_int(timr, 0);
+	overrun = timer_overrun_to_int(timr);
 	unlock_timer(timr, flags);
 
 	return overrun;

[patch V5 10/26] posix-cpu-timers: Use dedicated flag for CPU timer nanosleep

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

POSIX CPU timer nanosleep creates a k_itimer on stack and uses the sigq
pointer to detect the nanosleep case in the expiry function.

Prepare for embedding sigqueue into struct k_itimer by using a dedicated
flag for nanosleep.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h   | 4 +++-
 kernel/time/posix-cpu-timers.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)
---
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index 8c6d97412526..bcd01208d795 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -42,6 +42,7 @@ static inline int clockid_to_fd(const clockid_t clk)
  * @pid:	Pointer to target task PID
  * @elist:	List head for the expiry list
  * @firing:	Timer is currently firing
+ * @nanosleep:	Timer is used for nanosleep and is not a regular posix-timer
  * @handling:	Pointer to the task which handles expiry
  */
 struct cpu_timer {
@@ -49,7 +50,8 @@ struct cpu_timer {
 	struct timerqueue_head		*head;
 	struct pid			*pid;
 	struct list_head		elist;
-	int				firing;
+	bool				firing;
+	bool				nanosleep;
 	struct task_struct __rcu	*handling;
 };
 
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index bc2cd32b7a40..ea1835cb541a 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -588,7 +588,7 @@ static void cpu_timer_fire(struct k_itimer *timer)
 
 	timer->it_status = POSIX_TIMER_DISARMED;
 
-	if (unlikely(timer->sigq == NULL)) {
+	if (unlikely(ctmr->nanosleep)) {
 		/*
 		 * This a special case for clock_nanosleep,
 		 * not a normal timer from sys_timer_create.
@@ -1479,6 +1479,7 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags,
 	timer.it_overrun = -1;
 	error = posix_cpu_timer_create(&timer);
 	timer.it_process = current;
+	timer.it.cpu.nanosleep = true;
 
 	if (!error) {
 		static struct itimerspec64 zero_it;

Re: [patch V5 10/26] posix-cpu-timers: Use dedicated flag for CPU timer nanosleep

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:13AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> POSIX CPU timer nanosleep creates a k_itimer on stack and uses the sigq
> pointer to detect the nanosleep case in the expiry function.
> 
> Prepare for embedding sigqueue into struct k_itimer by using a dedicated
> flag for nanosleep.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 11/26] posix-timers: Add a refcount to struct k_itimer

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To cure the SIG_IGN handling for posix interval timers, the preallocated
sigqueue needs to be embedded into struct k_itimer to prevent life time
races of all sorts.

To make that work correctly it needs reference counting so that timer
deletion does not free the timer prematuraly when there is a signal queued
or delivered concurrently.

Add a rcuref to the posix timer part.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h | 14 ++++++++++++++
 kernel/time/posix-timers.c   |  7 ++++---
 2 files changed, 18 insertions(+), 3 deletions(-)
---
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h
index bcd01208d795..9740fd0c2933 100644
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -6,11 +6,13 @@
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/posix-timers_types.h>
+#include <linux/rcuref.h>
 #include <linux/spinlock.h>
 #include <linux/timerqueue.h>
 
 struct kernel_siginfo;
 struct task_struct;
+struct k_itimer;
 
 static inline clockid_t make_process_cpuclock(const unsigned int pid,
 		const clockid_t clock)
@@ -105,6 +107,7 @@ static inline void posix_cputimers_rt_watchdog(struct posix_cputimers *pct,
 
 void posixtimer_rearm_itimer(struct task_struct *p);
 bool posixtimer_deliver_signal(struct kernel_siginfo *info);
+void posixtimer_free_timer(struct k_itimer *timer);
 
 /* Init task static initializer */
 #define INIT_CPU_TIMERBASE(b) {						\
@@ -129,6 +132,7 @@ 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 bool posixtimer_deliver_signal(struct kernel_siginfo *info) { return false; }
+static inline void posixtimer_free_timer(struct k_itimer *timer) { }
 #endif
 
 #ifdef CONFIG_POSIX_CPU_TIMERS_TASK_WORK
@@ -156,6 +160,7 @@ static inline void posix_cputimers_init_work(void) { }
  * @it_signal:		Pointer to the creators signal struct
  * @it_pid:		The pid of the process/task targeted by the signal
  * @it_process:		The task to wakeup on clock_nanosleep (CPU timers)
+ * @rcuref:		Reference count for life time management
  * @sigq:		Pointer to preallocated sigqueue
  * @it:			Union representing the various posix timer type
  *			internals.
@@ -180,6 +185,7 @@ struct k_itimer {
 		struct task_struct	*it_process;
 	};
 	struct sigqueue		*sigq;
+	rcuref_t		rcuref;
 	union {
 		struct {
 			struct hrtimer	timer;
@@ -200,4 +206,12 @@ void set_process_cpu_timer(struct task_struct *task, unsigned int clock_idx,
 
 int update_rlimit_cpu(struct task_struct *task, unsigned long rlim_new);
 
+#ifdef CONFIG_POSIX_TIMERS
+static inline void posixtimer_putref(struct k_itimer *tmr)
+{
+	if (rcuref_put(&tmr->rcuref))
+		posixtimer_free_timer(tmr);
+}
+#endif /* !CONFIG_POSIX_TIMERS */
+
 #endif
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 1231efb7c30f..1c2f6090b767 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -417,6 +417,7 @@ static struct k_itimer * alloc_posix_timer(void)
 		return NULL;
 	}
 	clear_siginfo(&tmr->sigq->info);
+	rcuref_init(&tmr->rcuref, 1);
 	return tmr;
 }
 
@@ -427,7 +428,7 @@ static void k_itimer_rcu_free(struct rcu_head *head)
 	kmem_cache_free(posix_timers_cache, tmr);
 }
 
-static void posix_timer_free(struct k_itimer *tmr)
+void posixtimer_free_timer(struct k_itimer *tmr)
 {
 	put_pid(tmr->it_pid);
 	sigqueue_free(tmr->sigq);
@@ -439,7 +440,7 @@ static void posix_timer_unhash_and_free(struct k_itimer *tmr)
 	spin_lock(&hash_lock);
 	hlist_del_rcu(&tmr->t_hash);
 	spin_unlock(&hash_lock);
-	posix_timer_free(tmr);
+	posixtimer_putref(tmr);
 }
 
 static int common_timer_create(struct k_itimer *new_timer)
@@ -474,7 +475,7 @@ static int do_timer_create(clockid_t which_clock, struct sigevent *event,
 	 */
 	new_timer_id = posix_timer_add(new_timer);
 	if (new_timer_id < 0) {
-		posix_timer_free(new_timer);
+		posixtimer_free_timer(new_timer);
 		return new_timer_id;
 	}
 

Re: [patch V5 11/26] posix-timers: Add a refcount to struct k_itimer

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:14AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To cure the SIG_IGN handling for posix interval timers, the preallocated
> sigqueue needs to be embedded into struct k_itimer to prevent life time
> races of all sorts.
> 
> To make that work correctly it needs reference counting so that timer
> deletion does not free the timer prematuraly when there is a signal queued
> or delivered concurrently.
> 
> Add a rcuref to the posix timer part.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 12/26] signal: Split up __sigqueue_alloc()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To cure the SIG_IGN handling for posix interval timers, the preallocated
sigqueue needs to be embedded into struct k_itimer to prevent life time
races of all sorts.

Reorganize __sigqueue_alloc() so the ucounts retrieval and the
initialization can be used independently.

No functional change.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/signal.c | 52 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 17 deletions(-)
---
diff --git a/kernel/signal.c b/kernel/signal.c
index a99274287902..87c349a2ddf7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -396,16 +396,9 @@ void task_join_group_stop(struct task_struct *task)
 	task_set_jobctl_pending(task, mask | JOBCTL_STOP_PENDING);
 }
 
-/*
- * allocate a new signal queue record
- * - this may be called without locks if and only if t == current, otherwise an
- *   appropriate lock must be held to stop the target task from exiting
- */
-static struct sigqueue *
-__sigqueue_alloc(int sig, struct task_struct *t, gfp_t gfp_flags,
-		 int override_rlimit, const unsigned int sigqueue_flags)
+static struct ucounts *sig_get_ucounts(struct task_struct *t, int sig,
+				       int override_rlimit)
 {
-	struct sigqueue *q = NULL;
 	struct ucounts *ucounts;
 	long sigpending;
 
@@ -424,19 +417,44 @@ __sigqueue_alloc(int sig, struct task_struct *t, gfp_t gfp_flags,
 	if (!sigpending)
 		return NULL;
 
-	if (override_rlimit || likely(sigpending <= task_rlimit(t, RLIMIT_SIGPENDING))) {
-		q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
-	} else {
+	if (unlikely(!override_rlimit && sigpending > task_rlimit(t, RLIMIT_SIGPENDING))) {
+		dec_rlimit_put_ucounts(ucounts, UCOUNT_RLIMIT_SIGPENDING);
 		print_dropped_signal(sig);
+		return NULL;
 	}
 
-	if (unlikely(q == NULL)) {
+	return ucounts;
+}
+
+static void __sigqueue_init(struct sigqueue *q, struct ucounts *ucounts,
+			    const unsigned int sigqueue_flags)
+{
+	INIT_LIST_HEAD(&q->list);
+	q->flags = sigqueue_flags;
+	q->ucounts = ucounts;
+}
+
+/*
+ * allocate a new signal queue record
+ * - this may be called without locks if and only if t == current, otherwise an
+ *   appropriate lock must be held to stop the target task from exiting
+ */
+static struct sigqueue *__sigqueue_alloc(int sig, struct task_struct *t, gfp_t gfp_flags,
+					 int override_rlimit, const unsigned int sigqueue_flags)
+{
+	struct ucounts *ucounts = sig_get_ucounts(t, sig, override_rlimit);
+	struct sigqueue *q;
+
+	if (!ucounts)
+		return NULL;
+
+	q = kmem_cache_alloc(sigqueue_cachep, gfp_flags);
+	if (!q) {
 		dec_rlimit_put_ucounts(ucounts, UCOUNT_RLIMIT_SIGPENDING);
-	} else {
-		INIT_LIST_HEAD(&q->list);
-		q->flags = sigqueue_flags;
-		q->ucounts = ucounts;
+		return NULL;
 	}
+
+	__sigqueue_init(q, ucounts, sigqueue_flags);
 	return q;
 }
 

Re: [patch V5 12/26] signal: Split up __sigqueue_alloc()

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:16AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To cure the SIG_IGN handling for posix interval timers, the preallocated
> sigqueue needs to be embedded into struct k_itimer to prevent life time
> races of all sorts.
> 
> Reorganize __sigqueue_alloc() so the ucounts retrieval and the
> initialization can be used independently.
> 
> No functional change.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 13/26] signal: Provide posixtimer_sigqueue_init()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To cure the SIG_IGN handling for posix interval timers, the preallocated
sigqueue needs to be embedded into struct k_itimer to prevent life time
races of all sorts.

Provide a new function to initialize the embedded sigqueue to prepare for
that.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |    2 ++
 kernel/signal.c              |   11 +++++++++++
 2 files changed, 13 insertions(+)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -12,6 +12,7 @@
 
 struct kernel_siginfo;
 struct task_struct;
+struct sigqueue;
 struct k_itimer;
 
 static inline clockid_t make_process_cpuclock(const unsigned int pid,
@@ -106,6 +107,7 @@ static inline void posix_cputimers_rt_wa
 }
 
 void posixtimer_rearm_itimer(struct task_struct *p);
+bool posixtimer_init_sigqueue(struct sigqueue *q);
 bool posixtimer_deliver_signal(struct kernel_siginfo *info);
 void posixtimer_free_timer(struct k_itimer *timer);
 
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1907,6 +1907,17 @@ void flush_itimer_signals(void)
 	__flush_itimer_signals(&tsk->signal->shared_pending);
 }
 
+bool posixtimer_init_sigqueue(struct sigqueue *q)
+{
+	struct ucounts *ucounts = sig_get_ucounts(current, -1, 0);
+
+	if (!ucounts)
+		return false;
+	clear_siginfo(&q->info);
+	__sigqueue_init(q, ucounts, SIGQUEUE_PREALLOC);
+	return true;
+}
+
 struct sigqueue *sigqueue_alloc(void)
 {
 	return __sigqueue_alloc(-1, current, GFP_KERNEL, 0, SIGQUEUE_PREALLOC);

Re: [patch V5 13/26] signal: Provide posixtimer_sigqueue_init()

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:17AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To cure the SIG_IGN handling for posix interval timers, the preallocated
> sigqueue needs to be embedded into struct k_itimer to prevent life time
> races of all sorts.
> 
> Provide a new function to initialize the embedded sigqueue to prepare for
> that.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 14/26] posix-timers: Store PID type in the timer

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

instead of re-evaluating the signal delivery mode everywhere.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |    2 ++
 kernel/time/posix-timers.c   |    9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -5,6 +5,7 @@
 #include <linux/alarmtimer.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/pid.h>
 #include <linux/posix-timers_types.h>
 #include <linux/rcuref.h>
 #include <linux/spinlock.h>
@@ -180,6 +181,7 @@ struct k_itimer {
 	s64			it_overrun_last;
 	unsigned int		it_signal_seq;
 	int			it_sigev_notify;
+	enum pid_type		it_pid_type;
 	ktime_t			it_interval;
 	struct signal_struct	*it_signal;
 	union {
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -297,7 +297,6 @@ bool posixtimer_deliver_signal(struct ke
 int posix_timer_queue_signal(struct k_itimer *timr)
 {
 	enum posix_timer_state state = POSIX_TIMER_DISARMED;
-	enum pid_type type;
 	int ret;
 
 	lockdep_assert_held(&timr->it_lock);
@@ -308,8 +307,7 @@ int posix_timer_queue_signal(struct k_it
 	}
 	timr->it_status = state;
 
-	type = !(timr->it_sigev_notify & SIGEV_THREAD_ID) ? PIDTYPE_TGID : PIDTYPE_PID;
-	ret = send_sigqueue(timr->sigq, timr->it_pid, type, timr->it_signal_seq);
+	ret = send_sigqueue(timr->sigq, timr->it_pid, timr->it_pid_type, timr->it_signal_seq);
 	/* If we failed to send the signal the timer stops. */
 	return ret > 0;
 }
@@ -503,6 +501,11 @@ static int do_timer_create(clockid_t whi
 		new_timer->it_pid = get_pid(task_tgid(current));
 	}
 
+	if (new_timer->it_sigev_notify & SIGEV_THREAD_ID)
+		new_timer->it_pid_type = PIDTYPE_PID;
+	else
+		new_timer->it_pid_type = PIDTYPE_TGID;
+
 	new_timer->sigq->info.si_tid   = new_timer->it_id;
 	new_timer->sigq->info.si_code  = SI_TIMER;
 

Re: [patch V5 14/26] posix-timers: Store PID type in the timer

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:19AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> instead of re-evaluating the signal delivery mode everywhere.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 15/26] signal: Refactor send_sigqueue()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To handle posix timers which have their signal ignored via SIG_IGN properly
it is required to requeue a ignored signal for delivery when SIG_IGN is
lifted so the timer gets rearmed.

Split the required code out of send_sigqueue() so it can be reused in
context of sigaction().

While at it rename send_sigqueue() to posixtimer_send_sigqueue() so its
clear what this is about.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |    1 
 include/linux/sched/signal.h |    1 
 kernel/signal.c              |   82 +++++++++++++++++++++++--------------------
 kernel/time/posix-timers.c   |    2 -
 4 files changed, 47 insertions(+), 39 deletions(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -109,6 +109,7 @@ static inline void posix_cputimers_rt_wa
 
 void posixtimer_rearm_itimer(struct task_struct *p);
 bool posixtimer_init_sigqueue(struct sigqueue *q);
+int posixtimer_send_sigqueue(struct k_itimer *tmr);
 bool posixtimer_deliver_signal(struct kernel_siginfo *info);
 void posixtimer_free_timer(struct k_itimer *timer);
 
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -340,7 +340,6 @@ extern int send_sig(int, struct task_str
 extern int zap_other_threads(struct task_struct *p);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
-extern int send_sigqueue(struct sigqueue *, struct pid *, enum pid_type, int si_private);
 extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
 
 static inline void clear_notify_signal(void)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1949,40 +1949,54 @@ void sigqueue_free(struct sigqueue *q)
 		__sigqueue_free(q);
 }
 
-int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type, int si_private)
+static void posixtimer_queue_sigqueue(struct sigqueue *q, struct task_struct *t, enum pid_type type)
 {
-	int sig = q->info.si_signo;
 	struct sigpending *pending;
+	int sig = q->info.si_signo;
+
+	signalfd_notify(t, sig);
+	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
+	list_add_tail(&q->list, &pending->list);
+	sigaddset(&pending->signal, sig);
+	complete_signal(sig, t, type);
+}
+
+/*
+ * This function is used by POSIX timers to deliver a timer signal.
+ * Where type is PIDTYPE_PID (such as for timers with SIGEV_THREAD_ID
+ * set), the signal must be delivered to the specific thread (queues
+ * into t->pending).
+ *
+ * Where type is not PIDTYPE_PID, signals must be delivered to the
+ * process. In this case, prefer to deliver to current if it is in
+ * the same thread group as the target process, which avoids
+ * unnecessarily waking up a potentially idle task.
+ */
+static inline struct task_struct *posixtimer_get_target(struct k_itimer *tmr)
+{
+	struct task_struct *t = pid_task(tmr->it_pid, tmr->it_pid_type);
+
+	if (t && tmr->it_pid_type != PIDTYPE_PID && same_thread_group(t, current))
+		t = current;
+	return t;
+}
+
+int posixtimer_send_sigqueue(struct k_itimer *tmr)
+{
+	struct sigqueue *q = tmr->sigq;
+	int sig = q->info.si_signo;
 	struct task_struct *t;
 	unsigned long flags;
 	int ret, result;
 
-	if (WARN_ON_ONCE(!(q->flags & SIGQUEUE_PREALLOC)))
-		return 0;
-	if (WARN_ON_ONCE(q->info.si_code != SI_TIMER))
-		return 0;
+	guard(rcu)();
 
-	ret = -1;
-	rcu_read_lock();
-
-	/*
-	 * This function is used by POSIX timers to deliver a timer signal.
-	 * Where type is PIDTYPE_PID (such as for timers with SIGEV_THREAD_ID
-	 * set), the signal must be delivered to the specific thread (queues
-	 * into t->pending).
-	 *
-	 * Where type is not PIDTYPE_PID, signals must be delivered to the
-	 * process. In this case, prefer to deliver to current if it is in
-	 * the same thread group as the target process, which avoids
-	 * unnecessarily waking up a potentially idle task.
-	 */
-	t = pid_task(pid, type);
+	t = posixtimer_get_target(tmr);
 	if (!t)
-		goto ret;
-	if (type != PIDTYPE_PID && same_thread_group(t, current))
-		t = current;
+		return -1;
+
 	if (!likely(lock_task_sighand(t, &flags)))
-		goto ret;
+		return -1;
 
 	/*
 	 * Update @q::info::si_sys_private for posix timer signals with
@@ -1990,7 +2004,7 @@ int send_sigqueue(struct sigqueue *q, st
 	 * decides based on si_sys_private whether to invoke
 	 * posixtimer_rearm() or not.
 	 */
-	q->info.si_sys_private = si_private;
+	q->info.si_sys_private = tmr->it_signal_seq;
 
 	/*
 	 * Set the overrun count to zero unconditionally. The posix timer
@@ -2021,27 +2035,21 @@ int send_sigqueue(struct sigqueue *q, st
 	q->info.si_overrun = 0;
 
 	ret = 1; /* the signal is ignored */
-	result = TRACE_SIGNAL_IGNORED;
-	if (!prepare_signal(sig, t, false))
+	if (!prepare_signal(sig, t, false)) {
+		result = TRACE_SIGNAL_IGNORED;
 		goto out;
+	}
 
 	ret = 0;
 	if (unlikely(!list_empty(&q->list))) {
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
-
-	signalfd_notify(t, sig);
-	pending = (type != PIDTYPE_PID) ? &t->signal->shared_pending : &t->pending;
-	list_add_tail(&q->list, &pending->list);
-	sigaddset(&pending->signal, sig);
-	complete_signal(sig, t, type);
+	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out:
-	trace_signal_generate(sig, &q->info, t, type != PIDTYPE_PID, result);
+	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
 	unlock_task_sighand(t, &flags);
-ret:
-	rcu_read_unlock();
 	return ret;
 }
 #endif /* CONFIG_POSIX_TIMERS */
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -307,7 +307,7 @@ int posix_timer_queue_signal(struct k_it
 	}
 	timr->it_status = state;
 
-	ret = send_sigqueue(timr->sigq, timr->it_pid, timr->it_pid_type, timr->it_signal_seq);
+	ret = posixtimer_send_sigqueue(timr);
 	/* If we failed to send the signal the timer stops. */
 	return ret > 0;
 }

Re: [patch V5 15/26] signal: Refactor send_sigqueue()

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:20AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To handle posix timers which have their signal ignored via SIG_IGN properly
> it is required to requeue a ignored signal for delivery when SIG_IGN is
> lifted so the timer gets rearmed.
> 
> Split the required code out of send_sigqueue() so it can be reused in
> context of sigaction().
> 
> While at it rename send_sigqueue() to posixtimer_send_sigqueue() so its
> clear what this is about.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

In preparation for handling ignored posix timer signals correctly and
embedding the sigqueue struct into struct k_itimer, hand down a pointer to
the sigqueue struct into posix_timer_deliver_signal() instead of just
having a boolean flag.

No functional change.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V5: New patch
---
 include/linux/posix-timers.h |    5 +++--
 kernel/signal.c              |   24 ++++++++++++------------
 kernel/time/posix-timers.c   |    2 +-
 3 files changed, 16 insertions(+), 15 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -110,7 +110,7 @@ static inline void posix_cputimers_rt_wa
 void posixtimer_rearm_itimer(struct task_struct *p);
 bool posixtimer_init_sigqueue(struct sigqueue *q);
 int posixtimer_send_sigqueue(struct k_itimer *tmr);
-bool posixtimer_deliver_signal(struct kernel_siginfo *info);
+bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq);
 void posixtimer_free_timer(struct k_itimer *timer);
 
 /* Init task static initializer */
@@ -135,7 +135,8 @@ static inline void posix_cputimers_init(
 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 bool posixtimer_deliver_signal(struct kernel_siginfo *info) { return false; }
+static inline bool posixtimer_deliver_signal(struct kernel_siginfo *info,
+					     struct sigqueue *timer_sigq) { return false; }
 static inline void posixtimer_free_timer(struct k_itimer *timer) { }
 #endif
 
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -545,7 +545,7 @@ bool unhandled_signal(struct task_struct
 }
 
 static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info,
-			   bool *resched_timer)
+			   struct sigqueue **timer_sigq)
 {
 	struct sigqueue *q, *first = NULL;
 
@@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
 		list_del_init(&first->list);
 		copy_siginfo(info, &first->info);
 
-		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
-				 (info->si_code == SI_TIMER);
-
-		__sigqueue_free(first);
+		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
+			*timer_sigq = first;
+		else
+			__sigqueue_free(first);
 	} else {
 		/*
 		 * Ok, it wasn't in the queue.  This must be
@@ -588,12 +588,12 @@ static void collect_signal(int sig, stru
 }
 
 static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
-			kernel_siginfo_t *info, bool *resched_timer)
+			    kernel_siginfo_t *info, struct sigqueue **timer_sigq)
 {
 	int sig = next_signal(pending, mask);
 
 	if (sig)
-		collect_signal(sig, pending, info, resched_timer);
+		collect_signal(sig, pending, info, timer_sigq);
 	return sig;
 }
 
@@ -604,19 +604,19 @@ static int __dequeue_signal(struct sigpe
  */
 int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
 {
+	struct sigqueue *timer_sigq = NULL;
 	struct task_struct *tsk = current;
-	bool resched_timer = false;
 	int signr;
 
 	lockdep_assert_held(&tsk->sighand->siglock);
 
 again:
 	*type = PIDTYPE_PID;
-	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
+	signr = __dequeue_signal(&tsk->pending, mask, info, &timer_sigq);
 	if (!signr) {
 		*type = PIDTYPE_TGID;
 		signr = __dequeue_signal(&tsk->signal->shared_pending,
-					 mask, info, &resched_timer);
+					 mask, info, &timer_sigq);
 
 		if (unlikely(signr == SIGALRM))
 			posixtimer_rearm_itimer(tsk);
@@ -642,8 +642,8 @@ int dequeue_signal(sigset_t *mask, kerne
 		current->jobctl |= JOBCTL_STOP_DEQUEUED;
 	}
 
-	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(resched_timer)) {
-		if (!posixtimer_deliver_signal(info))
+	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(timer_sigq)) {
+		if (!posixtimer_deliver_signal(info, timer_sigq))
 			goto again;
 	}
 
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -253,7 +253,7 @@ static void common_hrtimer_rearm(struct
  * This function is called from the signal delivery code. It decides
  * whether the signal should be dropped and rearms interval timers.
  */
-bool posixtimer_deliver_signal(struct kernel_siginfo *info)
+bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq)
 {
 	struct k_itimer *timr;
 	unsigned long flags;

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:21AM +0200, Thomas Gleixner a écrit :
> In preparation for handling ignored posix timer signals correctly and
> embedding the sigqueue struct into struct k_itimer, hand down a pointer to
> the sigqueue struct into posix_timer_deliver_signal() instead of just
> having a boolean flag.
> 
> No functional change.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:21AM +0200, Thomas Gleixner a écrit :
> In preparation for handling ignored posix timer signals correctly and
> embedding the sigqueue struct into struct k_itimer, hand down a pointer to
> the sigqueue struct into posix_timer_deliver_signal() instead of just
> having a boolean flag.
> 
> No functional change.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

On second thoughts, a little concern:

> ---
> V5: New patch
> ---
>  include/linux/posix-timers.h |    5 +++--
>  kernel/signal.c              |   24 ++++++++++++------------
>  kernel/time/posix-timers.c   |    2 +-
>  3 files changed, 16 insertions(+), 15 deletions(-)
> 
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -110,7 +110,7 @@ static inline void posix_cputimers_rt_wa
>  void posixtimer_rearm_itimer(struct task_struct *p);
>  bool posixtimer_init_sigqueue(struct sigqueue *q);
>  int posixtimer_send_sigqueue(struct k_itimer *tmr);
> -bool posixtimer_deliver_signal(struct kernel_siginfo *info);
> +bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq);
>  void posixtimer_free_timer(struct k_itimer *timer);
>  
>  /* Init task static initializer */
> @@ -135,7 +135,8 @@ static inline void posix_cputimers_init(
>  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 bool posixtimer_deliver_signal(struct kernel_siginfo *info) { return false; }
> +static inline bool posixtimer_deliver_signal(struct kernel_siginfo *info,
> +					     struct sigqueue *timer_sigq) { return false; }
>  static inline void posixtimer_free_timer(struct k_itimer *timer) { }
>  #endif
>  
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -545,7 +545,7 @@ bool unhandled_signal(struct task_struct
>  }
>  
>  static void collect_signal(int sig, struct sigpending *list, kernel_siginfo_t *info,
> -			   bool *resched_timer)
> +			   struct sigqueue **timer_sigq)
>  {
>  	struct sigqueue *q, *first = NULL;
>  
> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
>  		list_del_init(&first->list);
>  		copy_siginfo(info, &first->info);
>  
> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
> -				 (info->si_code == SI_TIMER);
> -
> -		__sigqueue_free(first);
> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
> +			*timer_sigq = first;
> +		else
> +			__sigqueue_free(first);

So this isn't calling __sigqueue_free() unconditionally anymore. What if
the timer has been freed already, what is going to free the sigqueue?

Thanks.

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

On Tue, Oct 29 2024 at 16:56, Frederic Weisbecker wrote:
>> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
>>  		list_del_init(&first->list);
>>  		copy_siginfo(info, &first->info);
>>  
>> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
>> -				 (info->si_code == SI_TIMER);
>> -
>> -		__sigqueue_free(first);
>> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
>> +			*timer_sigq = first;
>> +		else
>> +			__sigqueue_free(first);
>
> So this isn't calling __sigqueue_free() unconditionally anymore. What if
> the timer has been freed already, what is going to free the sigqueue?

__sigqueue_free() does not free timers marked with SIGQUEUE_PREALLOC.

sigqueue_free() takes care of that, which is invoked from
posixtimer_free_timer(). It clears SIGQUEUE_PREALLOC and if it is queued
it lets it pending and delivery will free it.

That's not any different from before this change.

Though thinking more about it. As we drop the signal in that case
anyway, we could remove it from pending in sigqueue_free() directly. Let
me look into that.

Thanks,

        tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 29, 2024 at 05:22:17PM +0100, Thomas Gleixner a écrit :
> On Tue, Oct 29 2024 at 16:56, Frederic Weisbecker wrote:
> >> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
> >>  		list_del_init(&first->list);
> >>  		copy_siginfo(info, &first->info);
> >>  
> >> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
> >> -				 (info->si_code == SI_TIMER);
> >> -
> >> -		__sigqueue_free(first);
> >> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
> >> +			*timer_sigq = first;
> >> +		else
> >> +			__sigqueue_free(first);
> >
> > So this isn't calling __sigqueue_free() unconditionally anymore. What if
> > the timer has been freed already, what is going to free the sigqueue?
> 
> __sigqueue_free() does not free timers marked with SIGQUEUE_PREALLOC.
> 
> sigqueue_free() takes care of that, which is invoked from
> posixtimer_free_timer(). It clears SIGQUEUE_PREALLOC and if it is queued
> it lets it pending and delivery will free it.

But the delivery freeing used to be done with the __sigqueue_free()
above, which doesn't happen anymore, right?

> 
> That's not any different from before this change.
> 
> Though thinking more about it. As we drop the signal in that case
> anyway, we could remove it from pending in sigqueue_free() directly. Let
> me look into that.

Ok.

Thanks.

> 
> Thanks,
> 
>         tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

On Tue, Oct 29 2024 at 17:34, Frederic Weisbecker wrote:

> Le Tue, Oct 29, 2024 at 05:22:17PM +0100, Thomas Gleixner a écrit :
>> On Tue, Oct 29 2024 at 16:56, Frederic Weisbecker wrote:
>> >> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
>> >>  		list_del_init(&first->list);
>> >>  		copy_siginfo(info, &first->info);
>> >>  
>> >> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
>> >> -				 (info->si_code == SI_TIMER);
>> >> -
>> >> -		__sigqueue_free(first);
>> >> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
>> >> +			*timer_sigq = first;
>> >> +		else
>> >> +			__sigqueue_free(first);
>> >
>> > So this isn't calling __sigqueue_free() unconditionally anymore. What if
>> > the timer has been freed already, what is going to free the sigqueue?
>> 
>> __sigqueue_free() does not free timers marked with SIGQUEUE_PREALLOC.
>> 
>> sigqueue_free() takes care of that, which is invoked from
>> posixtimer_free_timer(). It clears SIGQUEUE_PREALLOC and if it is queued
>> it lets it pending and delivery will free it.
>
> But the delivery freeing used to be done with the __sigqueue_free()
> above, which doesn't happen anymore, right?

It still happens because SIGQUEUE_PREALLOC is cleared in sigqueue_free()

__sigqueue_free() has
       if (q->flags & PREALLOC)
       	     return;

So the old code called __sigqueue_free() unconditionally which just
returned. But now we have a condition to that effect already, so why
call into __sigqueue_free() for nothing?

Let me add a comment.

Thanks,

        tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 29, 2024 at 05:55:38PM +0100, Thomas Gleixner a écrit :
> On Tue, Oct 29 2024 at 17:34, Frederic Weisbecker wrote:
> 
> > Le Tue, Oct 29, 2024 at 05:22:17PM +0100, Thomas Gleixner a écrit :
> >> On Tue, Oct 29 2024 at 16:56, Frederic Weisbecker wrote:
> >> >> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
> >> >>  		list_del_init(&first->list);
> >> >>  		copy_siginfo(info, &first->info);
> >> >>  
> >> >> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
> >> >> -				 (info->si_code == SI_TIMER);
> >> >> -
> >> >> -		__sigqueue_free(first);
> >> >> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
> >> >> +			*timer_sigq = first;
> >> >> +		else
> >> >> +			__sigqueue_free(first);
> >> >
> >> > So this isn't calling __sigqueue_free() unconditionally anymore. What if
> >> > the timer has been freed already, what is going to free the sigqueue?
> >> 
> >> __sigqueue_free() does not free timers marked with SIGQUEUE_PREALLOC.
> >> 
> >> sigqueue_free() takes care of that, which is invoked from
> >> posixtimer_free_timer(). It clears SIGQUEUE_PREALLOC and if it is queued
> >> it lets it pending and delivery will free it.
> >
> > But the delivery freeing used to be done with the __sigqueue_free()
> > above, which doesn't happen anymore, right?
> 
> It still happens because SIGQUEUE_PREALLOC is cleared in sigqueue_free()
> 
> __sigqueue_free() has
>        if (q->flags & PREALLOC)
>        	     return;
> 
> So the old code called __sigqueue_free() unconditionally which just
> returned. But now we have a condition to that effect already, so why
> call into __sigqueue_free() for nothing?

1) Signal is queued
2) Timer is deleted, sigqueue() clears SIGQUEUE_PREALLOC but doesn't go
   further because the sigqueue is queued
3) Signal is collected and delivered but it's not calling __sigqueue_free()
   so the sigqueue is not released.

This is "fixed" on the subsequent patch which uses embedded sigqueue and
rcuref but this patch alone breaks.

Or am I missing something that prevents it?

Thanks.

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

On Tue, Oct 29 2024 at 18:55, Frederic Weisbecker wrote:
> Le Tue, Oct 29, 2024 at 05:55:38PM +0100, Thomas Gleixner a écrit :
>> It still happens because SIGQUEUE_PREALLOC is cleared in sigqueue_free()
>> 
>> __sigqueue_free() has
>>        if (q->flags & PREALLOC)
>>        	     return;
>> 
>> So the old code called __sigqueue_free() unconditionally which just
>> returned. But now we have a condition to that effect already, so why
>> call into __sigqueue_free() for nothing?
>
> 1) Signal is queued
> 2) Timer is deleted, sigqueue() clears SIGQUEUE_PREALLOC but doesn't go
>    further because the sigqueue is queued
> 3) Signal is collected and delivered but it's not calling __sigqueue_free()
>    so the sigqueue is not released.
>
> This is "fixed" on the subsequent patch which uses embedded sigqueue and
> rcuref but this patch alone breaks.
>
> Or am I missing something that prevents it?

Again:

> 1) Signal is queued
> 2) Timer is deleted, sigqueue() clears SIGQUEUE_PREALLOC but doesn't go
>    further because the sigqueue is queued

  3)
  
void collect_signal(..)

     if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
	*timer_sigq = first;    // Path NOT taken because SIGQUEUE_PREALLOC is not set
     else
     	__sigqueue_free(first); // Path taken and frees it

No?

Thanks,

        tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 29, 2024 at 08:16:27PM +0100, Thomas Gleixner a écrit :
> On Tue, Oct 29 2024 at 18:55, Frederic Weisbecker wrote:
> > Le Tue, Oct 29, 2024 at 05:55:38PM +0100, Thomas Gleixner a écrit :
> >> It still happens because SIGQUEUE_PREALLOC is cleared in sigqueue_free()
> >> 
> >> __sigqueue_free() has
> >>        if (q->flags & PREALLOC)
> >>        	     return;
> >> 
> >> So the old code called __sigqueue_free() unconditionally which just
> >> returned. But now we have a condition to that effect already, so why
> >> call into __sigqueue_free() for nothing?
> >
> > 1) Signal is queued
> > 2) Timer is deleted, sigqueue() clears SIGQUEUE_PREALLOC but doesn't go
> >    further because the sigqueue is queued
> > 3) Signal is collected and delivered but it's not calling __sigqueue_free()
> >    so the sigqueue is not released.
> >
> > This is "fixed" on the subsequent patch which uses embedded sigqueue and
> > rcuref but this patch alone breaks.
> >
> > Or am I missing something that prevents it?
> 
> Again:
> 
> > 1) Signal is queued
> > 2) Timer is deleted, sigqueue() clears SIGQUEUE_PREALLOC but doesn't go
> >    further because the sigqueue is queued
> 
>   3)
>   
> void collect_signal(..)
> 
>      if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
> 	*timer_sigq = first;    // Path NOT taken because SIGQUEUE_PREALLOC is not set
>      else
>      	__sigqueue_free(first); // Path taken and frees it
> 
> No?

Duh! Yes of course...

Thanks.

> 
> Thanks,
> 
>         tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

On Tue, Oct 29 2024 at 17:22, Thomas Gleixner wrote:
> On Tue, Oct 29 2024 at 16:56, Frederic Weisbecker wrote:
>>> @@ -568,10 +568,10 @@ static void collect_signal(int sig, stru
>>>  		list_del_init(&first->list);
>>>  		copy_siginfo(info, &first->info);
>>>  
>>> -		*resched_timer = (first->flags & SIGQUEUE_PREALLOC) &&
>>> -				 (info->si_code == SI_TIMER);
>>> -
>>> -		__sigqueue_free(first);
>>> +		if (unlikely((first->flags & SIGQUEUE_PREALLOC) && (info->si_code == SI_TIMER)))
>>> +			*timer_sigq = first;
>>> +		else
>>> +			__sigqueue_free(first);
>>
>> So this isn't calling __sigqueue_free() unconditionally anymore. What if
>> the timer has been freed already, what is going to free the sigqueue?
>
> __sigqueue_free() does not free timers marked with SIGQUEUE_PREALLOC.
>
> sigqueue_free() takes care of that, which is invoked from
> posixtimer_free_timer(). It clears SIGQUEUE_PREALLOC and if it is queued
> it lets it pending and delivery will free it.
>
> That's not any different from before this change.
>
> Though thinking more about it. As we drop the signal in that case
> anyway, we could remove it from pending in sigqueue_free() directly. Let
> me look into that.

Hrm. That requires to do partially what collect_signal() does to keep
the sigset correct. I'm not sure it's worth the trouble.

With the subsequent changes which embed the sigqueue into the timer
itself, the current flow is simple as it has just one place where it
drops the reference count which was acquired when queueing the timer,
i.e. in the delivery path.

Thanks,

        tglx

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:21AM +0200, Thomas Gleixner a écrit :
> @@ -604,19 +604,19 @@ static int __dequeue_signal(struct sigpe
>   */
>  int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
>  {
> +	struct sigqueue *timer_sigq = NULL;
>  	struct task_struct *tsk = current;
> -	bool resched_timer = false;
>  	int signr;
>  
>  	lockdep_assert_held(&tsk->sighand->siglock);
>  
>  again:
>  	*type = PIDTYPE_PID;
> -	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
> +	signr = __dequeue_signal(&tsk->pending, mask, info, &timer_sigq);
>  	if (!signr) {
>  		*type = PIDTYPE_TGID;
>  		signr = __dequeue_signal(&tsk->signal->shared_pending,
> -					 mask, info, &resched_timer);
> +					 mask, info, &timer_sigq);
>  
>  		if (unlikely(signr == SIGALRM))
>  			posixtimer_rearm_itimer(tsk);
> @@ -642,8 +642,8 @@ int dequeue_signal(sigset_t *mask, kerne
>  		current->jobctl |= JOBCTL_STOP_DEQUEUED;
>  	}
>  
> -	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(resched_timer)) {
> -		if (!posixtimer_deliver_signal(info))
> +	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(timer_sigq)) {
> +		if (!posixtimer_deliver_signal(info, timer_sigq))
>  			goto again;

If the signal has been refused, it goes goto again without clearing timer_sigq.
With the upcoming patch it becomes bad because if there is another signal
dequeued on the next iteration, posixtimer_deliver_signal() is called again with
two side effects:

_ The reference is put twice
_ The other dequeued signal is now ignored.

Thanks.

Re: [patch V5 16/26] signal: Replace resched_timer logic

[Thomas Gleixner]

On Tue, Oct 29 2024 at 17:04, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:21AM +0200, Thomas Gleixner a écrit :
>> @@ -604,19 +604,19 @@ static int __dequeue_signal(struct sigpe
>>   */
>>  int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
>>  {
>> +	struct sigqueue *timer_sigq = NULL;
>>  	struct task_struct *tsk = current;
>> -	bool resched_timer = false;
>>  	int signr;
>>  
>>  	lockdep_assert_held(&tsk->sighand->siglock);
>>  
>>  again:
>>  	*type = PIDTYPE_PID;
>> -	signr = __dequeue_signal(&tsk->pending, mask, info, &resched_timer);
>> +	signr = __dequeue_signal(&tsk->pending, mask, info, &timer_sigq);
>>  	if (!signr) {
>>  		*type = PIDTYPE_TGID;
>>  		signr = __dequeue_signal(&tsk->signal->shared_pending,
>> -					 mask, info, &resched_timer);
>> +					 mask, info, &timer_sigq);
>>  
>>  		if (unlikely(signr == SIGALRM))
>>  			posixtimer_rearm_itimer(tsk);
>> @@ -642,8 +642,8 @@ int dequeue_signal(sigset_t *mask, kerne
>>  		current->jobctl |= JOBCTL_STOP_DEQUEUED;
>>  	}
>>  
>> -	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(resched_timer)) {
>> -		if (!posixtimer_deliver_signal(info))
>> +	if (IS_ENABLED(CONFIG_POSIX_TIMERS) && unlikely(timer_sigq)) {
>> +		if (!posixtimer_deliver_signal(info, timer_sigq))
>>  			goto again;
>
> If the signal has been refused, it goes goto again without clearing
> timer_sigq.

That's right. timer_sigq needs to be set to NULL after again:

Good catch!

Thanks,

        tglx

[patch V5 17/26] posix-timers: Embed sigqueue in struct k_itimer

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To cure the SIG_IGN handling for posix interval timers, the preallocated
sigqueue needs to be embedded into struct k_itimer to prevent life time
races of all sorts.

Now that the prerequisites are in place, embed the sigqueue into struct
k_itimer and fixup the relevant usage sites.

Aside of preparing for proper SIG_IGN handling, this spares an extra
allocation.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 fs/proc/base.c               |    4 +--
 include/linux/posix-timers.h |   23 +++++++++++++++--
 kernel/signal.c              |    9 +++++-
 kernel/time/posix-timers.c   |   57 ++++++++++++++++++++++++++-----------------
 4 files changed, 65 insertions(+), 28 deletions(-)
---
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2553,8 +2553,8 @@ static int show_timer(struct seq_file *m
 
 	seq_printf(m, "ID: %d\n", timer->it_id);
 	seq_printf(m, "signal: %d/%px\n",
-		   timer->sigq->info.si_signo,
-		   timer->sigq->info.si_value.sival_ptr);
+		   timer->sigq.info.si_signo,
+		   timer->sigq.info.si_value.sival_ptr);
 	seq_printf(m, "notify: %s/%s.%d\n",
 		   nstr[notify & ~SIGEV_THREAD_ID],
 		   (notify & SIGEV_THREAD_ID) ? "tid" : "pid",
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -39,6 +39,8 @@ static inline int clockid_to_fd(const cl
 
 #ifdef CONFIG_POSIX_TIMERS
 
+#include <linux/signal_types.h>
+
 /**
  * cpu_timer - Posix CPU timer representation for k_itimer
  * @node:	timerqueue node to queue in the task/sig
@@ -166,7 +168,7 @@ static inline void posix_cputimers_init_
  * @it_pid:		The pid of the process/task targeted by the signal
  * @it_process:		The task to wakeup on clock_nanosleep (CPU timers)
  * @rcuref:		Reference count for life time management
- * @sigq:		Pointer to preallocated sigqueue
+ * @sigq:		Embedded sigqueue
  * @it:			Union representing the various posix timer type
  *			internals.
  * @rcu:		RCU head for freeing the timer.
@@ -190,7 +192,7 @@ struct k_itimer {
 		struct pid		*it_pid;
 		struct task_struct	*it_process;
 	};
-	struct sigqueue		*sigq;
+	struct sigqueue		sigq;
 	rcuref_t		rcuref;
 	union {
 		struct {
@@ -218,6 +220,23 @@ static inline void posixtimer_putref(str
 	if (rcuref_put(&tmr->rcuref))
 		posixtimer_free_timer(tmr);
 }
+
+static inline void posixtimer_sigqueue_getref(struct sigqueue *q)
+{
+	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
+
+	WARN_ON_ONCE(!rcuref_get(&tmr->rcuref));
+}
+
+static inline void posixtimer_sigqueue_putref(struct sigqueue *q)
+{
+	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
+
+	posixtimer_putref(tmr);
+}
+#else  /* CONFIG_POSIX_TIMERS */
+static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { }
+static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { }
 #endif /* !CONFIG_POSIX_TIMERS */
 
 #endif
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -460,8 +460,10 @@ static struct sigqueue *__sigqueue_alloc
 
 static void __sigqueue_free(struct sigqueue *q)
 {
-	if (q->flags & SIGQUEUE_PREALLOC)
+	if (q->flags & SIGQUEUE_PREALLOC) {
+		posixtimer_sigqueue_putref(q);
 		return;
+	}
 	if (q->ucounts) {
 		dec_rlimit_put_ucounts(q->ucounts, UCOUNT_RLIMIT_SIGPENDING);
 		q->ucounts = NULL;
@@ -1981,7 +1983,7 @@ static inline struct task_struct *posixt
 
 int posixtimer_send_sigqueue(struct k_itimer *tmr)
 {
-	struct sigqueue *q = tmr->sigq;
+	struct sigqueue *q = &tmr->sigq;
 	int sig = q->info.si_signo;
 	struct task_struct *t;
 	unsigned long flags;
@@ -2040,9 +2042,12 @@ int posixtimer_send_sigqueue(struct k_it
 
 	ret = 0;
 	if (unlikely(!list_empty(&q->list))) {
+		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
 		goto out;
 	}
+
+	posixtimer_sigqueue_getref(q);
 	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
 	result = TRACE_SIGNAL_DELIVERED;
 out:
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -251,12 +251,13 @@ static void common_hrtimer_rearm(struct
 
 /*
  * This function is called from the signal delivery code. It decides
- * whether the signal should be dropped and rearms interval timers.
+ * whether the signal should be dropped and rearms interval timers.  The
+ * timer can be unconditionally accessed as there is a reference held on
+ * it.
  */
 bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq)
 {
-	struct k_itimer *timr;
-	unsigned long flags;
+	struct k_itimer *timr = container_of(timer_sigq, struct k_itimer, sigq);
 	bool ret = false;
 
 	/*
@@ -264,12 +265,14 @@ bool posixtimer_deliver_signal(struct ke
 	 * timr::it_lock. Keep interrupts disabled.
 	 */
 	spin_unlock(&current->sighand->siglock);
+	spin_lock(&timr->it_lock);
 
-	timr = lock_timer(info->si_tid, &flags);
-	if (!timr)
-		goto out;
-
-	if (timr->it_signal_seq != info->si_sys_private)
+	/*
+	 * Check if the timer is still alive or whether it got modified
+	 * since the signal was queued. In either case, don't rearm and
+	 * drop the signal.
+	 */
+	if (!timr->it_signal || timr->it_signal_seq != info->si_sys_private)
 		goto out_unlock;
 
 	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
@@ -285,8 +288,10 @@ bool posixtimer_deliver_signal(struct ke
 	ret = true;
 
 out_unlock:
-	unlock_timer(timr, flags);
-out:
+	spin_unlock(&timr->it_lock);
+	/* Drop the reference which was acquired when the signal was queued */
+	posixtimer_putref(timr);
+
 	spin_lock(&current->sighand->siglock);
 
 	/* Don't expose the si_sys_private value to userspace */
@@ -404,17 +409,17 @@ static struct pid *good_sigevent(sigeven
 	}
 }
 
-static struct k_itimer * alloc_posix_timer(void)
+static struct k_itimer *alloc_posix_timer(void)
 {
 	struct k_itimer *tmr = kmem_cache_zalloc(posix_timers_cache, GFP_KERNEL);
 
 	if (!tmr)
 		return tmr;
-	if (unlikely(!(tmr->sigq = sigqueue_alloc()))) {
+
+	if (unlikely(!posixtimer_init_sigqueue(&tmr->sigq))) {
 		kmem_cache_free(posix_timers_cache, tmr);
 		return NULL;
 	}
-	clear_siginfo(&tmr->sigq->info);
 	rcuref_init(&tmr->rcuref, 1);
 	return tmr;
 }
@@ -429,7 +434,8 @@ static void k_itimer_rcu_free(struct rcu
 void posixtimer_free_timer(struct k_itimer *tmr)
 {
 	put_pid(tmr->it_pid);
-	sigqueue_free(tmr->sigq);
+	if (tmr->sigq.ucounts)
+		dec_rlimit_put_ucounts(tmr->sigq.ucounts, UCOUNT_RLIMIT_SIGPENDING);
 	call_rcu(&tmr->rcu, k_itimer_rcu_free);
 }
 
@@ -491,13 +497,13 @@ static int do_timer_create(clockid_t whi
 			goto out;
 		}
 		new_timer->it_sigev_notify     = event->sigev_notify;
-		new_timer->sigq->info.si_signo = event->sigev_signo;
-		new_timer->sigq->info.si_value = event->sigev_value;
+		new_timer->sigq.info.si_signo = event->sigev_signo;
+		new_timer->sigq.info.si_value = event->sigev_value;
 	} else {
 		new_timer->it_sigev_notify     = SIGEV_SIGNAL;
-		new_timer->sigq->info.si_signo = SIGALRM;
-		memset(&new_timer->sigq->info.si_value, 0, sizeof(sigval_t));
-		new_timer->sigq->info.si_value.sival_int = new_timer->it_id;
+		new_timer->sigq.info.si_signo = SIGALRM;
+		memset(&new_timer->sigq.info.si_value, 0, sizeof(sigval_t));
+		new_timer->sigq.info.si_value.sival_int = new_timer->it_id;
 		new_timer->it_pid = get_pid(task_tgid(current));
 	}
 
@@ -506,8 +512,8 @@ static int do_timer_create(clockid_t whi
 	else
 		new_timer->it_pid_type = PIDTYPE_TGID;
 
-	new_timer->sigq->info.si_tid   = new_timer->it_id;
-	new_timer->sigq->info.si_code  = SI_TIMER;
+	new_timer->sigq.info.si_tid = new_timer->it_id;
+	new_timer->sigq.info.si_code = SI_TIMER;
 
 	if (copy_to_user(created_timer_id, &new_timer_id, sizeof (new_timer_id))) {
 		error = -EFAULT;
@@ -591,7 +597,14 @@ static struct k_itimer *__lock_timer(tim
 	 *  1) Set timr::it_signal to NULL with timr::it_lock held
 	 *  2) Release timr::it_lock
 	 *  3) Remove from the hash under hash_lock
-	 *  4) Call RCU for removal after the grace period
+	 *  4) Put the reference count.
+	 *
+	 * The reference count might not drop to zero if timr::sigq is
+	 * queued. In that case the signal delivery or flush will put the
+	 * last reference count.
+	 *
+	 * When the reference count reaches zero, the timer is scheduled
+	 * for RCU removal after the grace period.
 	 *
 	 * Holding rcu_read_lock() accross the lookup ensures that
 	 * the timer cannot be freed.

Re: [patch V5 17/26] posix-timers: Embed sigqueue in struct k_itimer

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:23AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To cure the SIG_IGN handling for posix interval timers, the preallocated
> sigqueue needs to be embedded into struct k_itimer to prevent life time
> races of all sorts.
> 
> Now that the prerequisites are in place, embed the sigqueue into struct
> k_itimer and fixup the relevant usage sites.
> 
> Aside of preparing for proper SIG_IGN handling, this spares an extra
> allocation.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

Re: [patch V5 17/26] posix-timers: Embed sigqueue in struct k_itimer

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:23AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To cure the SIG_IGN handling for posix interval timers, the preallocated
> sigqueue needs to be embedded into struct k_itimer to prevent life time
> races of all sorts.
> 
> Now that the prerequisites are in place, embed the sigqueue into struct
> k_itimer and fixup the relevant usage sites.
> 
> Aside of preparing for proper SIG_IGN handling, this spares an extra
> allocation.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  fs/proc/base.c               |    4 +--
>  include/linux/posix-timers.h |   23 +++++++++++++++--
>  kernel/signal.c              |    9 +++++-
>  kernel/time/posix-timers.c   |   57 ++++++++++++++++++++++++++-----------------
>  4 files changed, 65 insertions(+), 28 deletions(-)
> ---
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2553,8 +2553,8 @@ static int show_timer(struct seq_file *m
>  
>  	seq_printf(m, "ID: %d\n", timer->it_id);
>  	seq_printf(m, "signal: %d/%px\n",
> -		   timer->sigq->info.si_signo,
> -		   timer->sigq->info.si_value.sival_ptr);
> +		   timer->sigq.info.si_signo,
> +		   timer->sigq.info.si_value.sival_ptr);
>  	seq_printf(m, "notify: %s/%s.%d\n",
>  		   nstr[notify & ~SIGEV_THREAD_ID],
>  		   (notify & SIGEV_THREAD_ID) ? "tid" : "pid",
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -39,6 +39,8 @@ static inline int clockid_to_fd(const cl
>  
>  #ifdef CONFIG_POSIX_TIMERS
>  
> +#include <linux/signal_types.h>
> +
>  /**
>   * cpu_timer - Posix CPU timer representation for k_itimer
>   * @node:	timerqueue node to queue in the task/sig
> @@ -166,7 +168,7 @@ static inline void posix_cputimers_init_
>   * @it_pid:		The pid of the process/task targeted by the signal
>   * @it_process:		The task to wakeup on clock_nanosleep (CPU timers)
>   * @rcuref:		Reference count for life time management
> - * @sigq:		Pointer to preallocated sigqueue
> + * @sigq:		Embedded sigqueue
>   * @it:			Union representing the various posix timer type
>   *			internals.
>   * @rcu:		RCU head for freeing the timer.
> @@ -190,7 +192,7 @@ struct k_itimer {
>  		struct pid		*it_pid;
>  		struct task_struct	*it_process;
>  	};
> -	struct sigqueue		*sigq;
> +	struct sigqueue		sigq;
>  	rcuref_t		rcuref;
>  	union {
>  		struct {
> @@ -218,6 +220,23 @@ static inline void posixtimer_putref(str
>  	if (rcuref_put(&tmr->rcuref))
>  		posixtimer_free_timer(tmr);
>  }
> +
> +static inline void posixtimer_sigqueue_getref(struct sigqueue *q)
> +{
> +	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
> +
> +	WARN_ON_ONCE(!rcuref_get(&tmr->rcuref));
> +}
> +
> +static inline void posixtimer_sigqueue_putref(struct sigqueue *q)
> +{
> +	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
> +
> +	posixtimer_putref(tmr);
> +}
> +#else  /* CONFIG_POSIX_TIMERS */
> +static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { }
> +static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { }
>  #endif /* !CONFIG_POSIX_TIMERS */
>  
>  #endif
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -460,8 +460,10 @@ static struct sigqueue *__sigqueue_alloc
>  
>  static void __sigqueue_free(struct sigqueue *q)
>  {
> -	if (q->flags & SIGQUEUE_PREALLOC)
> +	if (q->flags & SIGQUEUE_PREALLOC) {
> +		posixtimer_sigqueue_putref(q);
>  		return;
> +	}
>  	if (q->ucounts) {
>  		dec_rlimit_put_ucounts(q->ucounts, UCOUNT_RLIMIT_SIGPENDING);
>  		q->ucounts = NULL;
> @@ -1981,7 +1983,7 @@ static inline struct task_struct *posixt
>  
>  int posixtimer_send_sigqueue(struct k_itimer *tmr)
>  {
> -	struct sigqueue *q = tmr->sigq;
> +	struct sigqueue *q = &tmr->sigq;
>  	int sig = q->info.si_signo;
>  	struct task_struct *t;
>  	unsigned long flags;
> @@ -2040,9 +2042,12 @@ int posixtimer_send_sigqueue(struct k_it
>  
>  	ret = 0;
>  	if (unlikely(!list_empty(&q->list))) {
> +		/* This holds a reference count already */
>  		result = TRACE_SIGNAL_ALREADY_PENDING;
>  		goto out;
>  	}
> +
> +	posixtimer_sigqueue_getref(q);
>  	posixtimer_queue_sigqueue(q, t, tmr->it_pid_type);
>  	result = TRACE_SIGNAL_DELIVERED;
>  out:
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -251,12 +251,13 @@ static void common_hrtimer_rearm(struct
>  
>  /*
>   * This function is called from the signal delivery code. It decides
> - * whether the signal should be dropped and rearms interval timers.
> + * whether the signal should be dropped and rearms interval timers.  The
> + * timer can be unconditionally accessed as there is a reference held on
> + * it.
>   */
>  bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq)
>  {
> -	struct k_itimer *timr;
> -	unsigned long flags;
> +	struct k_itimer *timr = container_of(timer_sigq, struct k_itimer, sigq);
>  	bool ret = false;
>  
>  	/*
> @@ -264,12 +265,14 @@ bool posixtimer_deliver_signal(struct ke
>  	 * timr::it_lock. Keep interrupts disabled.
>  	 */
>  	spin_unlock(&current->sighand->siglock);
> +	spin_lock(&timr->it_lock);
>  
> -	timr = lock_timer(info->si_tid, &flags);
> -	if (!timr)
> -		goto out;
> -
> -	if (timr->it_signal_seq != info->si_sys_private)
> +	/*
> +	 * Check if the timer is still alive or whether it got modified
> +	 * since the signal was queued. In either case, don't rearm and
> +	 * drop the signal.
> +	 */
> +	if (!timr->it_signal || timr->it_signal_seq != info->si_sys_private)
>  		goto out_unlock;

It could be:

if (timr->it_signal_seq != info->si_sys_private ||
    WARN_ON_ONCE(!timr->it_signal))
   goto out_unlock;

Because if the timer has been deleted, the current seq should be different from
the queued seq.

Thanks.

>  
>  	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
> @@ -285,8 +288,10 @@ bool posixtimer_deliver_signal(struct ke
>  	ret = true;
>  
>  out_unlock:
> -	unlock_timer(timr, flags);
> -out:
> +	spin_unlock(&timr->it_lock);
> +	/* Drop the reference which was acquired when the signal was queued */
> +	posixtimer_putref(timr);
> +
>  	spin_lock(&current->sighand->siglock);
>  
>  	/* Don't expose the si_sys_private value to userspace */
> @@ -404,17 +409,17 @@ static struct pid *good_sigevent(sigeven
>  	}
>  }
>  
> -static struct k_itimer * alloc_posix_timer(void)
> +static struct k_itimer *alloc_posix_timer(void)
>  {
>  	struct k_itimer *tmr = kmem_cache_zalloc(posix_timers_cache, GFP_KERNEL);
>  
>  	if (!tmr)
>  		return tmr;
> -	if (unlikely(!(tmr->sigq = sigqueue_alloc()))) {
> +
> +	if (unlikely(!posixtimer_init_sigqueue(&tmr->sigq))) {
>  		kmem_cache_free(posix_timers_cache, tmr);
>  		return NULL;
>  	}
> -	clear_siginfo(&tmr->sigq->info);
>  	rcuref_init(&tmr->rcuref, 1);
>  	return tmr;
>  }
> @@ -429,7 +434,8 @@ static void k_itimer_rcu_free(struct rcu
>  void posixtimer_free_timer(struct k_itimer *tmr)
>  {
>  	put_pid(tmr->it_pid);
> -	sigqueue_free(tmr->sigq);
> +	if (tmr->sigq.ucounts)
> +		dec_rlimit_put_ucounts(tmr->sigq.ucounts, UCOUNT_RLIMIT_SIGPENDING);
>  	call_rcu(&tmr->rcu, k_itimer_rcu_free);
>  }
>  
> @@ -491,13 +497,13 @@ static int do_timer_create(clockid_t whi
>  			goto out;
>  		}
>  		new_timer->it_sigev_notify     = event->sigev_notify;
> -		new_timer->sigq->info.si_signo = event->sigev_signo;
> -		new_timer->sigq->info.si_value = event->sigev_value;
> +		new_timer->sigq.info.si_signo = event->sigev_signo;
> +		new_timer->sigq.info.si_value = event->sigev_value;
>  	} else {
>  		new_timer->it_sigev_notify     = SIGEV_SIGNAL;
> -		new_timer->sigq->info.si_signo = SIGALRM;
> -		memset(&new_timer->sigq->info.si_value, 0, sizeof(sigval_t));
> -		new_timer->sigq->info.si_value.sival_int = new_timer->it_id;
> +		new_timer->sigq.info.si_signo = SIGALRM;
> +		memset(&new_timer->sigq.info.si_value, 0, sizeof(sigval_t));
> +		new_timer->sigq.info.si_value.sival_int = new_timer->it_id;
>  		new_timer->it_pid = get_pid(task_tgid(current));
>  	}
>  
> @@ -506,8 +512,8 @@ static int do_timer_create(clockid_t whi
>  	else
>  		new_timer->it_pid_type = PIDTYPE_TGID;
>  
> -	new_timer->sigq->info.si_tid   = new_timer->it_id;
> -	new_timer->sigq->info.si_code  = SI_TIMER;
> +	new_timer->sigq.info.si_tid = new_timer->it_id;
> +	new_timer->sigq.info.si_code = SI_TIMER;
>  
>  	if (copy_to_user(created_timer_id, &new_timer_id, sizeof (new_timer_id))) {
>  		error = -EFAULT;
> @@ -591,7 +597,14 @@ static struct k_itimer *__lock_timer(tim
>  	 *  1) Set timr::it_signal to NULL with timr::it_lock held
>  	 *  2) Release timr::it_lock
>  	 *  3) Remove from the hash under hash_lock
> -	 *  4) Call RCU for removal after the grace period
> +	 *  4) Put the reference count.
> +	 *
> +	 * The reference count might not drop to zero if timr::sigq is
> +	 * queued. In that case the signal delivery or flush will put the
> +	 * last reference count.
> +	 *
> +	 * When the reference count reaches zero, the timer is scheduled
> +	 * for RCU removal after the grace period.
>  	 *
>  	 * Holding rcu_read_lock() accross the lookup ensures that
>  	 * the timer cannot be freed.
> 

Re: [patch V5 17/26] posix-timers: Embed sigqueue in struct k_itimer

[Thomas Gleixner]

On Wed, Oct 30 2024 at 14:55, Frederic Weisbecker wrote:
> Le Tue, Oct 01, 2024 at 10:42:23AM +0200, Thomas Gleixner a écrit :
>> -	if (timr->it_signal_seq != info->si_sys_private)
>> +	/*
>> +	 * Check if the timer is still alive or whether it got modified
>> +	 * since the signal was queued. In either case, don't rearm and
>> +	 * drop the signal.
>> +	 */
>> +	if (!timr->it_signal || timr->it_signal_seq != info->si_sys_private)
>>  		goto out_unlock;
>
> It could be:
>
> if (timr->it_signal_seq != info->si_sys_private ||
>     WARN_ON_ONCE(!timr->it_signal))
>    goto out_unlock;
>
> Because if the timer has been deleted, the current seq should be different from
> the queued seq.

Indeed

[patch V5 18/26] signal: Cleanup unused posix-timer leftovers

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Remove the leftovers of sigqueue preallocation as it's not longer used.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched/signal.h |    2 --
 kernel/signal.c              |   39 ++++-----------------------------------
 2 files changed, 4 insertions(+), 37 deletions(-)
---
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -338,8 +338,6 @@ extern void force_fatal_sig(int);
 extern void force_exit_sig(int);
 extern int send_sig(int, struct task_struct *, int);
 extern int zap_other_threads(struct task_struct *p);
-extern struct sigqueue *sigqueue_alloc(void);
-extern void sigqueue_free(struct sigqueue *);
 extern int do_sigaction(int, struct k_sigaction *, struct k_sigaction *);
 
 static inline void clear_notify_signal(void)
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -439,8 +439,8 @@ static void __sigqueue_init(struct sigqu
  * - this may be called without locks if and only if t == current, otherwise an
  *   appropriate lock must be held to stop the target task from exiting
  */
-static struct sigqueue *__sigqueue_alloc(int sig, struct task_struct *t, gfp_t gfp_flags,
-					 int override_rlimit, const unsigned int sigqueue_flags)
+static struct sigqueue *sigqueue_alloc(int sig, struct task_struct *t, gfp_t gfp_flags,
+				       int override_rlimit)
 {
 	struct ucounts *ucounts = sig_get_ucounts(t, sig, override_rlimit);
 	struct sigqueue *q;
@@ -454,7 +454,7 @@ static struct sigqueue *__sigqueue_alloc
 		return NULL;
 	}
 
-	__sigqueue_init(q, ucounts, sigqueue_flags);
+	__sigqueue_init(q, ucounts, 0);
 	return q;
 }
 
@@ -1062,7 +1062,7 @@ static int __send_signal_locked(int sig,
 	else
 		override_rlimit = 0;
 
-	q = __sigqueue_alloc(sig, t, GFP_ATOMIC, override_rlimit, 0);
+	q = sigqueue_alloc(sig, t, GFP_ATOMIC, override_rlimit);
 
 	if (q) {
 		list_add_tail(&q->list, &pending->list);
@@ -1918,37 +1918,6 @@ bool posixtimer_init_sigqueue(struct sig
 	return true;
 }
 
-struct sigqueue *sigqueue_alloc(void)
-{
-	return __sigqueue_alloc(-1, current, GFP_KERNEL, 0, SIGQUEUE_PREALLOC);
-}
-
-void sigqueue_free(struct sigqueue *q)
-{
-	spinlock_t *lock = &current->sighand->siglock;
-	unsigned long flags;
-
-	if (WARN_ON_ONCE(!(q->flags & SIGQUEUE_PREALLOC)))
-		return;
-	/*
-	 * We must hold ->siglock while testing q->list
-	 * to serialize with collect_signal() or with
-	 * __exit_signal()->flush_sigqueue().
-	 */
-	spin_lock_irqsave(lock, flags);
-	q->flags &= ~SIGQUEUE_PREALLOC;
-	/*
-	 * If it is queued it will be freed when dequeued,
-	 * like the "regular" sigqueue.
-	 */
-	if (!list_empty(&q->list))
-		q = NULL;
-	spin_unlock_irqrestore(lock, flags);
-
-	if (q)
-		__sigqueue_free(q);
-}
-
 static void posixtimer_queue_sigqueue(struct sigqueue *q, struct task_struct *t, enum pid_type type)
 {
 	struct sigpending *pending;

Re: [patch V5 18/26] signal: Cleanup unused posix-timer leftovers

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:24AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Remove the leftovers of sigqueue preallocation as it's not longer used.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 19/26] posix-timers: Move sequence logic into struct k_itimer

[Thomas Gleixner]

The posix timer signal handling uses siginfo::si_sys_private for handling
the sequence counter check. That indirection is not longer required and the
sequence count value at signal queueing time can be stored in struct
k_itimer itself.

This removes the requirement of treating siginfo::si_sys_private special as
it's now always zero as the kernel does not touch it anymore.

Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
V5: New patch
---
 include/linux/posix-timers.h |    2 ++
 kernel/signal.c              |    8 +++-----
 kernel/time/posix-timers.c   |    5 +----
 3 files changed, 6 insertions(+), 9 deletions(-)

--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -162,6 +162,7 @@ static inline void posix_cputimers_init_
  * @it_overrun:		The overrun counter for pending signals
  * @it_overrun_last:	The overrun at the time of the last delivered signal
  * @it_signal_seq:	Sequence count to control signal delivery
+ * @it_sigqueue_seq:	The sequence count at the point where the signal was queued
  * @it_sigev_notify:	The notify word of sigevent struct for signal delivery
  * @it_interval:	The interval for periodic timers
  * @it_signal:		Pointer to the creators signal struct
@@ -184,6 +185,7 @@ struct k_itimer {
 	s64			it_overrun;
 	s64			it_overrun_last;
 	unsigned int		it_signal_seq;
+	unsigned int		it_sigqueue_seq;
 	int			it_sigev_notify;
 	enum pid_type		it_pid_type;
 	ktime_t			it_interval;
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1968,12 +1968,10 @@ int posixtimer_send_sigqueue(struct k_it
 		return -1;
 
 	/*
-	 * Update @q::info::si_sys_private for posix timer signals with
-	 * sighand locked to prevent a race against dequeue_signal() which
-	 * decides based on si_sys_private whether to invoke
-	 * posixtimer_rearm() or not.
+	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
+	 * locked to prevent a race against dequeue_signal().
 	 */
-	q->info.si_sys_private = tmr->it_signal_seq;
+	tmr->it_sigqueue_seq = tmr->it_signal_seq;
 
 	/*
 	 * Set the overrun count to zero unconditionally. The posix timer
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -272,7 +272,7 @@ bool posixtimer_deliver_signal(struct ke
 	 * since the signal was queued. In either case, don't rearm and
 	 * drop the signal.
 	 */
-	if (!timr->it_signal || timr->it_signal_seq != info->si_sys_private)
+	if (!timr->it_signal || timr->it_signal_seq != timr->it_sigqueue_seq)
 		goto out_unlock;
 
 	if (timr->it_interval && timr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
@@ -293,9 +293,6 @@ bool posixtimer_deliver_signal(struct ke
 	posixtimer_putref(timr);
 
 	spin_lock(&current->sighand->siglock);
-
-	/* Don't expose the si_sys_private value to userspace */
-	info->si_sys_private = 0;
 	return ret;
 }
 

Re: [patch V5 19/26] posix-timers: Move sequence logic into struct k_itimer

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:26AM +0200, Thomas Gleixner a écrit :
> The posix timer signal handling uses siginfo::si_sys_private for handling
> the sequence counter check. That indirection is not longer required and the
> sequence count value at signal queueing time can be stored in struct
> k_itimer itself.
> 
> This removes the requirement of treating siginfo::si_sys_private special as
> it's now always zero as the kernel does not touch it anymore.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xmission.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

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

[patch V5 20/26] signal: Provide ignored_posix_timers list

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To prepare for handling posix timer signals on sigaction(SIG_IGN) properly,
add a list to task::signal.

This list will be used to queue posix timers so their signal can be
requeued when SIG_IGN is lifted later.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/sched/signal.h |    1 +
 init/init_task.c             |    5 +++--
 kernel/fork.c                |    1 +
 3 files changed, 5 insertions(+), 2 deletions(-)
---
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -138,6 +138,7 @@ struct signal_struct {
 	/* POSIX.1b Interval Timers */
 	unsigned int		next_posix_timer_id;
 	struct hlist_head	posix_timers;
+	struct hlist_head	ignored_posix_timers;
 
 	/* ITIMER_REAL timer for the process */
 	struct hrtimer real_timer;
--- a/init/init_task.c
+++ b/init/init_task.c
@@ -30,8 +30,9 @@ static struct signal_struct init_signals
 	.cred_guard_mutex = __MUTEX_INITIALIZER(init_signals.cred_guard_mutex),
 	.exec_update_lock = __RWSEM_INITIALIZER(init_signals.exec_update_lock),
 #ifdef CONFIG_POSIX_TIMERS
-	.posix_timers	= HLIST_HEAD_INIT,
-	.cputimer	= {
+	.posix_timers		= HLIST_HEAD_INIT,
+	.ignored_posix_timers	= HLIST_HEAD_INIT,
+	.cputimer		= {
 		.cputime_atomic	= INIT_CPUTIME_ATOMIC,
 	},
 #endif
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1864,6 +1864,7 @@ static int copy_signal(unsigned long clo
 
 #ifdef CONFIG_POSIX_TIMERS
 	INIT_HLIST_HEAD(&sig->posix_timers);
+	INIT_HLIST_HEAD(&sig->ignored_posix_timers);
 	hrtimer_init(&sig->real_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	sig->real_timer.function = it_real_fn;
 #endif

Re: [patch V5 20/26] signal: Provide ignored_posix_timers list

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:27AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To prepare for handling posix timer signals on sigaction(SIG_IGN) properly,
> add a list to task::signal.
> 
> This list will be used to queue posix timers so their signal can be
> requeued when SIG_IGN is lifted later.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[patch V5 21/26] posix-timers: Handle ignored list on delete and exit

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

To handle posix timer signals on sigaction(SIG_IGN) properly, the timers
will be queued on a separate ignored list.

Add the necessary cleanup code for timer_delete() and exit_itimers().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h |    4 +++-
 kernel/time/posix-timers.c   |   20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -152,7 +152,8 @@ static inline void posix_cputimers_init_
 
 /**
  * struct k_itimer - POSIX.1b interval timer structure.
- * @list:		List head for binding the timer to signals->posix_timers
+ * @list:		List node for binding the timer to tsk::signal::posix_timers
+ * @ignored_list:	List node for tracking ignored timers in tsk::signal::ignored_posix_timers
  * @t_hash:		Entry in the posix timer hash table
  * @it_lock:		Lock protecting the timer
  * @kclock:		Pointer to the k_clock struct handling this timer
@@ -176,6 +177,7 @@ static inline void posix_cputimers_init_
  */
 struct k_itimer {
 	struct hlist_node	list;
+	struct hlist_node	ignored_list;
 	struct hlist_node	t_hash;
 	spinlock_t		it_lock;
 	const struct k_clock	*kclock;
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1031,6 +1031,18 @@ int common_timer_del(struct k_itimer *ti
 	return 0;
 }
 
+/*
+ * If the deleted timer is on the ignored list, remove it and
+ * drop the associated reference.
+ */
+static inline void posix_timer_cleanup_ignored(struct k_itimer *tmr)
+{
+	if (!hlist_unhashed(&tmr->ignored_list)) {
+		hlist_del_init(&tmr->ignored_list);
+		posixtimer_putref(tmr);
+	}
+}
+
 static inline int timer_delete_hook(struct k_itimer *timer)
 {
 	const struct k_clock *kc = timer->kclock;
@@ -1063,6 +1075,7 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
 
 	spin_lock(&current->sighand->siglock);
 	hlist_del(&timer->list);
+	posix_timer_cleanup_ignored(timer);
 	spin_unlock(&current->sighand->siglock);
 	/*
 	 * A concurrent lookup could check timer::it_signal lockless. It
@@ -1114,6 +1127,8 @@ static void itimer_delete(struct k_itime
 	}
 	hlist_del(&timer->list);
 
+	posix_timer_cleanup_ignored(timer);
+
 	/*
 	 * Setting timer::it_signal to NULL is technically not required
 	 * here as nothing can access the timer anymore legitimately via
@@ -1146,6 +1161,11 @@ void exit_itimers(struct task_struct *ts
 	/* The timers are not longer accessible via tsk::signal */
 	while (!hlist_empty(&timers))
 		itimer_delete(hlist_entry(timers.first, struct k_itimer, list));
+
+	/* Mop up timers which are on the ignored list */
+	hlist_move_list(&tsk->signal->ignored_posix_timers, &timers);
+	while (!hlist_empty(&timers))
+		posix_timer_cleanup_ignored(hlist_entry(timers.first, struct k_itimer, list));
 }
 
 SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,

Re: [patch V5 21/26] posix-timers: Handle ignored list on delete and exit

[Frederic Weisbecker]

Le Tue, Oct 01, 2024 at 10:42:29AM +0200, Thomas Gleixner a écrit :
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To handle posix timer signals on sigaction(SIG_IGN) properly, the timers
> will be queued on a separate ignored list.
> 
> Add the necessary cleanup code for timer_delete() and exit_itimers().
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/posix-timers.h |    4 +++-
>  kernel/time/posix-timers.c   |   20 ++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
> ---
> --- a/include/linux/posix-timers.h
> +++ b/include/linux/posix-timers.h
> @@ -152,7 +152,8 @@ static inline void posix_cputimers_init_
>  
>  /**
>   * struct k_itimer - POSIX.1b interval timer structure.
> - * @list:		List head for binding the timer to signals->posix_timers
> + * @list:		List node for binding the timer to tsk::signal::posix_timers
> + * @ignored_list:	List node for tracking ignored timers in tsk::signal::ignored_posix_timers
>   * @t_hash:		Entry in the posix timer hash table
>   * @it_lock:		Lock protecting the timer
>   * @kclock:		Pointer to the k_clock struct handling this timer
> @@ -176,6 +177,7 @@ static inline void posix_cputimers_init_
>   */
>  struct k_itimer {
>  	struct hlist_node	list;
> +	struct hlist_node	ignored_list;
>  	struct hlist_node	t_hash;
>  	spinlock_t		it_lock;
>  	const struct k_clock	*kclock;
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -1031,6 +1031,18 @@ int common_timer_del(struct k_itimer *ti
>  	return 0;
>  }
>  
> +/*
> + * If the deleted timer is on the ignored list, remove it and
> + * drop the associated reference.
> + */
> +static inline void posix_timer_cleanup_ignored(struct k_itimer *tmr)
> +{
> +	if (!hlist_unhashed(&tmr->ignored_list)) {
> +		hlist_del_init(&tmr->ignored_list);
> +		posixtimer_putref(tmr);
> +	}
> +}
> +
>  static inline int timer_delete_hook(struct k_itimer *timer)
>  {
>  	const struct k_clock *kc = timer->kclock;
> @@ -1063,6 +1075,7 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
>  
>  	spin_lock(&current->sighand->siglock);
>  	hlist_del(&timer->list);
> +	posix_timer_cleanup_ignored(timer);
>  	spin_unlock(&current->sighand->siglock);
>  	/*
>  	 * A concurrent lookup could check timer::it_signal lockless. It
> @@ -1114,6 +1127,8 @@ static void itimer_delete(struct k_itime
>  	}
>  	hlist_del(&timer->list);
>  
> +	posix_timer_cleanup_ignored(timer);
> +
>  	/*
>  	 * Setting timer::it_signal to NULL is technically not required
>  	 * here as nothing can access the timer anymore legitimately via
> @@ -1146,6 +1161,11 @@ void exit_itimers(struct task_struct *ts
>  	/* The timers are not longer accessible via tsk::signal */
>  	while (!hlist_empty(&timers))
>  		itimer_delete(hlist_entry(timers.first, struct k_itimer, list));
> +
> +	/* Mop up timers which are on the ignored list */
> +	hlist_move_list(&tsk->signal->ignored_posix_timers, &timers);
> +	while (!hlist_empty(&timers))
> +		posix_timer_cleanup_ignored(hlist_entry(timers.first, struct k_itimer, list));

The ignored timers should have all been removed on timer_delete() and
itimer_delete(), right?

So should it be instead:

   WARN_ON_ONCE(!hlist_empty(&tsk->signal->ignored_posix_timers))

?

Thanks.

>  }
>  
>  SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock,
> 

Re: [patch V5 21/26] posix-timers: Handle ignored list on delete and exit

[Thomas Gleixner]

On Wed, Oct 30 2024 at 18:06, Frederic Weisbecker wrote:
>> +
>> +	/* Mop up timers which are on the ignored list */
>> +	hlist_move_list(&tsk->signal->ignored_posix_timers, &timers);
>> +	while (!hlist_empty(&timers))
>> +		posix_timer_cleanup_ignored(hlist_entry(timers.first, struct k_itimer, list));
>
> The ignored timers should have all been removed on timer_delete() and
> itimer_delete(), right?
>
> So should it be instead:
>
>    WARN_ON_ONCE(!hlist_empty(&tsk->signal->ignored_posix_timers))

Except for those which have not been removed by the application on
exit() or in a case where the kernel kills the task. Then the exit
cleanup has to mop up for them :)

Thanks,

        tglx

Re: [patch V5 21/26] posix-timers: Handle ignored list on delete and exit

[Thomas Gleixner]

On Wed, Oct 30 2024 at 18:34, Thomas Gleixner wrote:

> On Wed, Oct 30 2024 at 18:06, Frederic Weisbecker wrote:
>>> +
>>> +	/* Mop up timers which are on the ignored list */
>>> +	hlist_move_list(&tsk->signal->ignored_posix_timers, &timers);
>>> +	while (!hlist_empty(&timers))
>>> +		posix_timer_cleanup_ignored(hlist_entry(timers.first, struct k_itimer, list));
>>
>> The ignored timers should have all been removed on timer_delete() and
>> itimer_delete(), right?
>>
>> So should it be instead:
>>
>>    WARN_ON_ONCE(!hlist_empty(&tsk->signal->ignored_posix_timers))
>
> Except for those which have not been removed by the application on
> exit() or in a case where the kernel kills the task. Then the exit
> cleanup has to mop up for them :)

Bah. I'm an idiot. Of course they are already mopped up...

[patch V5 22/26] signal: Handle ignored signals in do_sigaction(action != SIG_IGN)

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

When a real handler (including SIG_DFL) is installed for a signal, which
had previously SIG_IGN set, then the list of ignored posix timers has to be
checked for timers which are affected by this change.

Add a list walk function which checks for the matching signal number and if
found requeues the timers signal, so the timer is rearmed on signal
delivery.

Rearming the timer right away is not possible because that requires to drop
sighand lock.

No functional change as the counter part which queues the timers on the
ignored list is still missing.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/signal.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2022,7 +2022,54 @@ int posixtimer_send_sigqueue(struct k_it
 	unlock_task_sighand(t, &flags);
 	return ret;
 }
-#endif /* CONFIG_POSIX_TIMERS */
+
+static void posixtimer_sig_unignore(struct task_struct *tsk, int sig)
+{
+	struct hlist_head *head = &tsk->signal->ignored_posix_timers;
+	struct hlist_node *tmp;
+	struct k_itimer *tmr;
+
+	if (likely(hlist_empty(head)))
+		return;
+
+	/*
+	 * Rearming a timer with sighand lock held is not possible due to
+	 * lock ordering vs. tmr::it_lock. Just stick the sigqueue back and
+	 * let the signal delivery path deal with it whether it needs to be
+	 * rearmed or not. This cannot be decided here w/o dropping sighand
+	 * lock and creating a loop retry horror show.
+	 */
+	hlist_for_each_entry_safe(tmr, tmp , head, ignored_list) {
+		struct task_struct *target;
+
+		/*
+		 * tmr::sigq.info.si_signo is immutable, so accessing it
+		 * without holding tmr::it_lock is safe.
+		 */
+		if (tmr->sigq.info.si_signo != sig)
+			continue;
+
+		hlist_del_init(&tmr->ignored_list);
+
+		/* This should never happen and leaks a reference count */
+		if (WARN_ON_ONCE(!list_empty(&tmr->sigq.list)))
+			continue;
+
+		/*
+		 * Get the target for the signal. If target is a thread and
+		 * has exited by now, drop the reference count.
+		 */
+		guard(rcu)();
+		target = posixtimer_get_target(tmr);
+		if (target)
+			posixtimer_queue_sigqueue(&tmr->sigq, target, tmr->it_pid_type);
+		else
+			posixtimer_putref(tmr);
+	}
+}
+#else /* CONFIG_POSIX_TIMERS */
+static inline void posixtimer_sig_unignore(struct task_struct *tsk, int sig) { }
+#endif /* !CONFIG_POSIX_TIMERS */
 
 void do_notify_pidfd(struct task_struct *task)
 {
@@ -4200,6 +4247,8 @@ int do_sigaction(int sig, struct k_sigac
 	sigaction_compat_abi(act, oact);
 
 	if (act) {
+		bool was_ignored = k->sa.sa_handler == SIG_IGN;
+
 		sigdelsetmask(&act->sa.sa_mask,
 			      sigmask(SIGKILL) | sigmask(SIGSTOP));
 		*k = *act;
@@ -4220,6 +4269,8 @@ int do_sigaction(int sig, struct k_sigac
 			flush_sigqueue_mask(p, &mask, &p->signal->shared_pending);
 			for_each_thread(p, t)
 				flush_sigqueue_mask(p, &mask, &t->pending);
+		} else if (was_ignored) {
+			posixtimer_sig_unignore(p, sig);
 		}
 	}
 

[patch V5 23/26] signal: Queue ignored posixtimers on ignore list

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Queue posixtimers which have their signal ignored on the ignored list:

   1) When the timer fires and the signal has SIG_IGN set

   2) When SIG_IGN is installed via sigaction() and a timer signal
      is already queued

This completes the SIG_IGN handling and such timers are not longer self
rearmed which avoids pointless wakeups.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/signal.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -723,6 +723,16 @@ void signal_wake_up_state(struct task_st
 		kick_process(t);
 }
 
+static inline void posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q);
+
+static void sigqueue_free_ignored(struct task_struct *tsk, struct sigqueue *q)
+{
+	if (likely(!(q->flags & SIGQUEUE_PREALLOC) || q->info.si_code != SI_TIMER))
+		__sigqueue_free(q);
+	else
+		posixtimer_sig_ignore(tsk, q);
+}
+
 /* Remove signals in mask from the pending set and queue. */
 static void flush_sigqueue_mask(struct task_struct *p, sigset_t *mask, struct sigpending *s)
 {
@@ -739,7 +749,7 @@ static void flush_sigqueue_mask(struct t
 	list_for_each_entry_safe(q, n, &s->list, list) {
 		if (sigismember(mask, q->info.si_signo)) {
 			list_del_init(&q->list);
-			__sigqueue_free(q);
+			sigqueue_free_ignored(p, q);
 		}
 	}
 }
@@ -1956,7 +1966,7 @@ int posixtimer_send_sigqueue(struct k_it
 	int sig = q->info.si_signo;
 	struct task_struct *t;
 	unsigned long flags;
-	int ret, result;
+	int result;
 
 	guard(rcu)();
 
@@ -2001,13 +2011,24 @@ int posixtimer_send_sigqueue(struct k_it
 	 */
 	q->info.si_overrun = 0;
 
-	ret = 1; /* the signal is ignored */
 	if (!prepare_signal(sig, t, false)) {
 		result = TRACE_SIGNAL_IGNORED;
+
+		/* Paranoia check. Try to survive. */
+		if (WARN_ON_ONCE(!list_empty(&q->list)))
+			goto out;
+
+		if (hlist_unhashed(&tmr->ignored_list)) {
+			hlist_add_head(&tmr->ignored_list, &t->signal->ignored_posix_timers);
+			posixtimer_sigqueue_getref(q);
+		}
 		goto out;
 	}
 
-	ret = 0;
+	/* This should never happen and leaks a reference count */
+	if (WARN_ON_ONCE(!hlist_unhashed(&tmr->ignored_list)))
+		hlist_del_init(&tmr->ignored_list);
+
 	if (unlikely(!list_empty(&q->list))) {
 		/* This holds a reference count already */
 		result = TRACE_SIGNAL_ALREADY_PENDING;
@@ -2020,7 +2041,14 @@ int posixtimer_send_sigqueue(struct k_it
 out:
 	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
 	unlock_task_sighand(t, &flags);
-	return ret;
+	return 0;
+}
+
+static inline void posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q)
+{
+	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
+
+	hlist_add_head(&tmr->ignored_list, &tsk->signal->ignored_posix_timers);
 }
 
 static void posixtimer_sig_unignore(struct task_struct *tsk, int sig)
@@ -2068,6 +2096,7 @@ static void posixtimer_sig_unignore(stru
 	}
 }
 #else /* CONFIG_POSIX_TIMERS */
+static inline void posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q) { }
 static inline void posixtimer_sig_unignore(struct task_struct *tsk, int sig) { }
 #endif /* !CONFIG_POSIX_TIMERS */
 

[patch V5 24/26] posix-timers: Cleanup SIG_IGN workaround leftovers

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Now that ignored posix timer signals are requeued and the timers are
rearmed on signal delivery the workaround to keep such timers alive and
self rearm them is not longer required.

Remove the relevant hacks and the not longer required return values from
the related functions. The alarm timer workarounds will be cleaned up in a
separate step.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/posix-timers.h   |    2 -
 kernel/signal.c                |    7 +---
 kernel/time/alarmtimer.c       |   47 +++++------------------------
 kernel/time/posix-cpu-timers.c |   18 ++---------
 kernel/time/posix-timers.c     |   65 +++--------------------------------------
 kernel/time/posix-timers.h     |    2 -
 6 files changed, 23 insertions(+), 118 deletions(-)
---
--- a/include/linux/posix-timers.h
+++ b/include/linux/posix-timers.h
@@ -111,7 +111,7 @@ static inline void posix_cputimers_rt_wa
 
 void posixtimer_rearm_itimer(struct task_struct *p);
 bool posixtimer_init_sigqueue(struct sigqueue *q);
-int posixtimer_send_sigqueue(struct k_itimer *tmr);
+void posixtimer_send_sigqueue(struct k_itimer *tmr);
 bool posixtimer_deliver_signal(struct kernel_siginfo *info, struct sigqueue *timer_sigq);
 void posixtimer_free_timer(struct k_itimer *timer);
 
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1960,7 +1960,7 @@ static inline struct task_struct *posixt
 	return t;
 }
 
-int posixtimer_send_sigqueue(struct k_itimer *tmr)
+void posixtimer_send_sigqueue(struct k_itimer *tmr)
 {
 	struct sigqueue *q = &tmr->sigq;
 	int sig = q->info.si_signo;
@@ -1972,10 +1972,10 @@ int posixtimer_send_sigqueue(struct k_it
 
 	t = posixtimer_get_target(tmr);
 	if (!t)
-		return -1;
+		return;
 
 	if (!likely(lock_task_sighand(t, &flags)))
-		return -1;
+		return;
 
 	/*
 	 * Update @tmr::sigqueue_seq for posix timer signals with sighand
@@ -2041,7 +2041,6 @@ int posixtimer_send_sigqueue(struct k_it
 out:
 	trace_signal_generate(sig, &q->info, t, tmr->it_pid_type != PIDTYPE_PID, result);
 	unlock_task_sighand(t, &flags);
-	return 0;
 }
 
 static inline void posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q)
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -197,28 +197,15 @@ static enum hrtimer_restart alarmtimer_f
 {
 	struct alarm *alarm = container_of(timer, struct alarm, timer);
 	struct alarm_base *base = &alarm_bases[alarm->type];
-	unsigned long flags;
-	int ret = HRTIMER_NORESTART;
-	int restart = ALARMTIMER_NORESTART;
 
-	spin_lock_irqsave(&base->lock, flags);
-	alarmtimer_dequeue(base, alarm);
-	spin_unlock_irqrestore(&base->lock, flags);
+	scoped_guard (spinlock_irqsave, &base->lock)
+		alarmtimer_dequeue(base, alarm);
 
 	if (alarm->function)
-		restart = alarm->function(alarm, base->get_ktime());
-
-	spin_lock_irqsave(&base->lock, flags);
-	if (restart != ALARMTIMER_NORESTART) {
-		hrtimer_set_expires(&alarm->timer, alarm->node.expires);
-		alarmtimer_enqueue(base, alarm);
-		ret = HRTIMER_RESTART;
-	}
-	spin_unlock_irqrestore(&base->lock, flags);
+		alarm->function(alarm, base->get_ktime());
 
 	trace_alarmtimer_fired(alarm, base->get_ktime());
-	return ret;
-
+	return HRTIMER_NORESTART;
 }
 
 ktime_t alarm_expires_remaining(const struct alarm *alarm)
@@ -567,30 +554,14 @@ static enum alarmtimer_type clock2alarm(
  *
  * Return: whether the timer is to be restarted
  */
-static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm,
-							ktime_t now)
+static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm, ktime_t now)
 {
-	struct k_itimer *ptr = container_of(alarm, struct k_itimer,
-					    it.alarm.alarmtimer);
-	enum alarmtimer_restart result = ALARMTIMER_NORESTART;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ptr->it_lock, flags);
+	struct k_itimer *ptr = container_of(alarm, struct k_itimer, it.alarm.alarmtimer);
 
-	if (posix_timer_queue_signal(ptr) && ptr->it_interval) {
-		/*
-		 * Handle ignored signals and rearm the timer. This will go
-		 * away once we handle ignored signals proper. Ensure that
-		 * small intervals cannot starve the system.
-		 */
-		ptr->it_overrun += __alarm_forward_now(alarm, ptr->it_interval, true);
-		++ptr->it_signal_seq;
-		ptr->it_status = POSIX_TIMER_ARMED;
-		result = ALARMTIMER_RESTART;
-	}
-	spin_unlock_irqrestore(&ptr->it_lock, flags);
+	guard(spinlock_irqsave)(&ptr->it_lock);
+	posix_timer_queue_signal(ptr);
 
-	return result;
+	return ALARMTIMER_NORESTART;
 }
 
 /**
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -595,21 +595,11 @@ static void cpu_timer_fire(struct k_itim
 		 */
 		wake_up_process(timer->it_process);
 		cpu_timer_setexpires(ctmr, 0);
-	} else if (!timer->it_interval) {
-		/*
-		 * One-shot timer.  Clear it as soon as it's fired.
-		 */
+	} else {
 		posix_timer_queue_signal(timer);
-		cpu_timer_setexpires(ctmr, 0);
-	} else if (posix_timer_queue_signal(timer)) {
-		/*
-		 * The signal did not get queued because the signal
-		 * was ignored, so we won't get any callback to
-		 * reload the timer.  But we need to keep it
-		 * ticking in case the signal is deliverable next time.
-		 */
-		posix_cpu_timer_rearm(timer);
-		++timer->it_signal_seq;
+		/* Disable oneshot timers */
+		if (!timer->it_interval)
+			cpu_timer_setexpires(ctmr, 0);
 	}
 }
 
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -296,10 +296,9 @@ bool posixtimer_deliver_signal(struct ke
 	return ret;
 }
 
-int posix_timer_queue_signal(struct k_itimer *timr)
+void posix_timer_queue_signal(struct k_itimer *timr)
 {
 	enum posix_timer_state state = POSIX_TIMER_DISARMED;
-	int ret;
 
 	lockdep_assert_held(&timr->it_lock);
 
@@ -309,9 +308,7 @@ int posix_timer_queue_signal(struct k_it
 	}
 	timr->it_status = state;
 
-	ret = posixtimer_send_sigqueue(timr);
-	/* If we failed to send the signal the timer stops. */
-	return ret > 0;
+	posixtimer_send_sigqueue(timr);
 }
 
 /*
@@ -324,62 +321,10 @@ int posix_timer_queue_signal(struct k_it
 static enum hrtimer_restart posix_timer_fn(struct hrtimer *timer)
 {
 	struct k_itimer *timr = container_of(timer, struct k_itimer, it.real.timer);
-	enum hrtimer_restart ret = HRTIMER_NORESTART;
-	unsigned long flags;
 
-	spin_lock_irqsave(&timr->it_lock, flags);
-
-	if (posix_timer_queue_signal(timr)) {
-		/*
-		 * The signal was not queued due to SIG_IGN. As a
-		 * consequence the timer is not going to be rearmed from
-		 * the signal delivery path. But as a real signal handler
-		 * can be installed later the timer must be rearmed here.
-		 */
-		if (timr->it_interval != 0) {
-			ktime_t now = hrtimer_cb_get_time(timer);
-
-			/*
-			 * FIXME: What we really want, is to stop this
-			 * timer completely and restart it in case the
-			 * SIG_IGN is removed. This is a non trivial
-			 * change to the signal handling code.
-			 *
-			 * For now let timers with an interval less than a
-			 * jiffy expire every jiffy and recheck for a
-			 * valid signal handler.
-			 *
-			 * This avoids interrupt starvation in case of a
-			 * very small interval, which would expire the
-			 * timer immediately again.
-			 *
-			 * Moving now ahead of time by one jiffy tricks
-			 * hrtimer_forward() to expire the timer later,
-			 * while it still maintains the overrun accuracy
-			 * for the price of a slight inconsistency in the
-			 * timer_gettime() case. This is at least better
-			 * than a timer storm.
-			 *
-			 * Only required when high resolution timers are
-			 * enabled as the periodic tick based timers are
-			 * automatically aligned to the next tick.
-			 */
-			if (IS_ENABLED(CONFIG_HIGH_RES_TIMERS)) {
-				ktime_t kj = TICK_NSEC;
-
-				if (timr->it_interval < kj)
-					now = ktime_add(now, kj);
-			}
-
-			timr->it_overrun += hrtimer_forward(timer, now, timr->it_interval);
-			ret = HRTIMER_RESTART;
-			++timr->it_signal_seq;
-			timr->it_status = POSIX_TIMER_ARMED;
-		}
-	}
-
-	unlock_timer(timr, flags);
-	return ret;
+	guard(spinlock_irqsave)(&timr->it_lock);
+	posix_timer_queue_signal(timr);
+	return HRTIMER_NORESTART;
 }
 
 static struct pid *good_sigevent(sigevent_t * event)
--- a/kernel/time/posix-timers.h
+++ b/kernel/time/posix-timers.h
@@ -42,7 +42,7 @@ extern const struct k_clock clock_proces
 extern const struct k_clock clock_thread;
 extern const struct k_clock alarm_clock;
 
-int posix_timer_queue_signal(struct k_itimer *timr);
+void posix_timer_queue_signal(struct k_itimer *timr);
 
 void common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting);
 int common_timer_set(struct k_itimer *timr, int flags,

[patch V5 25/26] alarmtimers: Remove the throttle mechanism from alarm_forward_now()

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Now that ignored posix timer signals are requeued and the timers are
rearmed on signal delivery the workaround to keep such timers alive and
self rearm them is not longer required.

Remove the unused alarm timer parts.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/time/alarmtimer.c |   28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)
---
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -467,35 +467,11 @@ u64 alarm_forward(struct alarm *alarm, k
 }
 EXPORT_SYMBOL_GPL(alarm_forward);
 
-static u64 __alarm_forward_now(struct alarm *alarm, ktime_t interval, bool throttle)
+u64 alarm_forward_now(struct alarm *alarm, ktime_t interval)
 {
 	struct alarm_base *base = &alarm_bases[alarm->type];
-	ktime_t now = base->get_ktime();
-
-	if (IS_ENABLED(CONFIG_HIGH_RES_TIMERS) && throttle) {
-		/*
-		 * Same issue as with posix_timer_fn(). Timers which are
-		 * periodic but the signal is ignored can starve the system
-		 * with a very small interval. The real fix which was
-		 * promised in the context of posix_timer_fn() never
-		 * materialized, but someone should really work on it.
-		 *
-		 * To prevent DOS fake @now to be 1 jiffy out which keeps
-		 * the overrun accounting correct but creates an
-		 * inconsistency vs. timer_gettime(2).
-		 */
-		ktime_t kj = NSEC_PER_SEC / HZ;
 
-		if (interval < kj)
-			now = ktime_add(now, kj);
-	}
-
-	return alarm_forward(alarm, now, interval);
-}
-
-u64 alarm_forward_now(struct alarm *alarm, ktime_t interval)
-{
-	return __alarm_forward_now(alarm, interval, false);
+	return alarm_forward(alarm, base->get_ktime(), interval);
 }
 EXPORT_SYMBOL_GPL(alarm_forward_now);
 

[patch V5 26/26] alarmtimers: Remove return value from alarm functions

[Thomas Gleixner]

From: Thomas Gleixner <tglx@linutronix.de>

Now that the SIG_IGN problem is solved in the core code, the alarmtimer
callbacks do not require a return value anymore.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/power/supply/charger-manager.c |    3 +--
 fs/timerfd.c                           |    4 +---
 include/linux/alarmtimer.h             |   10 ++--------
 kernel/time/alarmtimer.c               |   16 +++++-----------
 net/netfilter/xt_IDLETIMER.c           |    4 +---
 5 files changed, 10 insertions(+), 27 deletions(-)
---
--- a/drivers/power/supply/charger-manager.c
+++ b/drivers/power/supply/charger-manager.c
@@ -1412,10 +1412,9 @@ static inline struct charger_desc *cm_ge
 	return dev_get_platdata(&pdev->dev);
 }
 
-static enum alarmtimer_restart cm_timer_func(struct alarm *alarm, ktime_t now)
+static void cm_timer_func(struct alarm *alarm, ktime_t now)
 {
 	cm_timer_set = false;
-	return ALARMTIMER_NORESTART;
 }
 
 static int charger_manager_probe(struct platform_device *pdev)
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -79,13 +79,11 @@ static enum hrtimer_restart timerfd_tmrp
 	return HRTIMER_NORESTART;
 }
 
-static enum alarmtimer_restart timerfd_alarmproc(struct alarm *alarm,
-	ktime_t now)
+static void timerfd_alarmproc(struct alarm *alarm, ktime_t now)
 {
 	struct timerfd_ctx *ctx = container_of(alarm, struct timerfd_ctx,
 					       t.alarm);
 	timerfd_triggered(ctx);
-	return ALARMTIMER_NORESTART;
 }
 
 /*
--- a/include/linux/alarmtimer.h
+++ b/include/linux/alarmtimer.h
@@ -20,12 +20,6 @@ enum alarmtimer_type {
 	ALARM_BOOTTIME_FREEZER,
 };
 
-enum alarmtimer_restart {
-	ALARMTIMER_NORESTART,
-	ALARMTIMER_RESTART,
-};
-
-
 #define ALARMTIMER_STATE_INACTIVE	0x00
 #define ALARMTIMER_STATE_ENQUEUED	0x01
 
@@ -42,14 +36,14 @@ enum alarmtimer_restart {
 struct alarm {
 	struct timerqueue_node	node;
 	struct hrtimer		timer;
-	enum alarmtimer_restart	(*function)(struct alarm *, ktime_t now);
+	void			(*function)(struct alarm *, ktime_t now);
 	enum alarmtimer_type	type;
 	int			state;
 	void			*data;
 };
 
 void alarm_init(struct alarm *alarm, enum alarmtimer_type type,
-		enum alarmtimer_restart (*function)(struct alarm *, ktime_t));
+		void (*function)(struct alarm *, ktime_t));
 void alarm_start(struct alarm *alarm, ktime_t start);
 void alarm_start_relative(struct alarm *alarm, ktime_t start);
 void alarm_restart(struct alarm *alarm);
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -321,7 +321,7 @@ static int alarmtimer_resume(struct devi
 
 static void
 __alarm_init(struct alarm *alarm, enum alarmtimer_type type,
-	     enum alarmtimer_restart (*function)(struct alarm *, ktime_t))
+	     void (*function)(struct alarm *, ktime_t))
 {
 	timerqueue_init(&alarm->node);
 	alarm->timer.function = alarmtimer_fired;
@@ -337,7 +337,7 @@ static void
  * @function: callback that is run when the alarm fires
  */
 void alarm_init(struct alarm *alarm, enum alarmtimer_type type,
-		enum alarmtimer_restart (*function)(struct alarm *, ktime_t))
+		void (*function)(struct alarm *, ktime_t))
 {
 	hrtimer_init(&alarm->timer, alarm_bases[type].base_clockid,
 		     HRTIMER_MODE_ABS);
@@ -530,14 +530,12 @@ static enum alarmtimer_type clock2alarm(
  *
  * Return: whether the timer is to be restarted
  */
-static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm, ktime_t now)
+static void alarm_handle_timer(struct alarm *alarm, ktime_t now)
 {
 	struct k_itimer *ptr = container_of(alarm, struct k_itimer, it.alarm.alarmtimer);
 
 	guard(spinlock_irqsave)(&ptr->it_lock);
 	posix_timer_queue_signal(ptr);
-
-	return ALARMTIMER_NORESTART;
 }
 
 /**
@@ -698,18 +696,14 @@ static int alarm_timer_create(struct k_i
  * @now: time at the timer expiration
  *
  * Wakes up the task that set the alarmtimer
- *
- * Return: ALARMTIMER_NORESTART
  */
-static enum alarmtimer_restart alarmtimer_nsleep_wakeup(struct alarm *alarm,
-								ktime_t now)
+static void alarmtimer_nsleep_wakeup(struct alarm *alarm, ktime_t now)
 {
 	struct task_struct *task = alarm->data;
 
 	alarm->data = NULL;
 	if (task)
 		wake_up_process(task);
-	return ALARMTIMER_NORESTART;
 }
 
 /**
@@ -761,7 +755,7 @@ static int alarmtimer_do_nsleep(struct a
 
 static void
 alarm_init_on_stack(struct alarm *alarm, enum alarmtimer_type type,
-		    enum alarmtimer_restart (*function)(struct alarm *, ktime_t))
+		    void (*function)(struct alarm *, ktime_t))
 {
 	hrtimer_init_on_stack(&alarm->timer, alarm_bases[type].base_clockid,
 			      HRTIMER_MODE_ABS);
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -107,14 +107,12 @@ static void idletimer_tg_expired(struct
 	schedule_work(&timer->work);
 }
 
-static enum alarmtimer_restart idletimer_tg_alarmproc(struct alarm *alarm,
-							  ktime_t now)
+static void idletimer_tg_alarmproc(struct alarm *alarm, ktime_t now)
 {
 	struct idletimer_tg *timer = alarm->data;
 
 	pr_debug("alarm %s expired\n", timer->attr.attr.name);
 	schedule_work(&timer->work);
-	return ALARMTIMER_NORESTART;
 }
 
 static int idletimer_check_sysfs_name(const char *name, unsigned int size)

Re: [patch V5 00/26] posix-timers: Cure the SIG_IGN mess

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:

> This are the remaining bits to cure the SIG_IGN mess. Version 4 can be found
> here:
>
>    https://lore.kernel.org/lkml/20240927083900.989915582@linutronix.de
>
> Last year I reread a 15 years old comment about the SIG_IGN problem:
>
>  "FIXME: What we really want, is to stop this timer completely and restart
>   it in case the SIG_IGN is removed. This is a non trivial change which
>   involves sighand locking (sigh !), which we don't want to do late in the
>   release cycle.  ...  A more complex fix which solves also another related
>   inconsistency is already in the pipeline."
>
> The embarrasing part was that I put that comment in back then. So I went
> back and rumaged through old notes as I completely had forgotten why our
> attempts to fix this back then failed.
>
> It turned out that the comment is about right: sighand locking and life
> time issues. So I sat down with the old notes and started to wrap my head
> around this again.
>
> The problem to solve:
>
> Posix interval timers are not rearmed automatically by the kernel for
> various reasons:
>
>    1) To prevent DoS by extremly short intervals.
>    2) To avoid timer overhead when a signal is pending and has not
>       yet been delivered.
>
> This is achieved by queueing the signal at timer expiry and rearming the
> timer at signal delivery to user space. This puts the rearming basically
> under scheduler control and the work happens in context of the task which
> asked for the signal.
>
> There is a problem with that vs. SIG_IGN. If a signal has SIG_IGN installed
> as handler, the related signals are discarded. So in case of posix interval
> timers this means that such a timer is never rearmed even when SIG_IGN is
> replaced later with a real handler (including SIG_DFL).
>
> To work around that the kernel self rearms those timers and throttles them
> when the interval is smaller than a tick to prevent a DoS.
>
> That just keeps timers ticking, which obviously has effects on power and
> just creates work for nothing.
>
> So ideally these timers should be stopped and rearmed when SIG_IGN is
> replaced, which aligns with the regular handling of posix timers.
>
> Sounds trivial, but isn't:
>
>   1) Lock ordering.
>
>      The timer lock cannot be taken with sighand lock held which is
>      problematic vs. the atomicity of sigaction().
>
>   2) Life time rules
>
>      The timer and the sigqueue are separate entities which requires a
>      lookup of the timer ID in the signal rearm code. This can be handled,
>      but the separate life time rules are not necessarily robust.
>
>   3) Finding the relevant timers
>
>      Obviosly it is possible to walk the posix timer list under sighand
>      lock and handle it from there. That can be expensive especially in the
>      case that there are no affected timers as the walk would just end up
>      doing nothing.
>
> The following series is a new and this time actually working attempt to
> solve this. It addresses it by:
>
>   1) Embedding the preallocated sigqueue into struct k_itimer, which makes
>      the life time rules way simpler and just needs a trivial reference
>      count.
>
>   2) Having a separate list in task::signal on which ignored timers are
>      queued.
>
>      This avoids walking a potentially large timer list for nothing on a
>      SIG_IGN to handler transition.
>
>   3) Requeueing the timers signal in the relevant signal queue so the timer
>      is rearmed when the signal is actually delivered
>
>      That turned out to be the least complicated way to address the sighand
>      lock vs. timer lock ordering issue.
>
> With that timers which have their signal ignored are not longer self
> rearmed and the relevant workarounds including throttling for DoS
> prevention are removed.
>
> The series is also available from git:
>
>     git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git posixt-v5
>
> Changes vs. V4:
>
>     - Remove the si_sys_private restrictions - Eric
>
>     - Hand down the pointer to the preallocated sigqueue instead of relying
>       on si_sys_priv* magic - Eric

For the bits removing the dependency on si_sys_private
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

There are a lot of slightly subtle things going on in the surrounding
patches that my brain glossed over, so I can't speak for them, but
the removal of si_sys_private looks good.

Eric

> Thanks,
>
> 	tglx
> ---
>  drivers/power/supply/charger-manager.c |    3 
>  fs/proc/base.c                         |    4 
>  fs/timerfd.c                           |    4 
>  include/linux/alarmtimer.h             |   10 
>  include/linux/posix-timers.h           |   70 ++++
>  include/linux/sched/signal.h           |    4 
>  init/init_task.c                       |    5 
>  kernel/fork.c                          |    1 
>  kernel/signal.c                        |  465 +++++++++++++++++++--------------
>  kernel/time/alarmtimer.c               |   87 ------
>  kernel/time/itimer.c                   |   22 +
>  kernel/time/posix-cpu-timers.c         |   38 +-
>  kernel/time/posix-timers.c             |  222 +++++++--------
>  kernel/time/posix-timers.h             |    8 
>  net/netfilter/xt_IDLETIMER.c           |    4 
>  15 files changed, 502 insertions(+), 445 deletions(-)