Thread "Use exclusive lock for file_remove_privs" [1] reports an issue
which should have been found by asserts -- inode not write locked by the
caller.
It did not happen because the attempt to do it in notify_change:
WARN_ON_ONCE(!inode_is_locked(inode));
passes if the inode is only read-locked:
static inline int rwsem_is_locked(struct rw_semaphore *sem)
{
return atomic_long_read(&sem->count) != 0;
}
According to git blame this regressed from 2 commits:
1. 5955102c9984 ("wrappers for ->i_mutex access") which replaced a
bunch of mutex_is_locked with inode_is_locked
2. 9902af79c01a ("parallel lookups: actual switch to rwsem") which
implemented inode_is_locked as a mere check on the semaphore being
held in *any* manner
In order to remedy this I'm proposing lockdep-ing the check with 2
helpers: inode_assert_locked and inode_assert_write_locked
Below I'm adding the helpers and converting *some* of the spots modified
by the first patch. I boot tested it and nothing blow up on ext4, but
btrfs should cause a complaint.
I can finish the other spots originally touched by 1 and touch up the 3
uses I grepped in fs/namei.c, but ultimately filesystem maintainers are
going to have to patch their code at their leasure. On top of that there
are probably quite a few places which should assert, but don't.
Comments?
Link: https://lore.kernel.org/linux-fsdevel/20230830181519.2964941-1-bschubert@xxxxxxx/
---
fs/attr.c | 2 +-
fs/btrfs/xattr.c | 2 +-
fs/ext4/ext4.h | 4 ++--
fs/ext4/extents.c | 4 ++--
fs/ext4/inode.c | 4 ++--
include/linux/fs.h | 10 ++++++++++
6 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index a8ae5f6d9b16..90dec999a952 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));
+ inode_assert_write_locked(inode);
error = may_setattr(idmap, inode, ia_valid);
if (error)
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 96828a13dd43..46b268a433dd 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -120,7 +120,7 @@ int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode,
* locks the inode's i_mutex before calling setxattr or removexattr.
*/
if (flags & XATTR_REPLACE) {
- ASSERT(inode_is_locked(inode));
+ inode_assert_write_locked(inode);
di = btrfs_lookup_xattr(NULL, root, path,
btrfs_ino(BTRFS_I(inode)), name, name_len, 0);
if (!di)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 481491e892df..df428f22f624 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3364,8 +3364,8 @@ do { \
/* Update i_disksize. Requires i_rwsem to avoid races with truncate */
static inline void ext4_update_i_disksize(struct inode *inode, loff_t newsize)
{
- WARN_ON_ONCE(S_ISREG(inode->i_mode) &&
- !inode_is_locked(inode));
+ if (S_ISREG(inode->i_mode))
+ inode_assert_write_locked(inode);
down_write(&EXT4_I(inode)->i_data_sem);
if (newsize > EXT4_I(inode)->i_disksize)
WRITE_ONCE(EXT4_I(inode)->i_disksize, newsize);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 202c76996b62..149783ecfe16 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5588,8 +5588,8 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
BUG_ON(!rwsem_is_locked(&EXT4_I(inode1)->i_data_sem));
BUG_ON(!rwsem_is_locked(&EXT4_I(inode2)->i_data_sem));
- BUG_ON(!inode_is_locked(inode1));
- BUG_ON(!inode_is_locked(inode2));
+ inode_assert_write_locked(inode1);
+ inode_assert_write_locked(inode2);
ext4_es_remove_extent(inode1, lblk1, count);
ext4_es_remove_extent(inode2, lblk2, count);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 89737d5a1614..2ecdef6ddc88 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3797,7 +3797,7 @@ int ext4_update_disksize_before_punch(struct inode *inode, loff_t offset,
loff_t size = i_size_read(inode);
- WARN_ON(!inode_is_locked(inode));
+ inode_assert_write_locked(inode);
if (offset > size || offset + len < size)
return 0;
@@ -4068,7 +4068,7 @@ int ext4_truncate(struct inode *inode)
* have i_rwsem locked because it's not necessary.
*/
if (!(inode->i_state & (I_NEW|I_FREEING)))
- WARN_ON(!inode_is_locked(inode));
+ inode_assert_write_locked(inode);
trace_ext4_truncate_enter(inode);
if (!ext4_can_truncate(inode))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index c8ff4156a0a1..93d48b6b9f67 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -842,6 +842,16 @@ static inline void inode_lock_shared_nested(struct inode *inode, unsigned subcla
down_read_nested(&inode->i_rwsem, subclass);
}
+static inline void inode_assert_locked(struct inode *inode)
+{
+ lockdep_assert_held(&inode->i_rwsem);
+}
+
+static inline void inode_assert_write_locked(struct inode *inode)
+{
+ lockdep_assert_held_write(&inode->i_rwsem);
+}
+
static inline void filemap_invalidate_lock(struct address_space *mapping)
{
down_write(&mapping->invalidate_lock);
--
2.39.2
