Re: [PATCH bpf-next v3 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() modifies the interval tree before attempting a fallible allocation, leaving the tree in a corrupted state on OOM, similar to the bug fixed in range_tree_clear().
- [High] Transient NOWAIT allocation failure in range_tree_clear() causes arena_vm_fault() to trigger a fatal SIGSEGV.
--

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

bpf, arena: fix range_tree_clear inconsistency on kmalloc_nolock failure

range_tree_clear() pre-allocates the right-half node before modifying
the tree, so an allocation failure returns -ENOMEM without altering the
range tree.

> 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 isn't a bug introduced by this patch, but does range_tree_set() suffer
from this exact same problem?

Looking at range_tree_set() in kernel/bpf/range_tree.c:

	range_tree_clear(rt, start, len);
	...
	left = kmalloc_nolock(sizeof(struct range_node),
			      __GFP_ACCOUNT, NUMA_NO_NODE);
	if (!left)
		return -ENOMEM;

It modifies the tree by calling range_tree_clear() first, and then can fail
its allocation. If arena_free_pages() calls range_tree_set() to return slots
and it fails here, the slots seem to be silently and permanently leaked since
the return value is ignored.

Could we apply this same pre-allocation pattern to range_tree_set() as well?

[Severity: High]
This is another pre-existing issue, but does returning -ENOMEM from here
cause unexpected crashes in user space?

Looking at arena_vm_fault() in kernel/bpf/arena.c:

	ret = range_tree_clear(&arena->rt, vmf->pgoff, 1);
	if (ret)
		goto out_sigsegv_memcg;

Since arena_vm_fault() handles page faults under a raw spinlock with IRQs
disabled, this kmalloc_nolock() acts as a GFP_NOWAIT allocation. If it fails
under memory pressure, it returns -ENOMEM, which arena_vm_fault() then
translates into VM_FAULT_SIGSEGV.

This causes the faulting user application to be randomly killed with a
segmentation fault rather than retrying or returning OOM.

Could this error handling be improved to avoid crashing the task?

[ ... ]

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