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

[Tetsuo Handa]

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?

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).

Honestly speaking, I don't like use of make_bad_inode(). make_bad_inode() might
change file type. 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.

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?