linux-kernel - Re: [PATCH v9 22/32] timers: Keep the pinned timers separate from the others

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: <87fs0feijb.fsf@somnus>
Date:   Wed, 06 Dec 2023 11:23:36 +0100
From:   Anna-Maria Behnsen <anna-maria@...utronix.de>
To:     Sebastian Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        John Stultz <jstultz@...gle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Eric Dumazet <edumazet@...gle.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
        Arjan van de Ven <arjan@...radead.org>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Rik van Riel <riel@...riel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Giovanni Gherdovich <ggherdovich@...e.cz>,
        Lukasz Luba <lukasz.luba@....com>,
        "Gautham R . Shenoy" <gautham.shenoy@....com>,
        Srinivas Pandruvada <srinivas.pandruvada@...el.com>,
        K Prateek Nayak <kprateek.nayak@....com>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH v9 22/32] timers: Keep the pinned timers separate from
 the others

Sebastian Siewior <bigeasy@...utronix.de> writes:

> On 2023-12-01 10:26:44 [+0100], Anna-Maria Behnsen wrote:
>> --- a/kernel/time/timer.c
>> +++ b/kernel/time/timer.c
>> @@ -1985,10 +1998,31 @@ static inline u64 __get_next_timer_interrupt(unsigned long basej, u64 basem,
>>  		return expires;
>>  	}
>>  
>> -	raw_spin_lock(&base->lock);
>> -	nextevt = next_timer_interrupt(base, basej);
>> +	base_local = this_cpu_ptr(&timer_bases[BASE_LOCAL]);
>> +	base_global = this_cpu_ptr(&timer_bases[BASE_GLOBAL]);
>> +
>> +	raw_spin_lock(&base_local->lock);
>> +	raw_spin_lock_nested(&base_global->lock, SINGLE_DEPTH_NESTING);
>> +
>> +	nextevt_local = next_timer_interrupt(base_local, basej);
>> +	nextevt_global = next_timer_interrupt(base_global, basej);
>>  
>> -	if (base->timers_pending) {
>> +	/*
>> +	 * Check whether the local event is expiring before or at the same
>> +	 * time as the global event.
>> +	 *
>> +	 * Note, that nextevt_global and nextevt_local might be based on
>> +	 * different base->clk values. So it's not guaranteed that
>> +	 * comparing with empty bases results in a correct local_first.
>
> This ends like an unsolved mystery case. Could you add why one should
> not worry about an incorrect local_first?
>
> But seriously, how far apart can they get and what difference does it
> make?  At timer enqueue time clk equals jiffies. At this point one clk
> base could be at jiffies and the other might be a few jiffies before
> that.
> The next event (as in next_expiry) should be valid for both compare
> wise. Both must be larger than jiffies. The delta between jiffies and
> next event has to be less than NEXT_TIMER_MAX_DELTA for each base.
>
>> +	 */
>> +	if (base_local->timers_pending && base_global->timers_pending)
>> +		local_first = time_before_eq(nextevt_local, nextevt_global);
>> +	else
>> +		local_first = base_local->timers_pending;
>> +
>> +	nextevt = local_first ? nextevt_local : nextevt_global;
>> +
>> +	if (base_local->timers_pending || base_global->timers_pending) {
>>  		/* If we missed a tick already, force 0 delta */
>>  		if (time_before(nextevt, basej))
>>  			nextevt = basej;
>
> So if nextevt_local missed a tick and nextevt_global is
> NEXT_TIMER_MAX_DELTA-1 (so we get the largest difference possible
> between those two) then the time_before_eq() should still come out
> right. We could still miss more than one tick.
>

This problem was only there when comparing _empty_ bases
(!timer_base::timers_pending) because of the different base clocks and
the stale next_expiry.

But I didn't update the check and the comment after introducing the
forward of the next_expiry when !timer_base::timers_pending in
next_timer_interrupt(). So now it is sufficient to replace the
local_first detection by simply doing:

	local_first = time_before_eq(nextevt_local, nextevt_global);

Will fix it and will also add a comment to next_timer_interrupt() where
the next_expiry is updated when !timer_base::timers_pending.

Thanks,

	Anna-Maria