Re: hfsplus: possible recursive locking on extents_lock during unlink/truncate

Re: hfsplus: possible recursive locking on extents_lock during unlink/truncate

[Viacheslav Dubeyko]

On Thu, 2026-10-01 at 02:58 +0400, Kullanılmıyor Pasif wrote:
> # hfsplus: lockdep "possible recursive locking" on extents_lock
> during unlink/truncate
> 
> **Reporter:** Noroxi (Mahmut Emin Kurhan <guvenlik@noroxi.com>)
> **Subsystem:** fs/hfsplus
> **Class:** lockdep false-positive (possible recursive locking) — CWE-
> 667-ish
> **Severity:** low — lockdep splat while mounting/using a crafted HFS+
> image. Analysis
> below indicates this is a missing lock-nesting annotation, NOT a real
> runtime
> deadlock (the two locks are different inodes of the same lock class,
> acquired in a
> fixed order). No memory corruption.
> **Status:** analysis from source; observed under syzkaller
> (kmemleak/lockdep) fuzzing
> of HFS+ image mounts. No standalone reproducer extracted yet.
> 
> ## lockdep report
> 
> ```
> WARNING: possible recursive locking detected
> syz.3.60/2437 is trying to acquire lock:
>   (&HFSPLUS_I(inode)->extents_lock){+.+.}, at: hfsplus_get_block+...
> fs/hfsplus/extents.c:264
> but task is already holding lock:
>   (&HFSPLUS_I(inode)->extents_lock){+.+.}, at:
> hfsplus_file_truncate+... fs/hfsplus/extents.c:579
>  May be due to missing lock nesting notation
> ```
> 
> ## Call chain (from the report stack)
> 
> ```
> hfsplus_unlink                 fs/hfsplus/dir.c:420
>  hfsplus_delete_inode          fs/hfsplus/inode.c:553
>   hfsplus_file_truncate        fs/hfsplus/extents.c:579   <--
> mutex_lock(inode A ->extents_lock)
>    hfsplus_free_extents        fs/hfsplus/extents.c:364
>     hfsplus_block_free         fs/hfsplus/bitmap.c:185     <-- reads
> the allocation bitmap file
>      block_read_full_folio     fs/buffer.c
>       hfsplus_get_block        fs/hfsplus/extents.c:264   <--
> mutex_lock(inode B ->extents_lock)
> ```
> 
> ## Analysis (why this is annotation, not a real deadlock)
> 
> - **inode A** = the regular file being unlinked/truncated.
> - **inode B** = the **allocation bitmap file** (`sbi->alloc_file`):
> `hfsplus_block_free()`
>   reads a page of the allocation file, which goes through `-
> >get_block` and locks the
>   allocation file's own `extents_lock`.
> - The two lock addresses in the report differ, so these are two
> *different* inode
>   instances of the *same* lock class (`extents_lock`). lockdep flags
> this as
>   "recursive" and itself notes "May be due to missing lock nesting
> notation".
> - The acquisition order is fixed: a regular file's `extents_lock` is
> taken first (in
>   truncate/extend), then the allocation file's `extents_lock` while
> freeing/allocating
>   blocks. There is no reverse path (the allocation file's block I/O
> does not take a
>   regular file's `extents_lock`), so no real ABBA/self-deadlock
> exists.
> 
> ## Suggested fix direction
> 
> hfsplus already solves the identical problem for the B-tree
> `tree_lock` via
> `hfsplus_btree_lock_class()` (fs/hfsplus/hfsplus_fs.h), which returns
> a distinct
> lockdep subclass per tree CNID and is used with
> `mutex_lock_nested()`.
> 
> The same pattern applied to `extents_lock` would resolve this: assign
> a distinct
> lockdep subclass to the special-inode `extents_lock` (at least the
> allocation file,
> and for completeness the attributes/catalog files) and take it with
> `mutex_lock_nested(&hip->extents_lock,
> hfsplus_extents_lock_class(inode))` at the five
> lock sites (extents.c:153,264,457,579 and xattr.c:267). Regular files
> keep the default
> subclass, so a genuine regular-file-vs-regular-file recursion would
> still be detected.
> 
> We can provide a candidate patch if useful; deferring the exact
> subclass scheme to the
> maintainers since the lock design is yours.
> 
> ## Disclosure
> Normal fs bug report (not a security embargo). To:
> linux-fsdevel@vger.kernel.org;
> Cc: Viacheslav Dubeyko, John Paul Adrian Glaubitz, Yangtao Li, linux-
> kernel.
> 
> ## Credit
> Found by Noroxi via coverage-guided fuzzing (syzkaller) of HFS+ image
> mounts.

Please, send the formal patch and we can start the regular review and
discussion.

Thanks,
Slava.

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

[Mahmut Emin Kurhan]

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.
+ */
+enum hfsplus_extents_mutex_classes {
+	HFSPLUS_EXTENTS_LOCK_NORMAL,
+	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)
+{
+	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;
+	}
+}
+
 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);
 
