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

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, May 26, 2026 at 09:15:17PM +0000, sashiko-bot@kernel.org wrote:
> 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?

it's changed in following patch that renames the related benchmarks
  selftests/bpf: Change uprobe/usdt trigger bench code to use nop10

> 
> [ ... ]
> 
> > 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?

will change

> 
> 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?

the last 5 bytes is the call instruction that is always different

> 
> >  	/* [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.

all those changes are in separate cpatches for easier review (as I thought..)
I can squash them if that's what we want

jirka