Re: [PATCH bpf-next v5 2/3] selftests/bpf: Test fetching AND/OR/XOR atomics in arena

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/bpf: uaf test silently passes on ARM64 instead of skipping
--

commit a4f52eddc0e670a7a41a5b1939916ca84274f849
Author: Puranjay Mohan <puranjay@kernel.org>

selftests/bpf: Test fetching AND/OR/XOR atomics in arena
    
The arena and/or/xor tests never exercised the fetching form of the
instruction. Commit 2897b1e2a2f4 ("selftests/bpf: Fix arena_atomics
failure due to llvm change") deliberately switched them to
__c11_atomic_fetch_*() with memory_order_relaxed, which clang lowers to a
non-fetching locked instruction when the result is unused, because
x86-64 could not JIT the fetching one against an arena.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_atomics.c b/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
> --- a/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_atomics.c
[ ... ]
> @@ -160,6 +199,18 @@ static void test_uaf(struct arena_atomics *skel)
>  		return;
>  
>  	ASSERT_EQ(skel->arena->uaf_recovery_fails, 0, "uaf_recovery_fails");
> +
> +#ifndef __powerpc64__
> +	/*
> +	 * A fault clears the fetched register and leaves the address alone. The
> +	 * powerpc64 JIT does not clear the register of a faulting atomic yet.
> +	 */
> +	ASSERT_EQ(skel->arena->uaf_r1_after_dst_r0, 0, "uaf_r1_after_dst_r0");
> +	ASSERT_EQ(skel->arena->uaf_r0_after_dst_r0, skel->arena->uaf_r0_before_dst_r0,
> +		  "uaf_r0_after_dst_r0");
> +	ASSERT_EQ(skel->arena->uaf_r0_after_src_r0, 0, "uaf_r0_after_src_r0");
> +	ASSERT_EQ(skel->arena->uaf_reread, 0, "uaf_reread");
> +#endif
>  }

[Severity: Low]
Does this test silently pass on ARM64?

In the uaf program in arena_atomics.c, the fault testing logic is compiled
out when __TARGET_ARCH_arm64 is defined:

#if defined(ENABLE_ATOMICS_TESTS) && !defined(__TARGET_ARCH_arm64) && \
	defined(__BPF_FEATURE_ADDR_SPACE_CAST)

Because the BPF program does nothing on ARM64, the global variables
(uaf_r1_after_dst_r0, uaf_reread, etc.) remain untouched and hold their
zero-initialized default values. The host runner here unconditionally
asserts that these variables equal 0, which makes the assertions
tautologically succeed on ARM64.

Could the BPF program export a flag so the userspace runner can explicitly call
test__skip() when the test is unsupported, instead of silently passing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016171319.1623447-1-puranjay@kernel.org?part=2