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

[Shardul Bankar <shardul.b@mpiricsoftware.com>]

On Tue, 2025-12-16 at 20:28 +0000, Viacheslav Dubeyko wrote:
> 
> The fix in hfs_bmap_alloc() sounds reasonable to me. But I don't see
> the point
> of adding hfs_bnode_get() in hfs_bnode_create() for the case of
> erroneous
> situation [1]:
> 
>         if (node) {
>                 pr_crit("new node %u already hashed?\n", num);
>                 WARN_ON(1);
>                 return node;
>         }
> 
> It will be much better to return ERR_PTR(-EEXIST) here. Because, it
> is not
> situation of "doing business as usual". We should not continue to
> believe that
> "sun is shining for us", but we should stop the logic somehow.
> 
> Thanks,
> Slava.
> 
> [1] https://elixir.bootlin.com/linux/v6.18/source/fs/hfs/bnode.c#L518

Hi Slava,

Thanks, agreed.

I’ll keep the hfs_bmap_alloc() change to ensure node 0 is never
allocated.

And I agree that the “already hashed?” case in hfs_bnode_create()
should not try to continue by returning a pointer (even with an extra
hfs_bnode_get()). Callers like hfs_btree_inc_height() and
hfs_bnode_split() treat the returned node as a freshly allocated node
and immediately rewrite its header/records. If hfs_bnode_create()
returns an existing hashed node, that effectively overwrites live node
contents and amplifies corruption, which can then cascade into later
failures.

So I’ll rework v2 as a small series:
1/2: guard against allocating node 0 in hfs_bmap_alloc()
2/2: make the “already hashed?” path return ERR_PTR(-EEXIST) and
propagate the error

I’ll send the updated series shortly.

Thanks,
Shardul