Re: [PATCH bpf-next] selftests/bpf: fix also no-alu32 strobemeta selftest

[Yonghong Song <yhs@xxxxxx>]

On 11/1/21 4:01 PM, Andrii Nakryiko wrote:
> Previous fix aded bpf_clamp_umax() helper use to re-validate boundaries.
> While that works correctly, it introduces more branches, which blows up
> past 1 million instructions in no-alu32 variant of strobemeta selftests.
>
> Switching len variable from u32 to u64 also fixes the issue and reduces
> the number of validated instructions, so use that instead. Fix this
> patch and bpf_clamp_umax() removed, both alu32 and no-alu32 selftests
> pass.
>
> Fixes: 0133c20480b1 ("selftests/bpf: Fix strobemeta selftest regression")
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

This indeed is a better approach for workaround. It disabled offending
llvm optimization since "truncation" operation (64->32bit) is gone. BTW,
llvm patch https://reviews.llvm.org/D112938 is also landed in the
hope to fix the same issue to please the verifier.

Acked-by: Yonghong Song <yhs@xxxxxx>

> ---
>   tools/testing/selftests/bpf/progs/strobemeta.h | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/progs/strobemeta.h b/tools/testing/selftests/bpf/progs/strobemeta.h
> index 3687ea755ab5..60c93aee2f4a 100644
> --- a/tools/testing/selftests/bpf/progs/strobemeta.h
> +++ b/tools/testing/selftests/bpf/progs/strobemeta.h
> @@ -10,14 +10,6 @@
>   #include <linux/types.h>
>   #include <bpf/bpf_helpers.h>
>   
> -#define bpf_clamp_umax(VAR, UMAX)					\
> -	asm volatile (							\
> -		"if %0 <= %[max] goto +1\n"				\
> -		"%0 = %[max]\n"						\
> -		: "+r"(VAR)						\
> -		: [max]"i"(UMAX)					\
> -	)
> -
>   typedef uint32_t pid_t;
>   struct task_struct {};
>   
> @@ -366,7 +358,7 @@ static __always_inline uint64_t read_str_var(struct strobemeta_cfg *cfg,
>   					     void *payload)
>   {
>   	void *location;
> -	uint32_t len;
> +	uint64_t len;
>   
>   	data->str_lens[idx] = 0;
>   	location = calc_location(&cfg->str_locs[idx], tls_base);
> @@ -398,7 +390,7 @@ static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
>   	struct strobe_map_descr* descr = &data->map_descrs[idx];
>   	struct strobe_map_raw map;
>   	void *location;
> -	uint32_t len;
> +	uint64_t len;
>   	int i;
>   
>   	descr->tag_len = 0; /* presume no tag is set */
> @@ -421,7 +413,6 @@ static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
>   
>   	len = bpf_probe_read_user_str(payload, STROBE_MAX_STR_LEN, map.tag);
>   	if (len <= STROBE_MAX_STR_LEN) {
> -		bpf_clamp_umax(len, STROBE_MAX_STR_LEN);
>   		descr->tag_len = len;
>   		payload += len;
>   	}
> @@ -439,7 +430,6 @@ static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
>   		len = bpf_probe_read_user_str(payload, STROBE_MAX_STR_LEN,
>   					      map.entries[i].key);
>   		if (len <= STROBE_MAX_STR_LEN) {
> -			bpf_clamp_umax(len, STROBE_MAX_STR_LEN);
>   			descr->key_lens[i] = len;
>   			payload += len;
>   		}
> @@ -447,7 +437,6 @@ static __always_inline void *read_map_var(struct strobemeta_cfg *cfg,
>   		len = bpf_probe_read_user_str(payload, STROBE_MAX_STR_LEN,
>   					      map.entries[i].val);
>   		if (len <= STROBE_MAX_STR_LEN) {
> -			bpf_clamp_umax(len, STROBE_MAX_STR_LEN);
>   			descr->val_lens[i] = len;
>   			payload += len;
>   		}