linux-kernel - Re: [patch V2 04/17] posix-timers: Remove a few paranoid warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z8jMBqI0tuQBou4I@pavilion.home>
Date: Wed, 5 Mar 2025 23:11:18 +0100
From: Frederic Weisbecker <frederic@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Anna-Maria Behnsen <anna-maria@...utronix.de>,
	Benjamin Segall <bsegall@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Andrey Vagin <avagin@...nvz.org>,
	Pavel Tikhomirov <ptikhomirov@...tuozzo.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch V2 04/17] posix-timers: Remove a few paranoid warnings

Le Sun, Mar 02, 2025 at 08:36:49PM +0100, Thomas Gleixner a écrit :
> Warnings about a non-initialized timer or non-existing callbacks are just
> useful for implementing new posix clocks, but there a NULL pointer
> dereference is expected anyway. :)
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> V2: New patch
> ---
>  kernel/time/posix-timers.c |   37 ++++++++-----------------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)
> 
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -675,7 +675,6 @@ void common_timer_get(struct k_itimer *t
>  
>  static int do_timer_gettime(timer_t timer_id,  struct itimerspec64 *setting)
>  {
> -	const struct k_clock *kc;
>  	struct k_itimer *timr;
>  	unsigned long flags;
>  	int ret = 0;
> @@ -685,11 +684,7 @@ static int do_timer_gettime(timer_t time
>  		return -EINVAL;
>  
>  	memset(setting, 0, sizeof(*setting));
> -	kc = timr->kclock;
> -	if (WARN_ON_ONCE(!kc || !kc->timer_get))
> -		ret = -EINVAL;
> -	else
> -		kc->timer_get(timr, setting);
> +	timr->kclock->timer_get(timr, setting);
>  
>  	unlock_timer(timr, flags);
>  	return ret;
> @@ -817,7 +812,6 @@ static void common_timer_wait_running(st
>  static struct k_itimer *timer_wait_running(struct k_itimer *timer,
>  					   unsigned long *flags)
>  {
> -	const struct k_clock *kc = READ_ONCE(timer->kclock);
>  	timer_t timer_id = READ_ONCE(timer->it_id);
>  
>  	/* Prevent kfree(timer) after dropping the lock */
> @@ -828,8 +822,7 @@ static struct k_itimer *timer_wait_runni
>  	 * kc->timer_wait_running() might drop RCU lock. So @timer
>  	 * cannot be touched anymore after the function returns!
>  	 */
> -	if (!WARN_ON_ONCE(!kc->timer_wait_running))
> -		kc->timer_wait_running(timer);
> +	timer->kclock->timer_wait_running(timer);
>  
>  	rcu_read_unlock();
>  	/* Relock the timer. It might be not longer hashed. */
> @@ -892,7 +885,6 @@ static int do_timer_settime(timer_t time
>  			    struct itimerspec64 *new_spec64,
>  			    struct itimerspec64 *old_spec64)
>  {
> -	const struct k_clock *kc;
>  	struct k_itimer *timr;
>  	unsigned long flags;
>  	int error;
> @@ -915,11 +907,7 @@ static int do_timer_settime(timer_t time
>  	/* Prevent signal delivery and rearming. */
>  	timr->it_signal_seq++;
>  
> -	kc = timr->kclock;
> -	if (WARN_ON_ONCE(!kc || !kc->timer_set))
> -		error = -EINVAL;
> -	else
> -		error = kc->timer_set(timr, tmr_flags, new_spec64, old_spec64);
> +	error = timr->kclock->timer_set(timr, tmr_flags, new_spec64, old_spec64);
>  
>  	if (error == TIMER_RETRY) {
>  		// We already got the old time...
> @@ -1001,18 +989,6 @@ static inline void posix_timer_cleanup_i
>  	}
>  }
>  
> -static inline int timer_delete_hook(struct k_itimer *timer)
> -{
> -	const struct k_clock *kc = timer->kclock;
> -
> -	/* Prevent signal delivery and rearming. */
> -	timer->it_signal_seq++;
> -
> -	if (WARN_ON_ONCE(!kc || !kc->timer_del))
> -		return -EINVAL;
> -	return kc->timer_del(timer);
> -}
> -
>  /* Delete a POSIX.1b interval timer. */
>  SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
>  {
> @@ -1025,7 +1001,10 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
>  	if (!timer)
>  		return -EINVAL;
>  
> -	if (unlikely(timer_delete_hook(timer) == TIMER_RETRY)) {
> +	/* Prevent signal delivery and rearming. */
> +	timer->it_signal_seq++;
> +
> +	if (unlikely(timer->kclock->timer_del(timer) == TIMER_RETRY)) {
>  		/* Unlocks and relocks the timer if it still exists */
>  		timer = timer_wait_running(timer, &flags);
>  		goto retry_delete;
> @@ -1071,7 +1050,7 @@ static void itimer_delete(struct k_itime
>  	 * mechanism. Worse, that timer mechanism might run the expiry
>  	 * function concurrently.
>  	 */
> -	if (timer_delete_hook(timer) == TIMER_RETRY) {
> +	if (timer->kclock->timer_del(timer) == TIMER_RETRY) {

And itimer_delete() doesn't need ->it_signal_seq++ since the task is exiting
and signals are going to be flushed anyway, right...

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


>  		/*
>  		 * Timer is expired concurrently, prevent livelocks
>  		 * and pointless spinning on RT.
>