Re: [PATCH bpf-next v2 3/5] selftests/bpf: XOR and OR range computation tests.

[Cupertino Miranda <cupertino.miranda@oracle.com>]

Eduard Zingerman writes:

> On Wed, 2024-04-17 at 13:23 +0100, Cupertino Miranda wrote:
>
> [...]
>
>> +SEC("socket")
>> +__description("bounds check for reg32 <= 1, 0 xor (0,1)")
>> +__success __failure_unpriv
>> +__msg_unpriv("R0 min value is outside of the allowed memory range")
>> +__retval(0)
>> +__naked void t_0_xor_01(void)
>> +{
>> +	asm volatile ("					\
>> +	call %[bpf_get_prandom_u32];                    \
>> +	r6 = r0;                                        \
>> +	r1 = 0;						\
>> +	*(u64*)(r10 - 8) = r1;				\
>> +	r2 = r10;					\
>> +	r2 += -8;					\
>> +	r1 = %[map_hash_8b] ll;				\
>> +	call %[bpf_map_lookup_elem];			\
>> +	if r0 != 0 goto l0_%=;				\
>> +	exit;						\
>> +l0_%=:	w1 = 0;						\
>> +	r6 >>= 63;					\
>> +	w1 ^= w6;					\
>> +	if w1 <= 1 goto l1_%=;				\
>> +	r0 = *(u64*)(r0 + 8);				\
>> +l1_%=:	r0 = 0;						\
>> +	exit;						\
>> +"	:
>> +	: __imm(bpf_map_lookup_elem),
>> +	  __imm_addr(map_hash_8b),
>> +	  __imm(bpf_get_prandom_u32)
>> +	: __clobber_all);
>> +}
>> +
>
> I think that this test case (and one below) should be simplified,
> e.g. as follows:
>
> SEC("socket")
> __success __log_level(2)
> __msg("5: (af) r0 ^= r6                      ; R0_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=255,var_off=(0x0; 0xff))")
> __naked void non_const_xor_src_dst(void)
> {
> 	asm volatile ("					\
> 	call %[bpf_get_prandom_u32];                    \
> 	r6 = r0;					\
> 	call %[bpf_get_prandom_u32];                    \
> 	r6 &= 0xff;					\
> 	r0 &= 0x0f;					\
> 	r0 ^= r6;					\
> 	exit;						\
> "	:
> 	: __imm(bpf_map_lookup_elem),
> 	  __imm_addr(map_hash_8b),
> 	  __imm(bpf_get_prandom_u32)
> 	: __clobber_all);
> }
>
> Patch #2 allows verifier to compute dst range for xor operation with
> non-constant src and dst registers, which is exactly what checked when
> verifier log for instruction "r0 ^= r6" is verified.
> Manipulations with maps, unpriv behavior and retval are just a distraction.

Thanks for the suggestion.
I could not make it fail in the past without that control-flow in the
end of the test. I will try this.