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

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

On Mon, 2026-03-16 at 22:21 +0000, Viacheslav Dubeyko wrote:
> On Sun, 2026-03-15 at 22:50 +0530, Shardul Bankar wrote:
> > 
> > diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> > index 1220a2f22737..1c6a27e397fb 100644
> > --- a/fs/hfsplus/btree.c
> > +++ b/fs/hfsplus/btree.c
> > @@ -129,6 +129,95 @@ u32 hfsplus_calc_btree_clump_size(u32
> > block_size, u32 node_size,
> >         return clump_size;
> >  }
> >  
> > +/* Context for iterating b-tree map pages
> > + * @page_idx: The index of the page within the b-node's page array
> > + * @off: The byte offset within the mapped page
> > + * @len: The remaining length of the map record
> > + */
> > +struct hfs_bmap_ctx {
> > +       unsigned int page_idx;
> > +       unsigned int off;
> > +       u16 len;
> > +};
> > +
> > +/*
> > + * Finds the specific page containing the requested byte offset
> > within the map
> > + * record. Automatically handles the difference between header and
> > map nodes.
> > + * Returns the struct page pointer, or an ERR_PTR on failure.
> > + * Note: The caller is responsible for mapping/unmapping the
> > returned page.
> > + */
> > +static struct page *hfs_bmap_get_map_page(struct hfs_bnode *node,
> > struct hfs_bmap_ctx *ctx,
> > +                               u32 byte_offset)
> 
> I am simply guessing here... Should it be byte_offset? Why do we not
> use bit
> index here? Probably, byte_offset looks reasonable. However, all
> callers
> operates by bit index. I am not insisting of changing the interface.
> I am simply
> sharing some thoughts. :) What do you think?
> 

Keeping it as `byte_offset` felt slightly cleaner to me because the
helper's primary job is calculating page-level and byte-level
boundaries, leaving the bitwise mask math strictly to the wrapper
functions (`test_bit` and `clear_bit`). Since `hfs_bmap_alloc()` also
scans byte-by-byte, keeping the helper focused on bytes seemed like a
good separation of concerns. I will leave it as `byte_offset` for now
if that works for you!

> > +{
> > +       u16 rec_idx, off16;
> > +       unsigned int page_off;
> > +
> > +       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;
> 
> I worry about potential overflow error here because off16 is u16 and
> all other
> variables are unsigned int. What's about (u32)off16 here?
> 

Ah, great catch. While the C compiler will implicitly promote the u16
to an unsigned int during the addition, explicitly casting it to '(u32)
off16' makes the intention clear and silences any potential static
analysis warnings. I will add the cast in v7.

> > +       ctx->page_idx = page_off >> PAGE_SHIFT;
> > +       ctx->off = page_off & ~PAGE_MASK;
> > +
> > +       return node->page[ctx->page_idx];
> > +}
> > +
> > +/**
> > + * hfs_bmap_clear_bit - clear 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 0 on success, -EALREADY if already clear, or negative
> > error code.
> 
> I assume that error code is -EINVAL now.
> 

You are correct, I updated the code in v6 but missed updating the
docstring! I will fix this in v7.

Thanks,
Shardul