When hfs_bnode_create() finds an existing node in the hash table, it
returns the node without incrementing its reference count. This causes
the reference count to become inconsistent, leading to a kernel panic
when hfs_bnode_put() is later called with refcnt=0:

    BUG_ON(!atomic_read(&node->refcnt))

This occurs because hfs_bmap_alloc() calls hfs_bnode_create() expecting
to receive a node with a proper reference count, but if the node is
already in the hash table, it is returned without the required refcnt
increment.

Fix this by calling hfs_bnode_get() when returning an existing node,
ensuring the reference count is properly incremented. This follows the
same pattern as the fix in __hfs_bnode_create() (commit 152af1142878
 ("hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create")).

Note: While finding an existing node in hfs_bnode_create() is unexpected
(indicated by the pr_crit warning), we still need proper refcnt management
to prevent crashes. The warning will still fire to alert about the
underlying issue (e.g., bitmap corruption or logic error causing an
existing node to be requested for allocation).

Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa
Fixes: 634725a92938 ("[PATCH] hfs: cleanup HFS+ prints")
Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
---
 fs/hfsplus/bnode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 191661af9677..e098e05add43 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -629,6 +629,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
 	if (node) {
 		pr_crit("new node %u already hashed?\n", num);
 		WARN_ON(1);
+		hfs_bnode_get(node);
 		return node;
 	}
 	node = __hfs_bnode_create(tree, num);
-- 
2.34.1

On Sun, 2025-12-14 at 05:02 +0530, Shardul Bankar wrote:
> When hfs_bnode_create() finds an existing node in the hash table, it
> returns the node without incrementing its reference count. This causes
> the reference count to become inconsistent, leading to a kernel panic
> when hfs_bnode_put() is later called with refcnt=0:
> 
>     BUG_ON(!atomic_read(&node->refcnt))
> 
> This occurs because hfs_bmap_alloc() calls hfs_bnode_create() expecting
> to receive a node with a proper reference count, but if the node is
> already in the hash table, it is returned without the required refcnt
> increment.
> 
> Fix this by calling hfs_bnode_get() when returning an existing node,
> ensuring the reference count is properly incremented. This follows the
> same pattern as the fix in __hfs_bnode_create() (commit 152af1142878
>  ("hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create")).
> 
> Note: While finding an existing node in hfs_bnode_create() is unexpected
> (indicated by the pr_crit warning), we still need proper refcnt management
> to prevent crashes. The warning will still fire to alert about the
> underlying issue (e.g., bitmap corruption or logic error causing an
> existing node to be requested for allocation).
> 
> Link: https://syzkaller.appspot.com/bug?extid=1c8ff72d0cd8a50dfeaa  
> Fixes: 634725a92938 ("[PATCH] hfs: cleanup HFS+ prints")
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/bnode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 191661af9677..e098e05add43 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -629,6 +629,7 @@ struct hfs_bnode *hfs_bnode_create(struct hfs_btree *tree, u32 num)
>  	if (node) {
>  		pr_crit("new node %u already hashed?\n", num);
>  		WARN_ON(1);
> +		hfs_bnode_get(node);

Frankly speaking, I don't see the fix here. You are trying to hide the issue but
not fix it. This is situation of the wrong call because we sharing error message
and call WARN_ON() here. And the critical question here: why do we call
hfs_bnode_create() for already created node? Is it issue of tree->node_hash? Or
something wrong with logic that calls the hfs_bnode_create()? You don't suggest
answer to this question(s). I've tried to debug likewise issue two months ago
and I don't know the answer yet. So, you need to dive deeper in the issue or,
please, convince that I am wrong here. Currently, your commit message doesn't
convince me at all.

Thanks,
Slava.

>  		return node;
>  	}
>  	node = __hfs_bnode_create(tree, num);

On Mon, 2025-12-15 at 19:29 +0000, Viacheslav Dubeyko wrote:
> Frankly speaking, I don't see the fix here. You are trying to hide
> the issue but
> not fix it. This is situation of the wrong call because we sharing
> error message
> and call WARN_ON() here. And the critical question here: why do we
> call
> hfs_bnode_create() for already created node? Is it issue of tree-
> >node_hash? Or
> something wrong with logic that calls the hfs_bnode_create()? You
> don't suggest
> answer to this question(s). I've tried to debug likewise issue two
> months ago
> and I don't know the answer yet. So, you need to dive deeper in the
> issue or,
> please, convince that I am wrong here. Currently, your commit message
> doesn't
> convince me at all.
> 

Hi Slava,

Thank you for the review. You are absolutely correct- the panic is a
symptom of a deeper logic error where the allocator attempts to re-
allocate an existing node.

I have investigated the root cause using the Syzkaller reproducer and
analyzed the crash logs. I found two distinct issues that need to be
addressed, which I plan to submit as a v2 patch series.

1. The Root Cause: Corrupted Allocation Bitmap (Allocator Logic Error):
The Syzkaller-generated image has a corrupted allocation bitmap where
Node 0 (the Header Node) is marked as "Free" (0).

    Mechanism: hfs_bmap_alloc trusts the on-disk bitmap, sees bit 0 is
clear, and attempts to allocate Node 0.

    Conflict: It calls hfs_bnode_create(tree, 0). Since Node 0 is the
header, it is already in the hash table. hfs_bnode_create correctly
detects this and warns:
[41767.838946] hfsplus: new node 0 already hashed?
[41767.839097] WARNING: fs/hfsplus/bnode.c:631 at
hfsplus_bnode_create.cold+0x41/0x49

Proposed Fix (Patch 1): Modify hfs_bmap_alloc to explicitly guard
against allocating Node 0. Node 0 is the B-Tree header and is
structurally reserved; it should never be allocated as a record node,
regardless of what the bitmap claims.

2. The Crash: Unsafe Error Handling (Refcount Violation) Even though
the allocator shouldn't request Node 0, hfs_bnode_create currently
handles the "node exists" case unsafely.

    Mechanism: When it finds the existing node (the header), it prints
the warning but returns the pointer without incrementing the reference
count.

    Result: The caller receives a node pointer, uses it, and eventually
calls hfs_bnode_put. Since the refcount wasn't incremented, this leads
to a refcount underflow/panic.

    Evidence: The panic occurs later in the execution flow (e.g.,
inside hfs_bnode_split or hfsplus_create_cat), proving the system is
running with a "ticking time bomb" node pointer.
[41767.840709] kernel BUG at fs/hfsplus/bnode.c:676!
[41767.840751] RIP: 0010:hfsplus_bnode_put+0x4a0/0x5c0
[41767.840826] Call Trace:
[41767.840833]  hfs_btree_inc_height.isra.0+0x64e/0x8b0
[41767.840878]  hfsplus_brec_insert+0x97b/0xcf0

Proposed Fix (Patch 2): I still believe we should add
hfs_bnode_get(node) in hfs_bnode_create (the original patch). Even if
the allocator is fixed, hfs_bnode_create should be robust. If it
returns a valid pointer, it must guarantee that pointer has a valid
reference reference to prevent UAF/panics, consistent with the fix in
__hfs_bnode_create (commit 152af1142878).

Plan for v2:

    Patch 1: hfsplus: prevent allocation of header node (node 0) (Fixes
the logic error)

    Patch 2: hfsplus: fix missing hfs_bnode_get() in hfs_bnode_create()
(Fixes the crash safety)

Does this approach and analysis address your concerns?

Thanks, Shardul