linux-kernel - Re: [PATCH 1/5] locking: Add rwsem_is_write

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: <ZP5llBaVrJteHQf3@dread.disaster.area>
Date:   Mon, 11 Sep 2023 10:55:48 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, Chandan Babu R <chandan.babu@...cle.com>,
        "Darrick J . Wong" <djwong@...nel.org>, linux-xfs@...r.kernel.org
Subject: Re: [PATCH 1/5] locking: Add rwsem_is_write_locked()

On Mon, Sep 11, 2023 at 12:17:18AM +0100, Matthew Wilcox wrote:
> On Mon, Sep 11, 2023 at 08:56:45AM +1000, Dave Chinner wrote:
> > On Fri, Sep 08, 2023 at 12:44:34PM +0200, Peter Zijlstra wrote:
> > > Agreed, and this is fine. However there's been some very creative
> > > 'use' of the _is_locked() class of functions in the past that did not
> > > follow 'common' sense.
> > > 
> > > If all usage was: I should be holding this, lets check. I probably
> > > wouldn't have this bad feeling about things.
> > 
> > So your argument against such an interface is essentially "we can't
> > have nice things because someone might abuse them"?
> 
> Some people are very creative ...

Sure, but that's no reason to stop anyone else from making progress.

> I was thinking about how to handle this better.  We could have
> 
> static inline void rwsem_assert_locked(const struct rw_semaphore *sem)
> {
> 	BUG_ON(atomic_long_read(&sem->count) == 0);
> }
> 
> static inline void rwsem_assert_write_locked(const struct rw_semaphore *sem)
> {
> 	BUG_ON((atomic_long_read(&sem->count) & 1) != 1);
> }

We already have CONFIG_DEBUG_RWSEMS, so we can put these
introspection interfaces inside debug code, and make any attempt to
use them outside of debug builds break the build. e.g:

#if DEBUG_RWSEMS
/*
 * rwsem locked checks can only be used by conditionally compiled
 * subsystem debug code. It is not valid to use them in normal
 * production code.
 */
static inline bool rwsem_is_write_locked()
{
	....
}

static inline bool rwsem_is_locked()
{
	....
}
#else /* !DEBUG_RWSEMS */
#define rwsem_is_write_locked()		BUILD_BUG()
#define rwsem_is_locked()		BUILD_BUG()
#endif /* DEBUG_RWSEMS */

And now we simply add a single line to subsystem Kconfig debug
options to turn on rwsem introspection for their debug checks like
so:

 config XFS_DEBUG
 	bool "XFS Debugging support"
 	depends on XFS_FS
+	select RWSEM_DEBUG
 	help
 	  Say Y here to get an XFS build with many debugging features,
 	  including ASSERT checks, function wrappers around macros,

> but then we'd also need to change how XFS currently uses the ASSERT()
> macro to be ASSERT_LOCKED(ip, flags), and I understand this code is also
> used in userspace, so it'd involve changing that shim, and we're getting
> way past the amount of code I'm comfortable changing, and way past the
> amount of time I should be spending on this.
> 
> And then there'd be the inevitable bikeshedding about "don't use BUG_ON"
> and it's probably just for the best if I walk away at this point,
> becoming the third person to fail to remove the mrlock.

Yeah, which further points out how ridiculous the situation is. This
is useful debug code and it can *obviously* and *easily* be
constrained to debug environments.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com