linux-kernel - Re: Are jump labels broken on 6.11-rc1?

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87wmks2xhi.ffs@tglx>
Date: Wed, 07 Aug 2024 16:55:53 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Peter Zijlstra <peterz@...radead.org>
Cc: "Darrick J. Wong" <djwong@...nel.org>, Chandan Babu R
 <chandanbabu@...nel.org>, Matthew Wilcox <willy@...radead.org>, xfs
 <linux-xfs@...r.kernel.org>, linux-fsdevel
 <linux-fsdevel@...r.kernel.org>, linux-kernel
 <linux-kernel@...r.kernel.org>, x86@...nel.org
Subject: Re: Are jump labels broken on 6.11-rc1?

On Wed, Aug 07 2024 at 16:34, Peter Zijlstra wrote:
> On Wed, Aug 07, 2024 at 04:03:12PM +0200, Thomas Gleixner wrote:
>
>> > +	if (static_key_dec(key, true)) // dec-not-one
>> 
>> Eeew.
>
> :-) I knew you'd hate on that

So you added it just to make me grumpy enough to fix it for you, right?

>> +/*
>> + * Fastpath: Decrement if the reference count is greater than one
>> + *
>> + * Returns false, if the reference count is 1 or -1 to force the caller
>> + * into the slowpath.
>> + *
>> + * The -1 case is to handle a decrement during a concurrent first enable,
>> + * which sets the count to -1 in static_key_slow_inc_cpuslocked(). As the
>> + * slow path is serialized the caller will observe 1 once it acquired the
>> + * jump_label_mutex, so the slow path can succeed.
>> + */
>> +static bool static_key_dec_not_one(struct static_key *key)
>> +{
>> +	int v = static_key_dec(key, true);
>> +
>> +	return v != 1 && v != -1;
>
> 	if (v < 0)
> 		return false;

Hmm. I think we should do:

#define KEY_ENABLE_IN_PROGRESS		-1

or even a more distinct value like (INT_MIN / 2)

and replace all the magic -1 numbers with it. Then the check becomes
explicit:

        if (v == KEY_ENABLE_IN_PROGRESS)
        	return false;

> 	/*
> 	 * Notably, 0 (underflow) returns true such that it bails out
> 	 * without doing anything.
> 	 */
> 	return v != 1;
>
> Perhaps?

Sure.

>> +}
>> +
>> +/*
>> + * Slowpath: Decrement and test whether the refcount hit 0.
>> + *
>> + * Returns true if the refcount hit zero, i.e. the previous value was one.
>> + */
>> +static bool static_key_dec_and_test(struct static_key *key)
>> +{
>> +	int v = static_key_dec(key, false);
>> +
>> +	lockdep_assert_held(&jump_label_mutex);
>> +	return v == 1;
>>  }
>
> But yeah, this is nicer!

:)

Thanks,

        tglx