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
@@ -731,6 +731,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)
 {
@@ -747,7 +757,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);
 		}
 	}
 }
@@ -1964,7 +1974,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)();
 
@@ -1981,13 +1991,24 @@ int posixtimer_send_sigqueue(struct k_it
 	 */
 	tmr->it_sigqueue_seq = tmr->it_signal_seq;
 
-	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;
@@ -2000,7 +2021,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)
@@ -2048,6 +2076,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 */

Le Thu, Oct 31, 2024 at 04:46:43PM +0100, Thomas Gleixner a écrit :
> 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
> @@ -731,6 +731,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);

So this happens when the signal is ignored and delays it to when it will be
unignored. But the comment on do_sigaction() says:

		/*
		 * POSIX 3.3.1.3:
		 *  "Setting a signal action to SIG_IGN for a signal that is
		 *   pending shall cause the pending signal to be discarded,
		 *   whether or not it is blocked."
		 *
		 */

Are posix timers an exception to that rule?

Also I see flush_sigqueue_mask() called on other occasions, for example
when a STOP signal happens to remove pending CONT, not sure if posix
timers can set SIGCONT...

Thanks.

On Fri, Nov 01 2024 at 15:21, Frederic Weisbecker wrote:
> Le Thu, Oct 31, 2024 at 04:46:43PM +0100, Thomas Gleixner a écrit :
>> +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);
>
> So this happens when the signal is ignored and delays it to when it will be
> unignored. But the comment on do_sigaction() says:
>
> 		/*
> 		 * POSIX 3.3.1.3:
> 		 *  "Setting a signal action to SIG_IGN for a signal that is
> 		 *   pending shall cause the pending signal to be discarded,
> 		 *   whether or not it is blocked."
> 		 *
> 		 */
>
> Are posix timers an exception to that rule?
>
> Also I see flush_sigqueue_mask() called on other occasions, for example
> when a STOP signal happens to remove pending CONT, not sure if posix
> timers can set SIGCONT...

No. The problem with posix timers is that they are magically different
from regular signals in the case of periodic timers.

When the signal is ignored at expiry, then the signal is not delivered
and is 'dropped'. But when SIG_IGN is removed then the following period
expiry has to deliver the signal.

Right now the kernel ensures that by keeping the timer self rearming and
rate limiting it for obvious reasons. That's a completely pointless
exercise up to the point where SIG_IGN is removed.

The only way to avoid the self rearming is to actually stop the timer
when the signal is ignored and rearm it when SIG_IGN for the specific
signal is removed.

That's what this magic does...

Thanks,

        tglx

Le Fri, Nov 01, 2024 at 09:47:15PM +0100, Thomas Gleixner a écrit :
> On Fri, Nov 01 2024 at 15:21, Frederic Weisbecker wrote:
> > Le Thu, Oct 31, 2024 at 04:46:43PM +0100, Thomas Gleixner a écrit :
> >> +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);
> >
> > So this happens when the signal is ignored and delays it to when it will be
> > unignored. But the comment on do_sigaction() says:
> >
> > 		/*
> > 		 * POSIX 3.3.1.3:
> > 		 *  "Setting a signal action to SIG_IGN for a signal that is
> > 		 *   pending shall cause the pending signal to be discarded,
> > 		 *   whether or not it is blocked."
> > 		 *
> > 		 */
> >
> > Are posix timers an exception to that rule?
> >
> > Also I see flush_sigqueue_mask() called on other occasions, for example
> > when a STOP signal happens to remove pending CONT, not sure if posix
> > timers can set SIGCONT...
> 
> No. The problem with posix timers is that they are magically different
> from regular signals in the case of periodic timers.
> 
> When the signal is ignored at expiry, then the signal is not delivered
> and is 'dropped'. But when SIG_IGN is removed then the following period
> expiry has to deliver the signal.
>
> Right now the kernel ensures that by keeping the timer self rearming and
> rate limiting it for obvious reasons. That's a completely pointless
> exercise up to the point where SIG_IGN is removed.
> 
> The only way to avoid the self rearming is to actually stop the timer
> when the signal is ignored and rearm it when SIG_IGN for the specific
> signal is removed.
> 
> That's what this magic does...


