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

[Zilin Guan]

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