Re: [PATCH bpf-next v9 6/8] selftests/bpf: Add buddy allocator for libarena

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] Permanent memory leaks of newly allocated chunks and freed blocks when allocator lock acquisition fails.
- [Medium] Unnecessary global lock contention during thread-local chunk initialization.
- [Medium] Missing rollback of virtual address space reservation on initialization failure.
- [Low] Accidental inclusion of default.profraw binary file.
--

commit d3559de018f16408b8163bc0362e7b5fdd94366e
Author: Emil Tsalapatis <emil@etsalapatis.com>
selftests/bpf: Add buddy allocator for libarena

Add a byte-oriented buddy allocator for libarena. The buddy allocator
provides an alloc/free interface for small arena allocations ranging
from 16 bytes to 512 KiB.

> diff --git a/tools/testing/selftests/bpf/default.profraw b/tools/testing/selftests/bpf/default.profraw
> new file mode 100644
> index 000000000000..e865e87829f8
> Binary files /dev/null and b/tools/testing/selftests/bpf/default.profraw differ

This isn't a bug, but it looks like a binary LLVM profile data file was
accidentally included in the patch.

> diff --git a/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
> new file mode 100644
> index 000000000000..865e00803daa
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/libarena/src/buddy.bpf.c
[ ... ]
> +static buddy_chunk_t *buddy_chunk_get(buddy_t *buddy)
> +{
[ ... ]
> +	chunk = bpf_arena_alloc_pages(&arena, (void __arena *)vaddr,
> +				      BUDDY_CHUNK_PAGES, NUMA_NO_NODE, 0);
> +	if (!chunk) {
> +		arena_stderr("[ALLOC FAILED]");
> +		return NULL;
> +	}
> +
> +	if (buddy_lock(buddy)) {
> +		/*
> +		 * We cannot reclaim the vaddr space, but that is ok - this
> +		 * operation should always succeed. The error path is to catch
> +		 * accidental deadlocks that will cause -ENOMEMs to the program as
> +		 * the allocator fails to refill itself, in which case vaddr usage
> +		 * is the least of our worries.
> +		 */
> +		bpf_arena_free_pages(&arena, (void __arena *)vaddr, BUDDY_CHUNK_PAGES);
> +		return NULL;
> +	}
> +
> +	asan_poison(chunk, BUDDY_POISONED, BUDDY_CHUNK_PAGES * __PAGE_SIZE);
> +
> +	/* Unpoison the chunk itself. */
> +	asan_unpoison(chunk, sizeof(*chunk));
> +
> +	/* Mark all freelists as empty. */
> +	for (ord = zero; ord < BUDDY_CHUNK_NUM_ORDERS && can_loop; ord++)
> +		chunk->freelists[ord] = BUDDY_CHUNK_ITEMS;

Does holding the global allocator lock here create unnecessary contention?

Since this newly allocated chunk is not yet linked to the global allocator
state (which happens later in buddy_alloc_from_new_chunk()), could the
memory poisoning, unpoisoning, and freelist initialization loop be done
before acquiring the global lock?

[ ... ]
> +__weak int buddy_init(buddy_t __arg_arena *buddy)
> +{
> +	buddy_chunk_t *chunk;
> +	int ret;
> +
> +	if (!asan_ready())
> +		return -EINVAL;
> +
> +	/* Reserve enough address space to ensure allocations are aligned. */
> +	ret = buddy_reserve_arena_vaddr(buddy);
> +	if (ret)
> +		return ret;
> +
> +	_Static_assert(BUDDY_CHUNK_PAGES > 0,
> +		       "chunk must use one or more pages");
> +
> +	chunk = buddy_chunk_get(buddy);
> +
> +	if (buddy_lock(buddy)) {
> +		bpf_arena_free_pages(&arena, chunk, BUDDY_CHUNK_PAGES);
> +		return -EINVAL;
> +	}

If buddy_chunk_get() or buddy_lock() fails, does this permanently leak the
virtual address space reserved earlier?

It looks like buddy_unreserve_arena_vaddr() should be called in these
error paths to prevent exhaustion of the reserved range.

[ ... ]
> +static u64 buddy_alloc_from_new_chunk(buddy_t *buddy, buddy_chunk_t *chunk, int order)
> +{
> +	u64 address;
> +
> +	if (buddy_lock(buddy))
> +		return (u64)NULL;

If the allocator lock cannot be acquired, does this drop the newly allocated
chunk without linking it or freeing it?

Since bpf_arena_free_pages() isn't called on the chunk, it appears this
could permanently leak physical and virtual arena memory.

[ ... ]
> +__weak int buddy_free_internal(buddy_t __arg_arena *buddy, u64 addr)
> +{
> +	int ret;
> +
> +	if (!buddy)
> +		return -EINVAL;
> +
> +	/* Freeing NULL is a valid no-op. */
> +	if (!addr)
> +		return 0;
> +
> +	ret = buddy_lock(buddy);
> +	if (ret)
> +		return ret;

If lock acquisition fails here, does the block get permanently leaked?

Since the user-facing free() wrapper returns void, the caller has no way
of knowing the free failed, resulting in silently swallowed errors and
lost memory.

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