RE: [PATCH] hfsplus: fix held lock freed on hfsplus_fill_super()

[Viacheslav Dubeyko]

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@xxxxxxxxxxx>
> > 
> > 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.