linux-kernel - Re: [patch V2 08/12] rseq: Implement time slice extension enforcement timer

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: <20251028083356.cDl403Q9@linutronix.de>
Date: Tue, 28 Oct 2025 09:33:56 +0100
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Paul E. McKenney" <paulmck@...nel.org>,
	Boqun Feng <boqun.feng@...il.com>, Jonathan Corbet <corbet@....net>,
	Prakash Sangappa <prakash.sangappa@...cle.com>,
	Madadi Vineeth Reddy <vineethr@...ux.ibm.com>,
	K Prateek Nayak <kprateek.nayak@....com>,
	Steven Rostedt <rostedt@...dmis.org>, Arnd Bergmann <arnd@...db.de>,
	linux-arch@...r.kernel.org
Subject: Re: [patch V2 08/12] rseq: Implement time slice extension
 enforcement timer

On 2025-10-27 17:26:29 [+0100], Thomas Gleixner wrote:
> On Mon, Oct 27 2025 at 12:38, Sebastian Andrzej Siewior wrote:
> > On 2025-10-22 14:57:38 [+0200], Thomas Gleixner wrote:
> >> +static enum hrtimer_restart rseq_slice_expired(struct hrtimer *tmr)
> >> +{
> >> +	struct slice_timer *st = container_of(tmr, struct slice_timer, timer);
> >> +
> >> +	if (st->cookie == current && current->rseq.slice.state.granted) {
> >> +		rseq_stat_inc(rseq_stats.s_expired);
> >> +		set_need_resched_current();
> >> +	}
> >
> > You arm the timer while leaving to userland. Once in userland the task
> > can be migrated to another CPU. Once migrated, this CPU can host another
> > task while the timer fires and does nothing.
> 
> That's inevitable. If the scheduler decides to do that then there is
> nothing which can be done about it and that's why the cookie pointer
> exists.

Without an interrupt on the target CPU, there is nothing stopping the
task from overstepping its fair share.

> >> +	return HRTIMER_NORESTART;
> >> +}
> >> +
> > …
> >> +static void rseq_cancel_slice_extension_timer(void)
> >> +{
> >> +	struct slice_timer *st = this_cpu_ptr(&slice_timer);
> >> +
> >> +	/*
> >> +	 * st->cookie can be safely read as preemption is disabled and the
> >> +	 * timer is CPU local. The active check can obviously race with the
> >> +	 * hrtimer interrupt, but that's better than disabling interrupts
> >> +	 * unconditionally right away.
> >> +	 *
> >> +	 * As this is most probably the first expiring timer, the cancel is
> >> +	 * expensive as it has to reprogram the hardware, but that's less
> >> +	 * expensive than going through a full hrtimer_interrupt() cycle
> >> +	 * for nothing.
> >> +	 *
> >> +	 * hrtimer_try_to_cancel() is sufficient here as with interrupts
> >> +	 * disabled the timer callback cannot be running and the timer base
> >> +	 * is well determined as the timer is pinned on the local CPU.
> >> +	 */
> >> +	if (st->cookie == current && hrtimer_active(&st->timer)) {
> >> +		scoped_guard(irq)
> >> +			hrtimer_try_to_cancel(&st->timer);
> >
> > I don't see why hrtimer_active() and IRQ-disable is a benefit here.
> > Unless you want to avoid a branch to hrtimer_try_to_cancel().
> >
> > The function has its own hrtimer_active() check and disables interrupts
> > while accessing the hrtimer_base lock. Since preemption is disabled,
> > st->cookie remains stable.
> > It can fire right after the hrtimer_active() here. You could just
> >
> > 	if (st->cookie == current)
> > 		hrtimer_try_to_cancel(&st->timer);
> >
> > at the expense of a branch to hrtimer_try_to_cancel() if the timer
> > already expired (no interrupts off/on).
> 
> That's not equivalent. As this is CPU local the interrupt disable
> ensures that the timer is not running on this CPU. Otherwise you need
> hrtimer_cancel(). Read the comment. :)

Since it is a CPU local timer which is HRTIMER_MODE_HARD, from this CPUs
perspective it is either about to run or it did run. Therefore the
hrtimer_try_to_cancel() can't return -1 due to
hrtimer_callback_running() == true.
If you drop hrtimer_active() check and scoped_guard(irq),
hrtimer_try_to_cancel() will do the same hrtimer_active() check as you
have above followed by disable interrupts via lock_hrtimer_base() and
here hrtimer_callback_running() can't return true because interrupts are
disabled and the timer can't run on a remote CPU because it is a
CPU-local timer.

So you avoid a branch to hrtimer_try_to_cancel() if the timer already
fired.

> Thanks,
> 
>         tglx

Sebastian