Received-SPF: pass (google.com: domain of linux-kernel+bounces-94578-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <854e523c-c467-47f6-b977-933cbaadeb62@akamai.com>
Date: Wed, 6 Mar 2024 15:12:07 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Bug] WARNING in static_key_disable_cpuslocked
Content-Language: en-US
To: Josh Poimboeuf <jpoimboe@kernel.org>, Steven Rostedt <rostedt@goodmis.org>
Cc: Sam Sun <samsun1006219@gmail.com>, linux-kernel@vger.kernel.org,
        syzkaller@googlegroups.com, xrivendell7@gmail.com, ardb@kernel.org,
        peterz@infradead.org, linux-mm@kvack.org, akpm@linux-foundation.org,
        Paolo Bonzini <pbonzini@redhat.com>
References: <CAEkJfYNNZftjpYBpnH4tEnm82orKtQ6SQn9i3sg7YNO-Df3tSQ@mail.gmail.com>
 <20240306105420.6a6bea2c@gandalf.local.home>
 <20240306193101.s2g33o4viqi2azf3@treble>
From: Jason Baron <jbaron@akamai.com>
In-Reply-To: <20240306193101.s2g33o4viqi2azf3@treble>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 3/6/24 2:31 PM, Josh Poimboeuf wrote:
> On Wed, Mar 06, 2024 at 10:54:20AM -0500, Steven Rostedt wrote:
>> Now I guess the question is, why is something trying to disable something
>> that is not enabled? Is the above scenario OK? Or should the users of
>> static_key also prevent this?
> 
> Apparently that's an allowed scenario, as the jump label code seems to
> be actively trying to support it.  Basically the last one "wins".
> 
> See for example:
> 
>    1dbb6704de91 ("jump_label: Fix concurrent static_key_enable/disable()")
> 
> Also the purpose of the first atomic_read() is to do a quick test before
> grabbing the jump lock.  So instead of grabbing the jump lock earlier,
> it should actually do the first test atomically:

Makes sense but the enable path can also set key->enabled to -1. So I 
think a concurrent disable could then see the -1 in tmp and still 
trigger the WARN. So I think we could change the WARN to be:
WARN_ON_ONCE(tmp != 0 && tmp != -1). And also add a similar check
for enable if we have enable vs enable racing?

Although it seems like the set key->enabled to -1 while used in the 
inc/dec API isn't really doing anything in the enable/disable part here?
But then the key->enabled I think has to move in front of the 
jump_label_update() to make that part work right...

Thanks,

-Jason


> 
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index d9c822bbffb8..f29c47930d46 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -191,11 +191,14 @@ EXPORT_SYMBOL_GPL(static_key_slow_inc);
>   
>   void static_key_enable_cpuslocked(struct static_key *key)
>   {
> +	int tmp;
> +
>   	STATIC_KEY_CHECK_USE(key);
>   	lockdep_assert_cpus_held();
>   
> -	if (atomic_read(&key->enabled) > 0) {
> -		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
> +	tmp = atomic_read(&key->enabled);
> +	if (tmp != 0) {
> +		WARN_ON_ONCE(tmp != 1);
>   		return;
>   	}
>   
> @@ -222,11 +225,14 @@ EXPORT_SYMBOL_GPL(static_key_enable);
>   
>   void static_key_disable_cpuslocked(struct static_key *key)
>   {
> +	int tmp;
> +
>   	STATIC_KEY_CHECK_USE(key);
>   	lockdep_assert_cpus_held();
>   
> -	if (atomic_read(&key->enabled) != 1) {
> -		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
> +	tmp = atomic_read(&key->enabled);
> +	if (tmp != 1) {
> +		WARN_ON_ONCE(tmp != 0);
>   		return;
>   	}
>