Re: [PATCH] hfsplus: fix missing hfs_bnode_get() in __hfs_bnode_create

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Fri, 2025-08-29 at 17:39 +0800, Chenzhi Yang wrote:
> From: Yang Chenzhi <yang.chenzhi@vivo.com>
> 
> When sync() and link() are called concurrently, both threads may
> enter hfs_bnode_find() without finding the node in the hash table
> and proceed to create it.
> 

It's clear why link() could try to create a b-tree node. But it's not completely
clear why sync() should try to create a b-tree node? Usually, sync() should try
to flush already existed inodes. Especially, if we imply any system inodes that
are created mostly during mount operation. How is it possible that sync() could
try to create the node? Moreover, node_id 0 (root node) should be already
created if we created the b-tree.

I am investigating right now the issue when root node is re-written by another
node during inserting the record in HFS (but I assume that HFS+ could have the
same issue). So, I think your fix is good but we have some more serious issue in
the background.

So, if you are fixing the syzbot reported issue, then it could be good to have
call trace in the comment to understand the fix environment. And it could be
more clear in what environment sync() is trying to create the b-tree node. I
like your an analysis of the issue but not all details are shared. And I still
have questions how sync() could try to create the b-tree's node. Probably, this
fix could be justified by concurrent link() requests or something like this. But
concurrent creation of b-tree node by sync() and link() looks really suspicious.

Thanks,
Slava.

> Thread A:
>   hfsplus_write_inode()
>     -> hfsplus_write_system_inode()
>       -> hfs_btree_write()
>         -> hfs_bnode_find(tree, 0)
>           -> __hfs_bnode_create(tree, 0)
> 
> Thread B:
>   hfsplus_create_cat()
>     -> hfs_brec_insert()
>       -> hfs_bnode_split()
>         -> hfs_bmap_alloc()
>           -> hfs_bnode_find(tree, 0)
>             -> __hfs_bnode_create(tree, 0)
> 
> In this case, thread A creates the bnode, sets refcnt=1, and hashes it.
> Thread B also tries to create the same bnode, notices it has already
> been inserted, drops its own instance, and uses the hashed one without
> getting the node.
> 
> ```
> 
> 	node2 = hfs_bnode_findhash(tree, cnid);
> 	if (!node2) {                                 <- Thread A
> 		hash = hfs_bnode_hash(cnid);
> 		node->next_hash = tree->node_hash[hash];
> 		tree->node_hash[hash] = node;
> 		tree->node_hash_cnt++;
> 	} else {                                      <- Thread B
> 		spin_unlock(&tree->hash_lock);
> 		kfree(node);
> 		wait_event(node2->lock_wq,
> 			!test_bit(HFS_BNODE_NEW, &node2->flags));
> 		return node2;
> 	}
> ```
> 
> However, hfs_bnode_find() requires each call to take a reference.
> Here both threads end up setting refcnt=1. When they later put the node,
> this triggers:
> 
> BUG_ON(!atomic_read(&node->refcnt))
> 
> In this scenario, Thread B in fact finds the node in the hash table
> rather than creating a new one, and thus must take a reference.
> 
> Fix this by calling hfs_bnode_get() when reusing a bnode newly created by
> another thread to ensure the refcount is updated correctly.
> 
> A similar bug was fixed in HFS long ago in commit
> a9dc087fd3c4 ("fix missing hfs_bnode_get() in __hfs_bnode_create")
> but the same issue remained in HFS+ until now.
> 
> Reported-by: syzbot+005d2a9ecd9fbf525f6a@syzkaller.appspotmail.com
> Signed-off-by: Yang Chenzhi <yang.chenzhi@vivo.com>
> ---
>  fs/hfsplus/bnode.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/hfsplus/bnode.c b/fs/hfsplus/bnode.c
> index 14f4995588ff..e774bd4f40c3 100644
> --- a/fs/hfsplus/bnode.c
> +++ b/fs/hfsplus/bnode.c
> @@ -522,6 +522,7 @@ static struct hfs_bnode *__hfs_bnode_create(struct hfs_btree *tree, u32 cnid)
>  		tree->node_hash[hash] = node;
>  		tree->node_hash_cnt++;
>  	} else {
> +		hfs_bnode_get(node2);
>  		spin_unlock(&tree->hash_lock);
>  		kfree(node);
>  		wait_event(node2->lock_wq,