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

[Viacheslav Dubeyko]

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