[PATCH v4 0/2] hfsplus: fix lock leak and refactor hidden dir search

[PATCH v4 0/2] hfsplus: fix lock leak and refactor hidden dir search

[Zilin Guan]

This series updates hidden-directory lookup handling in
hfsplus_fill_super().

Patch 1 fixes a B-tree lock leak on an error path in hidden-directory
lookup.

Patch 2 refactors the lookup path into a helper to encapsulate resource
lifetime and simplify error handling.

Changes in v4:
- Refactor hidden-dir lookup branch handling in hfsplus_fill_super().

Changes in v3:
- Use unlikely(err) for hfs_find_init() and hfsplus_cat_build_key() checks.
- Map hfs_brec_read() errors to -ENOENT to preserve existing behavior.

Changes in v2:
- Add patch 2 to extract hidden-directory lookup into a helper function.

Zilin Guan (2):
  hfsplus: fix held lock freed on hfsplus_fill_super()
  hfsplus: extract hidden directory search into a helper function

 fs/hfsplus/super.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v4 1/2] hfsplus: fix held lock freed on hfsplus_fill_super()

[Zilin Guan]

hfsplus_fill_super() calls hfs_find_init() to initialize a search
structure, which acquires tree->tree_lock. If the subsequent call to
hfsplus_cat_build_key() fails, the function jumps to the out_put_root
error label without releasing the lock. The later cleanup path then
frees the tree data structure with the lock still held, triggering a
held lock freed warning.

Fix this by adding the missing hfs_find_exit(&fd) call before jumping
to the out_put_root error label. This ensures that tree->tree_lock is
properly released on the error path.

The bug was originally detected on v6.13-rc1 using an experimental
static analysis tool we are developing, and we have verified that the
issue persists in the latest mainline kernel. The tool is specifically
designed to detect memory management issues. It is currently under active
development and not yet publicly available.

We confirmed the bug by runtime testing under QEMU with x86_64 defconfig,
lockdep enabled, and CONFIG_HFSPLUS_FS=y. To trigger the error path, we
used GDB to dynamically shrink the max_unistr_len parameter to 1 before
hfsplus_asc2uni() is called. This forces hfsplus_asc2uni() to naturally
return -ENAMETOOLONG, which propagates to hfsplus_cat_build_key() and
exercises the faulty error path. The following warning was observed
during mount:

	=========================
	WARNING: held lock freed!
	7.0.0-rc3-00016-gb4f0dd314b39 #4 Not tainted
	-------------------------
	mount/174 is freeing memory ffff888103f92000-ffff888103f92fff, with a lock still held there!
	ffff888103f920b0 (&tree->tree_lock){+.+.}-{4:4}, at: hfsplus_find_init+0x154/0x1e0
	2 locks held by mount/174:
	#0: ffff888103f960e0 (&type->s_umount_key#42/1){+.+.}-{4:4}, at: alloc_super.constprop.0+0x167/0xa40
	#1: ffff888103f920b0 (&tree->tree_lock){+.+.}-{4:4}, at: hfsplus_find_init+0x154/0x1e0

	stack backtrace:
	CPU: 2 UID: 0 PID: 174 Comm: mount Not tainted 7.0.0-rc3-00016-gb4f0dd314b39 #4 PREEMPT(lazy)
	Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
	Call Trace:
	<TASK>
	dump_stack_lvl+0x82/0xd0
	debug_check_no_locks_freed+0x13a/0x180
	kfree+0x16b/0x510
	? hfsplus_fill_super+0xcb4/0x18a0
	hfsplus_fill_super+0xcb4/0x18a0
	? __pfx_hfsplus_fill_super+0x10/0x10
	? srso_return_thunk+0x5/0x5f
	? bdev_open+0x65f/0xc30
	? srso_return_thunk+0x5/0x5f
	? pointer+0x4ce/0xbf0
	? trace_contention_end+0x11c/0x150
	? __pfx_pointer+0x10/0x10
	? srso_return_thunk+0x5/0x5f
	? bdev_open+0x79b/0xc30
	? srso_return_thunk+0x5/0x5f
	? srso_return_thunk+0x5/0x5f
	? vsnprintf+0x6da/0x1270
	? srso_return_thunk+0x5/0x5f
	? __mutex_unlock_slowpath+0x157/0x740
	? __pfx_vsnprintf+0x10/0x10
	? srso_return_thunk+0x5/0x5f
	? srso_return_thunk+0x5/0x5f
	? mark_held_locks+0x49/0x80
	? srso_return_thunk+0x5/0x5f
	? srso_return_thunk+0x5/0x5f
	? irqentry_exit+0x17b/0x5e0
	? trace_irq_disable.constprop.0+0x116/0x150
	? __pfx_hfsplus_fill_super+0x10/0x10
	? __pfx_hfsplus_fill_super+0x10/0x10
	get_tree_bdev_flags+0x302/0x580
	? __pfx_get_tree_bdev_flags+0x10/0x10
	? vfs_parse_fs_qstr+0x129/0x1a0
	? __pfx_vfs_parse_fs_qstr+0x3/0x10
	vfs_get_tree+0x89/0x320
	fc_mount+0x10/0x1d0
	path_mount+0x5c5/0x21c0
	? __pfx_path_mount+0x10/0x10
	? trace_irq_enable.constprop.0+0x116/0x150
	? trace_irq_enable.constprop.0+0x116/0x150
	? srso_return_thunk+0x5/0x5f
	? srso_return_thunk+0x5/0x5f
	? kmem_cache_free+0x307/0x540
	? user_path_at+0x51/0x60
	? __x64_sys_mount+0x212/0x280
	? srso_return_thunk+0x5/0x5f
	__x64_sys_mount+0x212/0x280
	? __pfx___x64_sys_mount+0x10/0x10
	? srso_return_thunk+0x5/0x5f
	? trace_irq_enable.constprop.0+0x116/0x150
	? srso_return_thunk+0x5/0x5f
	do_syscall_64+0x111/0x680
	entry_SYSCALL_64_after_hwframe+0x77/0x7f
	RIP: 0033:0x7ffacad55eae
	Code: 48 8b 0d 85 1f 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 8
	RSP: 002b:00007fff1ab55718 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
	RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffacad55eae
	RDX: 000055740c64e5b0 RSI: 000055740c64e630 RDI: 000055740c651ab0
	RBP: 000055740c64e380 R08: 0000000000000000 R09: 0000000000000001
	R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
	R13: 000055740c64e5b0 R14: 000055740c651ab0 R15: 000055740c64e380
	</TASK>

