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

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

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.