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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] selftests/bpf: buddy_alloc_memory() grows allocator in NMI
  context
- [Medium] selftests/bpf: unvalidated write and error propagation in
  deferred free
- [Low] selftests/bpf: deferred frees stay unpoisoned in
  buddy_deferred_free_start()
--

commit 88f6bdc1e521e6f215ca4ea48a935038a6d8cf00
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Handle NMI reentrancy

This commit switches the libarena buddy allocator to use trylock in NMI
context and defers frees to an intrusive 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 13c55cf53e524..4d4d21356935d 100644
> --- a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
[ ... ]
> @@ -46,10 +47,14 @@ static int idx_is_allocated(struct buddy_chunk __arena *chunk, u64 idx, bool *al
>  static u64 arena_next_pow2(__u64 n);
>  static u64 size_to_order(size_t size);
>  static void __arena *buddy_alloc_memory(struct buddy __arena *buddy, size_t size,
> -					int order, size_t norder);
> +					int order, size_t norder, bool grow);
>  static int buddy_free_unlocked(struct buddy __arena *buddy, u64 addr);
> +static bool buddy_deferred_free_pending(struct buddy __arena *buddy);
> +static int buddy_deferred_free_complete(struct buddy __arena *buddy);
>  
> -#define buddy_lock(buddy, flags) (arena_spin_lock_irqsave(&(buddy)->lock, (flags)))
> +#define buddy_lock(buddy, flags)						\
> +	(bpf_in_nmi() ? arena_spin_trylock_irqsave(&(buddy)->lock, (flags)) :	\
> +			arena_spin_lock_irqsave(&(buddy)->lock, (flags)))
>  #define buddy_unlock(buddy, flags) (arena_spin_unlock_irqrestore(&(buddy)->lock, (flags)))

[Severity: Medium]
Can buddy_alloc_memory() still call buddy_chunk_get() to grow the allocator
when bpf_in_nmi() is true?

When an allocation in NMI context misses the per-cpu cache and existing
chunks, buddy_alloc_memory() calls buddy_chunk_get():

tools/testing/selftests/bpf/libarena/src/buddy.bpf.c:buddy_chunk_get() {
    ...
	ret = buddy_alloc_arena_vaddr(buddy, &vaddr);
	if (ret)
		return NULL;
    ...
	/* Unreserve the address space. */
	bpf_arena_free_pages(&arena, (void __arena *)vaddr,
			     BUDDY_CHUNK_PAGES);

	chunk = bpf_arena_alloc_pages(&arena, (void __arena *)vaddr,
				      BUDDY_CHUNK_PAGES, NUMA_NO_NODE, 0);
    ...
}

Because NMI programs are non-sleepable, bpf_arena_free_pages() defers
unreservation to a workqueue via irq_work instead of updating the arena
range tree synchronously.  The immediately following bpf_arena_alloc_pages()
at vaddr then always fails because the range is still marked reserved,
logging an allocation failure and leaking 1 MiB of buddy->vaddr address
space per attempt.  Should buddy_alloc_memory() avoid calling
buddy_chunk_get() when bpf_in_nmi() is true?

[ ... ]

