Re: [PATCH bpf-next v7 2/2] bpf, arena: fix range_tree_set inconsistency on kmalloc_nolock failure

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]

> bpf, arena: fix range_tree_set inconsistency on kmalloc_nolock failure
>
> range_tree_set() pre-allocates the node needed for a brand-new range
> before calling range_tree_clear(), so an allocation failure returns
> -ENOMEM without having modified the tree (previously the overlapping
> nodes were already removed by range_tree_clear() before the allocation
> was attempted, permanently losing the cleared sub-ranges).
>
> The adjacency lookups done before the clear already return the nodes
> that the merge below needs: range_tree_clear() frees only the nodes
> that lie entirely inside [start, last], trims the overlapping ones in
> place, and the split case cannot happen because of the 'already set'
> check above. Do the lookup once and drop the unreachable -EFAULT
> checks.
>
> Signed-off-by: Yuan Chen <chenyuan@kylinos.cn>

This looks like a bug fix for the clear-before-allocate ordering in
range_tree_set(). Should this include:

  Fixes: b795379757eb ("bpf: Introduce range_tree data structure and use it in bpf arena")

> diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
> index 15b588377a76..2460e78d6a55 100644
> --- a/kernel/bpf/range_tree.c
> +++ b/kernel/bpf/range_tree.c
> @@ -199,6 +199,7 @@ int is_range_tree_set(struct range_tree *rt, u32 start, u32 len)
>  int range_tree_set(struct range_tree *rt, u32 start, u32 len)
>  {
>  	u32 last = start + len - 1;
> +	struct range_node *new_rn = NULL;
>  	struct range_node *right;
>  	struct range_node *left;
>  	int err;
> @@ -208,20 +209,19 @@ int range_tree_set(struct range_tree *rt, u32 start, u32 len)
>  	if (left && left->rn_start <= start && left->rn_last >= last)
>  		return 0;
>
> +	left = range_it_iter_first(rt, start - 1, start - 1);
> +	right = range_it_iter_first(rt, last + 1, last + 1);

This isn't a bug, but could a short comment above these lookups record
why they are still valid after range_tree_clear() runs, the way the
changelog explains it? The correctness now rests on a non-obvious
argument: range_tree_clear() only trims a node overlapping start-1 or
last+1 in place (never frees it), so these pointers and their endpoints
are still the ones the merge branches below need. Every other decision
point in this function carries a one-line comment, so the two bare
lookups are the odd ones out, and the rationale currently lives only in
the changelog.

> +	if (!left && !right) {
> +		new_rn = kmalloc_nolock(sizeof(struct range_node),
> +					__GFP_ACCOUNT, NUMA_NO_NODE);
> +		if (!new_rn)
> +			return -ENOMEM;
> +	}
> +
>  	/* Clear out everything in the range we want to set. */
>  	err = range_tree_clear(rt, start, len);
>  	if (err)
> -		return err;

This isn't a bug, but since the changelog argues range_tree_clear()
cannot fail here (the split path is unreachable because of the 'already
set' check), is the new out_free_new path kept deliberately as
future-proofing, or could it go the same way as the -EFAULT checks? The
asymmetry is what reads oddly: the patch removes two unreachable checks
precisely for being unreachable, while keeping error propagation from an
int-returning helper that the changelog says cannot fail.

> +		goto out_free_new;
>
[ ... ]

> @@ -241,14 +241,16 @@ int range_tree_set(struct range_tree *rt, u32 start, u32 len)
>  		right->rn_start = start;
>  		range_it_insert(right, rt);
>  	} else {
> -		left = kmalloc_nolock(sizeof(struct range_node), __GFP_ACCOUNT, NUMA_NO_NODE);
> -		if (!left)
> -			return -ENOMEM;
> -		left->rn_start = start;
> -		left->rn_last = last;
> -		range_it_insert(left, rt);
> +		/* No adjacent ranges; use the pre-allocated node */
> +		new_rn->rn_start = start;
> +		new_rn->rn_last = last;
> +		range_it_insert(new_rn, rt);
>  	}
>  	return 0;
> +
> +out_free_new:
> +	kfree_nolock(new_rn);
> +	return err;
>  }
>
>  void range_tree_destroy(struct range_tree *rt)


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35841441642