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.
