Re: [PATCH bpf-next v3 10/12] selftests/bpf: Cover the low-32 link for narrowing stack fills

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] selftests/bpf: misplaced ifdef in sext_byte_fill_drops_sext_link()
--

commit 0f3b5c981ef84ecf43731c167adb9d7addf744eb
Author: Vineet Gupta <vineet.gupta@linux.dev>

selftests/bpf: Cover the low-32 link for narrowing stack fills

This commit adds eight BPF verifier selftests covering low-32 scalar link
tracking across narrowing and sign-extending stack fills, and introduces a
CAN_USE_LDSX macro in bpf_misc.h to guard tests that use sign-extending
loads.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> index 69efe1381eca9..ab78285153fb1 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_linked_scalars.c
[ ... ]
> @@ -1494,6 +1591,215 @@ __naked void sext_drops_stale_step(void)
>  	: __clobber_all);
>  }
[ ... ]
> +/*
> + * BPF_REG_LINK_SEXT says the register is (s32)(base + delta) -- a 32-bit sign
> + * extension specifically. An 8- or 16-bit ldsx is a different relation, so
> + * the exemption that keeps a sign-extending link across a sign-extending
> + * load has to be gated on the load width. Ungated, r2 below claims to be
> + * (s32)r6 when it is really (s8)((s32)r6).
> + */
> +SEC("socket")
> +__success __log_level(2)
> +#ifdef CAN_USE_MOVSX
> +/*
> + * Needs MOVSX as well as LDSX: the BPF_REG_LINK_SEXT under test is built with
> + * r7 = (s32)r6, and only the byte fill below is an ldsx.
> + */
> +/* a surviving link would print as R2=scalar(id=N+0.sx,smin=... */
> +__msg("(91) r2 = *(s8 *)(r10 {{.*}} R2=scalar(smin=")
> +__naked void sext_byte_fill_drops_sext_link(void)
> +{
[ ... ]
> +}
> +#endif /* CAN_USE_MOVSX */
> +
> +#endif /* CAN_USE_LDSX */
> +
>  char _license[] SEC("license") = "GPL";

[Severity: Low]
Should SEC("socket") and __success __log_level(2) on
sext_byte_fill_drops_sext_link() be placed inside the #ifdef CAN_USE_MOVSX
block?

In tools/testing/selftests/bpf/progs/bpf_misc.h, CAN_USE_LDSX and
CAN_USE_MOVSX can be enabled independently via __BPF_FEATURE_LDSX and
__BPF_FEATURE_MOVSX (for example, with bpf-gcc -mldsx/-msmov flags).

If CAN_USE_LDSX is defined while CAN_USE_MOVSX is not defined, the body of
the #ifdef CAN_USE_MOVSX block is preprocessed out, leaving those attributes
dangling so they attach to the following _license declaration (or to any test
function added after this block).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020161907.2115379-1-vineet.gupta@linux.dev?part=10