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

[PATCH] 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. A failed node was never handed out for use, so unlike
a deleted node it has no b-tree map bit to release and no contents to
zero out. 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>
---
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] 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;
> +		}
…

How do you think about to avoid a bit of duplicate source code?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.3#n526

Regards,
Markus

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

[Song jeonghak]

Hi Markus,

(Resending as plain text; my previous reply was rejected by the lists
because it contained an HTML part. Sorry for the duplicate.)

> How do you think about to avoid a bit of duplicate source code?

Thanks for looking at the patch. The two branches share the
unhash/unlock/free steps, but they handle different situations:

 - HFS_BNODE_DELETED is only set by hfs_bnode_unlink(), when the node
   is removed from the tree. Its slot becomes unused, so
   hfs_bnode_put() also clears its bit in the allocation map
   (hfs_bmap_free()) and, if HFSPLUS_VOL_UNUSED_NODE_FIX is set,
   zeroes it (hfs_bnode_clear()).

 - HFS_BNODE_ERROR means the node failed to load. It is still part of
   the on-disk tree; only the in-memory copy is dropped. Its map bit
   must stay set, otherwise the allocator could hand out a node that
   is still linked, and its on-disk contents should not be
   overwritten. In my reproduction the failing node is the header
   node (node 0), which must always stay allocated. From reading the
   code, hfs_bmap_free() would hit its BUG_ON(!node->this) for it;
   I have not run that.

I looked at how other put functions that take the lock with
atomic_dec_and_lock()/refcount_dec_and_lock() deal with this, and
both forms are used. ceph_put_snap_realm() in fs/ceph/snap.c repeats
the unlock in each branch, while fsnotify_put_mark() in
fs/notify/mark.c records the difference in a flag and does the common
part once. Following the second style, the two branches could become:

		bool deleted = test_bit(HFS_BNODE_DELETED, &node->flags);

		if (deleted || test_bit(HFS_BNODE_ERROR, &node->flags)) {
			hfs_bnode_unhash(node);
			spin_unlock(&tree->hash_lock);
			if (deleted) {
				if (hfs_bnode_need_zeroout(tree))
					hfs_bnode_clear(node, 0, tree->node_size);
				hfs_bmap_free(node);
			}
			hfs_bnode_free(node);
			return;
		}
		spin_unlock(&tree->hash_lock);

I am fine with either form and will use whichever the maintainers
prefer in v2.

While looking into this I found a mistake in my changelog. It says a
failed node "has no b-tree map bit to release and no contents to zero
out". That is wrong: the bit is set and the contents are on disk. They
are left alone on purpose, as described above. I will correct this in
v2.

Regards,
Song jeonghak

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

[Viacheslav Dubeyko]

On Mon, 2026-11-02 at 01:29 +0900, Song jeonghak wrote:
> Hi Markus,
> 
> (Resending as plain text; my previous reply was rejected by the lists
> because it contained an HTML part. Sorry for the duplicate.)
> 
> > How do you think about to avoid a bit of duplicate source code?
> 
> Thanks for looking at the patch. The two branches share the
> unhash/unlock/free steps, but they handle different situations:
> 
>  - HFS_BNODE_DELETED is only set by hfs_bnode_unlink(), when the node
>    is removed from the tree. Its slot becomes unused, so
>    hfs_bnode_put() also clears its bit in the allocation map
>    (hfs_bmap_free()) and, if HFSPLUS_VOL_UNUSED_NODE_FIX is set,
>    zeroes it (hfs_bnode_clear()).
> 
>  - HFS_BNODE_ERROR means the node failed to load. It is still part of
>    the on-disk tree; only the in-memory copy is dropped. Its map bit
>    must stay set, otherwise the allocator could hand out a node that
>    is still linked, and its on-disk contents should not be
>    overwritten. In my reproduction the failing node is the header
>    node (node 0), which must always stay allocated. From reading the
>    code, hfs_bmap_free() would hit its BUG_ON(!node->this) for it;
>    I have not run that.
> 
> I looked at how other put functions that take the lock with
> atomic_dec_and_lock()/refcount_dec_and_lock() deal with this, and
> both forms are used. ceph_put_snap_realm() in fs/ceph/snap.c repeats
> the unlock in each branch, while fsnotify_put_mark() in
> fs/notify/mark.c records the difference in a flag and does the common
> part once. Following the second style, the two branches could become:
> 
> 		bool deleted = test_bit(HFS_BNODE_DELETED, &node-
> >flags);
> 
> 		if (deleted || test_bit(HFS_BNODE_ERROR, &node-
> >flags)) {
> 			hfs_bnode_unhash(node);
> 			spin_unlock(&tree->hash_lock);
> 			if (deleted) {
> 				if (hfs_bnode_need_zeroout(tree))
> 					hfs_bnode_clear(node, 0,
> tree->node_size);
> 				hfs_bmap_free(node);
> 			}
> 			hfs_bnode_free(node);
> 			return;
> 		}
> 		spin_unlock(&tree->hash_lock);
> 
> I am fine with either form and will use whichever the maintainers
> prefer in v2.
> 
> While looking into this I found a mistake in my changelog. It says a
> failed node "has no b-tree map bit to release and no contents to zero
> out". That is wrong: the bit is set and the contents are on disk.
> They
> are left alone on purpose, as described above. I will correct this in
> v2.
> 
> 

Somehow I cannot find the patch in my email client. Please, double
check that you send the patch in plain text and, please, ping me if I
don't answer on v2 of the patch.

Thanks,
Slava.