-- 
2.43.0

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

[Viacheslav Dubeyko]

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

[PATCH v2] 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 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.

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@noroxi.com>
---
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,
+};
+
 /* 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;
+
+	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);
 
-- 
2.43.0

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@noroxi.com>
> ---
> 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);
>  

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

[Viacheslav Dubeyko]

On Fri, 2026-10-02 at 23:18 +0400, Kullanılmıyor Pasif wrote:
> Hi Slava,
> Thanks. You're right that tree->cnid and inode->i_ino match for the
> special inodes. hfsplus_btree_lock_class() itself can't be reused as-
> is, though: it takes a struct hfs_btree * and BUG()s on any CNID that
> isn't one of the three B-trees (CATALOG/EXTENTS/ATTR), while
> extents_lock is taken on regular inodes and on the allocation file,
> which are not B-trees.

Technically speaking, we can rework hfsplus_btree_lock_class() to be
applicable for both cases. This was my point.

> Looking more carefully, there is only one actual extents_lock
> nesting: a regular inode's extents_lock is held in
> hfsplus_file_truncate()/hfsplus_file_extend() while block alloc/free
> reads sbi->alloc_file (hfsplus_block_free() -> read_mapping_page() ->
> hfsplus_get_block() on the allocation file). The catalog/attr inodes
> are not taken nested under a regular inode's extents_lock (the
> extents-overflow inode returns early in hfsplus_get_block(), and the
> catalog/attr trees are reached under their own tree_lock).
> So the parallel enum + HFSPLUS_FIRSTUSER_CNID check is overkill (and,
> as you note, FIRSTUSER also covers folders). I'll simplify to a
> single distinction: give only the allocation file's extents_lock a
> separate subclass, leaving every other inode at the default. That
> removes the duplication and the file/folder ambiguity.
> Does that direction look right to you? I'll send v3 accordingly and
> fold the same into the HFS side.

I am not sure that I completely follow to your vision. Simply, send
another version of the patch and we will discuss it.

Thanks,
Slava.

> Thanks,
> Mahmut
> 
> Viacheslav Dubeyko <slava@dubeyko.com>, 2 Eki 2026 Cum, 22:10
> tarihinde şunu yazdı:
> > 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@noroxi.com>
> > > ---
> > > 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);
> > >  

[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@noroxi.com>
---
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

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@noroxi.com>
> ---
> 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.

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

[Mahmut Emin Kurhan]

Hi Slava,

Thanks. You're right that tree->cnid and inode->i_ino match for the special
inodes. hfsplus_btree_lock_class() itself can't be reused as-is, though: it
takes a struct hfs_btree * and BUG()s on any CNID that isn't one of the three
B-trees (CATALOG/EXTENTS/ATTR), while extents_lock is taken on regular inodes
and on the allocation file, which are not B-trees.

Looking more carefully, there is only one actual extents_lock nesting: a
regular inode's extents_lock is held in hfsplus_file_truncate() /
hfsplus_file_extend() while block alloc/free reads sbi->alloc_file
(hfsplus_block_free() -> read_mapping_page() -> hfsplus_get_block() on the
allocation file). The catalog/attr inodes are not taken nested under a regular
inode's extents_lock (the extents-overflow inode returns early in
hfsplus_get_block(), and the catalog/attr trees are reached under their own
tree_lock).

So the parallel enum + HFSPLUS_FIRSTUSER_CNID check is overkill (and, as you
note, FIRSTUSER also covers folders). I'll simplify to a single distinction:
give only the allocation file's extents_lock a separate subclass, leaving every
other inode at the default. That removes the duplication and the file/folder
ambiguity.

Does that direction look right to you? I'll send v3 accordingly and fold the
same into the HFS side.

Thanks,
Mahmut