Re: [PATCH 3/3] selftests/bpf: Test large allocations for both sleepable/nonsleepable arena users

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The validation loop's error path incorrectly returns the loop index `i`, meaning a memory validation failure on the first page (`i == 0`) will falsely report as a successful test.

Pre-existing issues:
- [High] Hard lockup (watchdog timeout) in `arena_alloc_pages_non_sleepable` due to a massive page allocation loop executing with local interrupts disabled.
--

--- Patch [3]: [PATCH 3/3] selftests/bpf: Test large allocations for both sleepable/nonsleepable arena users ---
commit 97b45503d1d3323d8eb083b751d71f1bec8430fe
Author: Emil Tsalapatis <[email protected]>

selftests/bpf: Test large allocations for both sleepable/nonsleepable arena users

We now use different code paths in the internal allocator when
allocating arena memory, depending on whether the caller is sleepable
or not. These paths mostly differ functionally for large allocations,
so add extra testing for that case.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_arena_large.c b/tools/testing/selftests/bpf/progs/verifier_arena_large.c
> --- a/tools/testing/selftests/bpf/progs/verifier_arena_large.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_arena_large.c

[ ... ]

> @@ -300,7 +301,31 @@ int big_alloc3(void *ctx)
>  	 */
>  	pages = bpf_arena_alloc_pages(&arena, NULL, 2051, NUMA_NO_NODE, 0);

[Severity: High]
This is a pre-existing issue, but does this allocation of 2051 pages trigger
a hard lockup in the underlying allocator?

Looking at arena_alloc_pages_non_sleepable() in kernel/bpf/arena.c, the code
disables interrupts and runs a batch allocation loop:

kernel/bpf/arena.c:arena_alloc_pages_non_sleepable() {
    ...
    while (remaining) {
        long this_batch = min(remaining, alloc_pages);
        memset(pages, 0, this_batch * sizeof(struct page *));
        ret = bpf_map_alloc_pages(&arena->map, node_id, this_batch, pages);
        ...
        ret = apply_to_page_range(&init_mm, ...);
    }
    ...
}

Since this path is executed with a raw spinlock held and local interrupts
disabled, could a large page_cnt like 2051 cause a watchdog timeout?

>  	if (!pages)
> -		return 0;
> +		return 1;
> +
> +	bpf_for(i, 0, 2051)
> +			pages[i * PAGE_SIZE] = 123;
> +	bpf_for(i, 0, 2051)
> +			if (pages[i * PAGE_SIZE] != 123)
> +				return i;

[Severity: Medium]
Does this error path mask validation failures on the first page?

Since the program expects 0 for success (__success __retval(0)), if the 
validation fails on the first page (i == 0), the loop returns 0. This
perfectly aliases the expected success code and completely masks the failure.

Additionally, failing on the second page (i == 1) returns 1, which aliases
the allocation failure code above. 

Could this return a distinct error code instead of the loop index?

> +
> +	bpf_arena_free_pages(&arena, pages, 2051);
> +#endif
> +	return 0;
> +}
> +
> +/* SYSCALL programs are always sleepable. */
> +SEC("syscall")
> +__success __retval(0)
> +int big_alloc4(void *ctx)
> +{
> +#if defined(__BPF_FEATURE_ADDR_SPACE_CAST)
> +	char __arena *pages;
> +	u64 i;
> +
> +	pages = bpf_arena_alloc_pages(&arena, NULL, 2051, NUMA_NO_NODE, 0);
> +	if (!pages)
> +		return 1;
>  
>  	bpf_for(i, 0, 2051)
>  			pages[i * PAGE_SIZE] = 123;
> +	bpf_for(i, 0, 2051)
> +			if (pages[i * PAGE_SIZE] != 123)
> +				return i;

[Severity: Medium]
Similarly, does returning the loop index here falsely report success if the
first page fails validation?

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