Received-SPF: pass (google.com: domain of linux-kernel+bounces-94767-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <8f586bd2-c436-4334-92af-762a284e1101@akamai.com>
Date: Wed, 6 Mar 2024 17:40:11 -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>
Cc: Steven Rostedt <rostedt@goodmis.org>, 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>
 <854e523c-c467-47f6-b977-933cbaadeb62@akamai.com>
 <20240306221650.sw3lha7kca2quv63@treble>
From: Jason Baron <jbaron@akamai.com>
In-Reply-To: <20240306221650.sw3lha7kca2quv63@treble>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 3/6/24 5:16 PM, Josh Poimboeuf wrote:
> On Wed, Mar 06, 2024 at 03:12:07PM -0500, Jason Baron wrote:
>>
>>
>> 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.
> 
> Ah, this code is really subtle :-/
> 
>> So I think a concurrent disable could then see the -1 in tmp and still
>> trigger the WARN.
> 
> I think this shouldn't be possible, for the same reason that
> static_key_slow_try_dec() warns on -1:  key->enabled can only be -1
> during the first enable.  And disable should never be called before
> then.

hmm, right but I think in this case the reproducer is writing to a sysfs 
file to enable/disable randomly so i'm not sure if there is anything 
that would enforce that ordering. I guess you could try the reproducer, 
I haven't really looked at it in any detail.

The code in question here is in mm/vmscan.c which actually already takes 
the local 'state_mutex' for some cases. So that could be extended I 
think easily to avoid this warning.

> 
>> 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?
> 
> My patch subtly changed the "key->enabled > 0" to "key->enabled != 0".
> If I change that back then it should be fine.
> 
>> 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...
> 
> Yeah, this code needs better comments.  Let me turn it into a proper
> patch.
>