Re: [PATCH v3 1/2] hfsplus: free cached B-tree nodes on hfs_btree_open() error path

[Viacheslav Dubeyko]

On Thu, 2026-10-01 at 23:38 +0200, Mahmut Emin Kurhan wrote:
> hfs_btree_open() can fail after hfs_bnode_find(tree,
> HFSPLUS_TREE_HEAD) has
> already inserted the head node into tree->node_hash.
> 
> __hfs_bnode_create() inserts the new bnode into tree->node_hash
> before it
> reads the node pages; if a page read fails it sets HFS_BNODE_ERROR
> and
> returns the node still hashed. hfs_bnode_find() then takes its
> node_error
> path and calls hfs_bnode_put(), which only frees a node once its
> refcount
> reaches zero *and* HFS_BNODE_DELETED is set. For the errored head
> node that
> flag is not set, so it stays in tree->node_hash with a zero refcount.
> 
> hfs_btree_open() then sees IS_ERR(node) and jumps to free_tree:,
> doing a
> bare kfree(tree). Only hfs_btree_close() walks tree->node_hash[] and
> frees
> the cached nodes, so the head node is leaked. Mounting a crafted HFS+
> image
> whose head B-tree node fails to read leaks kernel memory on every
> attempt.
> 
> Factor the node-hash freeing out of hfs_btree_close() into a small
> helper
> hfs_bnode_hash_free() and call it from both hfs_btree_close() and the
> hfs_btree_open() error path. The helper keeps the hash_lock
> serialization
> added in commit dce0e0248205 ("hfs/hfsplus: serialize B-tree close

I think it is not good idea of mentioning this. This patch is not
upstream yet.

> against
> folio release"); on the open error path the tree has not been
> published yet,
> so the lock is uncontended but harmless.
> 
> Found via coverage-guided fuzzing (syzkaller + kmemleak) by Noroxi.

Should we use Reported-by: tag?

> 
> Signed-off-by: Mahmut Emin Kurhan <guvenlik@xxxxxxxxxx>
> ---
>  fs/hfsplus/btree.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/hfsplus/btree.c b/fs/hfsplus/btree.c
> index bd4dbdbc8f..2183828d5d 100644
> --- a/fs/hfsplus/btree.c
> +++ b/fs/hfsplus/btree.c
> @@ -265,6 +265,27 @@ static const char *hfs_btree_name(u32 cnid)
>  }
>  
>  /* Get a reference to a B*Tree and do some initial checks */

Are you sure that it is correct place for the comment?

> +static void hfs_bnode_hash_free(struct hfs_btree *tree)

Maybe, hfs_btree_free_nodes() because function lives in btree.c?

> +{
> +	struct hfs_bnode *node;
> +	int i;
> +
> +	for (i = 0; i < NODE_HASH_SIZE; i++) {
> +		spin_lock(&tree->hash_lock);
> +		while ((node = tree->node_hash[i])) {
> +			hfs_bnode_unhash(node);
> +			spin_unlock(&tree->hash_lock);
> +			if (atomic_read(&node->refcnt))
> +				pr_crit("node %d:%d still has %d
> user(s)!\n",
> +					node->tree->cnid, node-
> >this,
> +					atomic_read(&node->refcnt));
> +			hfs_bnode_free(node);
> +			spin_lock(&tree->hash_lock);
> +		}
> +		spin_unlock(&tree->hash_lock);
> +	}
> +}
> +
>  struct hfs_btree *hfs_btree_open(struct super_block *sb, u32 id)
>  {
>  	struct hfs_btree *tree;
> @@ -403,6 +424,7 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  	tree->inode->i_mapping->a_ops = &hfsplus_aops;
>  	iput(tree->inode);
>   free_tree:
> +	hfs_bnode_hash_free(tree);

We have inconsistency here. The hfs_btree_close() calls
hfs_bnode_hash_free() and iput() then. Here we have opposite sequence.

Thanks,
Slava.

>  	kfree(tree);
>  	return NULL;
>  }
> @@ -410,27 +432,10 @@ struct hfs_btree *hfs_btree_open(struct
> super_block *sb, u32 id)
>  /* Release resources used by a btree */
>  void hfs_btree_close(struct hfs_btree *tree)
>  {
> -	struct hfs_bnode *node;
> -	int i;
> -
>  	if (!tree)
>  		return;
>  
> -	for (i = 0; i < NODE_HASH_SIZE; i++) {
> -		spin_lock(&tree->hash_lock);
> -		while ((node = tree->node_hash[i])) {
> -			hfs_bnode_unhash(node);
> -			spin_unlock(&tree->hash_lock);
> -			if (atomic_read(&node->refcnt))
> -				pr_crit("node %d:%d "
> -						"still has %d
> user(s)!\n",
> -					node->tree->cnid, node-
> >this,
> -					atomic_read(&node->refcnt));
> -			hfs_bnode_free(node);
> -			spin_lock(&tree->hash_lock);
> -		}
> -		spin_unlock(&tree->hash_lock);
> -	}
> +	hfs_bnode_hash_free(tree);
>  	iput(tree->inode);
>  	kfree(tree);
>  }