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

[Viacheslav Dubeyko]

On Thu, 2026-10-01 at 23:53 +0200, Mahmut Emin Kurhan wrote:
> lockdep reports a possible recursive locking on HFSPLUS_I(inode)-
> >extents_lock
> when unlinking a file on a crafted image: hfsplus_file_truncate()
> holds a
> regular file's extents_lock and, while freeing blocks, reads the
> allocation
> file, whose get_block() takes that file's own extents_lock. They are
> distinct
> inodes of the same lock class taken in a fixed order, so it is not a
> real
> deadlock, only a missing nesting annotation.
> 

As far as I can see, we have the same issue for HFS code.

> Give the special inodes (allocation, attributes, catalog) distinct
> extents_lock subclasses and take the lock with mutex_lock_nested(),
> mirroring
> hfsplus_btree_lock_class(). Regular files (CNID >=
> HFSPLUS_FIRSTUSER_CNID) keep
> their own subclass so a genuine regular-vs-regular recursion is still
> detected;
> the remaining reserved inodes use a separate subclass instead of
> being treated
> as regular files.
> 
> Found via coverage-guided fuzzing (syzkaller + lockdep) by Noroxi.
> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@xxxxxxxxxx>
> ---
> v2:
>  - shorten the comment (Slava Dubeyko)
>  - rename HFSPLUS_EXTENTS_LOCK_NORMAL to _REGULAR_FILE (Slava
> Dubeyko)
>  - use HFSPLUS_FIRSTUSER_CNID so reserved inodes are not treated as
>    regular files; add a separate _OTHER subclass (Slava Dubeyko)
>  - hfsplus_extents_lock_class() signature is 74 cols (< 80)
>  fs/hfsplus/extents.c    | 12 ++++++++----
>  fs/hfsplus/hfsplus_fs.h | 26 ++++++++++++++++++++++++++
>  fs/hfsplus/xattr.c      |  3 ++-
>  3 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c
> index eb7c11524d..54a05eaa8f 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 916e6552e3..7e2411b8f1 100644
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -37,6 +37,15 @@ enum hfsplus_btree_mutex_classes {
>  	ATTR_BTREE_MUTEX,
>  };
>  
> +/* lockdep subclasses for extents_lock: special inodes nest under
> regular files */
> +enum hfsplus_extents_mutex_classes {
> +	HFSPLUS_EXTENTS_LOCK_REGULAR_FILE,
> +	HFSPLUS_EXTENTS_LOCK_CATALOG,
> +	HFSPLUS_EXTENTS_LOCK_ALLOC,
> +	HFSPLUS_EXTENTS_LOCK_ATTR,
> +	HFSPLUS_EXTENTS_LOCK_OTHER,
> +};

After more careful consideration, I think it looks like a duplication.
We already have:

enum hfsplus_btree_mutex_classes {
	CATALOG_BTREE_MUTEX,
	EXTENTS_BTREE_MUTEX,
	ATTR_BTREE_MUTEX,
};

static inline enum hfsplus_btree_mutex_classes
hfsplus_btree_lock_class(struct hfs_btree *tree)
{
	enum hfsplus_btree_mutex_classes class;

	switch (tree->cnid) {
	case HFSPLUS_CAT_CNID:
		class = CATALOG_BTREE_MUTEX;
		break;
	case HFSPLUS_EXT_CNID:
		class = EXTENTS_BTREE_MUTEX;
		break;
	case HFSPLUS_ATTR_CNID:
		class = ATTR_BTREE_MUTEX;
		break;
	default:
		BUG();
	}
	return class;
}

Why we cannot reuse this already existing logic? Suggested logic uses
the inode->i_ino. But tree->cnid and inode->i_ino should be the same.
Maybe, we simply need to rework existing logic and reuse it? What do
you think?

> +
>  /* An HFS+ BTree held in memory */
>  struct hfs_btree {
>  	struct super_block *sb;
> @@ -568,6 +577,23 @@ hfsplus_btree_lock_class(struct hfs_btree *tree)
>  	return class;
>  }
>  
> +static inline unsigned int hfsplus_extents_lock_class(struct inode
> *inode)
> +{
> +	if (inode->i_ino >= HFSPLUS_FIRSTUSER_CNID)
> +		return HFSPLUS_EXTENTS_LOCK_REGULAR_FILE;

The HFSPLUS_FIRSTUSER_CNID coudl be as file as folder. Please, take a
look into my above comments. Maybe, we don't a new function at all.

Thanks,
Slava.

> +
> +	switch (inode->i_ino) {
> +	case HFSPLUS_CAT_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_CATALOG;
> +	case HFSPLUS_ALLOC_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_ALLOC;
> +	case HFSPLUS_ATTR_CNID:
> +		return HFSPLUS_EXTENTS_LOCK_ATTR;
> +	default:
> +		return HFSPLUS_EXTENTS_LOCK_OTHER;
> +	}
> +}
> +
>  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 21a1c196c7..7bf4ba8f18 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);
>