Re: [PATCH bpf-next v1 7/7] selftests/bpf: libarena: Cover buddy cache fast paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] selftests/bpf: buddy_cache_get_order() accepts interior pointers on free
- [Low] selftests/bpf: error return clobbered in buddy_free() during NMI drain
--

commit 05ed9b95b2e617e5142c6f1c186bbaa3122d07f7
Author: Emil Tsalapatis <emil@etsalapatis.com>

selftests/bpf: libarena: Cover buddy cache fast paths

This commit adds BPF selftests covering bulk allocations and per-CPU and
transfer cache fast paths in libarena's buddy allocator, and documents that
buddy_alloc_bulk() bypasses the per-CPU caches.

> diff --git a/tools/testing/selftests/bpf/libarena/selftests/test_buddy.bpf.c b/tools/testing/selftests/bpf/libarena/selftests/test_buddy.bpf.c
> index 6d123ec052b0f..f14c2945d1e11 100644
> --- a/tools/testing/selftests/bpf/libarena/selftests/test_buddy.bpf.c
> +++ b/tools/testing/selftests/bpf/libarena/selftests/test_buddy.bpf.c

[ ... ]

> @@ -221,4 +240,251 @@ __weak int test_buddy_alignment(void)

[ ... ]

> +SEC("syscall")
> +__weak int test_buddy_cache_paths(void)
> +{

[ ... ]

> +	/* Bulk allocation bypasses the cache. Fill the remaining local slots. */
> +	ret = buddy_alloc_bulk(&buddy, TEST_CACHE_SIZE,
> +			       arena_fls(BUDDY_CACHE_CAPACITY) - 1,
> +			       alloc_path_test.bulk);
> +	if (ret)
> +		goto out;

[ ... ]

> +	/* Interior pointers are rejected without consuming the allocation. */
> +	i = BUDDY_CACHE_CAPACITY - BUDDY_CACHE_BATCH_SIZE + 2;
> +	if (!buddy_free(&buddy, (u8 __arena *)alloc_path_test.bulk[i] +
> +				 BUDDY_MIN_ALLOC_BYTES)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}

[Severity: Medium]
Will this interior pointer check in test_buddy_cache_paths() fail when
BPF_ARENA_ASAN is disabled because buddy_cache_free() accepts the interior
pointer as an order-0 object?

When buddy_alloc_bulk() allocates the TEST_CACHE_SIZE (order 2) objects via
buddy_chunk_alloc() in tools/testing/selftests/bpf/libarena/src/buddy.bpf.c,
only the head granule of each object has its order and allocated bit recorded:

	for (i = zero, idx = retidx; i < (1U << order_allocs) && can_loop; i++) {
		if (idx_set_order(chunk, idx, order_req))
			return (u64)NULL;

		if (idx_set_allocated(chunk, idx, true))
			return (u64)NULL;

		idx += 1U << order_req;
	}

The interior granule at alloc_path_test.bulk[i] + BUDDY_MIN_ALLOC_BYTES
(idx + 1) retains its zero-initialized order nibble (0) and allocated bit (0).

When test_buddy_cache_paths() passes that interior pointer to buddy_free(),
the call chain:

  buddy_free()
    -> buddy_cache_free()
      -> buddy_cache_get_order()

reads the order directly without checking alignment or whether idx is marked
allocated in chunk->allocated:

static int buddy_cache_get_order(void __arena *addr)
{
	struct buddy_chunk __arena *chunk;
	u64 idx;

	chunk = (void __arena *)((u64)addr & ~BUDDY_CHUNK_OFFSET_MASK);
	idx = ((u64)addr & BUDDY_CHUNK_OFFSET_MASK) / BUDDY_MIN_ALLOC_BYTES;

	return idx_get_order(chunk, idx);
}

Because idx_get_order() returns 0 for the unallocated interior granule and
the order-0 per-CPU cache has room, buddy_cache_push() pushes the interior
pointer into ctx.cache->orders[0] and buddy_free() returns 0, causing
test_buddy_cache_paths() to return -EINVAL.

Should buddy_cache_get_order() (or buddy_cache_free() and
buddy_cache_transfer_free()) verify that addr is aligned to
BUDDY_MIN_ALLOC_BYTES and that idx is marked allocated via idx_is_allocated()
before accepting the pointer into the cache?

> +	ret = buddy_free(&buddy, alloc_path_test.bulk[i]);

[Severity: Low]
In tools/testing/selftests/bpf/libarena/src/buddy.bpf.c, can buddy_free()
lose the error returned by buddy_free_unlocked() when a deferred NMI free is
pending?

Looking at buddy_free():

	ret = buddy_free_unlocked(buddy, (u64)addr);

	/* Drain any frees stranded during an NMI. */
	if (unlikely(buddy_deferred_free_pending(buddy))) {
		ret = buddy_deferred_free_complete(buddy);
		if (ret)
			goto out_unlock;
	}

If buddy_free_unlocked() returns an error (such as -EINVAL for an invalid or
interior pointer that bypassed the cache) while buddy_deferred_free_pending()
is true, buddy_deferred_free_complete() overwrites ret with 0 on success.

By contrast, buddy_free_bulk() in the same file preserves any earlier error:

	/* Drain any frees stranded during an NMI. */
	if (unlikely(buddy_deferred_free_pending(buddy))) {
		err = buddy_deferred_free_complete(buddy);
		ret = ret ?: err;
	}

Should buddy_free() preserve ret in the same way when draining deferred frees?

> +
> +out:
> +	destroy_ret = buddy_destroy(&buddy);
> +	return ret ?: destroy_ret;
> +#endif
> +}

[ ... ]

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