Re: [PATCH bpf-next 1/3] selftests/bpf: Keep int return type for tailcall subprogs with maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The BPF array map `array` defines its value type as `__u64` (8 bytes), but the code passes pointers to local `int` variables (4 bytes) as the map value to `bpf_map_update_elem()`. This causes an out-of-bounds stack read by the BPF helper, leading to either BPF verifier rejection or incorrect map state.
--

commit 638a9e186d9da40d2e394848eda6a546f07a86fd
Author: Yonghong Song <yonghong.song@linux.dev>

selftests/bpf: Keep int return type for tailcall subprogs with maps

This commit prevents LLVM23 from changing the caller's return type in BPF
tailcalls by using a barrier_var. It also switches to using an Array map to
store subprog return values to preserve test results.

Link: https://github.com/llvm/llvm-project/pull/198426

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

[ ... ]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_ARRAY);
> +	__uint(max_entries, 2);
> +	__type(key, int);
> +	__type(value, __u64);
> +} array SEC(".maps");

[ ... ]

>  SEC("tc")
>  int entry(struct __sk_buff *skb)
>  {
> -	int ret = 1;
> +	int ret = 1, ret1, ret2, key1 = 0, key2 = 1;
>  
>  	clobber_regs_stack();
>  
>  	count++;
> -	subprog_tail(skb);
> -	subprog_tail(skb);
> +	ret1 = subprog_tail(skb);
> +	ret2 = subprog_tail(skb);
> +
> +	bpf_map_update_elem(&array, &key1, &ret1, 0);
> +	bpf_map_update_elem(&array, &key2, &ret2, 0);

[Severity: Medium]
Will the BPF verifier reject these bpf_map_update_elem() calls, or
will they read out-of-bounds on the stack?

The array map defines its value size as __u64 (8 bytes), but ret1 and
ret2 are declared as 32-bit int variables (4 bytes). When passed into
bpf_map_update_elem(), the helper will read 8 bytes starting from the
pointers to the 4-byte integers, potentially reading uninitialized
adjacent stack memory.

>  
>  	return ret;
>  }

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

[ ... ]

> +struct {
> +        __uint(type, BPF_MAP_TYPE_ARRAY);
> +        __uint(max_entries, 2);
> +        __type(key, int);
> +        __type(value, __u64);
> +} array SEC(".maps");

[ ... ]

>  SEC("fentry/dummy")
>  int BPF_PROG(fentry, struct sk_buff *skb)
>  {
> +	int key1 = 0, key2 = 1, ret1, ret2;
> +
>  	clobber_regs_stack();
>  
>  	count++;
> -	subprog_tail(ctx);
> -	subprog_tail(ctx);
> +	ret1 = subprog_tail(ctx);
> +	ret2 = subprog_tail(ctx);
> +	bpf_map_update_elem(&array, &key1, &ret1, 0);
> +	bpf_map_update_elem(&array, &key2, &ret2, 0);

[Severity: Medium]
Are ret1 and ret2 causing the same 4-byte stack read for an 8-byte map value
here in the fentry program?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609163947.1717694-1-yonghong.song@linux.dev?part=1