Re: [patch v6.1 17/20] signal: Queue ignored posixtimers on ignore list

[Frederic Weisbecker <frederic@kernel.org>]

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.