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

[Zilin Guan]

On Mon, Mar 16, 2026 at 10:46:14PM +0000, Viacheslav Dubeyko wrote:
> On Sat, 2026-03-14 at 11:36 +0800, Zilin Guan wrote:
> > 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.

Hi Slava,

When considering the solution, my primary focus was to strictly preserve 
the original execution logic. Therefore, I was focusing more on which 
function returned the error rather than the specific error code itself.

The issue with routing by error codes is that different functions can 
return the same code but require different handling. For example, 
both hfs_find_init() and hfs_brec_read() can return -ENOMEM 
(the latter via __hfs_bnode_create).

In the original code:

- hfs_find_init() returning -ENOMEM is fatal (must abort mount).
- hfs_brec_read() returning -ENOMEM is non-fatal (clean up and continue 
mount).

If a helper simply returns err, the caller cannot distinguish which 
function failed, making it impossible to safely decide whether to abort 
or continue.

Since the helper approach adds unnecessary complexity, wouldn't it be 
better to stick with my original patch?

Thanks,
Zilin