Agreed and that covers the case when the signal is queued while the handler
is SIG_IGN. But I didn't know about the case where the signal is queued and
then the handler is set to SIG_IGN before it get a chance to be delivered.
Which of course is not fundamentally different now that I think about it
twice.

And what about the other callers of flush_sigqueue_mask()? Such as SIGCONT
removed when SIGSTOP arrives? And the reverse as well? This moves the pending
timers signals to the ignore list until the signal is unignored, but in that
case SIGCONT is not ignored? Or perhaps SIGCONT and SIGSTOP can't be set for
posix timers?

Thanks.

> 
> Thanks,
> 
>         tglx
> 

On Sun, Nov 03 2024 at 00:46, Frederic Weisbecker wrote:
> And what about the other callers of flush_sigqueue_mask()? Such as SIGCONT
> removed when SIGSTOP arrives? And the reverse as well? This moves the pending
> timers signals to the ignore list until the signal is unignored, but in that
> case SIGCONT is not ignored? Or perhaps SIGCONT and SIGSTOP can't be set for
> posix timers?

You can set SIGSTOP/CONT on a posix timer. Whether that makes sense or
not is a different question :)

Even on current mainline the behaviour is pretty much unspecified. Some
combinations "work", some not. I just tried out of morbid curiousity. :)

Thanks,

        tglx

Le Sun, Nov 03, 2024 at 10:44:35AM +0100, Thomas Gleixner a écrit :
> On Sun, Nov 03 2024 at 00:46, Frederic Weisbecker wrote:
> > And what about the other callers of flush_sigqueue_mask()? Such as SIGCONT
> > removed when SIGSTOP arrives? And the reverse as well? This moves the pending
> > timers signals to the ignore list until the signal is unignored, but in that
> > case SIGCONT is not ignored? Or perhaps SIGCONT and SIGSTOP can't be set for
> > posix timers?
> 
> You can set SIGSTOP/CONT on a posix timer. Whether that makes sense or
> not is a different question :)
> 
> Even on current mainline the behaviour is pretty much unspecified. Some
> combinations "work", some not. I just tried out of morbid curiousity. :)

I see.

So probably we shouldn't care too much, right?

> 
> Thanks,
> 
>         tglx
> 
> 

On Fri, Nov 01 2024 at 21:47, Thomas Gleixner wrote:
> On Fri, Nov 01 2024 at 15:21, Frederic Weisbecker wrote:
>> Le Thu, Oct 31, 2024 at 04:46:43PM +0100, Thomas Gleixner a écrit :
>>> +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);
>>
>> So this happens when the signal is ignored and delays it to when it will be
>> unignored. But the comment on do_sigaction() says:
>>
>> 		/*
>> 		 * POSIX 3.3.1.3:
>> 		 *  "Setting a signal action to SIG_IGN for a signal that is
>> 		 *   pending shall cause the pending signal to be discarded,
>> 		 *   whether or not it is blocked."
>> 		 *
>> 		 */
>>
>> Are posix timers an exception to that rule?
>>
>> Also I see flush_sigqueue_mask() called on other occasions, for example
>> when a STOP signal happens to remove pending CONT, not sure if posix
>> timers can set SIGCONT...
>
> No. The problem with posix timers is that they are magically different
> from regular signals in the case of periodic timers.
>
> When the signal is ignored at expiry, then the signal is not delivered
> and is 'dropped'. But when SIG_IGN is removed then the following period
> expiry has to deliver the signal.
>
> Right now the kernel ensures that by keeping the timer self rearming and
> rate limiting it for obvious reasons. That's a completely pointless
> exercise up to the point where SIG_IGN is removed.
>
> The only way to avoid the self rearming is to actually stop the timer
> when the signal is ignored and rearm it when SIG_IGN for the specific
> signal is removed.
>
> That's what this magic does...

But it does not exclude oneshot timers from this. Their signal has to be
dropped for real.

Delta patch below.

Thanks,

        tglx
