linux-kernel - Re: [PATCH v2 4/5] xfs: Remove mrlock wrapper

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: <ZSMqb70f8mHCEaNl@dread.disaster.area>
Date:   Mon, 9 Oct 2023 09:17:19 +1100
From:   Dave Chinner <david@...morbit.com>
To:     "Matthew Wilcox (Oracle)" <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,
        Mateusz Guzik <mjguzik@...il.com>
Subject: Re: [PATCH v2 4/5] xfs: Remove mrlock wrapper

On Sat, Oct 07, 2023 at 09:35:42PM +0100, Matthew Wilcox (Oracle) wrote:
> mrlock was an rwsem wrapper that also recorded whether the lock was
> held for read or write.  Now that we can ask the generic code whether
> the lock is held for read or write, we can remove this wrapper and use
> an rwsem directly.
> 
> As the comment says, we can't use lockdep to assert that the ILOCK is
> held for write, because we might be in a workqueue, and we aren't able
> to tell lockdep that we do in fact own the lock.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@...radead.org>

.....

> @@ -338,10 +338,14 @@ xfs_assert_ilocked(
>  	struct xfs_inode	*ip,
>  	uint			lock_flags)
>  {
> +	/*
> +	 * Sometimes we assert the ILOCK is held exclusively, but we're in
> +	 * a workqueue, so lockdep doesn't know we're the owner.
> +	 */
>  	if (lock_flags & XFS_ILOCK_SHARED)
> -		rwsem_assert_held(&ip->i_lock.mr_lock);
> +		rwsem_assert_held(&ip->i_lock);
>  	else if (lock_flags & XFS_ILOCK_EXCL)
> -		BUG_ON(!ip->i_lock.mr_writer);
> +		__rwsem_assert_held_write(&ip->i_lock);

It took me ages to work out that the comment related to the "__"
variant of rwsem_assert_held_write() function. I really dislike the
use of "__" prefixes for a function with slightly different,
non-obvious semantics to the parent - it's way too subtle for it to
be clear that this is what the comment is refering to.

In this case, we effectively have rwsem_assert_held_write() that
does lockdep checks, and __rwsem_assert_held_write() that does not
do lockdep checks. Either the former needs to say "lockdep" or the
latter needs "nolockdep" in the name to indicate to the reader the
intent of the code calling the checking function....

Otherwise the code looks fine.

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