[PATCH v2] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[PATCH v2] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[Song jeonghak]

hfs_bnode_find() flags a node it fails to load with HFS_BNODE_ERROR and
then drops its reference with hfs_bnode_put(). When the last reference
goes away, hfs_bnode_put() only releases nodes flagged HFS_BNODE_DELETED.
Every other node stays hashed in tree->node_hash as a cache entry and is
freed later by hfs_btree_close().

That is fine while the tree is open, but hfs_btree_open() looks up the
header node itself. If that lookup fails, hfs_btree_open() unwinds with
iput() and kfree(tree) and never calls hfs_btree_close(). The failed node
is still hashed, nothing references it anymore, and it is leaked. syzbot
triggers this by mounting a corrupted image:

  hfsplus: failed to load extents file
  BUG: memory leak
  unreferenced object 0xffff8881040930c0 (size 96):
    __hfs_bnode_create+0x57/0x310 fs/hfsplus/bnode.c:476
    hfsplus_bnode_find+0x138/0x660 fs/hfsplus/bnode.c:554
    hfsplus_btree_open+0x30b/0x5d0 fs/hfsplus/btree.c:384
    hfsplus_fill_super+0x272/0x880 fs/hfsplus/super.c:546

Free a node that is flagged HFS_BNODE_ERROR when its last reference is
dropped instead of leaving it hashed. This follows the existing
HFS_BNODE_DELETED handling: unhash the node under hash_lock, drop the
lock, then free it. Unlike a deleted node, a node that failed to load is
still part of the on-disk tree; only its in-memory copy is dropped. Its
bit in the allocation map therefore stays set and its on-disk contents
are left alone, so neither hfs_bmap_free() nor hfs_bnode_clear() is
called for it. A later hfs_bnode_find() of the same node number now
reads the node from disk again instead of getting the cached failure.

Tested with the syzbot C reproducer in QEMU (x86_64, kmemleak) on
next-20261023: the leak was reported in 3 of 3 runs before the change and
in 0 of 3 runs after it, two of which ran with slub_debug=FZPU. On a
valid image, creating 3000 files, deleting half of them, and remounting
gives identical results before and after, and fsck.hfsplus reports the
volume as OK. checkpatch.pl --strict and sparse report nothing for
bnode.c. This was not tested with KASAN or with concurrent access to the
same node.

Claude (Anthropic) was used to help with the root cause analysis, the
reproduction setup and drafting this changelog. The code change itself
was written by the submitter.

Fixes: 8ad2c6a36ac4 ("hfsplus: validate b-tree node 0 bitmap at mount time")
Reported-by: syzbot+98547b0428b6a6a3467c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=98547b0428b6a6a3467c
Signed-off-by: Song jeonghak <thdwjdgkr123@gmail.com>
---
Changes in v2:
- Corrected the changelog. v1 said a failed node "has no b-tree map bit
  to release and no contents to zero out". That is wrong: the node is
  still part of the on-disk tree, so its map bit is set and its contents
  are on disk. They are left alone on purpose. No code change.
- Kept HFS_BNODE_ERROR as its own branch rather than folding it into
  the HFS_BNODE_DELETED one as Markus Elfring suggested, so the fix does
  not touch the existing HFS_BNODE_DELETED path. I can merge them if
  that is preferred.
- fs/hfsplus is unchanged between next-20261023 and next-20261102 and
  the patch applies as is, so the test results above still hold.

v1: https://lore.kernel.org/all/20261026152139.458677-1-thdwjdgkr123@gmail.com/

Viacheslav, v1 did not show up in your mail client. It was sent with
git send-email as plain text and is on lore at the link above. This
version is sent the same way.

This takes the direction Viacheslav suggested while reviewing Edward
Adam Davis's earlier attempt, "[PATCH] hfsplus: Supports freeing newly
created tree head" (April 2026): look at HFS_BNODE_ERROR in
hfs_bnode_put() rather than special-casing the header node. That thread
did not get a follow-up version.
Link: https://lore.kernel.org/all/tencent_C4F16CD096B0EAEAEE5ACDB07A266E382C0A@qq.com/

fs/hfs has the same structure: hfs_bnode_put() has no HFS_BNODE_ERROR
handling, and hfs_btree_open() looks up the header node and then frees
the tree without hfs_btree_close() on failure. I have not reproduced a
leak there, so this patch leaves fs/hfs alone.

