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

[Shardul Bankar]

On Mon, 2026-03-09 at 19:39 +0000, Viacheslav Dubeyko wrote:
> 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? :)
> 

Agreed.

I will get v6 prepared with all of these discussed changes.

Thank you for all the feedback!

Regards,
Shardul