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