Re: [PATCH] hfsplus: fix hfs_bnode_split() failure on sparsely-filled nodes

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Fri, 2026-04-24 at 03:19 +0530, Shardul Bankar wrote:
> hfs_bnode_split() determines the split point by scanning the node's
> offset table for the first record whose data offset exceeds a threshold
> derived from node_size / 2. When all record data fits within the first
> half of the node, no record offset exceeds the threshold, the loop
> exhausts all records, and the function returns -ENOSPC even though the
> node can be validly split. This causes xattr insertions to fail
> silently.
> 
> The failing code path is exercised by xfstests generic/070 and
> generic/642 during xattr stress operations.
> 
> Fix this by re-scanning with a threshold based on the actual data
> midpoint when the position-based scan exhausts. If the re-scan also
> exhausts, fall back to splitting off the last record.
> 
> Reported-by: Viacheslav Dubeyko <slava@dubeyko.com>
> Signed-off-by: Shardul Bankar <shardul.b@mpiricsoftware.com>
> ---
>  fs/hfsplus/brec.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c
> index e3df89284079..cfc909c808a4 100644
> --- a/fs/hfsplus/brec.c
> +++ b/fs/hfsplus/brec.c
> @@ -282,12 +282,32 @@ static struct hfs_bnode *hfs_bnode_split(struct hfs_find_data *fd)
>  		old_rec_off -= rec_size;
>  		if (++num_recs < node->num_recs)
>  			continue;
> -		hfs_bnode_put(node);
> -		hfs_bnode_unlink(new_node);
> -		hfs_bnode_put(new_node);
> -		if (next_node)
> -			hfs_bnode_put(next_node);
> -		return ERR_PTR(-ENOSPC);

I don't quite follow why do we completely remove this logic? Potentially, it
could be valid situation. No?

Also, I think we need to add hfs_bnode_unlink(new_node) here [1]. Otherwise,
hfs_bnode_put() will be unable to destroy the node.

> +		/*
> +		 * All data fits within the node_size/2 threshold,
> +		 * so re-scan using the actual data midpoint.
> +		 */

First of all, I don't see the answer on question. Why we call this method for
node that is not completely full (or near to be full). If we have half empty
node, then, what's the point to execute node splitting? We have enough space to
add more items. Do we have the same issue for Catalog File and Extents file?

> +		size = hfs_bnode_read_u16(node, tree->node_size -
> +			(node->num_recs + 1) * rec_size);

I prefer to executes likewise calculation more carefully. We could have
incorrect tree->node_size, weird node->num_recs value.

Why do we need node->num_recs + 1?

Potentially, we could have overflow here.

> +		size = ((int)node_desc_size + size) / 2;

How can we blindly rely on size that we received from hfs_bnode_read_u16()? What
if it was really huge or really small value?

> +		old_rec_off = tree->node_size - (2 * rec_size);

Why 2 was hardcoded here? What this value mean? Why do we need to multiple
rec_size on 2?

> +		num_recs = 1;
> +		for (;;) {

I slightly dislike infinite loop here. Why do we need to use infinite loop? We
have size or number of items estimation. What the point to use infinite loop? It
is always bad coding style.

> +			data_start = hfs_bnode_read_u16(node,
> +							old_rec_off);
> +			if (data_start > size)
> +				break;
> +			old_rec_off -= rec_size;
> +			if (++num_recs < node->num_recs)

This is can be used as for loop boundary. Why infinite loop has been used?

> +				continue;
> +			/* last record holds most of the data */
> +			num_recs = node->num_recs - 1;
> +			old_rec_off = tree->node_size -
> +				(num_recs + 1) * rec_size;

The same concerns related to calculation here.

Thanks,
Slava.

> +			data_start = hfs_bnode_read_u16(node,
> +							old_rec_off);
> +			break;
> +		}
> +		break;
>  	}
>  
>  	if (fd->record + 1 < num_recs) {

[1] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/brec.c#L262