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

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

On Tue, 2026-03-17 at 11:12 +0800, Zilin Guan wrote:
> 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.
> 

You can use the different error codes to distinguish these situations.

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

I prefer to see the correct refactoring because it could make the code much
cleaner. And, frankly speaking, I don't quite follow why do you see so many
troubles in really simple refactoring.

Thanks,
Slava.