RE: [PATCH v3] hfs: remove BUG() from hfs_release_folio()/hfs_test_inode()/hfs_write_inode()

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

On Thu, 2025-07-24 at 15:55 +0900, Tetsuo Handa wrote:
> Then, something like below change?
> 
> --- a/fs/hfs/inode.c
> +++ b/fs/hfs/inode.c
> @@ -318,6 +318,9 @@ static int hfs_read_inode(struct inode *inode, void *data)
>         struct hfs_iget_data *idata = data;
>         struct hfs_sb_info *hsb = HFS_SB(inode->i_sb);
>         hfs_cat_rec *rec;
> +       /* https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID   */

We already have all declarations in hfs.h:

/* Some special File ID numbers */
#define HFS_POR_CNID		1	/* Parent Of the Root */
#define HFS_ROOT_CNID		2	/* ROOT directory */
#define HFS_EXT_CNID		3	/* EXTents B-tree */
#define HFS_CAT_CNID		4	/* CATalog B-tree */
#define HFS_BAD_CNID		5	/* BAD blocks file */
#define HFS_ALLOC_CNID		6	/* ALLOCation file (HFS+) */
#define HFS_START_CNID		7	/* STARTup file (HFS+) */
#define HFS_ATTR_CNID		8	/* ATTRibutes file (HFS+) */
#define HFS_EXCH_CNID		15	/* ExchangeFiles temp id */
#define HFS_FIRSTUSER_CNID	16

So, adding the link here doesn't make any sense.

> +       static const u16 bad_cnid_list = (1 << 0) | (1 << 6) | (1 << 7) | (1 << 8) |
> +               (1 << 9) | (1 << 10) | (1 << 11) | (1 << 12) | (1 << 13);

I don't see any sense to introduce flags here. First of all, please, don't use
hardcoded values but you should use declared constants from hfs.h (for example,
HFS_EXT_CNID instead of 3). Secondly, you can simply compare the i_ino with
constants, for example:

bool is_inode_id_invalid(u64 ino) {
      switch (inode->i_ino) {
      case HFS_EXT_CNID:
      ...
          return true;

      }

      return false;
}

Thirdly, you can introduce an inline function that can do such check. And it
make sense to introduce constant for the case of zero value.

Why have you missed HFS_EXT_CNID, HFS_CAT_CNID? These values cannot used in
hfs_read_inode().

> 
>         HFS_I(inode)->flags = 0;
>         HFS_I(inode)->rsrc_inode = NULL;
> @@ -358,6 +361,8 @@ 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 < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);

It looks pretty complicated. You can simply use one above-mentioned function
with the check:

if (is_inode_id_invalid(be32_to_cpu(rec->dir.DirID)))
     <goto to make bad inode>

We can simply check the the inode ID in the beginning of the whole action:

<Make the check here>
		inode->i_ino = be32_to_cpu(rec->file.FlNum);
		inode->i_mode = S_IRUGO | S_IXUGO;
		if (!(rec->file.Flags & HFS_FIL_LOCK))
			inode->i_mode |= S_IWUGO;
		inode->i_mode &= ~hsb->s_file_umask;
		inode->i_mode |= S_IFREG;
		inode_set_mtime_to_ts(inode,
				      inode_set_atime_to_ts(inode,
inode_set_ctime_to_ts(inode, hfs_m_to_utime(rec->file.MdDat))));
		inode->i_op = &hfs_file_inode_operations;
		inode->i_fop = &hfs_file_operations;
		inode->i_mapping->a_ops = &hfs_aops;

It doesn't make any sense to construct inode if we will make in bad inode,
finally. Don't waste computational power. :)

>                 break;
>         case HFS_CDR_DIR:
>                 inode->i_ino = be32_to_cpu(rec->dir.DirID);
> @@ -368,6 +373,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 < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);

We already have make_bad_inode(inode) as default action. So, simply jump there.

>                 break;
>         default:
>                 make_bad_inode(inode);
> 
> 
> 
> But I can't be convinced that above change is sufficient, for if I do
> 
> +		static u8 serial;
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       inode->i_ino = (serial++) % 16;

I don't see the point in flags introduction. It makes logic very complicated.

> 
> instead of
> 
> +               if (inode->i_ino < HFS_FIRSTUSER_CNID && ((1U << inode->i_ino) & bad_cnid_list))
> +                       make_bad_inode(inode);
> 
> , the reproducer still hits BUG() for 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15
> because hfs_write_inode() handles only 2, 3 and 4.
> 

How can we go into hfs_write_inode() if we created the bad inode for invalid
inode ID? How is it possible?

Thanks,
Slava.

>         if (inode->i_ino < HFS_FIRSTUSER_CNID) {
>                 switch (inode->i_ino) {
>                 case HFS_ROOT_CNID:
>                         break;
>                 case HFS_EXT_CNID:
>                         hfs_btree_write(HFS_SB(inode->i_sb)->ext_tree);
>                         return 0;
>                 case HFS_CAT_CNID:
>                         hfs_btree_write(HFS_SB(inode->i_sb)->cat_tree);
>                         return 0;
>                 default:
>                         BUG();
>                         return -EIO;
>                 }
>         }
> 
> Unless this is because I'm modifying in-kernel memory than filesystem image,
> we will have to remove BUG() line.
> 
> On 2025/07/24 3:43, Viacheslav Dubeyko wrote:
> > This could be defined in Catalog File (maybe not). I didn't find anything
> > related to this in HFS specification.
> 
> https://developer.apple.com/library/archive/technotes/tn/tn1150.html#CNID  
> says "the CNID of zero is never used and serves as a nil value." That is,
> I think we can reject inode->i_ino == 0 case.
> 
> But I'm not sure for other values up to 15, expect values noted as "introduced
> with HFS Plus". We could filter values in bad_cnid_list bitmap, but filtering
> undefined values might not be sufficient for preserving BUG() line.