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

[Shardul Bankar]

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.


> > +}
> > +
> > +/**
> > + * 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.

> > + *
> > + * 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.

> > +       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().


Thanks,
Shardul