[PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar]

Add bitmap validation during HFS+ btree open to detect corruption where
node 0 (header node) is not marked allocated.

Syzkaller reported issues with corrupted HFS+ images where the btree
allocation bitmap indicates that the header node is free. Node 0 must
always be allocated as it contains the btree header record and the
allocation bitmap itself. Violating this invariant can lead to kernel
panics or undefined behavior when the filesystem attempts to allocate
blocks or manipulate the btree.

The validation checks the node allocation bitmap in the btree header
node (record #2) and verifies that bit 7 (MSB) of the first byte is
set.

Implementation details:
- Perform validation inside hfs_btree_open() to allow identifying the
  specific tree (Extents, Catalog, or Attributes) involved.
- Use hfs_bnode_find() and hfs_brec_lenoff() to safely access the
  bitmap record using existing infrastructure, ensuring correct handling
  of multi-page nodes and endianness.
- If corruption is detected, print a warning identifying the specific
  btree and force the filesystem to mount read-only (SB_RDONLY).

This prevents kernel panics from corrupted syzkaller-generated images
while enabling data recovery by allowing the mount to proceed in
read-only mode rather than failing completely.

Reported-by: syzbot+1c8ff72d0cd8a50dfeaa@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Link: https://lore.kernel.org/all/b78c1e380a17186b73bc8641b139eca56a8de964.camel@ibm.com/
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
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/

 fs/hfsplus/btree.c         | 27 +++++++++++++++++++++++++++
 include/linux/hfs_common.h |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 229f25dc7c49..ae81608ba3cf 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -135,9 +135,12 @@ 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;
+	u16 len, bitmap_off;
 	struct inode *inode;
 	struct page *page;
 	unsigned int size;
+	u8 bitmap_byte;
 
 	tree = kzalloc(sizeof(*tree), GFP_KERNEL);
 	if (!tree)
@@ -242,6 +245,30 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
 
 	kunmap_local(head);
 	put_page(page);
+
+	/*
+	 * Validate bitmap: node 0 (header node) must be marked allocated.
+	 */
+
+	node = hfs_bnode_find(tree, 0);
+	if (IS_ERR(node))
+		goto free_inode;
+
+	len = hfs_brec_lenoff(node,
+			HFSPLUS_BTREE_HDR_MAP_REC, &bitmap_off);
+
+	if (len != 0 && bitmap_off >= sizeof(struct hfs_bnode_desc)) {
+		hfs_bnode_read(node, &bitmap_byte, bitmap_off, 1);
+		if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
+			pr_warn("(%s): Btree 0x%x bitmap corruption detected, forcing read-only.\n",
+					sb->s_id, id);
+			pr_warn("Run fsck.hfsplus to repair.\n");
+			sb->s_flags |= SB_RDONLY;
+		}
+	}
+
+	hfs_bnode_put(node);
+
 	return tree;
 
  fail_page:
diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
index dadb5e0aa8a3..8d21d476cb57 100644
--- a/include/linux/hfs_common.h
+++ b/include/linux/hfs_common.h
@@ -510,7 +510,9 @@ 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		2	/* Map (bitmap) record in header node */
 #define HFSPLUS_BTREE_HDR_USER_BYTES		128
+#define HFSPLUS_BTREE_NODE0_BIT		0x80
 
 /* btree key type */
 #define HFSPLUS_KEY_CASEFOLDING		0xCF	/* case-insensitive */
-- 
2.34.1

Re: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Viacheslav Dubeyko]