> @@ -970,6 +977,13 @@ static void __arena *buddy_alloc_memory(struct buddy __arena *buddy, size_t size
>  	if (buddy_lock(buddy, flags))
>  		return NULL;
>  
> +	if (unlikely(buddy_deferred_free_pending(buddy))) {
> +		if (buddy_deferred_free_complete(buddy)) {
> +			buddy_unlock(buddy, flags);
> +			return NULL;
> +		}
> +	}
> +
>  	address = (u8 __arena *)buddy_alloc_from_existing_chunks(buddy, order, norder);
>  	buddy_unlock(buddy, flags);
>  	if (address)
> @@ -1176,10 +1190,90 @@ 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 __always_inline void buddy_deferred_free_push(struct buddy __arena *buddy,
> +						   void __arena *first, void __arena *last)
> +{
> +	void __arena *head;
> +
> +	do {
> +		head = READ_ONCE(buddy->deferred_free);
> +		*(void __arena * __arena *)last = head;
> +	} while (cmpxchg((void __arena * __arena *)&buddy->deferred_free,
> +			 head, first) != head && can_loop);
> +}
> +
> +static void buddy_deferred_free_start(struct buddy __arena *buddy, void __arena *addr)
> +{
> +	/*
> +	 * Make a linked list by chaining the allocations into a linked list.
> +	 * Possible because allocations are 16 bytes minimum and so fit a pointer.
> +	 */
> +	asan_unpoison(addr, sizeof(void *));
> +	buddy_deferred_free_push(buddy, addr, addr);
> +}
> +
> +static void buddy_deferred_free_start_bulk(struct buddy __arena *buddy,
> +		 void __arena * __arena *addrs, size_t naddrs)
> +{
> +	void __arena *first, *last, *addr;
> +	unsigned int i;
> +
> +	/*
> +	 * 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;
> +
> +		asan_unpoison(addr, sizeof(void *));
> +
> +		if (!first)
> +			first = addr;
> +		else
> +			*(void __arena * __arena *)last = addr;
> +
> +		last = addr;
> +	}
> +
> +	if (!first)
> +		return;
> +
> +	/* Splice the local list onto the deferred-free list. */
> +	buddy_deferred_free_push(buddy, first, last);
> +}

[Severity: Low]
Does buddy_deferred_free_start() leave deferred objects unpoisoned in the
asan shadow map until buddy_deferred_free_complete() runs?

Both buddy_deferred_free_start() and buddy_deferred_free_start_bulk() call
asan_unpoison(addr, sizeof(void *)) and leave the freed object (including
any padding bytes in the first 8 bytes when the allocation size was under 8
bytes) unpoisoned while it sits on buddy->deferred_free.  Use-after-free
accesses during that window will not be caught by asan, and writes to the
first 8 bytes can corrupt the intrusive list pointer without triggering an
asan violation.

> +
> +static int buddy_deferred_free_complete(struct buddy __arena *buddy)
> +{
> +	void __arena *head, *old;
> +	int ret = 0, err;
> +
> +	head = __sync_lock_test_and_set((void __arena * __arena *)&buddy->deferred_free,
> +					NULL);
> +
> +	while (head && can_loop) {
> +		old = head;
> +		head = *(void __arena * __arena *)head;
> +
> +		err = buddy_free_unlocked(buddy, (u64)old);
> +		ret = ret ?: err;
> +	}
> +
> +	return ret;
> +}
> +
>  __weak int buddy_free(struct buddy __arena *buddy, void __arena *addr)
>  {
>  	unsigned long flags;
> -	int ret;
> +	int ret, err;
>  
>  	if (!buddy)
>  		return -EINVAL;
> @@ -1193,20 +1287,28 @@ __weak int buddy_free(struct buddy __arena *buddy, void __arena *addr)
>  		return 0;
>  
>  	ret = buddy_lock(buddy, flags);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		buddy_deferred_free_start(buddy, addr);
> +		return 0;
> +	}
>  
>  	ret = buddy_free_unlocked(buddy, (u64)addr);
>  
> +	/* Drain any frees stranded during NMI/lock failure. */
> +	if (unlikely(buddy_deferred_free_pending(buddy))) {
> +		err = buddy_deferred_free_complete(buddy);
> +		ret = ret ?: err;
> +	}
> +
>  	buddy_unlock(buddy, flags);
>  
>  	return ret;
>  }

[Severity: Medium]
Can buddy_deferred_free_start() and buddy_deferred_free_start_bulk()
overwrite memory before validating that the address is an allocated block?

Unlike buddy_cache_free(), which checks alignment and allocation state via
buddy_cache_get_order() before touching the block, the deferred-free paths
unpoison and write the next pointer directly into *(void __arena * __arena *)addr
when buddy_lock() fails.  Passing an unaligned, interior,already-freed, or
duplicate address while the lock is contended corrupts 8 bytes at addr or
creates a cycle in buddy->deferred_free.

In addition, when buddy_deferred_free_complete() later drains an invalid
entry, propagating its error causes buddy_alloc_memory() to abort and return
NULL for an unrelated allocation, and causes buddy_free() to return an error
even when the current caller's addr was freed without issue.

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