---
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -731,7 +731,7 @@ 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 inline bool posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q);
 
 static void sigqueue_free_ignored(struct task_struct *tsk, struct sigqueue *q)
 {
@@ -1999,8 +1999,8 @@ int posixtimer_send_sigqueue(struct k_it
 			goto out;
 
 		if (hlist_unhashed(&tmr->ignored_list)) {
-			hlist_add_head(&tmr->ignored_list, &t->signal->ignored_posix_timers);
-			posixtimer_sigqueue_getref(q);
+			if (posixtimer_sig_ignore(t, tmr))
+				posixtimer_sigqueue_getref(q);
 		}
 		goto out;
 	}
@@ -2024,11 +2024,15 @@ int posixtimer_send_sigqueue(struct k_it
 	return 0;
 }
 
-static inline void posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q)
+static inline bool posixtimer_sig_ignore(struct task_struct *tsk, struct sigqueue *q)
 {
 	struct k_itimer *tmr = container_of(q, struct k_itimer, sigq);
 
+	/* Only queue periodic timer signals */
+	if (tmr->status != POSIX_TIMER_REQUEUE_PENDING)
+		return false;
 	hlist_add_head(&tmr->ignored_list, &tsk->signal->ignored_posix_timers);
+	return true;
 }
 
 static void posixtimer_sig_unignore(struct task_struct *tsk, int sig)

On Sat, Nov 02 2024 at 15:49, Thomas Gleixner wrote:
>> That's what this magic does...
>
> But it does not exclude oneshot timers from this. Their signal has to be
> dropped for real.
>
> Delta patch below.

Which is incomplete. I send a full replacement in a subsequent mail.

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>
---
V6.1: Handle oneshot timer expiry or transitioning from periodic to
      oneshot after a rearming correctly. - Frederic
---
 kernel/signal.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 5 deletions(-)
---

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -731,6 +731,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)
 {
@@ -747,7 +757,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);
 		}
 	}
 }
@@ -1964,7 +1974,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)();
 
@@ -1981,13 +1991,48 @@ int posixtimer_send_sigqueue(struct k_it
 	 */
 	tmr->it_sigqueue_seq = tmr->it_signal_seq;
 
-	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;
+
+		/* Periodic timers with SIG_IGN are queued on the ignored list */
+		if (tmr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
+			/*
+			 * Already queued means the timer was rearmed after
+			 * the previous expiry got it on the ignore list.
+			 * Nothing to do for that case.
+			 */
+			if (hlist_unhashed(&tmr->ignored_list)) {
+				/*
+				 * Take a signal reference and queue it on
+				 * the ignored list.
+				 */
+				posixtimer_sigqueue_getref(q);
+				posixtimer_sig_ignore(t, tmr);
+			}
+		} else if (!hlist_unhashed(&tmr->ignored_list)) {
+			/*
+			 * Covers the case where a timer was periodic and
+			 * then signal was ignored. Then it was rearmed as
+			 * oneshot timer. The previous signal is invalid
+			 * now, and the oneshot signal has to be dropped.
+			 * Remove it from the ignored list and drop the
+			 * reference count as the signal is not longer
+			 * queued.
+			 */
+			hlist_del_init(&tmr->ignored_list);
+			posixtimer_putref(tmr);
+		}
 		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;
@@ -2000,7 +2045,21 @@ 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);
+
+	/*
+	 * Only enqueue periodic timer signals to the ignored list. For
+	 * oneshot timers, drop the reference count.
+	 */
+	if (tmr->it_status == POSIX_TIMER_REQUEUE_PENDING)
+		hlist_add_head(&tmr->ignored_list, &tsk->signal->ignored_posix_timers);
+	else
+		posixtimer_putref(tmr);
 }
 
 static void posixtimer_sig_unignore(struct task_struct *tsk, int sig)
@@ -2048,6 +2107,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 */

