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

[Shardul Bankar]

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.

Thanks,
Shardul