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

[Shardul Bankar <shardul.b@mpiricsoftware.com>]

On Mon, 2026-03-16 at 22:35 +0000, Viacheslav Dubeyko wrote:
> On Sun, 2026-03-15 at 22:50 +0530, Shardul Bankar wrote:
> > 
> > +/**
> > + * hfs_bmap_test_bit - test a bit in the b-tree map
> > + * @node: the b-tree node containing the map record
> > + * @node_bit_idx: the relative bit index within the node's map
> > record
> > + *
> > + * Returns 1 if set, 0 if clear, or a negative error code on
> > failure.
> > + */
> > +static int hfs_bmap_test_bit(struct hfs_bnode *node, u32
> > node_bit_idx)
> 
> Why not return bool data type?
> 
> > +{
> > +       struct hfs_bmap_ctx ctx;
> > +       struct page *page;
> > +       u8 *bmap, byte, mask;
> > +
> > +       page = hfs_bmap_get_map_page(node, &ctx, node_bit_idx /
> > BITS_PER_BYTE);
> > +       if (IS_ERR(page))
> > +               return PTR_ERR(page);
> 
> We can return false for the case of error.
> 
> > +
> > +       bmap = kmap_local_page(page);
> > +       byte = bmap[ctx.off];
> > +       kunmap_local(bmap);
> > +
> > +       mask = 1 << (7 - (node_bit_idx % BITS_PER_BYTE));
> > +       return (byte & mask) ? 1 : 0;
> 
> This is why I would like to see bool data type. :)
> 

I completely agree. Changing the return type to `bool` and returning
`false` on both an IO error and a cleared bit vastly simplifies the
caller. I will update `hfs_bmap_test_bit()` to return a `bool`, and I
will collapse the two `res < 0` and `res == 0` validation checks in
`hfs_btree_open()` into a single `if (!hfs_bmap_test_bit(node, 0))`
block in v7.

> > +}
> > +
> > +
> >  /**
> >   * hfs_bmap_clear_bit - clear a bit in the b-tree map
> >   * @node: the b-tree node containing the map record
> > @@ -218,15 +244,36 @@ static int hfs_bmap_clear_bit(struct
> > hfs_bnode *node, u32 node_bit_idx)
> >         return 0;
> >  }
> >  
> > +#define HFS_EXTENT_TREE_NAME  "Extents"
> 
> Maybe we need to have Extents Overflow File (or B-tree), Catalog
> file,
> Attributes file?
> 

Good point, those are the proper structural names. For v7, I will
update the macros to "Extents Overflow File", "Catalog File", and
"Attributes File", and I will slightly tweak the `pr_warn` format
string to accommodate the new names gracefully.

> > +#define HFS_CATALOG_TREE_NAME "Catalog"
> > +#define HFS_ATTR_TREE_NAME    "Attributes"
> > +#define HFS_UNKNOWN_TREE_NAME "Unknown"
> > +
> > +static const char *hfs_btree_name(u32 cnid)
> > +{
> > +       switch (cnid) {
> > +       case HFSPLUS_EXT_CNID:
> > +               return HFS_EXTENT_TREE_NAME;
> > +       case HFSPLUS_CAT_CNID:
> > +               return HFS_CATALOG_TREE_NAME;
> > +       case HFSPLUS_ATTR_CNID:
> > +               return HFS_ATTR_TREE_NAME;
> > +       default:
> > +               return HFS_UNKNOWN_TREE_NAME;
> > +       }
> > +}
> > +
> >  /* Get a reference to a B*Tree and do some initial checks */
> >  struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
> >  {
> >         struct hfs_btree *tree;
> >         struct hfs_btree_header_rec *head;
> >         struct address_space *mapping;
> > +       struct hfs_bnode *node;
> >         struct inode *inode;
> >         struct page *page;
> >         unsigned int size;
> > +       int res;
> >  
> >         tree = kzalloc_obj(*tree);
> >         if (!tree)
> > @@ -331,6 +378,26 @@ struct hfs_btree *hfs_btree_open(struct
> > super_block *sb, u32 id)
> >  
> >         kunmap_local(head);
> >         put_page(page);
> > +
> > +       node = hfs_bnode_find(tree, HFSPLUS_TREE_HEAD);
> > +       if (IS_ERR(node))
> > +               goto free_inode;
> > +
> > +       res = hfs_bmap_test_bit(node, 0);
> 
> We definitely can return false for both cases.
> 

Ack'ed

I will prepare the v7 patchset with these final stylistic polishings. 

Thanks for the guidance throughout this series!
Shardul