fs/hfsplus/super.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
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.1On Wed, 2026-03-11 at 19:43 +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)) {
Makes sense.
Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
Frankly speaking, I think, potentially, we can introduce static inline function
for this code:
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))
goto out_put_root;
if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
hfs_find_exit(&fd);
if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
err = -EIO;
goto out_put_root;
}
inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
if (IS_ERR(inode)) {
err = PTR_ERR(inode);
goto out_put_root;
}
sbi->hidden_dir = inode;
} else
hfs_find_exit(&fd);
Because, hiding this code into small function will provide opportunity to call
hfs_find_exit() in one place only (as for normal as for erroneous flow).
What do you think?
Thanks,
Slava.
On Wed, Mar 11, 2026 at 09:17:59PM +0000, Viacheslav Dubeyko wrote:
> On Wed, 2026-03-11 at 19:43 +0800, Zilin Guan wrote:
> > 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)) {
>
> Makes sense.
>
> Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
>
> Frankly speaking, I think, potentially, we can introduce static inline function
> for this code:
>
> 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))
> goto out_put_root;
> if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> hfs_find_exit(&fd);
> if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> err = -EIO;
> goto out_put_root;
> }
> inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> if (IS_ERR(inode)) {
> err = PTR_ERR(inode);
> goto out_put_root;
> }
> sbi->hidden_dir = inode;
> } else
> hfs_find_exit(&fd);
>
> Because, hiding this code into small function will provide opportunity to call
> hfs_find_exit() in one place only (as for normal as for erroneous flow).
>
> What do you think?
>
> Thanks,
> Slava.
Thanks for the feedback, Slava.
While I see the merit in refactoring this into a helper to centralize the
cleanup, I’m concerned that doing so wouldn’t actually achieve a single
hfs_find_exit() call without compromising the resource lifecycle.
In the current logic, we need to call hfs_find_exit(&fd) as early as
possible—specifically before entering hfsplus_iget(), which might involve
further I/O or sleeping. If we were to use a single-exit goto pattern in a
helper function, we would end up holding the search data and its
associated buffers/locks longer than necessary. To maintain the current
early-release behavior, we would still be forced to sprinkle multiple
hfs_find_exit() calls across different branches within that helper anyway,
which defeats the purpose of the refactoring.
Given that this is a straightforward fix for a specific leak, I believe
keeping the logic inline preserves the optimal resource release timing
without adding unnecessary abstraction.
Best regards,
Zilin
On Thu, 2026-03-12 at 10:17 +0800, Zilin Guan wrote:
> On Wed, Mar 11, 2026 at 09:17:59PM +0000, Viacheslav Dubeyko wrote:
> > On Wed, 2026-03-11 at 19:43 +0800, Zilin Guan wrote:
> > > 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)) {
> >
> > Makes sense.
> >
> > Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> >
> > Frankly speaking, I think, potentially, we can introduce static inline function
> > for this code:
> >
> > 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))
> > goto out_put_root;
> > if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> > hfs_find_exit(&fd);
> > if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> > err = -EIO;
> > goto out_put_root;
> > }
> > inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> > if (IS_ERR(inode)) {
> > err = PTR_ERR(inode);
> > goto out_put_root;
> > }
> > sbi->hidden_dir = inode;
> > } else
> > hfs_find_exit(&fd);
> >
> > Because, hiding this code into small function will provide opportunity to call
> > hfs_find_exit() in one place only (as for normal as for erroneous flow).
> >
> > What do you think?
> >
> > Thanks,
> > Slava.
>
> Thanks for the feedback, Slava.
>
> While I see the merit in refactoring this into a helper to centralize the
> cleanup, I’m concerned that doing so wouldn’t actually achieve a single
> hfs_find_exit() call without compromising the resource lifecycle.
>
> In the current logic, we need to call hfs_find_exit(&fd) as early as
> possible—specifically before entering hfsplus_iget(), which might involve
> further I/O or sleeping. If we were to use a single-exit goto pattern in a
> helper function, we would end up holding the search data and its
> associated buffers/locks longer than necessary. To maintain the current
> early-release behavior, we would still be forced to sprinkle multiple
> hfs_find_exit() calls across different branches within that helper anyway,
> which defeats the purpose of the refactoring.
>
> Given that this is a straightforward fix for a specific leak, I believe
> keeping the logic inline preserves the optimal resource release timing
> without adding unnecessary abstraction.
>
I mean really simple solution:
static inline
int hfsplus_get_hidden_dir_entry(struct super_block *sb,
hfsplus_cat_entry *entry)
{
int err = 0;
str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
str.name = HFSP_HIDDENDIR_NAME;
err = hfs_find_init(sbi->cat_tree, &fd);
if (err)
goto finish_logic;
err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID,
&str);
if (unlikely(err < 0))
goto free_fd;
err = hfs_brec_read(&fd, entry, sizeof(*entry));
free_fd:
hfs_find_exit(&fd);
finish_logic:
return err;
}
static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
{
<skipped>
err = hfsplus_get_hidden_dir_entry(sb, &entry);
if (err)
goto process_error;
if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
err = -EIO;
goto finish_logic;
}
inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
if (IS_ERR(inode)) {
err = PTR_ERR(inode);
goto finish_logic;
}
sbi->hidden_dir = inode;
<skipped>
}
Does it makes sense to you?
Thanks,
Slava.
On Thu, Mar 12, 2026 at 05:36:38PM +0000, Viacheslav Dubeyko wrote:
> On Thu, 2026-03-12 at 10:17 +0800, Zilin Guan wrote:
> > On Wed, Mar 11, 2026 at 09:17:59PM +0000, Viacheslav Dubeyko wrote:
> > > On Wed, 2026-03-11 at 19:43 +0800, Zilin Guan wrote:
> > > > 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)) {
> > >
> > > Makes sense.
> > >
> > > Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > >
> > > Frankly speaking, I think, potentially, we can introduce static inline function
> > > for this code:
> > >
> > > 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))
> > > goto out_put_root;
> > > if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> > > hfs_find_exit(&fd);
> > > if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> > > err = -EIO;
> > > goto out_put_root;
> > > }
> > > inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> > > if (IS_ERR(inode)) {
> > > err = PTR_ERR(inode);
> > > goto out_put_root;
> > > }
> > > sbi->hidden_dir = inode;
> > > } else
> > > hfs_find_exit(&fd);
> > >
> > > Because, hiding this code into small function will provide opportunity to call
> > > hfs_find_exit() in one place only (as for normal as for erroneous flow).
> > >
> > > What do you think?
> > >
> > > Thanks,
> > > Slava.
> >
> > Thanks for the feedback, Slava.
> >
> > While I see the merit in refactoring this into a helper to centralize the
> > cleanup, I’m concerned that doing so wouldn’t actually achieve a single
> > hfs_find_exit() call without compromising the resource lifecycle.
> >
> > In the current logic, we need to call hfs_find_exit(&fd) as early as
> > possible—specifically before entering hfsplus_iget(), which might involve
> > further I/O or sleeping. If we were to use a single-exit goto pattern in a
> > helper function, we would end up holding the search data and its
> > associated buffers/locks longer than necessary. To maintain the current
> > early-release behavior, we would still be forced to sprinkle multiple
> > hfs_find_exit() calls across different branches within that helper anyway,
> > which defeats the purpose of the refactoring.
> >
> > Given that this is a straightforward fix for a specific leak, I believe
> > keeping the logic inline preserves the optimal resource release timing
> > without adding unnecessary abstraction.
> >
>
> I mean really simple solution:
>
> static inline
> int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> hfsplus_cat_entry *entry)
> {
> int err = 0;
>
> str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> str.name = HFSP_HIDDENDIR_NAME;
> err = hfs_find_init(sbi->cat_tree, &fd);
> if (err)
> goto finish_logic;
>
> err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID,
> &str);
> if (unlikely(err < 0))
> goto free_fd;
>
> err = hfs_brec_read(&fd, entry, sizeof(*entry));
>
> free_fd:
> hfs_find_exit(&fd);
> finish_logic:
> return err;
> }
>
> static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> <skipped>
>
> err = hfsplus_get_hidden_dir_entry(sb, &entry);
> if (err)
> goto process_error;
>
> if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> err = -EIO;
> goto finish_logic;
> }
> inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> if (IS_ERR(inode)) {
> err = PTR_ERR(inode);
> goto finish_logic;
> }
> sbi->hidden_dir = inode;
>
> <skipped>
> }
>
> Does it makes sense to you?
>
> Thanks,
> Slava.
Hi Slava,
Thanks for the detailed proposal. However, this proposed refactoring
changes the existing semantics and introduces a regression.
The hidden directory is optional. If hfs_brec_read() fails, the original
code simply calls hfs_find_exit() and proceeds with the mount. It is a
non-fatal error.
In contrast, failures from hfs_find_init() and hfsplus_cat_build_key() are
fatal and must abort the mount.
By wrapping these into a single helper and returning err, the caller can no
longer distinguish between them. A missing hidden directory will trigger
if (err) goto process_error; in hfsplus_fill_super(), making it a fatal
error. This will break mounting for any valid HFS+ volume that lacks the
private data directory.
Thanks,
Zilin
On Fri, 2026-03-13 at 09:49 +0800, Zilin Guan wrote:
> On Thu, Mar 12, 2026 at 05:36:38PM +0000, Viacheslav Dubeyko wrote:
> > On Thu, 2026-03-12 at 10:17 +0800, Zilin Guan wrote:
> > > On Wed, Mar 11, 2026 at 09:17:59PM +0000, Viacheslav Dubeyko wrote:
> > > > On Wed, 2026-03-11 at 19:43 +0800, Zilin Guan wrote:
> > > > > 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)) {
> > > >
> > > > Makes sense.
> > > >
> > > > Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > > >
> > > > Frankly speaking, I think, potentially, we can introduce static inline function
> > > > for this code:
> > > >
> > > > 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))
> > > > goto out_put_root;
> > > > if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> > > > hfs_find_exit(&fd);
> > > > if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> > > > err = -EIO;
> > > > goto out_put_root;
> > > > }
> > > > inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> > > > if (IS_ERR(inode)) {
> > > > err = PTR_ERR(inode);
> > > > goto out_put_root;
> > > > }
> > > > sbi->hidden_dir = inode;
> > > > } else
> > > > hfs_find_exit(&fd);
> > > >
> > > > Because, hiding this code into small function will provide opportunity to call
> > > > hfs_find_exit() in one place only (as for normal as for erroneous flow).
> > > >
> > > > What do you think?
> > > >
> > > > Thanks,
> > > > Slava.
> > >
> > > Thanks for the feedback, Slava.
> > >
> > > While I see the merit in refactoring this into a helper to centralize the
> > > cleanup, I’m concerned that doing so wouldn’t actually achieve a single
> > > hfs_find_exit() call without compromising the resource lifecycle.
> > >
> > > In the current logic, we need to call hfs_find_exit(&fd) as early as
> > > possible—specifically before entering hfsplus_iget(), which might involve
> > > further I/O or sleeping. If we were to use a single-exit goto pattern in a
> > > helper function, we would end up holding the search data and its
> > > associated buffers/locks longer than necessary. To maintain the current
> > > early-release behavior, we would still be forced to sprinkle multiple
> > > hfs_find_exit() calls across different branches within that helper anyway,
> > > which defeats the purpose of the refactoring.
> > >
> > > Given that this is a straightforward fix for a specific leak, I believe
> > > keeping the logic inline preserves the optimal resource release timing
> > > without adding unnecessary abstraction.
> > >
> >
> > I mean really simple solution:
> >
> > static inline
> > int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> > hfsplus_cat_entry *entry)
> > {
> > int err = 0;
> >
> > str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
> > str.name = HFSP_HIDDENDIR_NAME;
> > err = hfs_find_init(sbi->cat_tree, &fd);
> > if (err)
> > goto finish_logic;
> >
> > err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID,
> > &str);
> > if (unlikely(err < 0))
> > goto free_fd;
> >
> > err = hfs_brec_read(&fd, entry, sizeof(*entry));
> >
> > free_fd:
> > hfs_find_exit(&fd);
> > finish_logic:
> > return err;
> > }
> >
> > static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
> > {
> > <skipped>
> >
> > err = hfsplus_get_hidden_dir_entry(sb, &entry);
> > if (err)
> > goto process_error;
> >
> > if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> > err = -EIO;
> > goto finish_logic;
> > }
> > inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> > if (IS_ERR(inode)) {
> > err = PTR_ERR(inode);
> > goto finish_logic;
> > }
> > sbi->hidden_dir = inode;
> >
> > <skipped>
> > }
> >
> > Does it makes sense to you?
> >
> > Thanks,
> > Slava.
>
> Hi Slava,
>
> Thanks for the detailed proposal. However, this proposed refactoring
> changes the existing semantics and introduces a regression.
>
I don't quite follow to your point. I don't suggest to change the logic. I am
suggesting the small refactoring without changing the execution flow. Do you
mean that current hfsplus_fill_super() logic is incorrect and has bugs?
> The hidden directory is optional. If hfs_brec_read() fails, the original
> code simply calls hfs_find_exit() and proceeds with the mount. It is a
> non-fatal error.
>
You simply need slightly modify my suggestion to make it right:
err = hfsplus_get_hidden_dir_entry(sb, &entry);
if (!err) {
if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
err = -EIO;
goto finish_logic;
}
inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
if (IS_ERR(inode)) {
err = PTR_ERR(inode);
goto finish_logic;
}
sbi->hidden_dir = inode;
}
I simply shared the raw suggestion but you can make it right.
> In contrast, failures from hfs_find_init() and hfsplus_cat_build_key() are
> fatal and must abort the mount.
>
> By wrapping these into a single helper and returning err, the caller can no
> longer distinguish between them. A missing hidden directory will trigger
> if (err) goto process_error; in hfsplus_fill_super(), making it a fatal
> error. This will break mounting for any valid HFS+ volume that lacks the
> private data directory.
>
>
Simply make my suggestion better and correct. That's all.
Thanks,
Slava.
On Fri, Mar 13, 2026 at 06:38:14PM +0000, Viacheslav Dubeyko wrote:
> On Fri, 2026-03-13 at 09:49 +0800, Zilin Guan wrote:
> > Hi Slava,
> >
> > Thanks for the detailed proposal. However, this proposed refactoring
> > changes the existing semantics and introduces a regression.
> >
>
> I don't quite follow to your point. I don't suggest to change the logic. I am
> suggesting the small refactoring without changing the execution flow. Do you
> mean that current hfsplus_fill_super() logic is incorrect and has bugs?
Actually, I don't mean the original logic is incorrect. My concern is that
extracting this block into a helper makes it very difficult to preserve
that correct execution flow without complicating the error handling.
> > The hidden directory is optional. If hfs_brec_read() fails, the original
> > code simply calls hfs_find_exit() and proceeds with the mount. It is a
> > non-fatal error.
> >
>
> You simply need slightly modify my suggestion to make it right:
>
> err = hfsplus_get_hidden_dir_entry(sb, &entry);
> if (!err) {
>
> if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> err = -EIO;
> goto finish_logic;
> }
> inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> if (IS_ERR(inode)) {
> err = PTR_ERR(inode);
> goto finish_logic;
> }
> sbi->hidden_dir = inode;
> }
>
> I simply shared the raw suggestion but you can make it right.
The issue with this updated snippet is that it silently ignores fatal
errors from hfs_find_init() and hfsplus_cat_build_key() (e.g., -ENOMEM).
If they fail, the mount incorrectly continues. In the original code,
these correctly trigger goto out_put_root.
> > In contrast, failures from hfs_find_init() and hfsplus_cat_build_key() are
> > fatal and must abort the mount.
> >
> > By wrapping these into a single helper and returning err, the caller can no
> > longer distinguish between them. A missing hidden directory will trigger
> > if (err) goto process_error; in hfsplus_fill_super(), making it a fatal
> > error. This will break mounting for any valid HFS+ volume that lacks the
> > private data directory.
> >
> >
>
> Simply make my suggestion better and correct. That's all.
>
> Thanks,
> Slava.
To make the helper completely correct, we face another issue: the original
code ignores all errors from hfs_brec_read() (which can return -ENOENT,
-EINVAL, -EIO, etc.), treating them as non-fatal.
If we combine the fatal setup functions and the non-fatal read function
into one helper, it cannot simply return a standard error code. It would
need to return three distinct states:
1. Fatal error -> caller must abort mount.
2. Non-fatal read error -> caller must continue mount, but skip init.
3. Success -> caller must init hidden_dir.
To handle all these cases properly, the helper would have to look
something like this:
/* Returns < 0 on fatal error, 0 on missing/read error, 1 on success */
static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
hfsplus_cat_entry *entry)
{
struct hfs_find_data fd;
int err;
int ret = 0;
/* ... init str ... */
err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
if (err)
return err; /* Fatal, fd not initialized */
err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
if (unlikely(err < 0)) {
ret = err;
goto free_fd; /* Fatal */
}
err = hfs_brec_read(&fd, entry, sizeof(*entry));
if (err) {
ret = 0; /* Non-fatal, but no entry to init */
goto free_fd;
}
ret = 1; /* Success */
free_fd:
hfs_find_exit(&fd);
return ret;
}
And the caller:
err = hfsplus_get_hidden_dir_entry(sb, &entry);
if (err < 0)
goto out_put_root;
if (err == 1) {
/* ... init hidden_dir ... */
}
We would have to invent a custom return state convention (1, 0, < 0) just to
hide a single hfs_find_exit() call.
Given this, I think the current inline logic in my patch is much cleaner
and avoids this convoluted error routing.
What do you prefer?
Thanks,
Zilin
On Sat, 2026-03-14 at 11:36 +0800, Zilin Guan wrote:
> On Fri, Mar 13, 2026 at 06:38:14PM +0000, Viacheslav Dubeyko wrote:
> > On Fri, 2026-03-13 at 09:49 +0800, Zilin Guan wrote:
> > > Hi Slava,
> > >
> > > Thanks for the detailed proposal. However, this proposed refactoring
> > > changes the existing semantics and introduces a regression.
> > >
> >
> > I don't quite follow to your point. I don't suggest to change the logic. I am
> > suggesting the small refactoring without changing the execution flow. Do you
> > mean that current hfsplus_fill_super() logic is incorrect and has bugs?
>
> Actually, I don't mean the original logic is incorrect. My concern is that
> extracting this block into a helper makes it very difficult to preserve
> that correct execution flow without complicating the error handling.
>
> > > The hidden directory is optional. If hfs_brec_read() fails, the original
> > > code simply calls hfs_find_exit() and proceeds with the mount. It is a
> > > non-fatal error.
> > >
> >
> > You simply need slightly modify my suggestion to make it right:
> >
> > err = hfsplus_get_hidden_dir_entry(sb, &entry);
> > if (!err) {
> >
> > if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
> > err = -EIO;
> > goto finish_logic;
> > }
> > inode = hfsplus_iget(sb, be32_to_cpu(entry.folder.id));
> > if (IS_ERR(inode)) {
> > err = PTR_ERR(inode);
> > goto finish_logic;
> > }
> > sbi->hidden_dir = inode;
> > }
> >
> > I simply shared the raw suggestion but you can make it right.
>
> The issue with this updated snippet is that it silently ignores fatal
> errors from hfs_find_init() and hfsplus_cat_build_key() (e.g., -ENOMEM).
> If they fail, the mount incorrectly continues. In the original code,
> these correctly trigger goto out_put_root.
>
> > > In contrast, failures from hfs_find_init() and hfsplus_cat_build_key() are
> > > fatal and must abort the mount.
> > >
> > > By wrapping these into a single helper and returning err, the caller can no
> > > longer distinguish between them. A missing hidden directory will trigger
> > > if (err) goto process_error; in hfsplus_fill_super(), making it a fatal
> > > error. This will break mounting for any valid HFS+ volume that lacks the
> > > private data directory.
> > >
> > >
> >
> > Simply make my suggestion better and correct. That's all.
> >
> > Thanks,
> > Slava.
>
> To make the helper completely correct, we face another issue: the original
> code ignores all errors from hfs_brec_read() (which can return -ENOENT,
> -EINVAL, -EIO, etc.), treating them as non-fatal.
>
> If we combine the fatal setup functions and the non-fatal read function
> into one helper, it cannot simply return a standard error code. It would
> need to return three distinct states:
>
> 1. Fatal error -> caller must abort mount.
> 2. Non-fatal read error -> caller must continue mount, but skip init.
> 3. Success -> caller must init hidden_dir.
>
> To handle all these cases properly, the helper would have to look
> something like this:
>
> /* Returns < 0 on fatal error, 0 on missing/read error, 1 on success */
> static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> hfsplus_cat_entry *entry)
> {
> struct hfs_find_data fd;
> int err;
> int ret = 0;
> /* ... init str ... */
>
> err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> if (err)
> return err; /* Fatal, fd not initialized */
>
> err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> if (unlikely(err < 0)) {
> ret = err;
> goto free_fd; /* Fatal */
> }
>
> err = hfs_brec_read(&fd, entry, sizeof(*entry));
> if (err) {
> ret = 0; /* Non-fatal, but no entry to init */
> goto free_fd;
> }
>
> ret = 1; /* Success */
>
> free_fd:
> hfs_find_exit(&fd);
> return ret;
> }
>
> And the caller:
>
> err = hfsplus_get_hidden_dir_entry(sb, &entry);
> if (err < 0)
> goto out_put_root;
> if (err == 1) {
> /* ... init hidden_dir ... */
> }
>
> We would have to invent a custom return state convention (1, 0, < 0) just to
> hide a single hfs_find_exit() call.
>
> Given this, I think the current inline logic in my patch is much cleaner
> and avoids this convoluted error routing.
>
> What do you prefer?
>
I don't quite follow to your trouble. Any function can return various error
codes and caller could process the different error codes by different logics:
err = hfsplus_get_hidden_dir_entry(sb, &entry);
if (err == -ENOENT) {
<process -ENOENT>
} else if (err == -EINVAL) {
<process -EINVAL>
} else if (err == -EIO) {
<process -EIO>
} else if (err == <some other error>) {
<process this case>
}
Does it solve your trouble?
Thanks,
Slava.
On Mon, Mar 16, 2026 at 10:46:14PM +0000, Viacheslav Dubeyko wrote:
> On Sat, 2026-03-14 at 11:36 +0800, Zilin Guan wrote:
> > To make the helper completely correct, we face another issue: the original
> > code ignores all errors from hfs_brec_read() (which can return -ENOENT,
> > -EINVAL, -EIO, etc.), treating them as non-fatal.
> >
> > If we combine the fatal setup functions and the non-fatal read function
> > into one helper, it cannot simply return a standard error code. It would
> > need to return three distinct states:
> >
> > 1. Fatal error -> caller must abort mount.
> > 2. Non-fatal read error -> caller must continue mount, but skip init.
> > 3. Success -> caller must init hidden_dir.
> >
> > To handle all these cases properly, the helper would have to look
> > something like this:
> >
> > /* Returns < 0 on fatal error, 0 on missing/read error, 1 on success */
> > static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> > hfsplus_cat_entry *entry)
> > {
> > struct hfs_find_data fd;
> > int err;
> > int ret = 0;
> > /* ... init str ... */
> >
> > err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> > if (err)
> > return err; /* Fatal, fd not initialized */
> >
> > err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> > if (unlikely(err < 0)) {
> > ret = err;
> > goto free_fd; /* Fatal */
> > }
> >
> > err = hfs_brec_read(&fd, entry, sizeof(*entry));
> > if (err) {
> > ret = 0; /* Non-fatal, but no entry to init */
> > goto free_fd;
> > }
> >
> > ret = 1; /* Success */
> >
> > free_fd:
> > hfs_find_exit(&fd);
> > return ret;
> > }
> >
> > And the caller:
> >
> > err = hfsplus_get_hidden_dir_entry(sb, &entry);
> > if (err < 0)
> > goto out_put_root;
> > if (err == 1) {
> > /* ... init hidden_dir ... */
> > }
> >
> > We would have to invent a custom return state convention (1, 0, < 0) just to
> > hide a single hfs_find_exit() call.
> >
> > Given this, I think the current inline logic in my patch is much cleaner
> > and avoids this convoluted error routing.
> >
> > What do you prefer?
> >
>
> I don't quite follow to your trouble. Any function can return various error
> codes and caller could process the different error codes by different logics:
>
> err = hfsplus_get_hidden_dir_entry(sb, &entry);
> if (err == -ENOENT) {
> <process -ENOENT>
> } else if (err == -EINVAL) {
> <process -EINVAL>
> } else if (err == -EIO) {
> <process -EIO>
> } else if (err == <some other error>) {
> <process this case>
> }
>
> Does it solve your trouble?
>
> Thanks,
> Slava.
Hi Slava,
When considering the solution, my primary focus was to strictly preserve
the original execution logic. Therefore, I was focusing more on which
function returned the error rather than the specific error code itself.
The issue with routing by error codes is that different functions can
return the same code but require different handling. For example,
both hfs_find_init() and hfs_brec_read() can return -ENOMEM
(the latter via __hfs_bnode_create).
In the original code:
- hfs_find_init() returning -ENOMEM is fatal (must abort mount).
- hfs_brec_read() returning -ENOMEM is non-fatal (clean up and continue
mount).
If a helper simply returns err, the caller cannot distinguish which
function failed, making it impossible to safely decide whether to abort
or continue.
Since the helper approach adds unnecessary complexity, wouldn't it be
better to stick with my original patch?
Thanks,
Zilin
On Tue, 2026-03-17 at 11:12 +0800, Zilin Guan wrote:
> On Mon, Mar 16, 2026 at 10:46:14PM +0000, Viacheslav Dubeyko wrote:
> > On Sat, 2026-03-14 at 11:36 +0800, Zilin Guan wrote:
> > > To make the helper completely correct, we face another issue: the original
> > > code ignores all errors from hfs_brec_read() (which can return -ENOENT,
> > > -EINVAL, -EIO, etc.), treating them as non-fatal.
> > >
> > > If we combine the fatal setup functions and the non-fatal read function
> > > into one helper, it cannot simply return a standard error code. It would
> > > need to return three distinct states:
> > >
> > > 1. Fatal error -> caller must abort mount.
> > > 2. Non-fatal read error -> caller must continue mount, but skip init.
> > > 3. Success -> caller must init hidden_dir.
> > >
> > > To handle all these cases properly, the helper would have to look
> > > something like this:
> > >
> > > /* Returns < 0 on fatal error, 0 on missing/read error, 1 on success */
> > > static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> > > hfsplus_cat_entry *entry)
> > > {
> > > struct hfs_find_data fd;
> > > int err;
> > > int ret = 0;
> > > /* ... init str ... */
> > >
> > > err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> > > if (err)
> > > return err; /* Fatal, fd not initialized */
> > >
> > > err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> > > if (unlikely(err < 0)) {
> > > ret = err;
> > > goto free_fd; /* Fatal */
> > > }
> > >
> > > err = hfs_brec_read(&fd, entry, sizeof(*entry));
> > > if (err) {
> > > ret = 0; /* Non-fatal, but no entry to init */
> > > goto free_fd;
> > > }
> > >
> > > ret = 1; /* Success */
> > >
> > > free_fd:
> > > hfs_find_exit(&fd);
> > > return ret;
> > > }
> > >
> > > And the caller:
> > >
> > > err = hfsplus_get_hidden_dir_entry(sb, &entry);
> > > if (err < 0)
> > > goto out_put_root;
> > > if (err == 1) {
> > > /* ... init hidden_dir ... */
> > > }
> > >
> > > We would have to invent a custom return state convention (1, 0, < 0) just to
> > > hide a single hfs_find_exit() call.
> > >
> > > Given this, I think the current inline logic in my patch is much cleaner
> > > and avoids this convoluted error routing.
> > >
> > > What do you prefer?
> > >
> >
> > I don't quite follow to your trouble. Any function can return various error
> > codes and caller could process the different error codes by different logics:
> >
> > err = hfsplus_get_hidden_dir_entry(sb, &entry);
> > if (err == -ENOENT) {
> > <process -ENOENT>
> > } else if (err == -EINVAL) {
> > <process -EINVAL>
> > } else if (err == -EIO) {
> > <process -EIO>
> > } else if (err == <some other error>) {
> > <process this case>
> > }
> >
> > Does it solve your trouble?
> >
> > Thanks,
> > Slava.
>
> Hi Slava,
>
> When considering the solution, my primary focus was to strictly preserve
> the original execution logic. Therefore, I was focusing more on which
> function returned the error rather than the specific error code itself.
>
> The issue with routing by error codes is that different functions can
> return the same code but require different handling. For example,
> both hfs_find_init() and hfs_brec_read() can return -ENOMEM
> (the latter via __hfs_bnode_create).
>
> In the original code:
>
> - hfs_find_init() returning -ENOMEM is fatal (must abort mount).
> - hfs_brec_read() returning -ENOMEM is non-fatal (clean up and continue
> mount).
>
> If a helper simply returns err, the caller cannot distinguish which
> function failed, making it impossible to safely decide whether to abort
> or continue.
>
You can use the different error codes to distinguish these situations.
> Since the helper approach adds unnecessary complexity, wouldn't it be
> better to stick with my original patch?
>
>
I prefer to see the correct refactoring because it could make the code much
cleaner. And, frankly speaking, I don't quite follow why do you see so many
troubles in really simple refactoring.
Thanks,
Slava.
© 2016 - 2026 Red Hat, Inc.