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

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-10-01 at 11:52 +0200, Mahmut Emin Kurhan wrote:
> lockdep reports possible recursive locking on HFSPLUS_I(inode)-
> >extents_lock
> when unlinking a file on a crafted image:
> 
>   hfsplus_file_truncate()  holds inode A ->extents_lock
>    hfsplus_free_extents()
>     hfsplus_block_free()   reads the allocation bitmap file
>      block_read_full_folio()
>       hfsplus_get_block()  takes inode B ->extents_lock
> 
> inode A is the regular file being truncated and inode B is the
> allocation
> file (sbi->alloc_file, i_ino == HFSPLUS_ALLOC_CNID). They are two
> different
> inodes of the same lock class, taken in a fixed order (regular file
> first,
> then the allocation/attributes/catalog file), so this is not a real
> deadlock, only a missing lock-nesting annotation.
> 
> Mirror the existing hfsplus_btree_lock_class() approach: give the
> special
> inodes distinct extents_lock subclasses and take the lock with
> mutex_lock_nested(). Regular files keep the default subclass, so a
> genuine
> regular-vs-regular recursion would still be detected.
> 
> Found via coverage-guided fuzzing (syzkaller + lockdep) by Noroxi.
> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@noroxi.com>
> ---
>  fs/hfsplus/extents.c    | 12 ++++++++----
>  fs/hfsplus/hfsplus_fs.h | 28 ++++++++++++++++++++++++++++
>  fs/hfsplus/xattr.c      |  3 ++-
>  3 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524..54a05eaa8 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_extents_lock_class(inode));
>  	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_extents_lock_class(inode));
>  
>  	/*
>  	 * hfsplus_ext_read_extent will write out a cached extent
> into
> @@ -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_extents_lock_class(inode));
>  	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_extents_lock_class(inode));
>  
>  	alloc_cnt = hip->alloc_blocks;
>  	if (blk_cnt == alloc_cnt)
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 1e5b58e6a..2cbd8074c 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -36,6 +36,20 @@ enum hfsplus_btree_mutex_classes {
>  	ATTR_BTREE_MUTEX,
>  };
>  
> +/*
> + * extents_lock nested subclasses. Special inodes (allocation,
> attributes,
> + * catalog) use the same lock class as regular files but their
> extents_lock is
> + * taken while a regular file extents_lock is already held -- e.g.
> the
> + * allocation file is read from hfsplus_block_free() during
> truncate. Give them
> + * distinct subclasses so lockdep does not report a false recursive
> locking.
> + */

Could we have more shorter and more informative comment? Currently, it
is long and pretty useless.

> +enum hfsplus_extents_mutex_classes {
> +	HFSPLUS_EXTENTS_LOCK_NORMAL,

Maybe HFSPLUS_EXTENTS_LOCK_REGULAR_FILE?

> +	HFSPLUS_EXTENTS_LOCK_ALLOC,
> +	HFSPLUS_EXTENTS_LOCK_ATTR,
> +	HFSPLUS_EXTENTS_LOCK_CATALOG,
> +};
> +
>  /* An HFS+ BTree held in memory */
>  struct hfs_btree {
>  	struct super_block *sb;
> @@ -572,6 +586,20 @@ hfsplus_btree_lock_class(struct hfs_btree *tree)
>  	return class;
>  }
>  
> +static inline unsigned int hfsplus_extents_lock_class(struct inode
> *inode)

Are we longer than 80 symbols? Then we need to rework it slightly, it's
too long.

> +{
> +	switch (inode->i_ino) {
> +	case HFSPLUS_ALLOC_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_ALLOC;
> +	case HFSPLUS_ATTR_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_ATTR;
> +	case HFSPLUS_CAT_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_CATALOG;
> +	default:
> +		return HFSPLUS_EXTENTS_LOCK_NORMAL;

What about HFS_EXT_CNID? Should we process other reserved IDs too [1]?
Because, they are not regular file.

Thanks,
Slava.

> +	}
> +}
> +
>  static inline
>  bool is_bnode_offset_valid(struct hfs_bnode *node, u32 off)
>  {
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 21a1c196c..7bf4ba8f1 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_extents_lock_class(attr_file));
>  	hip->clump_blocks = clump_size >> sbi->alloc_blksz_shift;
>  	mutex_unlock(&hip->extents_lock);
>  

[1]
https://elixir.bootlin.com/linux/v7.3-rc5/source/include/linux/hfs_common.h#L141