Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Mateusz Guzik <mjguzik@gmail.com>
To:     brauner@kernel.org
Cc:     viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, bernd.schubert@fastmail.fm,
        Mateusz Guzik <mjguzik@gmail.com>
Subject: [RFC PATCH] vfs: add inode lockdep assertions
Date:   Thu, 31 Aug 2023 17:14:14 +0200
Message-Id: <20230831151414.2714750-1-mjguzik@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@ddn.com/

---
 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