linux-kernel - Re: [Bug] WARNING in static_key_disable

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: <20240306234253.zporv6cypoc7yihs@treble>
Date: Wed, 6 Mar 2024 15:42:53 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Jason Baron <jbaron@...mai.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Sam Sun <samsun1006219@...il.com>,
	linux-kernel@...r.kernel.org, syzkaller@...glegroups.com,
	xrivendell7@...il.com, ardb@...nel.org, peterz@...radead.org,
	linux-mm@...ck.org, akpm@...ux-foundation.org,
	Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [Bug] WARNING in static_key_disable_cpuslocked

On Wed, Mar 06, 2024 at 05:40:11PM -0500, Jason Baron wrote:
> 
> 
> 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.

Hm, right... For now I'll just continue to allow "disable before enable"
(or "double disable") since it may be harmless and I don't want to
introduce any unnecessary constraints, unless we manage to convince
ourselves that it's the right thing to do.

I'll work up a patch.

-- 
Josh