Re: [PATCH] hfsplus: Supports freeing newly created tree head

[Edward Adam Davis]

Hi Viacheslav Dubeyko,
Apologies for the late reply.

On Wed, 22 Apr 2026 11:19:10 -0700, Viacheslav Dubeyko wrote:
> On Sat, 2026-04-18 at 17:37 +0800, Edward Adam Davis wrote:
> > On Fri, 17 Apr 2026 15:03:17 -0700, Viacheslav Dubeyko wrote:
> > > > hfs_bnode_put() does not support deallocating a newly created btree
> > > > head node; therefore, regardless of whether hfsplus_bnode_find() succeeds
> > > > or fails, it cannot effectively reclaim the memory allocated for a newly
> > > > created head node.
> > > >
> > > > When finding a head node, if the node is a newly created one, we can use
> > > > hfs_bnode_free() to reclaim its memory.
> > > >
> > > > [1]
> > > > BUG: memory leak
> > > > unreferenced object 0xffff88811cabc840 (size 96):
> > > >   backtrace (crc 3e2dadb7):
> > > >     __hfs_bnode_create+0x59/0x310 fs/hfsplus/bnode.c:469
> > > >     hfsplus_bnode_find+0x13e/0x580 fs/hfsplus/bnode.c:547
> > > >     hfsplus_btree_open+0x2fa/0x6d0 fs/hfsplus/btree.c:382
> > > >     hfsplus_fill_super+0x272/0x880 fs/hfsplus/super.c:548
> > >
> > > I need slightly more detailed and clear explanation how the issue has been
> > > happened and why we need to fix it in a such way. Currently, I don't have the
> > > complete picture how this happened. I am sure that you have the complete
> > > picture. :) Could you please share what you have in your mind? :) Don't hide the
> > > wisdom. ;)
> >     fc_mount()
> >      vfs_get_tree()
> >       get_tree_bdev_flags()
> >        hfsplus_fill_super()
> 
> We have multiple calls of hfsplus_btree_open(). Which particular b-tree do you
> mean here?
> 
> >         hfsplus_btree_open()
> 
> I assume that you mean hfs_btree_open().
> 
> >          hfsplus_bnode_find(tree, HFSPLUS_TREE_HEAD)
> >
> 
> We call hfs_bnode_find() here.
> 
> >
> >           __hfs_bnode_create()
> > 	   ...
> > 	   node->this = cnid; // cnid is HFSPLUS_TREE_HEAD, it is 0
> > 	   ...
> > 	  ...
> > 	  desc = (struct hfs_bnode_desc *)(kmap_local_page(node->page[0])
> > 	  node->type = desc->type;
> > 	  switch (node->type) {
> > 	  ...
> > 	  default:  // because node->type is 127, so run here
Following "default," the comment I added clearly indicates that the
issue stems from the `type` value being 127.
> > 	  	goto node_error;
> 
> I think, maybe, we need to add some error message(s) when some error is
> happened. What do you think?
> 
> > 	  ...
> > 	  hfs_bnode_put()
> > 	   ...
> > 	   if (test_bit(HFS_BNODE_DELETED, &node->flags)) { /* head node not set HFS_BNODE_DELETED flag
> > 							     * so, the node will never be freed.
> > 							     * Furthermore, even if the node were marked
> > 							     * with the HFS_BNODE_DELETED flag, hfs_bmap_free()
> > 							     * would still trigger BUG_ON(!node->this).
Please note the comments I added above.
> > 	   						     */
> 
>  node_error:
>  	set_bit(HFS_BNODE_ERROR, &node->flags);
> -	clear_bit(HFS_BNODE_NEW, &node->flags);
> -	wake_up(&node->lock_wq);
> +	if (num != HFSPLUS_TREE_HEAD) {
> 
> Why do we need to distinguish is it HFSPLUS_TREE_HEAD or not? I don't see any
> difference in node processing.
> 
> +		clear_bit(HFS_BNODE_NEW, &node->flags);
> 
> I think that we don't need to clear the HFS_BNODE_NEW. Because, we need to
> analyze HFS_BNODE_ERROR in hfs_bnode_put().
> 
> +		wake_up(&node->lock_wq);
> 
> I assume that wake_up() should be called anyway. Why have you removed this call
> for HFSPLUS_TREE_HEAD case?
Based on my analysis, hfs_bnode_find() does not support searching for
HFSPLUS_TREE_HEAD.
> 
> +	}
>  	hfs_bnode_put(node);
>  	return ERR_PTR(-EIO);
> 
> @@ -694,6 +698,10 @@ void hfs_bnode_put(struct hfs_bnode *node)
>  			hfs_bnode_free(node);
>  			return;
>  		}
> +		if (test_bit(HFS_BNODE_NEW, &node->flags)) {
> 
> I suggest to analyze HFS_BNODE_ERROR here.
Only the new head node will reach this point, no further analysis is
required.
> 
> +			hfs_bnode_unhash(node);
> +			hfs_bnode_free(node);
> +		}
> 
> @@ -598,14 +598,18 @@ struct hfs_bnode *hfs_bnode_find(struct hfs_btree *tree,
> u32 num)
>  		if (key_size >= entry_size || key_size & 1)
>  			goto node_error;
>  	}
> -	clear_bit(HFS_BNODE_NEW, &node->flags);
> -	wake_up(&node->lock_wq);
> +	if (num != HFSPLUS_TREE_HEAD) {
> +		clear_bit(HFS_BNODE_NEW, &node->flags);
> +		wake_up(&node->lock_wq);
> 
> What the point in this code? Why have you change the logic here?
To support tree head.
> 
> +	}
>  	return node;
> 
> And, could you please add these new details into the commit message?
OK.

Edward
BR