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

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

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.