[PATCH v5 0/2] hfsplus: prevent b-tree allocator corruption

[PATCH v5 0/2] hfsplus: prevent b-tree allocator corruption

[Shardul Bankar]

Hi all,

This series addresses a Syzkaller-reported vulnerability where fuzzed
HFS+ images mark the B-tree Header Node (Node 0) as free in the
allocation bitmap. This violates a core filesystem invariant and
leads to allocator corruption and kernel panics.

To fix this safely and cleanly, the series is split into two parts:

Patch 1 introduces a unified API for B-tree map record access
(struct hfs_bmap_ctx, hfs_bmap_test_bit, and hfs_bmap_clear_bit) and
refactors the boilerplate kmap_local_page() logic out of
hfs_bmap_alloc() and hfs_bmap_free().
Patch 2 utilizes this new API to perform a mount-time validation of
Node 0, forcing a safe read-only mount if structural or bit-level
corruption is detected.

Note on Allocator Optimization: Following discussions in v4, there is
a recognized opportunity to optimize hfs_bmap_alloc() from a first-fit
to a next-fit allocator by caching an in-core allocation hint (roving
pointer) and bounding the scan with tree->node_count. To keep the
scope of this series strictly aligned with the Syzkaller corruption
fix, that architectural optimization is deferred to a separate,
follow-up patchset/thread.

Link: https://lore.kernel.org/all/20260226091235.927749-1-shardul.b@mpiricsoftware.com/

v5:
 - API Encapsulation: Introduced struct hfs_bmap_ctx to cleanly bundle
   offset, length, and page index state instead of passing multiple
   pointers, addressing reviewer feedback.

 - Bit-Level Helpers: Added hfs_bmap_test_bit() and hfs_bmap_clear_bit()
   to safely encapsulate mapping/unmapping for single-bit accesses
   (like the mount-time check and node freeing).

 - Performance Retention: Retained the page-level mapping approach for
   the linear scan inside hfs_bmap_alloc() to prevent the severe
   performance regression of mapping/unmapping on a per-byte basis,
   while refactoring it to use the new ctx struct.

 - Hexagon Overflow Fix: Fixed a 0-day Kernel Test Robot warning on
   architectures with 256KB page sizes by upgrading the offset variables
   in the new struct hfs_bmap_ctx to unsigned int, preventing 16-bit shift
   overflows.
   Link: https://lore.kernel.org/all/202602270310.eBmeD8VX-lkp@intel.com/

 - Map Record Spanning: Added a byte_offset parameter to the page mapper
   to correctly handle large map records that span across multiple 4KB
   pages.

 - Loop Mask Revert: Reverted the 0x80 bitmask in the alloc() inner loop
   back to its original state (and dropped the HFSPLUS_BTREE_NODE0_BIT
   macro), as it represents a generic sliding mask, not specifically
   Node 0.

 - String Array Cleanup: Replaced the verbose switch(id) block in the
   mount validation with a clean static array of constant strings for
   the CNID names, per reviewer feedback.

v4:
 - Split the changes into a 2-patch series (Refactoring + Bug Fix).
 - Extracted map node traversal into a generic helper (hfs_bmap_get_map_page)
   as per Slava's feedback, replacing manual offset/page management.
 - Added node-type validation (HFS_NODE_HEADER vs HFS_NODE_MAP) inside the
   helper to defend against structurally corrupted linkages.
 - Replaced hardcoded values with named macros (HFSPLUS_BTREE_NODE0_BIT, etc).
 - Handled invalid map offsets/lengths as corruption, continuing the mount
   as SB_RDONLY instead of failing it completely to preserve data recovery.

v3:
  - Moved validation logic inline into hfs_btree_open() to allow
    reporting the specific corrupted tree ID.
  - Replaced custom offset calculations with existing hfs_bnode_find()
    and hfs_brec_lenoff() infrastructure to handle node sizes and
    page boundaries correctly.
  - Removed temporary 'btree_bitmap_corrupted' superblock flag; setup
    SB_RDONLY directly upon detection.
  - Moved logging to hfs_btree_open() to include the specific tree ID in
    the warning message
  - Used explicit bitwise check (&) instead of test_bit() to ensure
    portability. test_bit() bit-numbering is architecture-dependent
    (e.g., bit 0 vs bit 7 can swap meanings on BE vs LE), whereas
    masking 0x80 consistently targets the MSB required by the HFS+
    on-disk format.

