On 8/31/23 12:18, Mateusz Guzik wrote:
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.
I guess that might help to discover these issues. Maybe keep the
existing WARN_ON_ONCE, as it would annotate a missing lock, when lockdep
is turned off?
Another code path is file_modified() and I just wonder if there are more
issues.
btrfs_punch_hole() takes inode->i_mmap_lock. Although I guess that
should be found by the existing WARN_ON_ONCE? Actually same in
btrfs_fallocate(). Or does btrfs always have IS_NOSEC?
Thanks,
Bernd