On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> Add bitmap validation during HFS+ btree open to detect corruption where
> node 0 (header node) is not marked allocated.
> 
> Syzkaller reported issues with corrupted HFS+ images where the btree
> allocation bitmap indicates that the header node is free. Node 0 must
> always be allocated as it contains the btree header record and the
> allocation bitmap itself. Violating this invariant can lead to kernel
> panics or undefined behavior when the filesystem attempts to allocate
> blocks or manipulate the btree.
> 
> The validation checks the node allocation bitmap in the btree header
> node (record #2) and verifies that bit 7 (MSB) of the first byte is
> set.
> 
> Implementation details:
> - Perform validation inside hfs_btree_open() to allow identifying the
>   specific tree (Extents, Catalog, or Attributes) involved.
> - Use hfs_bnode_find() and hfs_brec_lenoff() to safely access the
>   bitmap record using existing infrastructure, ensuring correct handling
>   of multi-page nodes and endianness.
> - If corruption is detected, print a warning identifying the specific
>   btree and force the filesystem to mount read-only (SB_RDONLY).
> 
> This prevents kernel panics from corrupted syzkaller-generated images
> while enabling data recovery by allowing the mount to proceed in
> read-only mode rather than failing completely.
> 
> 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=ZiJOY272ajYn0UgEhQq0U3C9KoL8bXVIz-hEdqVFFyq4-BUdF-Y9LyG7SY6KXPxy&s=-ky4imqw2w8Seb2r8c8J_0WPl8IixUJ_l5gd_Qa5jaY&e= 
> Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_b78c1e380a17186b73bc8641b139eca56a8de964.camel-40ibm.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=ZiJOY272ajYn0UgEhQq0U3C9KoL8bXVIz-hEdqVFFyq4-BUdF-Y9LyG7SY6KXPxy&s=i7Wn6enf_IfV0HJbT1gEQzRANQeS2mYs7PxZCfeFDpo&e= 
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
> 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://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_oe-2Dkbuild-2Dall_202601251011.kJUhBF3P-2Dlkp-40intel.com_&d=DwIDAg&c=BSDicqBQBDjDI9RkVyTcHQ&r=q5bIm4AXMzc8NJu1_RGmnQ2fMWKq4Y4RAkElvUgSs00&m=ZiJOY272ajYn0UgEhQq0U3C9KoL8bXVIz-hEdqVFFyq4-BUdF-Y9LyG7SY6KXPxy&s=xyhPoIYqdHtYWxSD6xwNESfdIbOCKcu-xjE10KCMsAk&e= 
> 
>  fs/hfsplus/btree.c         | 27 +++++++++++++++++++++++++++
>  include/linux/hfs_common.h |  2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 229f25dc7c49..ae81608ba3cf 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -135,9 +135,12 @@ 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;
> +	u16 len, bitmap_off;
>  	struct inode *inode;
>  	struct page *page;
>  	unsigned int size;
> +	u8 bitmap_byte;
>  
>  	tree = kzalloc(sizeof(*tree), GFP_KERNEL);
>  	if (!tree)
> @@ -242,6 +245,30 @@ struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  
>  	kunmap_local(head);
>  	put_page(page);
> +
> +	/*
> +	 * Validate bitmap: node 0 (header node) must be marked allocated.
> +	 */
> +
> +	node = hfs_bnode_find(tree, 0);

If you introduce named constant for herder node, then you don't need add this
comment. And I don't like hardcoded value, anyway. :)

> +	if (IS_ERR(node))
> +		goto free_inode;
> +
> +	len = hfs_brec_lenoff(node,
> +			HFSPLUS_BTREE_HDR_MAP_REC, &bitmap_off);
> +
> +	if (len != 0 && bitmap_off >= sizeof(struct hfs_bnode_desc)) {

If we read incorrect len and/or bitmap_off, then it sounds like corruption too.
We need to process this issue somehow but you ignore this, currently. ;)

> +		hfs_bnode_read(node, &bitmap_byte, bitmap_off, 1);

I assume that 1 is the size of byte, then sizeof(u8) or sizeof(bitmap_byte)
could look much better than hardcoded value.

> +		if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {

Why don't use the test_bit() [1] here? I believe that code will be more simple
in such case.

> +			pr_warn("(%s): Btree 0x%x bitmap corruption detected, forcing read-only.\n",

I prefer to mention what do we mean by 0x%x. Currently, it looks complicated to
follow.

> +					sb->s_id, id);
> +			pr_warn("Run fsck.hfsplus to repair.\n");
> +			sb->s_flags |= SB_RDONLY;
> +		}
> +	}
> +
> +	hfs_bnode_put(node);
> +
>  	return tree;
>  
>   fail_page:
> diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
> index dadb5e0aa8a3..8d21d476cb57 100644
> --- a/include/linux/hfs_common.h
> +++ b/include/linux/hfs_common.h
> @@ -510,7 +510,9 @@ 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		2	/* Map (bitmap) record in header node */