sparse reports a few warnings elsewhere in fs/hfsplus (ioctl.c and
attributes.c) that are not touched by this patch.

 fs/hfsplus/bnode.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
index 3a1a1fa6f2e2..3d159db229e5 100644
--- a/fs/hfsplus/bnode.c
+++ b/fs/hfsplus/bnode.c
@@ -702,6 +702,14 @@ void hfs_bnode_put(struct hfs_bnode *node)
 			hfs_bnode_free(node);
 			return;
 		}
+
+		if (test_bit(HFS_BNODE_ERROR, &node->flags)) {
+			hfs_bnode_unhash(node);
+			spin_unlock(&tree->hash_lock);
+			hfs_bnode_free(node);
+			return;
+		}
+
 		spin_unlock(&tree->hash_lock);
 	}
 }
-- 
2.43.0

Re: [PATCH v2] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[Markus Elfring]

…
> +++ b/fs/hfsplus/bnode.c
> @@ -702,6 +702,14 @@ void hfs_bnode_put(struct hfs_bnode *node)
>  			hfs_bnode_free(node);
>  			return;
>  		}
> +
> +		if (test_bit(HFS_BNODE_ERROR, &node->flags)) {
> +			hfs_bnode_unhash(node);
> +			spin_unlock(&tree->hash_lock);
> +			hfs_bnode_free(node);
> +			return;
> +		}
> +
>  		spin_unlock(&tree->hash_lock);
>  	}
>  }

Would you dare to use an additional label in such a function implementation?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.4-rc1#n526
https://elixir.bootlin.com/linux/v7.3/source/fs/hfs/bnode.c#L513-L542

Regards,
Markus

Re: [PATCH v2] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[Viacheslav Dubeyko]

On Tue, 2026-11-03 at 11:25 +0900, Song jeonghak wrote:
> hfs_bnode_find() flags a node it fails to load with HFS_BNODE_ERROR
> and
> then drops its reference with hfs_bnode_put(). When the last
> reference
> goes away, hfs_bnode_put() only releases nodes flagged
> HFS_BNODE_DELETED.
> Every other node stays hashed in tree->node_hash as a cache entry and
> is
> freed later by hfs_btree_close().
> 
> That is fine while the tree is open, but hfs_btree_open() looks up
> the
> header node itself. If that lookup fails, hfs_btree_open() unwinds
> with
> iput() and kfree(tree) and never calls hfs_btree_close(). The failed
> node
> is still hashed, nothing references it anymore, and it is leaked.
> syzbot
> triggers this by mounting a corrupted image:
> 
>   hfsplus: failed to load extents file
>   BUG: memory leak
>   unreferenced object 0xffff8881040930c0 (size 96):
>     __hfs_bnode_create+0x57/0x310 fs/hfsplus/bnode.c:476
>     hfsplus_bnode_find+0x138/0x660 fs/hfsplus/bnode.c:554
>     hfsplus_btree_open+0x30b/0x5d0 fs/hfsplus/btree.c:384
>     hfsplus_fill_super+0x272/0x880 fs/hfsplus/super.c:546
> 
> Free a node that is flagged HFS_BNODE_ERROR when its last reference
> is
> dropped instead of leaving it hashed. This follows the existing
> HFS_BNODE_DELETED handling: unhash the node under hash_lock, drop the
> lock, then free it. Unlike a deleted node, a node that failed to load
> is
> still part of the on-disk tree; only its in-memory copy is dropped.
> Its
> bit in the allocation map therefore stays set and its on-disk
> contents
> are left alone, so neither hfs_bmap_free() nor hfs_bnode_clear() is
> called for it. A later hfs_bnode_find() of the same node number now
> reads the node from disk again instead of getting the cached failure.
> 
> Tested with the syzbot C reproducer in QEMU (x86_64, kmemleak) on
> next-20261023: the leak was reported in 3 of 3 runs before the change
> and
> in 0 of 3 runs after it, two of which ran with slub_debug=FZPU. On a
> valid image, creating 3000 files, deleting half of them, and
> remounting
> gives identical results before and after, and fsck.hfsplus reports
> the
> volume as OK. checkpatch.pl --strict and sparse report nothing for
> bnode.c. This was not tested with KASAN or with concurrent access to
> the
> same node.
> 
> Claude (Anthropic) was used to help with the root cause analysis, the
> reproduction setup and drafting this changelog. The code change
> itself
> was written by the submitter.
> 
> Fixes: 8ad2c6a36ac4 ("hfsplus: validate b-tree node 0 bitmap at mount
> time")
> Reported-by: syzbot+98547b0428b6a6a3467c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=98547b0428b6a6a3467c
> Signed-off-by: Song jeonghak <thdwjdgkr123@gmail.com>
> ---
> Changes in v2:
> - Corrected the changelog. v1 said a failed node "has no b-tree map
> bit
>   to release and no contents to zero out". That is wrong: the node is
>   still part of the on-disk tree, so its map bit is set and its
> contents
>   are on disk. They are left alone on purpose. No code change.
> - Kept HFS_BNODE_ERROR as its own branch rather than folding it into
>   the HFS_BNODE_DELETED one as Markus Elfring suggested, so the fix
> does
>   not touch the existing HFS_BNODE_DELETED path. I can merge them if
>   that is preferred.
> - fs/hfsplus is unchanged between next-20261023 and next-20261102 and
>   the patch applies as is, so the test results above still hold.
> 
> v1:
> https://lore.kernel.org/all/20261026152139.458677-1-thdwjdgkr123@gmail.com/
> 
> Viacheslav, v1 did not show up in your mail client. It was sent with
> git send-email as plain text and is on lore at the link above. This
> version is sent the same way.
> 
> This takes the direction Viacheslav suggested while reviewing Edward
> Adam Davis's earlier attempt, "[PATCH] hfsplus: Supports freeing
> newly
> created tree head" (April 2026): look at HFS_BNODE_ERROR in
> hfs_bnode_put() rather than special-casing the header node. That
> thread
> did not get a follow-up version.
> Link:
> https://lore.kernel.org/all/tencent_C4F16CD096B0EAEAEE5ACDB07A266E382C0A@qq.com/
> 
> fs/hfs has the same structure: hfs_bnode_put() has no HFS_BNODE_ERROR
> handling, and hfs_btree_open() looks up the header node and then
> frees
> the tree without hfs_btree_close() on failure. I have not reproduced
> a
> leak there, so this patch leaves fs/hfs alone.
> 
> sparse reports a few warnings elsewhere in fs/hfsplus (ioctl.c and
> attributes.c) that are not touched by this patch.
> 
>  fs/hfsplus/bnode.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 3a1a1fa6f2e2..3d159db229e5 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -702,6 +702,14 @@ void hfs_bnode_put(struct hfs_bnode *node)
>  			hfs_bnode_free(node);
>  			return;
>  		}
> +
> +		if (test_bit(HFS_BNODE_ERROR, &node->flags)) {
> +			hfs_bnode_unhash(node);
> +			spin_unlock(&tree->hash_lock);
> +			hfs_bnode_free(node);
> +			return;
> +		}
> +
>  		spin_unlock(&tree->hash_lock);
>  	}
>  }