Le Sat, Nov 02, 2024 at 10:05:08PM +0100, Thomas Gleixner a écrit :
> 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>
> ---
> V6.1: Handle oneshot timer expiry or transitioning from periodic to
>       oneshot after a rearming correctly. - Frederic
> ---
>  kernel/signal.c |   70 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 65 insertions(+), 5 deletions(-)
> ---
> 
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -731,6 +731,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)
>  {
> @@ -747,7 +757,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);
>  		}
>  	}
>  }
> @@ -1964,7 +1974,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)();
>  
> @@ -1981,13 +1991,48 @@ int posixtimer_send_sigqueue(struct k_it
>  	 */
>  	tmr->it_sigqueue_seq = tmr->it_signal_seq;
>  
> -	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;
> +
> +		/* Periodic timers with SIG_IGN are queued on the ignored list */
> +		if (tmr->it_status == POSIX_TIMER_REQUEUE_PENDING) {
> +			/*
> +			 * Already queued means the timer was rearmed after
> +			 * the previous expiry got it on the ignore list.
> +			 * Nothing to do for that case.
> +			 */
> +			if (hlist_unhashed(&tmr->ignored_list)) {
> +				/*
> +				 * Take a signal reference and queue it on
> +				 * the ignored list.
> +				 */
> +				posixtimer_sigqueue_getref(q);
> +				posixtimer_sig_ignore(t, tmr);
> +			}
> +		} else if (!hlist_unhashed(&tmr->ignored_list)) {
> +			/*
> +			 * Covers the case where a timer was periodic and
> +			 * then signal was ignored. Then it was rearmed as
> +			 * oneshot timer. The previous signal is invalid
> +			 * now, and the oneshot signal has to be dropped.
> +			 * Remove it from the ignored list and drop the
> +			 * reference count as the signal is not longer
> +			 * queued.
> +			 */
> +			hlist_del_init(&tmr->ignored_list);
> +			posixtimer_putref(tmr);
> +		}
>  		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;
> @@ -2000,7 +2045,21 @@ 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);
> +
> +	/*
> +	 * Only enqueue periodic timer signals to the ignored list. For
> +	 * oneshot timers, drop the reference count.
> +	 */
> +	if (tmr->it_status == POSIX_TIMER_REQUEUE_PENDING)

This is read locklessly against timer lock. So it only makes sure that
the last write to POSIX_TIMER_REQUEUE_PENDING will be visible due to the
sighand locking. However it may or may not see the other states written after.
Which looks ok because if the timer is concurrently armed / disarmed / or even
requeue pending again, then either the signal is dropped and it's fine or the
signal is moved to the ignored list and the seq will take care of the validity
upon delivery.

Still this may need a WRITE_ONCE() / READ_ONCE().

But there is something more problematic against the delete() path:
                                   
Thread within                  Signal target             Timer target
signal target group
--------------------           -------------             -------------
                                                         timr->it_status = POSIX_TIMER_REQUEUE_PENDING
                                                         posixtimer_send_sigqueue();
                                                         do_exit();
timer_delete()
    posix_cpu_timer_del()
        // return NULL
        cpu_timer_task_rcu()
        // timer->it_status NOT set
        // to POSIX_TIMER_DISARMED
    hlist_del(&timer->list);
    posix_timer_cleanup_ignored()


                               do_sigaction(SIG_IGN...)
                               flush_sigqueue_mask()
                                  sigqueue_free_ignored()
                                      posixtimer_sig_ignore()
                                          // Observe POSIX_TIMER_REQUEUE_PENDING
                                          hlist_add_head(...ignored_posix_timers)
                               do_exit()
                                   exit_itimers()
                                   if (hlist_empty(&tsk->signal->posix_timers))
                                       return;
                                   // leaked timer queued to migrate list
                                   


> +		hlist_add_head(&tmr->ignored_list, &tsk->signal->ignored_posix_timers);
> +	else
> +		posixtimer_putref(tmr);
>  }

Thanks.

