Re: [PATCHv4 09/13] selftests/bpf: Change uprobe syscall tests to use nop10

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The usdt.h header was not updated to redefine the default USDT_NOP to 10 bytes, which will cause the usdt test to reliably fail.
- [Medium] check_detach() only verifies the restoration of the first 5 bytes of the 10-byte NOP and uses a signed char array that can cause compiler warnings.
- [Medium] Benchmark functions and external reporting names were not updated to reflect the new 10-byte NOP behavior, producing misleading metrics.
--

commit d8fb3aec4d3eab73b6231c402e78efd3cd47f8f0
Author: Jiri Olsa <jolsa@kernel.org>

selftests/bpf: Change uprobe syscall tests to use nop10

Optimized uprobes are now on top of 10-bytes nop instructions,
reflect that in existing tests.

> diff --git a/tools/testing/selftests/bpf/benchs/bench_trigger.c b/tools/testing/selftests/bpf/benchs/bench_trigger.c
> index 2f22ec61667b9..a60b8173cdc46 100644
> --- a/tools/testing/selftests/bpf/benchs/bench_trigger.c
> +++ b/tools/testing/selftests/bpf/benchs/bench_trigger.c
> @@ -398,7 +398,7 @@ static void *uprobe_producer_ret(void *input)
>  #ifdef __x86_64__
>  __nocf_check __weak void uprobe_target_nop5(void)
>  {
> -	asm volatile (".byte 0x0f, 0x1f, 0x44, 0x00, 0x00");
> +	asm volatile (".byte 0x66, 0x2e, 0x0f, 0x1f, 0x84, 0x00, 0x00, 0x00, 0x00, 0x00");
>  }

[Severity: Medium]
This isn't a bug, but the function name uprobe_target_nop5 and the associated
reporting names in the benchmark registry (like uprobe-nop5) still use the
nop5 suffix. Since they now use a 10-byte NOP, should these be updated to
nop10 to avoid misleading benchmark metrics?

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> index 955a37751b52d..90851d1e4624f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
[ ... ]
> @@ -430,9 +432,11 @@ static void *check_attach(struct uprobe_syscall_executed *skel, trigger_t trigge
>  
>  static void check_detach(void *addr, void *tramp)
>  {
> +	static const char nop10_prefix[] = { 0x66, 0x2e, 0x0f, 0x1f, 0x84 };
> +

[Severity: Medium]
Is char the correct type here? The value 0x84 exceeds the maximum positive
value for a signed char, which can trigger compiler warnings on architectures
where char is signed. Could this be changed to unsigned char instead?

Also, the optimized uprobe attachment replaces the full 10-byte NOP. Does
verifying only the first 5 bytes adequately test that the detach correctly
restored the original instruction stream?

>  	/* [uprobes_trampoline] stays after detach */
>  	ASSERT_OK(find_uprobes_trampoline(tramp), "uprobes_trampoline");
> -	ASSERT_OK(memcmp(addr, nop5, 5), "nop5");
> +	ASSERT_OK(memcmp(addr, nop10_prefix, 5), "nop10_prefix");
>  }

[ ... ]

> diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
> index 69759b27794d7..fda3a298ccfc7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
> @@ -252,7 +252,7 @@ extern void usdt_1(void);
>  extern void usdt_2(void);
>  
>  static unsigned char nop1[1] = { 0x90 };
> -static unsigned char nop1_nop5_combo[6] = { 0x90, 0x0f, 0x1f, 0x44, 0x00, 0x00 };
> +static unsigned char nop1_nop10_combo[11] = { 0x90, 0x66, 0x2e, 0x0f, 0x1f, 0x84, 0x00, 0x00, 0x00, 0x00, 0x00 };

[ ... ]

> diff --git a/tools/testing/selftests/bpf/usdt_2.c b/tools/testing/selftests/bpf/usdt_2.c
> index 789883aaca4c3..b359b389f6c00 100644
> --- a/tools/testing/selftests/bpf/usdt_2.c
> +++ b/tools/testing/selftests/bpf/usdt_2.c
> @@ -3,7 +3,7 @@
>  #if defined(__x86_64__)
>  
>  /*
> - * Include usdt.h with default nop,nop5 instructions combo.
> + * Include usdt.h with default nop,nop10 instructions combo.
>   */
>  #include "usdt.h"

[Severity: Medium]
It appears tools/testing/selftests/bpf/usdt.h was not updated in this patch
to redefine USDT_NOP to 10 bytes for x86_64.

Since usdt_2.c includes usdt.h to build its probes, won't it still compile
with the old 6-byte sequence? This would cause find_instr() in usdt.c to fail
to locate nop1_nop10_combo and result in test failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526205840.173790-10-jolsa@kernel.org?part=1