RE: [PATCH v5 1/2] hfsplus: refactor b-tree map page access and add node-type validation

[Viacheslav Dubeyko]

On Mon, 2026-03-09 at 17:16 +0530, Shardul Bankar wrote:
> On Mon, 2026-03-02 at 23:25 +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 1220a2f22737..87650e23cd65 100644
> > > --- a/fs/hfsplus/btree.c
> > > +++ b/fs/hfsplus/btree.c
> > > @@ -129,6 +129,116 @@ u32 hfsplus_calc_btree_clump_size(u32
> > > block_size, u32 node_size,
> > >         return clump_size;
> > >  }
> > >  
> > > +/* Context for iterating b-tree map pages */
> > 
> > If we have some comments here, then let's add the description of
> > fields.
> > 
> 
> Ack'ed.
> 
> > > +struct hfs_bmap_ctx {
> > > +       unsigned int page_idx;
> > > +       unsigned int off;
> > > +       u16 len;
> > > +};
> > > +
> > > +/*
> > > + * Maps the specific page containing the requested byte offset
> > > within the map
> > > + * record.
> > > + * Automatically handles the difference between header and map
> > > nodes.
> > > + * Returns the mapped data pointer, or an ERR_PTR on failure.
> > > + * Note: The caller is responsible for calling kunmap_local(data).
> > > + */
> > > +static u8 *hfs_bmap_get_map_page(struct hfs_bnode *node, struct
> > > hfs_bmap_ctx *ctx,
> > > +                               u32 byte_offset)
> > > +{
> > > +       u16 rec_idx, off16;
> > > +       unsigned int page_off; /* 32-bit math prevents LKP overflow
> > > warnings */
> > 
> > Do we really need this comment?
> > 
> 
> Ack'ed, will remove it.
> 
> > > +
> > > +       if (node->this == HFSPLUS_TREE_HEAD) {
> > > +               if (node->type != HFS_NODE_HEADER) {
> > > +                       pr_err("hfsplus: invalid btree header
> > > node\n");
> > > +                       return ERR_PTR(-EIO);
> > > +               }
> > > +               rec_idx = HFSPLUS_BTREE_HDR_MAP_REC_INDEX;
> > > +       } else {
> > > +               if (node->type != HFS_NODE_MAP) {
> > > +                       pr_err("hfsplus: invalid btree map
> > > node\n");
> > > +                       return ERR_PTR(-EIO);
> > > +               }
> > > +               rec_idx = HFSPLUS_BTREE_MAP_NODE_REC_INDEX;
> > > +       }
> > > +
> > > +       ctx->len = hfs_brec_lenoff(node, rec_idx, &off16);
> > > +       if (!ctx->len)
> > > +               return ERR_PTR(-ENOENT);
> > > +
> > > +       if (!is_bnode_offset_valid(node, off16))
> > > +               return ERR_PTR(-EIO);
> > > +
> > > +       ctx->len = check_and_correct_requested_length(node, off16,
> > > ctx->len);
> > > +
> > > +       if (byte_offset >= ctx->len)
> > > +               return ERR_PTR(-EINVAL);
> > > +
> > > +       page_off = off16 + node->page_offset + byte_offset;
> > > +       ctx->page_idx = page_off >> PAGE_SHIFT;
> > > +       ctx->off = page_off & ~PAGE_MASK;
> > > +
> > > +       return kmap_local_page(node->page[ctx->page_idx]);
> > 
> > This pattern makes me really nervous. :) What if we can calculate the
> > struct
> > hfs_bmap_ctx *ctx in this function only. And, then, caller will use
> > kmap_local_page()/kunmap_local() in one place.
> > 
> 
> Good point. Hiding the kmap inside the helper while forcing the caller
> to kunmap is a dangerous pattern.
> In v6, I will rename the helper to hfs_bmap_compute_ctx(). It will
> solely perform the offset math and populate the ctx. The caller will
> then explicitly map and unmap the page, ensuring the lifecycle is
> perfectly clear.
> 

Potentially, this method can return pointer on memory page and caller can do
kmap_local_page()/kunmap_local().

> 
> > > +}
> > > +
> > > +/**
> > > + * hfs_bmap_test_bit - test a bit in the b-tree map
> > > + * @node: the b-tree node containing the map record
> > > + * @bit_idx: the bit index relative to the start of the map record
> > 
> > This sounds slightly confusing. Is it bit starting from the whole map
> > or from
> > particular map's portion?
> > 
> 
> The bit_idx passed to these helpers is strictly relative to the start
> of the current map node's record.

Sounds good.

> 
> > > + *
> > > + * 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 bit_idx)
> > > +{
> > > +       struct hfs_bmap_ctx ctx;
> > > +       u8 *data, byte, m;
> > 
> > I think we can use bmap instead of data. The bmap name can show the
> > nature of
> > data here. Do you agree?
> > 
> > I can follow what byte name means. Frankly speaking, I don't know why
> > m name is
> > used. :)
> > 
> 
> Ack'ed. For v6, I will use bmap, mask (instead of m).
> 
> > > +       int res;
> > 
> > I am not sure that you are really need this variable.
> > 
> 
> Ack'ed.
> 
> > > +
> > > +       data = hfs_bmap_get_map_page(node, &ctx, bit_idx / 8);
> > 
> > What's about BITS_PER_BYTE instead of hardcoded 8?
> > 
> 
> Ack'ed.
> 
> > > +       if (IS_ERR(data))
> > > +               return PTR_ERR(data);
> > > +
> > > +       byte = data[ctx.off];
> > > +       kunmap_local(data);
> > > +
> > > +       /* In HFS+ bitmaps, bit 0 is the MSB (0x80) */
> > > +       m = 1 << (~bit_idx & 7);
> > 
> > I am not sure that this calculation is correct. Because bit_idx is
> > absolute
> > index in the whole map but this operation is inside of particular
> > portion of the
> > map. Are you sure that this logic will be correct if we have b-tree
> > map in
> > several nodes?
> > 
> 
> I completely understand the concern here, and you are right that if
> bit_idx were the absolute index across the entire B-tree, this bitwise
> math would break when crossing node boundaries.
> 
> However, the architecture here relies on a separation of concerns:
> hfs_bmap_free() handles traversing the map chain, while
> hfs_bmap_clear_bit() operates strictly on a single node.
> In hfs_bmap_free(), the "while (nidx >= len * 8)" loop continuously
> subtracts the previous nodes' capacities (nidx -= len * 8) as it
> traverses the chain. By the time it calls hfs_bmap_clear_bit(node,
> nidx), the index is strictly relative to the start of that specific
> hfs_bnode's map record. Because the index is relative to the node, the
> bitwise math safely calculates the correct byte and bit.
> 
> To ensure future developers do not mistake this for an absolute index
> and misuse the API, I will rename the argument from bit_idx to
> node_bit_idx (or relative_idx) in v6 and explicitly document that it
> must be bounded by the node's record length.

Makes sense.

> 
> > > +       res = (byte & m) ? 1 : 0;
> > 
> > You can simply return this statement.
> 
> Ack'ed.
> 
> > 
> > > +
> > > +       return res;
> > > +}
> > > +
> > > +/**
> > > + * hfs_bmap_clear_bit - clear a bit in the b-tree map
> > > + * @node: the b-tree node containing the map record
> > > + * @bit_idx: the bit index relative to the start of the map record
> > > + *
> > > + * Returns 0 on success, -EALREADY if already clear, or negative
> > > error code.
> > > + */
> > > +static int hfs_bmap_clear_bit(struct hfs_bnode *node, u32 bit_idx)
> > 
> > I have the same remarks and concerns for this method too. Please, see
> > my remarks
> > above.
> > 
> 
> Addressed above.
> 
> > > +{
> > > +       struct hfs_bmap_ctx ctx;
> > > +       u8 *data, m;
> > > +
> > > +       data = hfs_bmap_get_map_page(node, &ctx, bit_idx / 8);
> > > +       if (IS_ERR(data))
> > > +               return PTR_ERR(data);
> > > +
> > > +       m = 1 << (~bit_idx & 7);
> > > +
> > > +       if (!(data[ctx.off] & m)) {
> > > +               kunmap_local(data);
> > > +               return -EALREADY;
> > 
> > I am not sure about this error code:
> > 
> > #define EALREADY        114     /* Operation already in progress */
> > 
> > It sounds more like -EINVAL.
> > 
> 
> Agreed, I will change the error code from -EALREADY to -EINVAL in v6.
> 
> One additional note for v6: I realized that introducing
> hfs_bmap_test_bit() in Patch 1 without calling it until Patch 2
> triggers a -Wunused-function compiler warning, which breaks git bisect
> cleanliness. To fix this, I will move the introduction of
> hfs_bmap_test_bit() into Patch 2 where it is actually consumed by the
> mount-time check. hfs_bmap_clear_bit() will remain in Patch 1 since it
> is immediately consumed by hfs_bmap_free().
> 
> 

OK. Makes sense.

Thanks,
Slava.