Re: [PATCH RFC bpf-next 2/7] selftests/bpf: Test for empty intersection of tnum and u64

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] Fragile test design relies on verifier's ignorance of `bpf_get_prandom_u32()` bounds, risking silent test decay.
- [Low] Misplaced and factually incorrect inline comments misrepresent verifier state.
--

commit 8da3729bc39d37f67823aa031e7ba1b8b0d69fe0
Author: Paul Chaignon <paul.chaignon@gmail.com>

selftests/bpf: Test for empty intersection of tnum and u64

This patch adds a verifier selftest for the case where tnum and u64
don't intersect on a branch, allowing the verifier to deduce that the
branch is dead code.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_bounds.c b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> index c1ae013dee29c..151c7a479a6b8 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_bounds.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_bounds.c
> @@ -2184,4 +2184,29 @@ __naked void tnums_equal_impossible_constant(void *ctx)

[ ... ]

> +SEC("socket")
> +__description("dead branch: tnum and u64 don't intersect on true branch")
> +__success
> +__naked void empty_tnum_u64_intersection(void *ctx)
> +{
> +	asm volatile("									\
> +	call %[bpf_get_prandom_u32];							\
> +	r6 = r0;									\
> +	r1 = 0xe00000002 ll;								\
> +	r2 = 0xf00000000 ll;								\
> +	if r6 s< r1 goto +2;								\

Will this test silently decay if the verifier improves its tracking of helper
return values? 

bpf_get_prandom_u32() returns a 32-bit unsigned integer. If the verifier is
updated to track that this helper returns a value bounded by 0xffffffff, it
will statically deduce that the condition r6 s< 0xe00000002 is always true. 

This would prune the fallthrough path as dead code, causing the test to
silently pass without ever exercising the complex tnum vs u64 intersection
logic it was explicitly created to test.

Could we use a true 64-bit unknown scalar, such as bpf_ktime_get_ns(), to
ensure the test remains robust?

> +	/* On fallthrough, r6's u64=[0xe00000002;0xf00000000] */			\
> +	/*                      tnum=(0xe00000000; 0x1ffffffff) */			\
> +	if r6 s> r2 goto +1;								\

Are these inline comments misplaced? 

The upper bound of 0xf00000000 is only established after the 
if r6 s> r2 goto +1 instruction falls through, not before it. 

> +	/* On fallthrough, r6's tnum=(0xe00000001; 0x100000000) */			\
> +	/* It doesn't intersect with the u64 so the condition is always false */	\
> +	if w6 == 1 goto +1;								\

Does this tnum state actually exist on the fallthrough? 

According to the verifier log in the commit message, the tnum state of
(0xe00000001; 0x100000000) only exists on the true branch of 
if w6 == 1 goto +1, not on the fallthrough. 

Describing the taken branch as a fallthrough and placing the state annotations
before the instructions that actually establish them might make the test code
confusing for maintainers trying to understand the exact verifier state at
each step.

> +	exit;										\
> +	r10 = 0;									\
> +	exit;										\
> +"	:
> +	: __imm(bpf_get_prandom_u32)
> +	: __clobber_all);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1776344897.git.paul.chaignon@gmail.com?part=2