RE: [PATCH v5 2/2] hfsplus: validate b-tree node 0 bitmap at mount time

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

On Mon, 2026-03-09 at 17:16 +0530, Shardul Bankar wrote:
> On Mon, 2026-03-02 at 23:45 +0000, Viacheslav Dubeyko wrote:
> > On Sat, 2026-02-28 at 17:53 +0530, Shardul Bankar wrote:
> > > diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> > > index 87650e23cd65..ee1edb03a38e 100644
> > > --- a/fs/hfsplus/btree.c
> > > +++ b/fs/hfsplus/btree.c
> > > @@ -239,15 +239,31 @@ static int hfs_bmap_clear_bit(struct
> > > hfs_bnode *node, u32 bit_idx)
> > >         return 0;
> > >  }
> > >  
> > > +static const char *hfs_btree_name(u32 cnid)
> > > +{
> > > +       static const char * const tree_names[] = {
> > > +               [HFSPLUS_EXT_CNID] = "Extents",
> > > +               [HFSPLUS_CAT_CNID] = "Catalog",
> > > +               [HFSPLUS_ATTR_CNID] = "Attributes",
> > > +       };
> > > +
> > > +       if (cnid < ARRAY_SIZE(tree_names) && tree_names[cnid])
> > > +               return tree_names[cnid];
> > > +
> > 
> > #define HFS_POR_CNID            1       /* Parent Of the Root */
> > #define HFSPLUS_POR_CNID        HFS_POR_CNID
> > #define HFS_ROOT_CNID           2       /* ROOT directory */
> > #define HFSPLUS_ROOT_CNID       HFS_ROOT_CNID
> > #define HFS_EXT_CNID            3       /* EXTents B-tree */
> > #define HFSPLUS_EXT_CNID        HFS_EXT_CNID
> > #define HFS_CAT_CNID            4       /* CATalog B-tree */
> > #define HFSPLUS_CAT_CNID        HFS_CAT_CNID
> > #define HFS_BAD_CNID            5       /* BAD blocks file */
> > #define HFSPLUS_BAD_CNID        HFS_BAD_CNID
> > #define HFS_ALLOC_CNID          6       /* ALLOCation file (HFS+) */
> > #define HFSPLUS_ALLOC_CNID      HFS_ALLOC_CNID
> > #define HFS_START_CNID          7       /* STARTup file (HFS+) */
> > #define HFSPLUS_START_CNID      HFS_START_CNID
> > #define HFS_ATTR_CNID           8       /* ATTRibutes file (HFS+) */
> > #define HFSPLUS_ATTR_CNID       HFS_ATTR_CNID
> > #define HFS_EXCH_CNID           15      /* ExchangeFiles temp id */
> > #define HFSPLUS_EXCH_CNID       HFS_EXCH_CNID
> > #define HFS_FIRSTUSER_CNID      16      /* first available user id */
> > #define HFSPLUS_FIRSTUSER_CNID  HFS_FIRSTUSER_CNID
> > 
> > What if cnid will be 1, 2, 5? How correctly will logic works? For may
> > taste, the
> > declaration looks slightly dangerous.
> > 
> > It will much easier simply introduce the string constants:
> > 
> > #define HFS_EXTENT_TREE_NAME  "Extents"
> > ...
> > #define HFS_UNKNOWN_BTREE_NAME  "Unknown"
> > 
> > Probably, simple switch will be simpler implementation here:
> > 
> > switch (cnid) {
> > case HFSPLUS_EXT_CNID:
> >     return HFS_EXTENT_TREE_NAME;
> > ...
> > default:
> >     return HFS_UNKNOWN_BTREE_NAME;
> > }
> > 
> > Or it needs to introduce array that will initialize all items from 0
> > - 15.
> > 
> > Maybe, I am too picky here. This logic should work. But I prefer to
> > have string
> > declarations outside of function.
> > 
> 
> I originally used the array based on your feedback from the v4 review,
> where you mentioned preferring an array of constant strings over a
> switch statement.
> 
> To address your concern about unlisted indices like 1, 2, and 5: I
> tested this case locally to be absolutely sure. Because of how the
> compiler initializes arrays, any index not explicitly defined is set to
> NULL (0). For example, I temporarily removed HFSPLUS_CAT_CNID from the
> array and triggered the bug. The if (tree_names[cnid]) condition
> successfully caught the NULL and the kernel safely logged:
> hfsplus: (loop0): Unknown Btree (cnid 0x4) bitmap corruption
> detected...
> 
> That being said, I agree that defining the strings as macros outside
> the function combined with a standard switch statement makes the
> definitions much more visible to the rest of the subsystem. I am more
> than happy to rewrite it using the #define and switch approach exactly
> as you suggested for v6. Let me know which approach you prefer.
> 
> 

I think we need to declare the strings outside of the method. I recommended the
strings array because it's nice to have. But I missed the point that we don't
have the contiguous sequence of indexes. Because, we have only three strings to
distinguish, then solution based on switch looks like more clean and simple one.
Do you agree? :)

Thanks,
Slava.