On Mon, Nov 04 2024 at 12:42, Frederic Weisbecker wrote:
> But there is something more problematic against the delete() path:
>                                    
> Thread within                  Signal target             Timer target
> signal target group
> --------------------           -------------             -------------
>                                                          timr->it_status = POSIX_TIMER_REQUEUE_PENDING
>                                                          posixtimer_send_sigqueue();
>                                                          do_exit();
> timer_delete()
>     posix_cpu_timer_del()
>         // return NULL
>         cpu_timer_task_rcu()
>         // timer->it_status NOT set
>         // to POSIX_TIMER_DISARMED
>     hlist_del(&timer->list);
>     posix_timer_cleanup_ignored()
>
>
>                                do_sigaction(SIG_IGN...)
>                                flush_sigqueue_mask()
>                                   sigqueue_free_ignored()
>                                       posixtimer_sig_ignore()
>                                           // Observe POSIX_TIMER_REQUEUE_PENDING
>                                           hlist_add_head(...ignored_posix_timers)
>                                do_exit()
>                                    exit_itimers()
>                                    if (hlist_empty(&tsk->signal->posix_timers))
>                                        return;
>                                    // leaked timer queued to migrate list
>                                    

Duh. Let me stare at that some more.

On Mon, Nov 04 2024 at 16:21, Thomas Gleixner wrote:

> On Mon, Nov 04 2024 at 12:42, Frederic Weisbecker wrote:
>> But there is something more problematic against the delete() path:
>>                                    
>> Thread within                  Signal target             Timer target
>> signal target group
>> --------------------           -------------             -------------
>>                                                          timr->it_status = POSIX_TIMER_REQUEUE_PENDING
>>                                                          posixtimer_send_sigqueue();
>>                                                          do_exit();
>> timer_delete()
>>     posix_cpu_timer_del()
>>         // return NULL
>>         cpu_timer_task_rcu()
>>         // timer->it_status NOT set
>>         // to POSIX_TIMER_DISARMED
>>     hlist_del(&timer->list);
>>     posix_timer_cleanup_ignored()
>>
>>
>>                                do_sigaction(SIG_IGN...)
>>                                flush_sigqueue_mask()
>>                                   sigqueue_free_ignored()
>>                                       posixtimer_sig_ignore()
>>                                           // Observe POSIX_TIMER_REQUEUE_PENDING
>>                                           hlist_add_head(...ignored_posix_timers)
>>                                do_exit()
>>                                    exit_itimers()
>>                                    if (hlist_empty(&tsk->signal->posix_timers))
>>                                        return;
>>                                    // leaked timer queued to migrate list
>>                                    
>
> Duh. Let me stare at that some more.

The delete() issue is actually easy to address:

  posixtimer_sig_ignore() must check timer->it_signal, which is set to
  NULL in timer_delete(). This write must move into the sighand lock
  held section, where posix_timer_cleanup_ignored() is invoked.

  If NULL, posixtimer_sig_ignore() drops the reference.

  If do_sigaction() locked sighand first and moved it to the ignored
  list, then posix_timer_cleanup_ignored() will remove it again.


The status part is hard to get right without sighand lock being held,
but it is required to ensure that a pending one-shot timer signal is
dropped in do_sigaction(SIG_IGN).

There is an easy fix for that too:

  posixtimer_send_siqqueue() does the following under sighand lock:

    timer->it_sig_periodic = timer->it_status == POSIX_TIMER_REQUEUE_PENDING;
         
  posixtimer_sig_ignore() checks that flag. If not set it can drop the
  reference independent of the actual status of the timer. If the timer
  was rearmed as periodic, then it did not expire yet because the expiry
  would have set the flag. If it expires concurrently the expiry
  function is stuck on sighand::siglock.

  If the flag is set then the signal will go onto the ignored list and
  un-ignore will move it back to the pending list. That's not a problem
  in the case that the timer was re/dis-armed before or after moving, as
  this is all covered by the sequence check.

All of that works because in both cases the protection scheme on the
timer side is that both timer::lock and sighand::siglock have to be held
for modifying

   timer::it_sigqueue_seq
   timer::it_signal
   timer::it_sig_periodic

which means that on the signal side holding sighand::siglock is enough.

In posixtimer_deliver_signal() holding the timer lock is sufficient to
do the sequence validation against timer::it_sig_periodic.

I'll fixup the inconsistent state thing in posix-cpu-timers too and send
out a v7 soon.