After applying this patch, the warning no longer appears.

Fixes: 89ac9b4d3d1a ("hfsplus: fix longname handling")
CC: stable@vger.kernel.org
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 fs/hfsplus/super.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 7229a8ae89f9..f396fee19ab8 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -569,8 +569,10 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (err)
 		goto out_put_root;
 	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
-	if (unlikely(err < 0))
+	if (unlikely(err < 0)) {
+		hfs_find_exit(&fd);
 		goto out_put_root;
+	}
 	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
 		hfs_find_exit(&fd);
 		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
-- 
2.34.1

[PATCH v4 2/2] hfsplus: extract hidden directory search into a helper function

[Zilin Guan]

In hfsplus_fill_super(), the process of looking up the hidden directory
involves initializing a catalog search, building a search key, reading
the b-tree record, and releasing the search data.

Currently, this logic is open-coded directly within the main superblock
initialization routine. This makes hfsplus_fill_super() quite lengthy
and its error handling paths less straightforward.

Extract the hidden directory search sequence into a new helper function,
hfsplus_get_hidden_dir_entry(). This improves overall code readability,
cleanly encapsulates the hfs_find_data lifecycle, and simplifies the
error exits in hfsplus_fill_super().

Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
---
 fs/hfsplus/super.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index f396fee19ab8..5ea2d1852591 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -424,12 +424,35 @@ void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
 	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
 }
 
