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

[Viacheslav Dubeyko]

On Fri, 2026-10-02 at 23:16 +0200, Mahmut Emin Kurhan wrote:
> 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.

Reported-by: tag?

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

I prefer to keep naming convention like it was before. Because,
CATALOG_BTREE_MUTEX can be used in HFS and HFS+ without any changes in
the future. I still would like to introduce something like library that
contain similar code in HFS/HFS+. 

>  };
>  
>  /* 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);
>  

The extents_lock is initialized in two places: fs/hfsplus/super.c:86
(hfsplus_iget() path) and fs/hfsplus/inode.c:494 (hfsplus_new_inode()).
Each mutex_init() site gets its own static lockdep key, so lockdep sees
two unrelated extents_lock classes:
- inodes read from disk, which includes all system files;
- inodes created during this mount.

So a nesting through a freshly created file never conflicts with one
through an existing file, and ordering checks between the two groups
are lost. That's an existing problem, not caused by this patch. Moving
the mutex_init() into hfsplus_alloc_inode(), or into a single helper,
would fix it. What do you think?

Thanks,
Slava.