Maybe, HFSPLUS_BTREE_HDR_MAP_REC_INDEX?

>  #define HFSPLUS_BTREE_HDR_USER_BYTES		128
> +#define HFSPLUS_BTREE_NODE0_BIT		0x80

Maybe, (1 << something) instead of 0x80? I am OK with constant too.

>  
>  /* btree key type */
>  #define HFSPLUS_KEY_CASEFOLDING		0xCF	/* case-insensitive */

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.19-rc5/source/include/linux/bitops.h#L60

Re: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar]

On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> > 
> > +
> > +       /*
> > +        * Validate bitmap: node 0 (header node) must be marked
> > allocated.
> > +        */
> > +
> > +       node = hfs_bnode_find(tree, 0);
> 
> If you introduce named constant for herder node, then you don't need
> add this
> comment. And I don't like hardcoded value, anyway. :)

Hi Slava, thank you for the review.

Ack'ed. I will use HFSPLUS_TREE_HEAD (0) in v4.

> > +       len = hfs_brec_lenoff(node,
> > +                       HFSPLUS_BTREE_HDR_MAP_REC, &bitmap_off);
> > +
> > +       if (len != 0 && bitmap_off >= sizeof(struct
> > hfs_bnode_desc)) {
> 
> If we read incorrect len and/or bitmap_off, then it sounds like
> corruption too.
> We need to process this issue somehow but you ignore this, currently.
> ;)
> 

I agree that invalid offsets constitute corruption. However, properly
validating the structure of the record table and offsets is a larger
scope change. I prefer to keep this patch focused specifically on the
"unallocated node 0" vulnerability reported by syzbot. I am happy to
submit a follow-up patch to harden hfs_brec_lenoff usage. As per your
suggestion, ignoring this currently. ;)

> > +               hfs_bnode_read(node, &bitmap_byte, bitmap_off, 1);
> 
> I assume that 1 is the size of byte, then sizeof(u8) or
> sizeof(bitmap_byte)
> could look much better than hardcoded value.

Ack'ed. Changing to sizeof(bitmap_byte).

> 
> > +               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> 
> Why don't use the test_bit() [1] here? I believe that code will be
> more simple
> in such case.

I reviewed test_bit(), but I believe the explicit mask is safer and
more correct here for three reasons:
1. Endianness:
The value we’re checking is an on-disk bitmap byte (MSB of the first
byte in the header map record). test_bit() is designed for CPU-native
memory bitmaps. HFS+ bitmaps use Big-Endian bit ordering (Node 0 is the
MSB/0x80). On Little-Endian architectures (like x86), test_bit(0, ...)
checks the LSB (0x01). Using it here could introduce bit-numbering
ambiguity.

For example, reading into an unsigned long:
    unsigned long word = 0;
    hfs_bnode_read(node, &word, bitmap_off, sizeof(word));
    if (!test_bit(N, &word))
        ...

...but now N is not obviously “MSB of first on-disk byte”; it depends
on CPU endianness/bit numbering conventions, so it becomes easy to get
wrong.

2. Consistency with Existing HFS+ Bitmap Code:
The existing allocator code already handles on-disk bitmap bytes via
explicit masking (hfs_bmap_alloc uses 0x80, 0x40, ...), so for
consistency with existing on-disk bitmap handling and to avoid the
above ambiguity, I kept the explicit mask check here as well:
    if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT))   (with
HFSPLUS_BTREE_NODE0_BIT = BIT(7) (or (1 <<7)))

3. Buffer safety:
Reading exactly 1 byte (u8) guarantees we never read more data than
strictly required, avoiding potential boundary issues.

Am I missing something here or does this make sense?

If there's a strong preference for bitops helpers, I could investigate
the big-endian bit helpers (*_be), but for this single-byte invariant
check, the explicit mask seems clearest and most consistent with
existing code.

