linux-kernel - Re: [patch V2 08/17] posix-timers: Rework timer removal

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: <3e38af8c-1d06-49bd-aa30-f4e8b0263492@virtuozzo.com>
Date: Tue, 4 Mar 2025 18:10:47 +0800
From: Pavel Tikhomirov <ptikhomirov@...tuozzo.com>
To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Anna-Maria Behnsen <anna-maria@...utronix.de>,
 Frederic Weisbecker <frederic@...nel.org>,
 Benjamin Segall <bsegall@...gle.com>, Eric Dumazet <edumazet@...gle.com>,
 Andrey Vagin <avagin@...nvz.org>, Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch V2 08/17] posix-timers: Rework timer removal

   > -/* Delete a POSIX.1b interval timer. */
> -SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
> +static void posix_timer_delete(struct k_itimer *timer)
>   {
> -	struct k_itimer *timer = lock_timer(timer_id);
> -
> -retry_delete:
> -	if (!timer)
> -		return -EINVAL;
> -
> -	/* Prevent signal delivery and rearming. */
> +	/*
> +	 * Invalidate the timer, remove it from the linked list and remove
> +	 * it from the ignored list if pending.
> +	 *
> +	 * The invalidation must be written with siglock held so that the
> +	 * signal code observes timer->it_valid == false in do_sigaction(),
> +	 * which prevents it from moving a pending signal of a deleted
> +	 * timer to the ignore list.
> +	 *
> +	 * The invalidation also prevents signal queueing, signal delivery
> +	 * and therefore rearming from the signal delivery path.
> +	 *
> +	 * A concurrent lookup can still find the timer in the hash, but it
> +	 * will check timer::it_signal with timer::it_lock held and observe
> +	 * bit 0 set, which invalidates it. That also prevents the timer ID
> +	 * from being handed out before this timer is completely gone.
> +	 */
>   	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);
> -		goto retry_delete;
> -	}
> -
>   	scoped_guard (spinlock, &current->sighand->siglock) {
> +		unsigned long sig = (unsigned long)timer->it_signal | 1UL;
> +
> +		WRITE_ONCE(timer->it_signal, (struct signal_struct *)sig);
>   		hlist_del(&timer->list);
>   		posix_timer_cleanup_ignored(timer);
> -		/*
> -		 * A concurrent lookup could check timer::it_signal lockless. It
> -		 * will reevaluate with timer::it_lock held and observe the NULL.
> -		 *
> -		 * It must be written with siglock held so that the signal code
> -		 * observes timer->it_signal == NULL in do_sigaction(SIG_IGN),
> -		 * which prevents it from moving a pending signal of a deleted
> -		 * timer to the ignore list.
> -		 */
> -		WRITE_ONCE(timer->it_signal, NULL);
>   	}
>   
> -	unlock_timer(timer);
> -	posix_timer_unhash_and_free(timer);
> -	return 0;
> +	while (timer->kclock->timer_del(timer) == TIMER_RETRY) {
> +		guard(rcu)();
> +		spin_unlock_irq(&timer->it_lock);

Maybe "guard(spinlock_irq)(&timer->it_lock);", since we already use 
guard constructions everywhere else?

> +		timer_wait_running(timer);
> +		spin_lock_irq(&timer->it_lock);
> +	}
>   }
>

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.