RE: [PATCH v2 2/2] hfs: Update sanity check of the root record

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

On Fri, 2025-11-14 at 23:18 +0900, Tetsuo Handa wrote:
> On 2025/11/12 7:56, Viacheslav Dubeyko wrote:
> > The file system is mounted only if hfs_fill_super() created root node and return
> > 0 [1]. However, if hfs_iget() return bad inode [2] and we will call
> > is_bad_inode() here [3]:
> > 
> > 	root_inode = hfs_iget(sb, &fd.search_key->cat, &rec);
> > 	hfs_find_exit(&fd);
> > 	if (!root_inode || is_bad_inode(root_inode)) <-- call will be here
> > 		goto bail_no_root;
> > 
> > then, mount will fail. So, no successful mount will happen because
> > is_valid_cnid() will manage the check in hfs_read_inode().
> 
> Do you admit that mounting (and optionally fuzzing on) a bad inode (an inode
> which was subjected to make_bad_inode()) is useless?
> 

Mount will fail in the case of bad root inode. I don't see any issue here.

> Adding is_bad_inode() check without corresponding iput() in error path causes
> an inode leak bug. Also, error code will differ (my patch returns -EIO while
> your approach will return -EINVAL).
> 

I don't see any problem to add iput(root_inode) as part of failure management in
hfs_fill_super(). And we can return any proper error code for the case of bad
root inode.

> Honestly speaking, I don't like use of make_bad_inode(). make_bad_inode() might
> change file type.
> 

If Catalog File's entry is corrupted, then we cannot assume which particular
file type is correct one. And nobody will operate by bad inode. So, it doesn't
matter which type bad inode has.

>  Also, I worry that make_bad_inode() causes a subtle race bug
> like https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21   which has not
> come to a conclusion.
> 

Frankly speaking, I don't see how this issue is related to the HFS mount
operation.

> Why can't we remove make_bad_inode() usage from hfs_read_inode() and return non-0 value
> (so that inode_insert5() will return NULL and iget5_locked() will call destroy_inode()
> and return NULL) when hfs_read_inode() encountered an invalid entry?

Bunch of other file systems use the make_bad_inode() and in fill_super() call
too. I don't see what is wrong with calling make_bad_inode().

Thanks,
Slava.