> 
> > +                       pr_warn("(%s): Btree 0x%x bitmap corruption
> > detected, forcing read-only.\n",
> 
> I prefer to mention what do we mean by 0x%x. Currently, it looks
> complicated to
> follow.

Ack'ed. I am adding a lookup to print the human-readable tree name
(Catalog, Extents, Attributes) alongside the ID.

> 
> >  #define HFSPLUS_ATTR_TREE_NODE_SIZE            8192
> >  #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT      3
> > +#define HFSPLUS_BTREE_HDR_MAP_REC              2       /* Map
> > (bitmap) record in header node */
> 
> Maybe, HFSPLUS_BTREE_HDR_MAP_REC_INDEX?

Ack'ed.

> 
> >  #define HFSPLUS_BTREE_HDR_USER_BYTES           128
> > +#define HFSPLUS_BTREE_NODE0_BIT                0x80
> 
> Maybe, (1 << something) instead of 0x80? I am OK with constant too.

Ack'ed, will use (1 << 7). Can also use BIT(7) if you prefer.

> Thanks,
Shardul

RE: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Viacheslav Dubeyko]

On Tue, 2026-02-03 at 14:28 +0530, Shardul Bankar wrote:
> On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> > On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> > > 
> > > +
> > > +       /*
> > > +        * Validate bitmap: node 0 (header node) must be marked
> > > allocated.
> > > +        */
> > > +
> > > +       node = hfs_bnode_find(tree, 0);
> > 
> > If you introduce named constant for herder node, then you don't need
> > add this
> > comment. And I don't like hardcoded value, anyway. :)
> 
> Hi Slava, thank you for the review.
> 
> Ack'ed. I will use HFSPLUS_TREE_HEAD (0) in v4.
> 
> > > +       len = hfs_brec_lenoff(node,
> > > +                       HFSPLUS_BTREE_HDR_MAP_REC, &bitmap_off);
> > > +
> > > +       if (len != 0 && bitmap_off >= sizeof(struct
> > > hfs_bnode_desc)) {
> > 
> > If we read incorrect len and/or bitmap_off, then it sounds like
> > corruption too.
> > We need to process this issue somehow but you ignore this, currently.
> > ;)
> > 
> 
> I agree that invalid offsets constitute corruption. However, properly
> validating the structure of the record table and offsets is a larger
> scope change. I prefer to keep this patch focused specifically on the
> "unallocated node 0" vulnerability reported by syzbot. I am happy to
> submit a follow-up patch to harden hfs_brec_lenoff usage. As per your
> suggestion, ignoring this currently. ;)

I am not suggesting to check everything. But because you are using these values
and you can detect that these value is incorrect, then it makes sense to report
the error if something is wrong.

> 
> > > +               hfs_bnode_read(node, &bitmap_byte, bitmap_off, 1);
> > 
> > I assume that 1 is the size of byte, then sizeof(u8) or
> > sizeof(bitmap_byte)
> > could look much better than hardcoded value.
> 
> Ack'ed. Changing to sizeof(bitmap_byte).
> 
> > 
> > > +               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> > 
> > Why don't use the test_bit() [1] here? I believe that code will be
> > more simple
> > in such case.
> 
> I reviewed test_bit(), but I believe the explicit mask is safer and
> more correct here for three reasons:
> 1. Endianness:
> The value we’re checking is an on-disk bitmap byte (MSB of the first
> byte in the header map record). test_bit() is designed for CPU-native
> memory bitmaps. HFS+ bitmaps use Big-Endian bit ordering (Node 0 is the
> MSB/0x80). On Little-Endian architectures (like x86), test_bit(0, ...)
> checks the LSB (0x01). Using it here could introduce bit-numbering
> ambiguity.
> 
> For example, reading into an unsigned long:
>     unsigned long word = 0;
>     hfs_bnode_read(node, &word, bitmap_off, sizeof(word));
>     if (!test_bit(N, &word))
>         ...
> 
> ...but now N is not obviously “MSB of first on-disk byte”; it depends
> on CPU endianness/bit numbering conventions, so it becomes easy to get
> wrong.
> 
> 2. Consistency with Existing HFS+ Bitmap Code:
> The existing allocator code already handles on-disk bitmap bytes via
> explicit masking (hfs_bmap_alloc uses 0x80, 0x40, ...), so for
> consistency with existing on-disk bitmap handling and to avoid the
> above ambiguity, I kept the explicit mask check here as well:
>     if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT))   (with
> HFSPLUS_BTREE_NODE0_BIT = BIT(7) (or (1 <<7)))
> 
> 3. Buffer safety:
> Reading exactly 1 byte (u8) guarantees we never read more data than
> strictly required, avoiding potential boundary issues.
> 
> Am I missing something here or does this make sense?

