RE: [PATCH v4] hfs: update sanity check of the root record

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

On Fri, 2025-08-01 at 06:12 +0900, Tetsuo Handa wrote:
> On 2025/08/01 3:03, Viacheslav Dubeyko wrote:
> > On Thu, 2025-07-31 at 07:02 +0900, Tetsuo Handa wrote:
> > > On 2025/07/31 4:24, Viacheslav Dubeyko wrote:
> > > > If we considering case HFS_CDR_DIR in hfs_read_inode(), then we know that it
> > > > could be HFS_POR_CNID, HFS_ROOT_CNID, or >= HFS_FIRSTUSER_CNID. Do you mean that
> > > > HFS_POR_CNID could be a problem in hfs_write_inode()?
> > > 
> > > Yes. Passing one of 1, 5 or 15 instead of 2 from hfs_fill_super() triggers BUG()
> > > in hfs_write_inode(). We *MUST* validate at hfs_fill_super(), or hfs_read_inode()
> > > shall have to also reject 1, 5 and 15 (and as a result only accept 2).
> > 
> > The fix should be in hfs_read_inode(). Currently, suggested solution hides the
> > issue but not fix the problem.
> 
> Not fixing this problem might be hiding other issues, by hitting BUG() before
> other issues shows up.
> 

I am not going to start a philosophical discussion. We simply need to fix the
bug. The suggested patch doesn't fix the issue.

> > Because b-tree nodes could contain multiple
> > corrupted records. Now, this patch checks only record for root folder. Let's
> > imagine that root folder record will be OK but another record(s) will be
> > corrupted in such way.
> 
> Can the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) be something other than HFS_ROOT_CNID ?
> 
> If the inode number of the record retrieved as a result of
> hfs_cat_find_brec(HFS_ROOT_CNID) must be HFS_ROOT_CNID, this patch itself will be
> a complete fix for this problem.
> 

You are working with corrupted volume. In this case, you can extract any state
of the Catalog File's record.

> > Finally, we will have successful mount but operation with
> > corrupted record(s) will trigger this issue. So, I cannot consider this patch as
> > a complete fix of the problem.
> 
> Did you try what you think as a fix of this problem (I guess something like
> shown below will be needed for avoid hitting BUG()) using
> https://lkml.kernel.org/r/a8f8da77-f099-499b-98e0-39ed159b6a2d@I-love.SAKURA.ne.jp   ?
> 

If you believe that you have another version of the patch, then simply send it
and I will review it. Sorry, I haven't enough time to discuss every movement of
your thoughts.

> diff --git a/fs/hfs/inode.c b/fs/hfs/inode.c
> index a81ce7a740b9..d60395111ed5 100644
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -81,7 +81,8 @@ static bool hfs_release_folio(struct folio *folio, gfp_t mask)
>  		tree = HFS_SB(sb)->cat_tree;
>  		break;
>  	default:
> -		BUG();
> +		pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +		       inode->i_ino);
>  		return false;
>  	}
>  
> @@ -305,11 +306,31 @@ static int hfs_test_inode(struct inode *inode, void *data)
>  	case HFS_CDR_FIL:
>  		return inode->i_ino == be32_to_cpu(rec->file.FlNum);
>  	default:
> -		BUG();
> +		pr_err("detected unknown type %u, running fsck.hfs is recommended.\n", rec->type);
>  		return 1;
>  	}
>  }
>  
> +static bool is_bad_id(unsigned long ino)
> +{
> +	switch (ino) {
> +	case 0:
> +	case 3:
> +	case 4:
> +	case 6:
> +	case 7:
> +	case 8:
> +	case 9:
> +	case 10:
> +	case 11:
> +	case 12:
> +	case 13:
> +	case 14:
> +		return true;
> +	}
> +	return false;
> +}

Please, don't use hardcoded value. I already shared the point that we must use
the declared constants.

This function is incorrect and it cannot work for folders and files at the same
time.

Thanks,
Slava.

> +
>  /*
>   * hfs_read_inode
>   */
> @@ -348,6 +369,10 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		}
>  
>  		inode->i_ino = be32_to_cpu(rec->file.FlNum);
> +		if (is_bad_id(inode->i_ino)) {
> +			make_bad_inode(inode);
> +			break;
> +		}
>  		inode->i_mode = S_IRUGO | S_IXUGO;
>  		if (!(rec->file.Flags & HFS_FIL_LOCK))
>  			inode->i_mode |= S_IWUGO;
> @@ -358,9 +383,15 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  		inode->i_op = &hfs_file_inode_operations;
>  		inode->i_fop = &hfs_file_operations;
>  		inode->i_mapping->a_ops = &hfs_aops;
> +		if (inode->i_ino < 16)
> +			pr_info("HFS_CDR_FIL i_ino=%ld\n", inode->i_ino);
>  		break;
>  	case HFS_CDR_DIR:
>  		inode->i_ino = be32_to_cpu(rec->dir.DirID);
> +		if (is_bad_id(inode->i_ino)) {
> +			make_bad_inode(inode);
> +			break;
> +		}
>  		inode->i_size = be16_to_cpu(rec->dir.Val) + 2;
>  		HFS_I(inode)->fs_blocks = 0;
>  		inode->i_mode = S_IFDIR | (S_IRWXUGO & ~hsb->s_dir_umask);
> @@ -368,6 +399,8 @@ static int hfs_read_inode(struct inode *inode, void *data)
>  				      inode_set_atime_to_ts(inode, inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->dir.MdDat))));
>  		inode->i_op = &hfs_dir_inode_operations;
>  		inode->i_fop = &hfs_dir_operations;
> +		if (inode->i_ino < 16)
> +			pr_info("HFS_CDR_DIR i_ino=%ld\n", inode->i_ino);
>  		break;
>  	default:
>  		make_bad_inode(inode);
> @@ -441,7 +474,8 @@ int hfs_write_inode(struct inode *inode, struct writeback_control *wbc)
>  			hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>  			return 0;
>  		default:
> -			BUG();
> +			pr_err("detected unknown inode %lu, running fsck.hfs is recommended.\n",
> +			       inode->i_ino);
>  			return -EIO;
>  		}
>  	}
> 
> 
> # for i in $(seq 0 15); do timeout 1 unshare -m ./hfs $i; done
> # dmesg | grep fsck
> [   52.563547] [    T479] hfs: detected unknown inode 1, running fsck.hfs is recommended.
> [   56.606238] [    T255] hfs: detected unknown inode 5, running fsck.hfs is recommended.
> [   66.694795] [    T500] hfs: detected unknown inode 15, running fsck.hfs is recommended.