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

[Zilin Guan]

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.

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