Correct me if I am wrong, but I suspect that I am right. :) The endianness is
about bytes not bits. I am googled this: "Endianness defines the order in which
bytes, constituting multibyte data (like 16, 32, or 64-bit integers), are stored
in memory or transmitted over a network.". So, if you need manage endianness of
of values, then you can use cpu_to_bexx()/bexx_to_cpu() family of methods. But
it's not about bits. If you take byte only, then the representation of byte is
the same in Big-Endian (BE) or Little-Endian (LE). Am I right here? :)

> 
> If there's a strong preference for bitops helpers, I could investigate
> the big-endian bit helpers (*_be), but for this single-byte invariant
> check, the explicit mask seems clearest and most consistent with
> existing code.
> 
> > 
> > > +                       pr_warn("(%s): Btree 0x%x bitmap corruption
> > > detected, forcing read-only.\n",
> > 
> > I prefer to mention what do we mean by 0x%x. Currently, it looks
> > complicated to
> > follow.
> 
> Ack'ed. I am adding a lookup to print the human-readable tree name
> (Catalog, Extents, Attributes) alongside the ID.
> 
> > 
> > >  #define HFSPLUS_ATTR_TREE_NODE_SIZE            8192
> > >  #define HFSPLUS_BTREE_HDR_NODE_RECS_COUNT      3
> > > +#define HFSPLUS_BTREE_HDR_MAP_REC              2       /* Map
> > > (bitmap) record in header node */
> > 
> > Maybe, HFSPLUS_BTREE_HDR_MAP_REC_INDEX?
> 
> Ack'ed.
> 
> > 
> > >  #define HFSPLUS_BTREE_HDR_USER_BYTES           128
> > > +#define HFSPLUS_BTREE_NODE0_BIT                0x80
> > 
> > Maybe, (1 << something) instead of 0x80? I am OK with constant too.
> 
> Ack'ed, will use (1 << 7). Can also use BIT(7) if you prefer.

OK. So, are you sure that node #0 corresponds to bit #7 but not bit #0? :) I am
started to doubt that we are correct here.

Thanks,
Slava.

Re: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar]

On Tue, 2026-02-03 at 23:12 +0000, Viacheslav Dubeyko wrote:
> On Tue, 2026-02-03 at 14:28 +0530, Shardul Bankar wrote:
> > On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> > > On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> > > 
> > > > 
> > > > +       len = hfs_brec_lenoff(node,
> > > > +                       HFSPLUS_BTREE_HDR_MAP_REC,
> > > > &bitmap_off);
> > > > +
> > > > +       if (len != 0 && bitmap_off >= sizeof(struct
> > > > hfs_bnode_desc)) {
> > > 
> > > If we read incorrect len and/or bitmap_off, then it sounds like
> > > corruption too.
> > > We need to process this issue somehow but you ignore this,
> > > currently.
> > > ;)
> 
> I am not suggesting to check everything. But because you are using
> these values
> and you can detect that these value is incorrect, then it makes sense
> to report
> the error if something is wrong.

Hi Slava,

Ah, I misunderstood you previously!

You are right. I will add an explicit error log and force SB_RDONLY for
this invalid offset/length case in v4 as well.

