On Fri, 2025-07-25 at 07:05 +0900, Tetsuo Handa wrote: > On 2025/07/25 4:49, Viacheslav Dubeyko wrote: > > 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 > > These declarations does not define 14, and some flags are never used despite > being declared here. > > > > > 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); > > Some of values in this constant are not declared. > It means that we need to declare the missing values. But hardcoded bare numbers are really bad practice. > > > > 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: > > This will save a lot of computational power compared to switch(). > Even if you would like to use flags, then the logic must to be simple and understandable. You still can use special inline function and do not create a mess in hfs_read_inode(). Especially, you can declare the mask one time in header, for example, but not to prepare the bad_cnid_list for every function call. Currently, the code looks really messy. > > > > 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(). > > Is hfs_read_inode() never called for HFS_EXT_CNID and HFS_CAT_CNID ? > The location of Catalog File and Extents File are defined in superblock. As a result, Catalog File cannot contain a record with CNID HFS_EXT_CNID or HFS_CAT_CNID. And if hfs_read_inode() receives these values, then it is some corruption of Catalog File. > > > > > > > > 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. > > The point of this change is to excecise inode->i_ino for all values between 0 and 15. > Some of values between 0 and 15 must be valid as inode->i_ino , doesn't these? Then, > If you have mask of valid or/and invalid, then you can simply check that this mask contain the flag. It will bed simple bit state checking. Currently, the code looks weird, not clear, complicated, and inefficient. > > > > > > > > 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? > > are all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 invalid value for hfs_read_inode() ? > > If all of 0, 1, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14 and 15 are invalid value for hfs_read_inode(), > and 3 and 4 are also invalid value for hfs_read_inode(), hfs_read_inode() would accept only 2. > Something is crazily wrong. > > Can we really filter some of values between 0 and 15 at hfs_read_inode() ? 0 value is invalid. #define HFS_POR_CNID 1 /* Parent Of the Root */ #define HFS_ROOT_CNID 2 /* ROOT directory */ These values are legitimate values. #define HFS_EXT_CNID 3 /* EXTents B-tree */ #define HFS_CAT_CNID 4 /* CATalog B-tree */ This metadata structures are defined in MDB. This is invalid values for hfs_read_inode(). #define HFS_BAD_CNID 5 /* BAD blocks file */ This could be defined in Catalog File because MDB has nothing for this metadata structure. However, it's ancient technology. #define HFS_ALLOC_CNID 6 /* ALLOCation file (HFS+) */ #define HFS_START_CNID 7 /* STARTup file (HFS+) */ #define HFS_ATTR_CNID 8 /* ATTRibutes file (HFS+) */ These value are invalid for HFS. 9, 10, 11, 12, 13, 14 can be defined as constants and it is invalid values. Foe example: #define HFS_RESERVED_CNID_9 9 #define HFS_RESERVED_CNID_10 10 ... #define HFS_RESERVED_CNID_14 14 #define HFS_EXCH_CNID 15 /* ExchangeFiles temp id */ This could be defined in Catalog File (maybe not). I didn't find anything related to this in HFS specification. So, 1, 2, 5, 15, etc can be accepted by hfs_read_inode(). 0, 3, 4, 6, 7, 8, 9, 10, 11, 12, 13, 14 is invalid values for hfs_read_inode(). Thanks, Slava.