v2:
  - Fix compiler warning about comparing u16 bitmap_off with PAGE_SIZE which
can exceed u16 maximum on some architectures
  - Cast bitmap_off to unsigned int for the PAGE_SIZE comparison to avoid
tautological constant-out-of-range comparison warning.
  - Link: https://lore.kernel.org/oe-kbuild-all/202601251011.kJUhBF3P-lkp@intel.com/

Shardul Bankar (2):
  hfsplus: refactor b-tree map page access and add node-type validation
  hfsplus: validate b-tree node 0 bitmap at mount time

 fs/hfsplus/btree.c         | 222 +++++++++++++++++++++++++++++--------
 include/linux/hfs_common.h |   2 +
 2 files changed, 177 insertions(+), 47 deletions(-)

-- 
2.34.1

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

[Shardul Bankar]

In HFS+ b-trees, the node allocation bitmap is stored across multiple
records. The first chunk resides in the b-tree Header Node at record
index 2, while all subsequent chunks are stored in dedicated Map Nodes
at record index 0.

This structural quirk forces callers like hfs_bmap_alloc() and
hfs_bmap_free() to duplicate boilerplate code to validate offsets, correct
lengths, and map the underlying pages via kmap_local_page(). There is
also currently no strict node-type validation before reading these
records, leaving the allocator vulnerable if a corrupted image points a
map linkage to an Index or Leaf node.

Introduce a unified bit-level API to encapsulate the map record access:
1. A new 'struct hfs_bmap_ctx' to cleanly pass state and safely handle
   page math across all architectures.
2. 'hfs_bmap_get_map_page()': Automatically validates node types
   (HFS_NODE_HEADER vs HFS_NODE_MAP), infers the correct record index,
   and handles page-boundary math for records that span multiple pages.
3. 'hfs_bmap_test_bit()' and 'hfs_bmap_clear_bit()': Clean wrappers that
   internally handle page mapping/unmapping for single-bit operations.

