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: <ZP5OfhXhPkntaEkc@casper.infradead.org>
Date:   Mon, 11 Sep 2023 00:17:18 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Dave Chinner <david@...morbit.com>
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 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 ...

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);
}

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.

I'll keep reading this thread to see if some consensus emerges, but I'm
not optimistic.