Re: [PATCH bpf-next v2 1/3] bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `range_tree_set()` leaves the interval tree in a corrupted state if node allocation fails.
- [Medium] Ignored return value from `range_tree_set()` leads to permanent virtual space leak in the BPF arena on OOM.
--

--- Patch [1]: [PATCH bpf-next v2 1/3] bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure ---
commit d0c75ee613a9a640e30aac20ab84b5820201a99d
Author: Yuan Chen <[email protected]>

bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure

This commit modifies range_tree_clear() to pre-allocate the right-half node
before modifying the tree. This ensures that an allocation failure returns
-ENOMEM without leaving the range tree in an altered state.

> diff --git a/kernel/bpf/range_tree.c b/kernel/bpf/range_tree.c
> index 2f28886f3ff75..15b588377a769 100644
> --- a/kernel/bpf/range_tree.c
> +++ b/kernel/bpf/range_tree.c
> @@ -143,16 +143,22 @@ int range_tree_clear(struct range_tree *rt, u32 start, u32 len)
>  		if (rn->rn_start < start && rn->rn_last > last) {
>  			u32 old_last = rn->rn_last;
>  
> +			/*
> +			 * Pre-allocate the right-half node before modifying
> +			 * the tree. If allocation fails we return -ENOMEM
> +			 * without altering the range tree.
> +			 */
> +			new_rn = kmalloc_nolock(sizeof(struct range_node),
> +						__GFP_ACCOUNT, NUMA_NO_NODE);
> +			if (!new_rn)
> +				return -ENOMEM;
> +

[Severity: High]
This is a pre-existing issue, but does range_tree_set() suffer from a
similar state corruption if its node allocation fails?

kernel/bpf/range_tree.c:range_tree_set() {
    ...
    /* Clear out everything in the range we want to set. */
    err = range_tree_clear(rt, start, len);
    if (err)
        return err;
    ...
    } else {
        left = kmalloc_nolock(sizeof(struct range_node), ...);
        if (!left)
            return -ENOMEM;

If range_tree_set() fails to allocate 'left' due to OOM, it returns -ENOMEM
after range_tree_clear() has already destructively removed sub-ranges.
Should range_tree_set() also pre-allocate its nodes before modifying the
tree to prevent state corruption?


[Severity: Medium]
This is also a pre-existing issue, but are the return values of
range_tree_set() intentionally ignored in the arena code?

In kernel/bpf/arena.c:arena_alloc_pages():
...
        ret = bpf_map_alloc_pages(&arena->map, node_id, this_batch, pages);
        if (ret)
            goto out;
    ...
    out:
        range_tree_set(&arena->rt, pgoff + mapped, page_cnt - mapped);

And similarly in kernel/bpf/arena.c:arena_free_pages():
    ...
        range_tree_set(&arena->rt, pgoff, page_cnt);

If range_tree_set() encounters an allocation failure and returns -ENOMEM,
the failure is silently ignored. Could this cause the freed pages' virtual
space to be permanently leaked and inaccessible?

>  			/* Overlaps with the entire clearing range */
>  			range_it_remove(rn, rt);
>  			rn->rn_last = start - 1;
>  			range_it_insert(rn, rt);
>  
> -			/* Add a range */
> -			new_rn = kmalloc_nolock(sizeof(struct range_node), __GFP_ACCOUNT,
> -						NUMA_NO_NODE);
> -			if (!new_rn)
> -				return -ENOMEM;
> +			/* Add right-half range */
>  			new_rn->rn_start = last + 1;
>  			new_rn->rn_last = old_last;
>  			range_it_insert(new_rn, rt);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1