I've accepted this patchset [1] recently with some modifications on my
side. I assume that it has fixed the issue already. Am I wrong?

Thanks,
Slava.

[1]
https://lore.kernel.org/linux-fsdevel/17c099490060e40254ef25dacdad713d5356c2b7.camel@dubeyko.com/

Re: [PATCH v2] hfsplus: free failed b-tree nodes in hfs_bnode_put()

[Song jeonghak]

Hi Slava,

> I've accepted this patchset [1] recently with some modifications on my
> side. I assume that it has fixed the issue already. Am I wrong?

You are right, it fixes the issue. I missed that patchset: it was not
sent to syzbot, so it is not listed on the syzbot page, and that was
where I looked for earlier fixes. Sorry for the noise.

To be sure, I applied ed8b389cf5e1 ("hfsplus: fix B-tree node leak on
hfs_btree_open() error path") from your for-next branch on top of
next-20261023 and ran the syzbot C reproducer in QEMU (x86_64, kmemleak):

  next-20261023:               leak reported in 1 of 1 runs
  next-20261023 + ed8b389cf5e1: no leak in 3 of 3 runs, two of which
                               ran with slub_debug=FZPU

Please drop my patch.

That commit has no Reported-by or Closes tag, so syzbot will keep this
report open. If you can still amend it, could you add these?

  Reported-by: syzbot+98547b0428b6a6a3467c@syzkaller.appspotmail.com
  Closes: https://syzkaller.appspot.com/bug?extid=98547b0428b6a6a3467c

Otherwise I can send a "#syz fix:" command once the commit is in
linux-next.

Tested-by: Song jeonghak <thdwjdgkr123@gmail.com>

Regards,
Song jeonghak