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.
