Re: [PATCH 0/2] Use exclusive lock for file_remove_privs

[Mateusz Guzik <mjguzik@gmail.com>]

On Wed, Aug 30, 2023 at 08:15:17PM +0200, Bernd Schubert wrote:
> While adding shared direct IO write locks to fuse Miklos noticed
> that file_remove_privs() needs an exclusive lock. I then
> noticed that btrfs actually has the same issue as I had in my patch,
> it was calling into that function with a shared lock.
> This series adds a new exported function file_needs_remove_privs(),
> which used by the follow up btrfs patch and will be used by the
> DIO code path in fuse as well. If that function returns any mask
> the shared lock needs to be dropped and replaced by the exclusive
> variant.
> 

No comments on the patchset itself.

So I figured an assert should be there on the write lock held, then the
issue would have been automagically reported.

Turns out notify_change has the following:
        WARN_ON_ONCE(!inode_is_locked(inode));

Which expands to:
static inline int rwsem_is_locked(struct rw_semaphore *sem)
{
        return atomic_long_read(&sem->count) != 0;
}

So it does check the lock, except it passes *any* locked state,
including just readers.

According to git blame this regressed from commit 5955102c9984
("wrappers for ->i_mutex access") by Al -- a bunch of mutex_is_locked
were replaced with inode_is_locked, which unintentionally provides
weaker guarantees.

I don't see a rwsem helper for wlock check and I don't think it is all
that beneficial to add. Instead, how about a bunch of lockdep, like so:
diff --git a/fs/attr.c b/fs/attr.c
index a8ae5f6d9b16..f47e718766d1 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -387,7 +387,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
        struct timespec64 now;
        unsigned int ia_valid = attr->ia_valid;

-       WARN_ON_ONCE(!inode_is_locked(inode));
+       lockdep_assert_held_write(&inode->i_rwsem);

        error = may_setattr(idmap, inode, ia_valid);
        if (error)

Alternatively hide it behind inode_assert_is_wlocked() or whatever other
name.

I can do the churn if this sounds like a plan.