> 
> > 
> > > > +               hfs_bnode_read(node, &bitmap_byte, bitmap_off,
> > > > 1);
> > > 
> > > I assume that 1 is the size of byte, then sizeof(u8) or
> > > sizeof(bitmap_byte)
> > > could look much better than hardcoded value.
> > 
> > Ack'ed. Changing to sizeof(bitmap_byte).
> > 
> > > 
> > > > +               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> > > 
> > > Why don't use the test_bit() [1] here? I believe that code will
> > > be
> > > more simple
> > > in such case.
> > 
> > 
> 
> Correct me if I am wrong, but I suspect that I am right. :) The
> endianness is
> about bytes not bits. I am googled this: "Endianness defines the
> order in which
> bytes, constituting multibyte data (like 16, 32, or 64-bit integers),
> are stored
> in memory or transmitted over a network.". So, if you need manage
> endianness of
> of values, then you can use cpu_to_bexx()/bexx_to_cpu() family of
> methods. But
> it's not about bits. If you take byte only, then the representation
> of byte is
> the same in Big-Endian (BE) or Little-Endian (LE). Am I right here?
> :)

You are right that a single byte's representation is the same in memory
regardless of CPU endianness. To be completely transparent, the precise
interplay between cross-architecture kernel bitops, memory alignment,
and on-disk format endianness has always been a bit of a conceptual
blind spot for me.

My initial hesitation with `test_bit()` was also that it strictly
requires an `unsigned long *`. Reading a 1-byte on-disk requirement
directly into an `unsigned long` buffer felt sub-optimal and could risk
out-of-bounds stack reads if not handled carefully.

However, I want to follow your recommendation to standardize on kernel
bitops. To safely use `test_bit()`, I propose reading the byte safely
into a `u8`, and then promoting it to an `unsigned long` before
testing; something like this on top of the patch:

diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
index 4bd0d18ac6c6..7ce1708e423c 100644
--- a/fs/hfsplus/btree.c
+++ b/fs/hfsplus/btree.c
@@ -135,6 +135,7 @@ 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;
+       unsigned long bitmap_word;
        struct hfs_bnode *node;
        u16 len, bitmap_off;
        struct inode *inode;
