[PATCH v3] hfsplus: annotate extents_lock nesting to silence lockdep false positive

[Mahmut Emin Kurhan]

lockdep reports a possible recursive locking on
HFSPLUS_I(inode)->extents_lock when truncating or extending a file on a
crafted image: the regular file holds its own extents_lock and, while
freeing or allocating blocks, reads the allocation file, whose
->get_block() takes the allocation file's extents_lock.  These are two
different inodes locked in a fixed order, so it is not a real deadlock,
only a missing nesting annotation.

tree->cnid and inode->i_ino share the CNID namespace, so fold the
B-tree tree_lock subclass helper and the extents_lock classification
into a single hfsplus_lock_class(cnid): the allocation file gets its own
subclass and every other inode uses the default, while the catalog,
extents and attributes B-trees keep the subclasses they had.  Take
extents_lock with mutex_lock_nested() at the sites that nest.  A genuine
recursion on one inode's extents_lock -- including the allocation
file on itself -- is still reported.

Found via coverage-guided fuzzing (syzkaller + lockdep) by Noroxi.

Signed-off-by: Mahmut Emin Kurhan <guvenlik@xxxxxxxxxx>
---
Slava, this is the reworked version you suggested: one CNID-keyed
hfsplus_lock_class() serving both the B-tree tree_lock and the inode
extents_lock, instead of a second enum/function.

The B-tree tree_lock keeps CATALOG/EXTENTS/ATTR (same values as before).
For extents_lock the only real nesting is a regular file's extents_lock
(held in truncate/extend) over the allocation file's extents_lock (taken
from its ->get_block() during block alloc/free), so the allocation file
gets its own subclass and everything else uses the default.  That also
drops the FIRSTUSER file/folder ambiguity from v2.

One behaviour change worth flagging: hfsplus_lock_class() returns
HFSPLUS_DEFAULT_MUTEX for an unexpected CNID where the old
hfsplus_btree_lock_class() called BUG().  The B-tree callers still only
pass CAT/EXT/ATTR, so this affects only the now-shared default path.
Happy to keep a WARN there if you prefer.

v3:
 - rework hfsplus_btree_lock_class() into a single CNID-keyed
   hfsplus_lock_class() shared by tree_lock and extents_lock (Slava
   Dubeyko); drop the separate enum/function and the FIRSTUSER check
v2:
 - shorten the comment, rename the subclass, use FIRSTUSER (superseded)
 fs/hfsplus/bfind.c      |  2 +-
 fs/hfsplus/extents.c    | 18 +++++++++++-------
 fs/hfsplus/hfsplus_fs.h | 39 +++++++++++++++++++++------------------
 fs/hfsplus/super.c      |  2 +-
 fs/hfsplus/xattr.c      |  3 ++-
 5 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/fs/hfsplus/bfind.c b/fs/hfsplus/bfind.c
index ca9813f58a..ed72d61f64 100644
--- a/fs/hfsplus/bfind.c
+++ b/fs/hfsplus/bfind.c
@@ -27,7 +27,7 @@ int hfs_find_init(struct hfs_btree *tree, struct hfs_find_data *fd)
 	hfs_dbg("cnid %d, caller %ps\n",
 		tree->cnid, __builtin_return_address(0));
 	mutex_lock_nested(&tree->tree_lock,
-			hfsplus_btree_lock_class(tree));
+			hfsplus_lock_class(tree->cnid));
 	return 0;
 }
 
diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
index eb7c11524d..ae1369a4cb 100644
--- a/fs/hfsplus/extents.c
+++ b/fs/hfsplus/extents.c
@@ -150,7 +150,8 @@ int hfsplus_ext_write_extent(struct inode *inode)
 {
 	int res;
 
-	mutex_lock(&HFSPLUS_I(inode)->extents_lock);
+	mutex_lock_nested(&HFSPLUS_I(inode)->extents_lock,
+			  hfsplus_lock_class(inode->i_ino));
 	res = hfsplus_ext_write_extent_locked(inode);
 	mutex_unlock(&HFSPLUS_I(inode)->extents_lock);
 
@@ -261,7 +262,8 @@ int hfsplus_get_block(struct inode *inode, sector_t iblock,
 	if (inode->i_ino == HFSPLUS_EXT_CNID)
 		return -EIO;
 
-	mutex_lock(&hip->extents_lock);
+	mutex_lock_nested(&hip->extents_lock,
+			  hfsplus_lock_class(inode->i_ino));
 
 	/*
 	 * hfsplus_ext_read_extent will write out a cached extent into
@@ -430,7 +432,7 @@ int hfsplus_free_fork(struct super_block *sb, u32 cnid,
 				     total_blocks);
 		total_blocks = start;
 		mutex_lock_nested(&fd.tree->tree_lock,
-			hfsplus_btree_lock_class(fd.tree));
+			hfsplus_lock_class(fd.tree->cnid));
 	} while (total_blocks > blocks);
 	hfs_find_exit(&fd);
 
@@ -454,7 +456,8 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout)
 		return -ENOSPC;
 	}
 
-	mutex_lock(&hip->extents_lock);
+	mutex_lock_nested(&hip->extents_lock,
+			  hfsplus_lock_class(inode->i_ino));
 	if (hip->alloc_blocks == hip->first_blocks)
 		goal = hfsplus_ext_lastblock(hip->first_extents);
 	else {
@@ -576,7 +579,8 @@ void hfsplus_file_truncate(struct inode *inode)
 	blk_cnt = (inode->i_size + HFSPLUS_SB(sb)->alloc_blksz - 1) >>
 			HFSPLUS_SB(sb)->alloc_blksz_shift;
 
-	mutex_lock(&hip->extents_lock);
+	mutex_lock_nested(&hip->extents_lock,
+			  hfsplus_lock_class(inode->i_ino));
 
 	alloc_cnt = hip->alloc_blocks;
 	if (blk_cnt == alloc_cnt)
@@ -597,7 +601,7 @@ void hfsplus_file_truncate(struct inode *inode)
 			hfsplus_dump_extent(hip->first_extents);
 			hip->first_blocks = blk_cnt;
 			mutex_lock_nested(&fd.tree->tree_lock,
-				hfsplus_btree_lock_class(fd.tree));
+				hfsplus_lock_class(fd.tree->cnid));
 			break;
 		}
 		res = __hfsplus_ext_cache_extent(&fd, inode, alloc_cnt);
@@ -612,7 +616,7 @@ void hfsplus_file_truncate(struct inode *inode)
 				     alloc_cnt - start, alloc_cnt - blk_cnt);
 		hfsplus_dump_extent(hip->cached_extents);
 		mutex_lock_nested(&fd.tree->tree_lock,
-				hfsplus_btree_lock_class(fd.tree));
+				hfsplus_lock_class(fd.tree->cnid));
 		if (blk_cnt > start) {
 			hip->extent_state |= HFSPLUS_EXT_DIRTY;
 			break;
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 916e6552e3..22b961b454 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -30,11 +30,19 @@ typedef int (*btree_keycmp)(const hfsplus_btree_key *,
 
 #define NODE_HASH_SIZE	256
 
-/* B-tree mutex nested subclasses */
-enum hfsplus_btree_mutex_classes {
-	CATALOG_BTREE_MUTEX,
-	EXTENTS_BTREE_MUTEX,
-	ATTR_BTREE_MUTEX,
+/*
+ * Nested-locking subclasses shared by the B-tree tree_lock and the inode
+ * extents_lock.  Both are keyed by CNID (tree->cnid or inode->i_ino), so one
+ * mapping serves both.  The allocation file gets its own subclass because a
+ * regular file's extents_lock is held across block alloc/free, which takes the
+ * allocation file's extents_lock in turn; every other inode uses the default.
+ */
+enum hfsplus_mutex_classes {
+	HFSPLUS_CATALOG_MUTEX,
+	HFSPLUS_EXTENTS_MUTEX,
+	HFSPLUS_ATTR_MUTEX,
+	HFSPLUS_ALLOC_MUTEX,
+	HFSPLUS_DEFAULT_MUTEX,
 };
 
 /* An HFS+ BTree held in memory */
@@ -547,25 +555,20 @@ static inline __be32 __hfsp_ut2mt(time64_t ut)
 	return cpu_to_be32(lower_32_bits(ut) + HFSPLUS_UTC_OFFSET);
 }
 
-static inline enum hfsplus_btree_mutex_classes
-hfsplus_btree_lock_class(struct hfs_btree *tree)
+static inline unsigned int hfsplus_lock_class(u32 cnid)
 {
-	enum hfsplus_btree_mutex_classes class;
-
-	switch (tree->cnid) {
+	switch (cnid) {
 	case HFSPLUS_CAT_CNID:
-		class = CATALOG_BTREE_MUTEX;
-		break;
+		return HFSPLUS_CATALOG_MUTEX;
 	case HFSPLUS_EXT_CNID:
-		class = EXTENTS_BTREE_MUTEX;
-		break;
+		return HFSPLUS_EXTENTS_MUTEX;
 	case HFSPLUS_ATTR_CNID:
-		class = ATTR_BTREE_MUTEX;
-		break;
+		return HFSPLUS_ATTR_MUTEX;
+	case HFSPLUS_ALLOC_CNID:
+		return HFSPLUS_ALLOC_MUTEX;
 	default:
-		BUG();
+		return HFSPLUS_DEFAULT_MUTEX;
 	}
-	return class;
 }
 
 static inline
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index ff7d6b3336..49059bf3e7 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -152,7 +152,7 @@ static int hfsplus_system_write_inode(struct inode *inode)
 	hfsplus_inode_write_fork(inode, fork);
 	if (tree) {
 		mutex_lock_nested(&tree->tree_lock,
-				  hfsplus_btree_lock_class(tree));
+				  hfsplus_lock_class(tree->cnid));
 		int err = hfs_btree_write(tree);
 		mutex_unlock(&tree->tree_lock);
 
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 21a1c196c7..f540d4c382 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -264,7 +264,8 @@ static int hfsplus_create_attributes_file(struct super_block *sb)
 						    sbi->sect_count,
 						    HFSPLUS_ATTR_CNID);
 
-	mutex_lock(&hip->extents_lock);
+	mutex_lock_nested(&hip->extents_lock,
+			  hfsplus_lock_class(attr_file->i_ino));
 	hip->clump_blocks = clump_size >> sbi->alloc_blksz_shift;
 	mutex_unlock(&hip->extents_lock);
 
-- 
2.43.0