+static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
+					       const struct qstr *str,
+					       hfsplus_cat_entry *entry)
+{
+	struct hfs_find_data fd;
+	int err;
+
+	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
+	if (unlikely(err))
+		return err;
+
+	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
+	if (unlikely(err))
+		goto free_fd;
+
+	err = hfs_brec_read(&fd, entry, sizeof(*entry));
+	if (err)
+		err = -ENOENT;
+
+free_fd:
+	hfs_find_exit(&fd);
+	return err;
+}
+
 static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 {
 	struct hfsplus_vh *vhdr;
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
 	hfsplus_cat_entry entry;
-	struct hfs_find_data fd;
 	struct inode *root, *inode;
 	struct qstr str;
 	struct nls_table *nls;
@@ -565,16 +588,14 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 
 	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
 	str.name = HFSP_HIDDENDIR_NAME;
-	err = hfs_find_init(sbi->cat_tree, &fd);
-	if (err)
-		goto out_put_root;
-	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
-	if (unlikely(err < 0)) {
-		hfs_find_exit(&fd);
+	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
+	if (err == -ENOENT) {
+		/*
+		 * Hidden directory is absent or it cannot be read.
+		 */
+	} else if (unlikely(err)) {
 		goto out_put_root;
-	}
-	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
-		hfs_find_exit(&fd);
+	} else {
 		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
 			err = -EIO;
 			goto out_put_root;
@@ -585,8 +606,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
 			goto out_put_root;
 		}
 		sbi->hidden_dir = inode;
-	} else
-		hfs_find_exit(&fd);
+	}
 
 	if (!sb_rdonly(sb)) {
 		/*
-- 
2.34.1

Re: [PATCH v4 1/2] hfsplus: fix held lock freed on hfsplus_fill_super()

[Viacheslav Dubeyko]

On Sat, 2026-03-21 at 16:01 +0800, Zilin Guan wrote:
> hfsplus_fill_super() calls hfs_find_init() to initialize a search
> structure, which acquires tree->tree_lock. If the subsequent call to
> hfsplus_cat_build_key() fails, the function jumps to the out_put_root
> error label without releasing the lock. The later cleanup path then
> frees the tree data structure with the lock still held, triggering a
> held lock freed warning.
> 
> Fix this by adding the missing hfs_find_exit(&fd) call before jumping
> to the out_put_root error label. This ensures that tree->tree_lock is
> properly released on the error path.
> 
> The bug was originally detected on v6.13-rc1 using an experimental
> static analysis tool we are developing, and we have verified that the
> issue persists in the latest mainline kernel. The tool is specifically
> designed to detect memory management issues. It is currently under active
> development and not yet publicly available.
> 
> We confirmed the bug by runtime testing under QEMU with x86_64 defconfig,
> lockdep enabled, and CONFIG_HFSPLUS_FS=y. To trigger the error path, we
> used GDB to dynamically shrink the max_unistr_len parameter to 1 before
> hfsplus_asc2uni() is called. This forces hfsplus_asc2uni() to naturally
> return -ENAMETOOLONG, which propagates to hfsplus_cat_build_key() and
> exercises the faulty error path. The following warning was observed
> during mount:
> 
> 	=========================
> 	WARNING: held lock freed!
> 	7.0.0-rc3-00016-gb4f0dd314b39 #4 Not tainted
> 	-------------------------
> 	mount/174 is freeing memory ffff888103f92000-ffff888103f92fff, with a lock still held there!
> 	ffff888103f920b0 (&tree->tree_lock){+.+.}-{4:4}, at: hfsplus_find_init+0x154/0x1e0
> 	2 locks held by mount/174:
> 	#0: ffff888103f960e0 (&type->s_umount_key#42/1){+.+.}-{4:4}, at: alloc_super.constprop.0+0x167/0xa40
> 	#1: ffff888103f920b0 (&tree->tree_lock){+.+.}-{4:4}, at: hfsplus_find_init+0x154/0x1e0
> 
> 	stack backtrace:
> 	CPU: 2 UID: 0 PID: 174 Comm: mount Not tainted 7.0.0-rc3-00016-gb4f0dd314b39 #4 PREEMPT(lazy)
> 	Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.15.0-1 04/01/2014
> 	Call Trace:
> 	<TASK>
> 	dump_stack_lvl+0x82/0xd0
> 	debug_check_no_locks_freed+0x13a/0x180
> 	kfree+0x16b/0x510
> 	? hfsplus_fill_super+0xcb4/0x18a0
> 	hfsplus_fill_super+0xcb4/0x18a0
> 	? __pfx_hfsplus_fill_super+0x10/0x10
> 	? srso_return_thunk+0x5/0x5f
> 	? bdev_open+0x65f/0xc30
> 	? srso_return_thunk+0x5/0x5f
> 	? pointer+0x4ce/0xbf0
> 	? trace_contention_end+0x11c/0x150
> 	? __pfx_pointer+0x10/0x10
> 	? srso_return_thunk+0x5/0x5f
> 	? bdev_open+0x79b/0xc30
> 	? srso_return_thunk+0x5/0x5f
> 	? srso_return_thunk+0x5/0x5f
> 	? vsnprintf+0x6da/0x1270
> 	? srso_return_thunk+0x5/0x5f
> 	? __mutex_unlock_slowpath+0x157/0x740
> 	? __pfx_vsnprintf+0x10/0x10
> 	? srso_return_thunk+0x5/0x5f
> 	? srso_return_thunk+0x5/0x5f
> 	? mark_held_locks+0x49/0x80
> 	? srso_return_thunk+0x5/0x5f
> 	? srso_return_thunk+0x5/0x5f
> 	? irqentry_exit+0x17b/0x5e0
> 	? trace_irq_disable.constprop.0+0x116/0x150
> 	? __pfx_hfsplus_fill_super+0x10/0x10
> 	? __pfx_hfsplus_fill_super+0x10/0x10
> 	get_tree_bdev_flags+0x302/0x580
> 	? __pfx_get_tree_bdev_flags+0x10/0x10
> 	? vfs_parse_fs_qstr+0x129/0x1a0
> 	? __pfx_vfs_parse_fs_qstr+0x3/0x10
> 	vfs_get_tree+0x89/0x320
> 	fc_mount+0x10/0x1d0
> 	path_mount+0x5c5/0x21c0
> 	? __pfx_path_mount+0x10/0x10
> 	? trace_irq_enable.constprop.0+0x116/0x150
> 	? trace_irq_enable.constprop.0+0x116/0x150
> 	? srso_return_thunk+0x5/0x5f
> 	? srso_return_thunk+0x5/0x5f
> 	? kmem_cache_free+0x307/0x540
> 	? user_path_at+0x51/0x60
> 	? __x64_sys_mount+0x212/0x280
> 	? srso_return_thunk+0x5/0x5f
> 	__x64_sys_mount+0x212/0x280
> 	? __pfx___x64_sys_mount+0x10/0x10
> 	? srso_return_thunk+0x5/0x5f
> 	? trace_irq_enable.constprop.0+0x116/0x150
> 	? srso_return_thunk+0x5/0x5f
> 	do_syscall_64+0x111/0x680
> 	entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 	RIP: 0033:0x7ffacad55eae
> 	Code: 48 8b 0d 85 1f 0f 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 a5 00 00 8
> 	RSP: 002b:00007fff1ab55718 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
> 	RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffacad55eae
> 	RDX: 000055740c64e5b0 RSI: 000055740c64e630 RDI: 000055740c651ab0
> 	RBP: 000055740c64e380 R08: 0000000000000000 R09: 0000000000000001
> 	R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> 	R13: 000055740c64e5b0 R14: 000055740c651ab0 R15: 000055740c64e380
> 	</TASK>
> 
> After applying this patch, the warning no longer appears.
> 
> Fixes: 89ac9b4d3d1a ("hfsplus: fix longname handling")
> CC: stable@vger.kernel.org
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  fs/hfsplus/super.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 7229a8ae89f9..f396fee19ab8 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -569,8 +569,10 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  	if (err)
>  		goto out_put_root;
>  	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> -	if (unlikely(err < 0))
> +	if (unlikely(err < 0)) {
> +		hfs_find_exit(&fd);
>  		goto out_put_root;
> +	}
>  	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
>  		hfs_find_exit(&fd);
>  		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

The xfstests didn't revealed any new issues.

Tested-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

Re: [PATCH v4 2/2] hfsplus: extract hidden directory search into a helper function

[Viacheslav Dubeyko]

On Sat, 2026-03-21 at 16:01 +0800, Zilin Guan wrote:
> In hfsplus_fill_super(), the process of looking up the hidden directory
> involves initializing a catalog search, building a search key, reading
> the b-tree record, and releasing the search data.
> 
> Currently, this logic is open-coded directly within the main superblock
> initialization routine. This makes hfsplus_fill_super() quite lengthy
> and its error handling paths less straightforward.
> 
> Extract the hidden directory search sequence into a new helper function,
> hfsplus_get_hidden_dir_entry(). This improves overall code readability,
> cleanly encapsulates the hfs_find_data lifecycle, and simplifies the
> error exits in hfsplus_fill_super().
> 
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  fs/hfsplus/super.c | 44 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index f396fee19ab8..5ea2d1852591 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -424,12 +424,35 @@ void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
>  	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
>  }
>  
> +static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> +					       const struct qstr *str,
> +					       hfsplus_cat_entry *entry)
> +{
> +	struct hfs_find_data fd;
> +	int err;
> +
> +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> +	if (unlikely(err))
> +		return err;
> +
> +	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
> +	if (unlikely(err))
> +		goto free_fd;
> +
> +	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> +	if (err)
> +		err = -ENOENT;
> +
> +free_fd:
> +	hfs_find_exit(&fd);
> +	return err;
> +}
> +
>  static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	struct hfsplus_vh *vhdr;
>  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
>  	hfsplus_cat_entry entry;
> -	struct hfs_find_data fd;
>  	struct inode *root, *inode;
>  	struct qstr str;
>  	struct nls_table *nls;
> @@ -565,16 +588,14 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  
>  	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
>  	str.name = HFSP_HIDDENDIR_NAME;
> -	err = hfs_find_init(sbi->cat_tree, &fd);
> -	if (err)
> -		goto out_put_root;
> -	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> -	if (unlikely(err < 0)) {
> -		hfs_find_exit(&fd);
> +	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
> +	if (err == -ENOENT) {
> +		/*
> +		 * Hidden directory is absent or it cannot be read.
> +		 */
> +	} else if (unlikely(err)) {
>  		goto out_put_root;
> -	}
> -	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> -		hfs_find_exit(&fd);
> +	} else {
>  		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
>  			err = -EIO;
>  			goto out_put_root;
> @@ -585,8 +606,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  			goto out_put_root;
>  		}
>  		sbi->hidden_dir = inode;
> -	} else
> -		hfs_find_exit(&fd);
> +	}
>  
>  	if (!sb_rdonly(sb)) {
>  		/*

Looks good.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

The xfstests didn't revealed any new issues.

Tested-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.