@@ -255,7 +256,8 @@ struct hfs_btree *hfs_btree_open(struct super_block
*sb, u32 id)
 
        if (len != 0 && bitmap_off >= sizeof(struct hfs_bnode_desc)) {
                hfs_bnode_read(node, &bitmap_byte, bitmap_off,
sizeof(bitmap_byte));
-               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
+               bitmap_word = bitmap_byte;
+               if (!test_bit(HFSPLUS_BTREE_NODE0_BIT, &bitmap_word)) {
                        const char *tree_name;
 
                        switch (id) {
diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
index f2f70975727c..52e9b8969406 100644
--- a/include/linux/hfs_common.h
+++ b/include/linux/hfs_common.h
@@ -512,7 +512,7 @@ struct hfs_btree_header_rec {
 #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_HDR_USER_BYTES           128
-#define HFSPLUS_BTREE_NODE0_BIT                0x80
+#define HFSPLUS_BTREE_NODE0_BIT                7
 
 /* btree key type */
 #define HFSPLUS_KEY_CASEFOLDING                0xCF    /* case-
insensitive */


> > > >  #define HFSPLUS_BTREE_HDR_USER_BYTES           128
> > > > +#define HFSPLUS_BTREE_NODE0_BIT                0x80
> > > 
> > > Maybe, (1 << something) instead of 0x80? I am OK with constant
> > > too.
> > 
> > Ack'ed, will use (1 << 7). Can also use BIT(7) if you prefer.
> 
> OK. So, are you sure that node #0 corresponds to bit #7 but not bit
> #0? :) I am
> started to doubt that we are correct here.

According to Apple's Tech Note TN1150 [1], HFS+ maps Node 0 to the Most
Significant Bit (MSB).

Specifically, TN1150 states:

1. Under "Map Record": "The bits are interpreted in the same way as the
bits in the allocation file."
2. Under "Allocation File": "Within each byte, the most significant bit
holds information about the allocation block with the lowest number..."

Apple also provides a C code snippet in the document (Listing 1)
demonstrating this:
`return (thisByte & (1 << (7 - (thisAllocationBlock % 8)))) != 0;`

If we evaluate this for Node 0 (`thisAllocationBlock = 0`), it resolves
to `1 << 7` (or bit 7). Therefore, checking bit 7 is mathematically
required to target Node 0.

If these changes look proper to you, I will add them to my code along
with the rest of your feedback and send out v4 shortly.

Thanks again for the guidance, continued review and for bearing with me
on this!

Regards,
Shardul

[1] Apple Tech Note TN1150:
https://developer.apple.com/library/archive/technotes/tn/tn1150.html#AllocationFile

RE: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Viacheslav Dubeyko]

On Sun, 2026-02-15 at 23:27 +0530, Shardul Bankar wrote:
> On Tue, 2026-02-03 at 23:12 +0000, Viacheslav Dubeyko wrote:
> > On Tue, 2026-02-03 at 14:28 +0530, Shardul Bankar wrote:
> > > On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> > > > On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> > > > 
> > > > > 
> > > > > +       len = hfs_brec_lenoff(node,
> > > > > +                       HFSPLUS_BTREE_HDR_MAP_REC,
> > > > > &bitmap_off);
> > > > > +
> > > > > +       if (len != 0 && bitmap_off >= sizeof(struct
> > > > > hfs_bnode_desc)) {
> > > > 
> > > > If we read incorrect len and/or bitmap_off, then it sounds like
> > > > corruption too.
> > > > We need to process this issue somehow but you ignore this,
> > > > currently.
> > > > ;)
> > 
> > I am not suggesting to check everything. But because you are using
> > these values
> > and you can detect that these value is incorrect, then it makes sense
> > to report
> > the error if something is wrong.
> 
> Hi Slava,
> 
> Ah, I misunderstood you previously!
> 
> You are right. I will add an explicit error log and force SB_RDONLY for
> this invalid offset/length case in v4 as well.
> 
> > 
> > > 
> > > > > +               hfs_bnode_read(node, &bitmap_byte, bitmap_off,
> > > > > 1);
> > > > 
> > > > I assume that 1 is the size of byte, then sizeof(u8) or
> > > > sizeof(bitmap_byte)
> > > > could look much better than hardcoded value.
> > > 
> > > Ack'ed. Changing to sizeof(bitmap_byte).
> > > 
> > > > 
> > > > > +               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> > > > 
> > > > Why don't use the test_bit() [1] here? I believe that code will
> > > > be
> > > > more simple
> > > > in such case.
> > > 
> > > 
> > 
> > Correct me if I am wrong, but I suspect that I am right. :) The
> > endianness is
> > about bytes not bits. I am googled this: "Endianness defines the
> > order in which
> > bytes, constituting multibyte data (like 16, 32, or 64-bit integers),
> > are stored
> > in memory or transmitted over a network.". So, if you need manage
> > endianness of
> > of values, then you can use cpu_to_bexx()/bexx_to_cpu() family of
> > methods. But
> > it's not about bits. If you take byte only, then the representation
> > of byte is
> > the same in Big-Endian (BE) or Little-Endian (LE). Am I right here?
> > :)
> 
> You are right that a single byte's representation is the same in memory
> regardless of CPU endianness. To be completely transparent, the precise
> interplay between cross-architecture kernel bitops, memory alignment,
> and on-disk format endianness has always been a bit of a conceptual
> blind spot for me.
> 
> My initial hesitation with `test_bit()` was also that it strictly
> requires an `unsigned long *`. Reading a 1-byte on-disk requirement
> directly into an `unsigned long` buffer felt sub-optimal and could risk
> out-of-bounds stack reads if not handled carefully.
> 
> However, I want to follow your recommendation to standardize on kernel
> bitops. To safely use `test_bit()`, I propose reading the byte safely
> into a `u8`, and then promoting it to an `unsigned long` before
> testing; something like this on top of the patch:
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index 4bd0d18ac6c6..7ce1708e423c 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -135,6 +135,7 @@ 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;
> +       unsigned long bitmap_word;
>         struct hfs_bnode *node;
>         u16 len, bitmap_off;
>         struct inode *inode;
> @@ -255,7 +256,8 @@ struct hfs_btree *hfs_btree_open(struct super_block
> *sb, u32 id)
>  
>         if (len != 0 && bitmap_off >= sizeof(struct hfs_bnode_desc)) {
>                 hfs_bnode_read(node, &bitmap_byte, bitmap_off,
> sizeof(bitmap_byte));
> -               if (!(bitmap_byte & HFSPLUS_BTREE_NODE0_BIT)) {
> +               bitmap_word = bitmap_byte;
> +               if (!test_bit(HFSPLUS_BTREE_NODE0_BIT, &bitmap_word)) {
>                         const char *tree_name;

OK. I see. I don't think that we need to make the code more complicated. If we
can simply read the bitmap_word and then to use it for test_bit(), then it
sounds like reasonable one. Otherwise, I think we don't need to use some tricks
to use the test_bit(). Your previous code looks OK, then.

But, I started to think about something bigger. Please, see my comments below.

>  
>                         switch (id) {
> diff --git a/include/linux/hfs_common.h b/include/linux/hfs_common.h
> index f2f70975727c..52e9b8969406 100644
> --- a/include/linux/hfs_common.h
> +++ b/include/linux/hfs_common.h
> @@ -512,7 +512,7 @@ struct hfs_btree_header_rec {
>  #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_HDR_USER_BYTES           128
> -#define HFSPLUS_BTREE_NODE0_BIT                0x80
> +#define HFSPLUS_BTREE_NODE0_BIT                7
>  
>  /* btree key type */
>  #define HFSPLUS_KEY_CASEFOLDING                0xCF    /* case-
> insensitive */
> 
> 
> > > > >  #define HFSPLUS_BTREE_HDR_USER_BYTES           128
> > > > > +#define HFSPLUS_BTREE_NODE0_BIT                0x80
> > > > 
> > > > Maybe, (1 << something) instead of 0x80? I am OK with constant
> > > > too.
> > > 
> > > Ack'ed, will use (1 << 7). Can also use BIT(7) if you prefer.
> > 
> > OK. So, are you sure that node #0 corresponds to bit #7 but not bit
> > #0? :) I am
> > started to doubt that we are correct here.
> 
> According to Apple's Tech Note TN1150 [1], HFS+ maps Node 0 to the Most
> Significant Bit (MSB).
> 
> Specifically, TN1150 states:
> 
> 1. Under "Map Record": "The bits are interpreted in the same way as the
> bits in the allocation file."
> 2. Under "Allocation File": "Within each byte, the most significant bit
> holds information about the allocation block with the lowest number..."
> 
> Apple also provides a C code snippet in the document (Listing 1)
> demonstrating this:
> `return (thisByte & (1 << (7 - (thisAllocationBlock % 8)))) != 0;`
> 
> If we evaluate this for Node 0 (`thisAllocationBlock = 0`), it resolves
> to `1 << 7` (or bit 7). Therefore, checking bit 7 is mathematically
> required to target Node 0.
> 
> If these changes look proper to you, I will add them to my code along
> with the rest of your feedback and send out v4 shortly.
> 
> Thanks again for the guidance, continued review and for bearing with me
> on this!
> 

As far as I can see, the main logic of working with b-tree map is here [1].
Ideally, we should have a generic method that can be used as here [1] as in your
patch. Could you try to make the search in b-tree map by generic method?

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/btree.c#L412

Re: [PATCH v3] hfsplus: validate btree bitmap during mount and handle corruption gracefully

[Shardul Bankar]

On Tue, 2026-02-17 at 00:41 +0000, Viacheslav Dubeyko wrote:
> On Sun, 2026-02-15 at 23:27 +0530, Shardul Bankar wrote:
> > On Tue, 2026-02-03 at 23:12 +0000, Viacheslav Dubeyko wrote:
> > > On Tue, 2026-02-03 at 14:28 +0530, Shardul Bankar wrote:
> > > > On Mon, 2026-02-02 at 20:52 +0000, Viacheslav Dubeyko wrote:
> > > > > On Sat, 2026-01-31 at 19:34 +0530, Shardul Bankar wrote:
> As far as I can see, the main logic of working with b-tree map is
> here [1].
> Ideally, we should have a generic method that can be used as here [1]
> as in your
> patch. Could you try to make the search in b-tree map by generic
> method?
> 
> Thanks,
> Slava.
> 
> [1]
> https://elixir.bootlin.com/linux/v6.19-rc5/source/fs/hfsplus/btree.c#L412

Hi Slava,

I’ve posted the updated v4 series addressing this feedback in a new
thread.

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

Thanks,
Shardul