Refactor hfs_bmap_alloc() and hfs_bmap_free() to utilize this new API.
This deduplicates the allocator logic, hardens the map traversal against
fuzzed images, and provides the exact abstractions needed for upcoming
mount-time validation checks.

Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 fs/hfsplus/btree.c         | 186 +++++++++++++++++++++++++++----------
 include/linux/hfs_common.h |   2 +
 2 files changed, 141 insertions(+), 47 deletions(-)

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 */
+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 */
+
+	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]);
+}
+
+/**
+ * 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
+ *
+ * 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;
+	int res;
+
+	data = hfs_bmap_get_map_page(node, &ctx, bit_idx / 8);
+	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);
+	res = (byte & m) ? 1 : 0;
+
+	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)
+{
+	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;
+	}
+
+	data[ctx.off] &= ~m;
+	set_page_dirty(node->page[ctx.page_idx]);
+	kunmap_local(data);
+
+	return 0;
+}
+
 /* Get a reference to a B*Tree and do some initial checks */
 struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 {
@@ -374,11 +484,8 @@ int hfs_bmap_reserve(struct hfs_btree *tree, u32 rsvd_nodes)
 struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 {
 	struct hfs_bnode *node, *next_node;
-	struct page **pagep;
+	struct hfs_bmap_ctx ctx;
 	u32 nidx, idx;
-	unsigned off;
-	u16 off16;
-	u16 len;
 	u8 *data, byte, m;
 	int i, res;
 
@@ -390,30 +497,25 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 	node = hfs_bnode_find(tree, nidx);
 	if (IS_ERR(node))
 		return node;
-	len = hfs_brec_lenoff(node, 2, &off16);
-	off = off16;
 
-	if (!is_bnode_offset_valid(node, off)) {
+	data = hfs_bmap_get_map_page(node, &ctx, 0);
+	if (IS_ERR(data)) {
+		res = PTR_ERR(data);
 		hfs_bnode_put(node);
-		return ERR_PTR(-EIO);
+		return ERR_PTR(res);
 	}
-	len = check_and_correct_requested_length(node, off, len);
 
-	off += node->page_offset;
-	pagep = node->page + (off >> PAGE_SHIFT);
-	data = kmap_local_page(*pagep);
-	off &= ~PAGE_MASK;
 	idx = 0;
 
 	for (;;) {
-		while (len) {
-			byte = data[off];
+		while (ctx.len) {
+			byte = data[ctx.off];
 			if (byte != 0xff) {
 				for (m = 0x80, i = 0; i < 8; m >>= 1, i++) {
 					if (!(byte & m)) {
 						idx += i;
-						data[off] |= m;
-						set_page_dirty(*pagep);
+						data[ctx.off] |= m;
+						set_page_dirty(node->page[ctx.page_idx]);
 						kunmap_local(data);
 						tree->free_nodes--;
 						mark_inode_dirty(tree->inode);
@@ -423,13 +525,13 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 					}
 				}
 			}
-			if (++off >= PAGE_SIZE) {
+			if (++ctx.off >= PAGE_SIZE) {
 				kunmap_local(data);
-				data = kmap_local_page(*++pagep);
-				off = 0;
+				data = kmap_local_page(node->page[++ctx.page_idx]);
+				ctx.off = 0;
 			}
 			idx += 8;
-			len--;
+			ctx.len--;
 		}
 		kunmap_local(data);
 		nidx = node->next;
@@ -443,22 +545,21 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
 			return next_node;
 		node = next_node;
 
-		len = hfs_brec_lenoff(node, 0, &off16);
-		off = off16;
-		off += node->page_offset;
-		pagep = node->page + (off >> PAGE_SHIFT);
-		data = kmap_local_page(*pagep);
-		off &= ~PAGE_MASK;
+		data = hfs_bmap_get_map_page(node, &ctx, 0);
+		if (IS_ERR(data)) {
+			res = PTR_ERR(data);
+			hfs_bnode_put(node);
+			return ERR_PTR(res);
+		}
 	}
 }
 
 void hfs_bmap_free(struct hfs_bnode *node)
 {
 	struct hfs_btree *tree;
-	struct page *page;
 	u16 off, len;
 	u32 nidx;
-	u8 *data, byte, m;
+	int res;
 
 	hfs_dbg("node %u\n", node->this);
 	BUG_ON(!node->this);
@@ -495,24 +596,15 @@ void hfs_bmap_free(struct hfs_bnode *node)
 		}
 		len = hfs_brec_lenoff(node, 0, &off);
 	}
-	off += node->page_offset + nidx / 8;
-	page = node->page[off >> PAGE_SHIFT];
-	data = kmap_local_page(page);
-	off &= ~PAGE_MASK;
-	m = 1 << (~nidx & 7);
-	byte = data[off];
-	if (!(byte & m)) {
-		pr_crit("trying to free free bnode "
-				"%u(%d)\n",
-			node->this, node->type);
-		kunmap_local(data);
-		hfs_bnode_put(node);
-		return;
+
+	res = hfs_bmap_clear_bit(node, nidx);
+	if (res == -EALREADY) {
+		pr_crit("trying to free free bnode %u(%d)\n",
+				node->this, node->type);
+	} else if (!res) {
+		tree->free_nodes++;
+		mark_inode_dirty(tree->inode);
 	}
-	data[off] = byte & ~m;
-	set_page_dirty(page);
-	kunmap_local(data);
+
 	hfs_bnode_put(node);
-	tree->free_nodes++;
-	mark_inode_dirty(tree->inode);
 }
diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
index dadb5e0aa8a3..be24c687858e 100644
--- a/include/linux/hfs_common.h
+++ b/include/linux/hfs_common.h
@@ -510,6 +510,8 @@ struct hfs_btree_header_rec {
 #define HFSPLUS_NODE_MXSZ			32768
 #define HFSPLUS_ATTR_TREE_NODE_SIZE		8192
 #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT	3
+#define HFSPLUS_BTREE_HDR_MAP_REC_INDEX		2	/* Map (bitmap) record in Header node */
+#define HFSPLUS_BTREE_MAP_NODE_REC_INDEX	0	/* Map record in Map Node */
 #define HFSPLUS_BTREE_HDR_USER_BYTES		128
 
 /* btree key type */
-- 
2.34.1

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

[Shardul Bankar]

Syzkaller reported an issue with corrupted HFS+ images where the b-tree
allocation bitmap indicates that the header node (Node 0) is free. Node 0
must always be allocated as it contains the b-tree header record and the
allocation bitmap itself. Violating this invariant leads to allocator
corruption, which cascades into kernel panics or undefined behavior when
the filesystem attempts to allocate blocks.

Prevent trusting a corrupted allocator state by adding a validation check
during hfs_btree_open(). Using the newly introduced hfs_bmap_test_bit()
helper, verify that the MSB of the first bitmap byte (representing Node 0)
is marked as allocated.

If corruption is detected (either structurally invalid map records or an
illegally cleared bit), print a warning identifying the specific
corrupted tree and force the filesystem to mount read-only (SB_RDONLY).
This prevents kernel panics from corrupted images while enabling data
recovery.

Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Link: https://lore.kernel.org/all/54dc9336b514fb10547e27c7d6e1b8b967ee2eda.camel@ibm.com/
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 fs/hfsplus/btree.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

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];
+
+	return "Unknown";
+}
+
 /* 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)
@@ -352,6 +368,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);
+	if (res < 0) {
+		pr_warn("(%s): %s Btree (cnid 0x%x) map record invalid/corrupted, forcing read-only.\n",
+				sb->s_id, hfs_btree_name(id), id);
+		pr_warn("Run fsck.hfsplus to repair.\n");
+		sb->s_flags |= SB_RDONLY;
+	} else if (res == 0) {
+		pr_warn("(%s): %s Btree (cnid 0x%x) bitmap corruption detected, forcing read-only.\n",
+				sb->s_id, hfs_btree_name(id), id);
+		pr_warn("Run fsck.hfsplus to repair.\n");
+		sb->s_flags |= SB_RDONLY;
+	}
+
+	hfs_bnode_put(node);
+
 	return tree;
 
  fail_page:
-- 
2.34.1

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

[Viacheslav Dubeyko]

On Sat, 2026-02-28 at 17:53 +0530, Shardul Bankar wrote:
> In HFS+ b-trees, the node allocation bitmap is stored across multiple
> records. The first chunk resides in the b-tree Header Node at record
> index 2, while all subsequent chunks are stored in dedicated Map Nodes
> at record index 0.
> 
> This structural quirk forces callers like hfs_bmap_alloc() and
> hfs_bmap_free() to duplicate boilerplate code to validate offsets, correct
> lengths, and map the underlying pages via kmap_local_page(). There is
> also currently no strict node-type validation before reading these
> records, leaving the allocator vulnerable if a corrupted image points a
> map linkage to an Index or Leaf node.
> 
> Introduce a unified bit-level API to encapsulate the map record access:
> 1. A new 'struct hfs_bmap_ctx' to cleanly pass state and safely handle
>    page math across all architectures.
> 2. 'hfs_bmap_get_map_page()': Automatically validates node types
>    (HFS_NODE_HEADER vs HFS_NODE_MAP), infers the correct record index,
>    and handles page-boundary math for records that span multiple pages.
> 3. 'hfs_bmap_test_bit()' and 'hfs_bmap_clear_bit()': Clean wrappers that
>    internally handle page mapping/unmapping for single-bit operations.
> 
> Refactor hfs_bmap_alloc() and hfs_bmap_free() to utilize this new API.
> This deduplicates the allocator logic, hardens the map traversal against
> fuzzed images, and provides the exact abstractions needed for upcoming
> mount-time validation checks.
> 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/btree.c         | 186 +++++++++++++++++++++++++++----------
>  include/linux/hfs_common.h |   2 +
>  2 files changed, 141 insertions(+), 47 deletions(-)
> 
> 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.

> +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?

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

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

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

> +	int res;

I am not sure that you are really need this variable.

> +
> +	data = hfs_bmap_get_map_page(node, &ctx, bit_idx / 8);

What's about BITS_PER_BYTE instead of hardcoded 8?

> +	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?

> +	res = (byte & m) ? 1 : 0;

You can simply return this statement.

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

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

Thanks,
Slava.

> +	}
> +
> +	data[ctx.off] &= ~m;
> +	set_page_dirty(node->page[ctx.page_idx]);
> +	kunmap_local(data);
> +
> +	return 0;
> +}
> +
>  /* Get a reference to a B*Tree and do some initial checks */
>  struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  {
> @@ -374,11 +484,8 @@ int hfs_bmap_reserve(struct hfs_btree *tree, u32 rsvd_nodes)
>  struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
>  {
>  	struct hfs_bnode *node, *next_node;
> -	struct page **pagep;
> +	struct hfs_bmap_ctx ctx;
>  	u32 nidx, idx;
> -	unsigned off;
> -	u16 off16;
> -	u16 len;
>  	u8 *data, byte, m;
>  	int i, res;
>  
> @@ -390,30 +497,25 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
>  	node = hfs_bnode_find(tree, nidx);
>  	if (IS_ERR(node))
>  		return node;
> -	len = hfs_brec_lenoff(node, 2, &off16);
> -	off = off16;
>  
> -	if (!is_bnode_offset_valid(node, off)) {
> +	data = hfs_bmap_get_map_page(node, &ctx, 0);
> +	if (IS_ERR(data)) {
> +		res = PTR_ERR(data);
>  		hfs_bnode_put(node);
> -		return ERR_PTR(-EIO);
> +		return ERR_PTR(res);
>  	}
> -	len = check_and_correct_requested_length(node, off, len);
>  
> -	off += node->page_offset;
> -	pagep = node->page + (off >> PAGE_SHIFT);
> -	data = kmap_local_page(*pagep);
> -	off &= ~PAGE_MASK;
>  	idx = 0;
>  
>  	for (;;) {
> -		while (len) {
> -			byte = data[off];
> +		while (ctx.len) {
> +			byte = data[ctx.off];
>  			if (byte != 0xff) {
>  				for (m = 0x80, i = 0; i < 8; m >>= 1, i++) {
>  					if (!(byte & m)) {
>  						idx += i;
> -						data[off] |= m;
> -						set_page_dirty(*pagep);
> +						data[ctx.off] |= m;
> +						set_page_dirty(node->page[ctx.page_idx]);
>  						kunmap_local(data);
>  						tree->free_nodes--;
>  						mark_inode_dirty(tree->inode);
> @@ -423,13 +525,13 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
>  					}
>  				}
>  			}
> -			if (++off >= PAGE_SIZE) {
> +			if (++ctx.off >= PAGE_SIZE) {
>  				kunmap_local(data);
> -				data = kmap_local_page(*++pagep);
> -				off = 0;
> +				data = kmap_local_page(node->page[++ctx.page_idx]);
> +				ctx.off = 0;
>  			}
>  			idx += 8;
> -			len--;
> +			ctx.len--;
>  		}
>  		kunmap_local(data);
>  		nidx = node->next;
> @@ -443,22 +545,21 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
>  			return next_node;
>  		node = next_node;
>  
> -		len = hfs_brec_lenoff(node, 0, &off16);
> -		off = off16;
> -		off += node->page_offset;
> -		pagep = node->page + (off >> PAGE_SHIFT);
> -		data = kmap_local_page(*pagep);
> -		off &= ~PAGE_MASK;
> +		data = hfs_bmap_get_map_page(node, &ctx, 0);
> +		if (IS_ERR(data)) {
> +			res = PTR_ERR(data);
> +			hfs_bnode_put(node);
> +			return ERR_PTR(res);
> +		}
>  	}
>  }
>  
>  void hfs_bmap_free(struct hfs_bnode *node)
>  {
>  	struct hfs_btree *tree;
> -	struct page *page;
>  	u16 off, len;
>  	u32 nidx;
> -	u8 *data, byte, m;
> +	int res;
>  
>  	hfs_dbg("node %u\n", node->this);
>  	BUG_ON(!node->this);
> @@ -495,24 +596,15 @@ void hfs_bmap_free(struct hfs_bnode *node)
>  		}
>  		len = hfs_brec_lenoff(node, 0, &off);
>  	}
> -	off += node->page_offset + nidx / 8;
> -	page = node->page[off >> PAGE_SHIFT];
> -	data = kmap_local_page(page);
> -	off &= ~PAGE_MASK;
> -	m = 1 << (~nidx & 7);
> -	byte = data[off];
> -	if (!(byte & m)) {
> -		pr_crit("trying to free free bnode "
> -				"%u(%d)\n",
> -			node->this, node->type);
> -		kunmap_local(data);
> -		hfs_bnode_put(node);
> -		return;
> +
> +	res = hfs_bmap_clear_bit(node, nidx);
> +	if (res == -EALREADY) {
> +		pr_crit("trying to free free bnode %u(%d)\n",
> +				node->this, node->type);
> +	} else if (!res) {
> +		tree->free_nodes++;
> +		mark_inode_dirty(tree->inode);
>  	}
> -	data[off] = byte & ~m;
> -	set_page_dirty(page);
> -	kunmap_local(data);
> +
>  	hfs_bnode_put(node);
> -	tree->free_nodes++;
> -	mark_inode_dirty(tree->inode);
>  }
> diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
> index dadb5e0aa8a3..be24c687858e 100644
> --- a/include/linux/hfs_common.h
> +++ b/include/linux/hfs_common.h
> @@ -510,6 +510,8 @@ struct hfs_btree_header_rec {
>  #define HFSPLUS_NODE_MXSZ			32768
>  #define HFSPLUS_ATTR_TREE_NODE_SIZE		8192
>  #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT	3
> +#define HFSPLUS_BTREE_HDR_MAP_REC_INDEX		2	/* Map (bitmap) record in Header node */
> +#define HFSPLUS_BTREE_MAP_NODE_REC_INDEX	0	/* Map record in Map Node */
>  #define HFSPLUS_BTREE_HDR_USER_BYTES		128
>  
>  /* btree key type */

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

[Viacheslav Dubeyko]

On Sat, 2026-02-28 at 17:53 +0530, Shardul Bankar wrote:
> Syzkaller reported an issue with corrupted HFS+ images where the b-tree
> allocation bitmap indicates that the header node (Node 0) is free. Node 0
> must always be allocated as it contains the b-tree header record and the
> allocation bitmap itself. Violating this invariant leads to allocator
> corruption, which cascades into kernel panics or undefined behavior when
> the filesystem attempts to allocate blocks.
> 
> Prevent trusting a corrupted allocator state by adding a validation check
> during hfs_btree_open(). Using the newly introduced hfs_bmap_test_bit()
> helper, verify that the MSB of the first bitmap byte (representing Node 0)
> is marked as allocated.
> 
> If corruption is detected (either structurally invalid map records or an
> illegally cleared bit), print a warning identifying the specific
> corrupted tree and force the filesystem to mount read-only (SB_RDONLY).
> This prevents kernel panics from corrupted images while enabling data
> recovery.
> 
> Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3D1c8ff72d0cd8a50dfeaa&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=3Xh1Zs8_REuLRLZWUeZtdPNWJAn9_uLWnGXCc-c5fi_fDbKHAGZHLiy9hnVwiCdw&s=28-20JIeoIS56JYKcsVH4GIMrpUgMAnM8UAVmznGshc&e= 
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_54dc9336b514fb10547e27c7d6e1b8b967ee2eda.camel-40ibm.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=3Xh1Zs8_REuLRLZWUeZtdPNWJAn9_uLWnGXCc-c5fi_fDbKHAGZHLiy9hnVwiCdw&s=njxWkO06rTfLLR1NjYq1vfuJLtGXfPVmWMjIuvQhpWY&e= 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/btree.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> 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.

> +	return "Unknown";
> +}
> +
>  /* 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)
> @@ -352,6 +368,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);
> +	if (res < 0) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) map record invalid/corrupted, forcing read-only.\n",
> +				sb->s_id, hfs_btree_name(id), id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	} else if (res == 0) {
> +		pr_warn("(%s): %s Btree (cnid 0x%x) bitmap corruption detected, forcing read-only.\n",
> +				sb->s_id, hfs_btree_name(id), id);
> +		pr_warn("Run fsck.hfsplus to repair.\n");
> +		sb->s_flags |= SB_RDONLY;
> +	}
> +
> +	hfs_bnode_put(node);
> +
>  	return tree;
>  
>   fail_page:

This logic looks mostly good. My main remarks are in the first patch.

Thanks,
Slava.

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

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

[Shardul Bankar]

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.

Thanks,
Shardul

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.

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

[Viacheslav Dubeyko]

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? :)

Thanks,
Slava.

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