Re: [PATCH bpf v3 2/2] selftests/bpf: Test r0 and ref lifetime after BPF-BPF call with abnormal return

["Arthur Fabre" <afabre@cloudflare.com>]

On Mon Jan 6, 2025 at 9:34 PM CET, Eduard Zingerman wrote:
> On Mon, 2025-01-06 at 18:15 +0100, Arthur Fabre wrote:
[...]
> > +#define TEST(NAME, CALLEE) \
> > +	SEC("socket")					\
> > +	__description("r0: " #NAME)	\
> > +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> > +	__naked int check_abnormal_ret_r0_##NAME(void)	\
> > +	{						\
> > +		asm volatile("				\
> > +		r6 = r1;				\
> > +		r2 = r10;				\
> > +		r2 += -8;				\
> > +		call " #CALLEE ";			\
> > +		r6 += r0;				\
> > +		r0 = 0;					\
> > +		exit;					\
> > +	"	:					\
> > +		:					\
> > +		: __clobber_all);			\
> > +	}						\
> > +							\
> > +	SEC("socket")					\
> > +	__description("ref: " #NAME)	\
> > +	__failure __msg("math between ctx pointer and register with unbounded min value") \
> > +	__naked int check_abnormal_ret_ref_##NAME(void)	\
> > +	{						\
> > +		asm volatile("				\
> > +		r6 = r1;				\
> > +		r7 = r10;				\
> > +		r7 += -8;				\
> > +		r2 = r7;				\
> > +		call " #CALLEE ";			\
> > +		r0 = *(u64*)(r7 + 0);			\
> > +		r6 += r0;				\
> > +		exit;					\
> > +	"	:					\
> > +		:					\
> > +		: __clobber_all);			\
> > +	}
>
> Nit: I think having both cases is an overkill, as both effectively
>      test if branching occur.

Fair enough, I can drop the reference tests.

> [...]
>
> > +struct {
> > +	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> > +	__uint(max_entries, 1);
> > +	__uint(key_size, sizeof(int));
> > +	__array(values, void(void));
> > +} map_prog SEC(".maps") = {
> > +	.values = {
> > +		[0] = (void *)&dummy_prog,
> > +	},
> > +};
> > +
> > +static __noinline __used
> > +int callee_tail_call(struct __sk_buff *skb, __u64 *foo)
> > +{
> > +	bpf_tail_call(skb, &map_prog, 0);
> > +	*foo = 1;
> > +	return 0;
> > +}
>
> Nit: I'd also add a test where invalid action is taken
>      after bpf_tail_call inside the callee,
>      just to make sure that both branches are explored.

Good idea, I'll add that in and resend. Thanks for the feedback!

>
> > +
> > +SEC("socket")
> > +__description("r0 not set by tail_call")
> > +__failure __msg("R0 !read_ok")
> > +int check_abnormal_ret_tail_call_fail(struct __sk_buff *skb)
> > +{
> > +	return bpf_tail_call(skb, &map_prog, 0);
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";