Re: [PATCH bpf-next v5 6/9] selftests/bpf: libarena: Handle NMI reentrancy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] selftests/bpf: inconsistent error return in
  buddy_deferred_free_start_bulk()
--

commit 6751ae768506dfa7101c71c63c2aa51a0d3eb6cc
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Handle NMI reentrancy

This commit handles NMI reentrancy in the libarena buddy allocator by using
a trylock in NMI context and deferring frees onto an intrusive lockless list
when the buddy lock cannot be acquired.

> diff --git a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> index 58e666210fdbd..7fd74f4c50526 100644
> --- a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c

[ ... ]

> @@ -1187,6 +1201,98 @@ static __always_inline int buddy_free_unlocked(struct buddy __arena *buddy, u64
>  	return 0;
>  }
>  
> +static bool buddy_deferred_free_pending(struct buddy __arena *buddy)
> +{
> +	return buddy->deferred_free != NULL;
> +}

[ ... ]

> +static int buddy_deferred_free_start_bulk(struct buddy __arena *buddy,
> +		 void __arena * __arena *addrs, size_t naddrs)
> +{
> +	void __arena *first, *last, *addr;
> +	unsigned int i;
> +	int ret = 0, order;
> +
> +	/*
> +	 * Chain the allocations into a local linked list. Possible because
> +	 * allocations are 16 bytes minimum and so fit a pointer.
> +	 */
> +	first = NULL;
> +	last = NULL;
> +	for (i = zero; i < naddrs && can_loop; i++) {
> +		addr = addrs[i];
> +		if (!addr)
> +			continue;
> +
> +		order = buddy_cache_get_order(addr);
> +		if (order >= BUDDY_CHUNK_NUM_ORDERS) {
> +			ret = -EINVAL;
> +			continue;
> +		}
> +
> +		asan_poison(addr, BUDDY_POISONED, BUDDY_MIN_ALLOC_BYTES << order);
> +		asan_unpoison(addr, sizeof(void *));
> +
> +		if (!first)
> +			first = addr;
> +		else
> +			*(void __arena * __arena *)last = addr;
> +
> +		last = addr;
> +	}
> +
> +	if (!first)
> +		return ret;
> +
> +	/* Splice the local list onto the deferred-free list. */
> +	buddy_deferred_free_push(buddy, first, last);
> +	return 0;
> +}

[Severity: Low]
Is the return value of buddy_deferred_free_start_bulk() intended when an
address in addrs[] has an invalid order?

When a batch contains both an invalid address and at least one valid
address, the final "return 0" discards the "ret = -EINVAL" recorded in the
loop.  Conversely, when every non-NULL address in the batch is invalid,
"!first" is true and this returns -EINVAL, whereas the locked path in
buddy_free_bulk() returns 0 and leaves invalid-object reporting to
buddy_free_unlocked() (which is also skipped for addresses filtered out
here).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110003838.8739-1-emil@etsalapatis.com?part=6