Thanks a lot for studying this in detail!

        tglx

Le Mon, Nov 04, 2024 at 10:31:57PM +0100, Thomas Gleixner a écrit :
> On Mon, Nov 04 2024 at 16:21, Thomas Gleixner wrote:
> 
> > On Mon, Nov 04 2024 at 12:42, Frederic Weisbecker wrote:
> >> But there is something more problematic against the delete() path:
> >>                                    
> >> Thread within                  Signal target             Timer target
> >> signal target group
> >> --------------------           -------------             -------------
> >>                                                          timr->it_status = POSIX_TIMER_REQUEUE_PENDING
> >>                                                          posixtimer_send_sigqueue();
> >>                                                          do_exit();
> >> timer_delete()
> >>     posix_cpu_timer_del()
> >>         // return NULL
> >>         cpu_timer_task_rcu()
> >>         // timer->it_status NOT set
> >>         // to POSIX_TIMER_DISARMED
> >>     hlist_del(&timer->list);
> >>     posix_timer_cleanup_ignored()
> >>
> >>
> >>                                do_sigaction(SIG_IGN...)
> >>                                flush_sigqueue_mask()
> >>                                   sigqueue_free_ignored()
> >>                                       posixtimer_sig_ignore()
> >>                                           // Observe POSIX_TIMER_REQUEUE_PENDING
> >>                                           hlist_add_head(...ignored_posix_timers)
> >>                                do_exit()
> >>                                    exit_itimers()
> >>                                    if (hlist_empty(&tsk->signal->posix_timers))
> >>                                        return;
> >>                                    // leaked timer queued to migrate list
> >>                                    
> >
> > Duh. Let me stare at that some more.
> 
> The delete() issue is actually easy to address:
> 
>   posixtimer_sig_ignore() must check timer->it_signal, which is set to
>   NULL in timer_delete(). This write must move into the sighand lock
>   held section, where posix_timer_cleanup_ignored() is invoked.
> 
>   If NULL, posixtimer_sig_ignore() drops the reference.
> 
>   If do_sigaction() locked sighand first and moved it to the ignored
>   list, then posix_timer_cleanup_ignored() will remove it again.

Indeed. Unconditionally writing ->it_status to DISARMED in posix_cpu_timer_del()
would also do the trick as that write would be released by the sighand unlock after
posix_timer_cleanup_ignored(). But testing ->it_signal after guaranteeing it is
written inside sighand looks like a more straightforward way to go.

(Still it makes sense to unconditionally set ->it_status to DISARMED after delete()).

> 
> 
> The status part is hard to get right without sighand lock being held,
> but it is required to ensure that a pending one-shot timer signal is
> dropped in do_sigaction(SIG_IGN).
> 
> There is an easy fix for that too:
> 
>   posixtimer_send_siqqueue() does the following under sighand lock:
> 
>     timer->it_sig_periodic = timer->it_status == POSIX_TIMER_REQUEUE_PENDING;
>          
>   posixtimer_sig_ignore() checks that flag. If not set it can drop the
>   reference independent of the actual status of the timer. If the timer
>   was rearmed as periodic, then it did not expire yet because the expiry
>   would have set the flag. If it expires concurrently the expiry
>   function is stuck on sighand::siglock.
> 
>   If the flag is set then the signal will go onto the ignored list and
>   un-ignore will move it back to the pending list. That's not a problem
>   in the case that the timer was re/dis-armed before or after moving, as
>   this is all covered by the sequence check.
> 
> All of that works because in both cases the protection scheme on the
> timer side is that both timer::lock and sighand::siglock have to be held
> for modifying
> 
>    timer::it_sigqueue_seq
>    timer::it_signal
>    timer::it_sig_periodic
> 
> which means that on the signal side holding sighand::siglock is enough.
> 
> In posixtimer_deliver_signal() holding the timer lock is sufficient to
> do the sequence validation against timer::it_sig_periodic.

Exactly.

> 
> I'll fixup the inconsistent state thing in posix-cpu-timers too and send
> out a v7 soon.

Thanks!