Re: [PATCH 4/7] selftests/bpf: Change uprobe syscall tests to use nop10

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, May 14, 2026 at 08:51:36PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Multiple test validation checks use hardcoded, incorrect byte lengths when comparing instruction sequences, leaving trailing bytes unverified.
> --
> 
> commit ca88e95623000540d5b706ce364d5b5abf75b4c1
> Author: Jiri Olsa <jolsa@kernel.org>
> 
> selftests/bpf: Change uprobe syscall tests to use nop10
> 
> This commit updates the uprobe syscall tests to reflect that optimized
> uprobes are now placed on top of 10-byte nop instructions rather than
> 5-byte nop instructions.
> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > index 955a37751b52d..c2e9e549c737d 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c
> [ ... ]
> > @@ -388,14 +388,16 @@ static int find_uprobes_trampoline(void *tramp_addr)
> [ ... ]
> > -static void *find_nop5(void *fn)
> > +static void *find_nop10(void *fn)
> >  {
> >  	int i;
> >  
> > -	for (i = 0; i < 10; i++) {
> > -		if (!memcmp(nop5, fn + i, 5))
> > +	for (i = 0; i < 128; i++) {
> > +		if (!memcmp(nop10, fn + i, 9))
> 
> Is this memcmp() checking enough bytes? The nop10 sequence is 10 bytes long,
> but this only compares the first 9 bytes, leaving the final byte unverified.

yep, should be 10.. leftover from nop9 infatuation

> 
> >  			return fn + i;
> >  	}
> >  	return NULL;
> > @@ -420,7 +422,8 @@ static void *check_attach(struct uprobe_syscall_executed *skel, trigger_t trigge
> >  	ASSERT_EQ(skel->bss->executed, executed, "executed");
> >  
> >  	/* .. and check the trampoline is as expected. */
> > -	call = (struct __arch_relative_insn *) addr;
> > +	ASSERT_OK(memcmp(addr, lea_rsp, 4), "lea_rsp");
> 
> Does this memcmp() skip verifying the displacement byte? The lea_rsp array
> is 5 bytes long, and the 0x80 (-128) byte proves the red zone is bypassed
> correctly. Comparing only 4 bytes seems to miss this detail.

should be 5

> 
> > +	call = (struct __arch_relative_insn *)(addr + 5);
> >  	tramp = (void *) (call + 1) + call->raddr;
> >  	ASSERT_EQ(call->op, 0xe8, "call");
> >  	ASSERT_OK(find_uprobes_trampoline(tramp), "uprobes_trampoline");
> [ ... ]
> > diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
> > index 69759b27794d7..be34c4087ff56 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
> [ ... ]
> > @@ -271,17 +271,17 @@ static void subtest_optimized_attach(void)
> >  	__u8 *addr_1, *addr_2;
> >  
> >  	/* usdt_1 USDT probe has single nop instruction */
> > -	addr_1 = find_instr(usdt_1, nop1_nop5_combo, 6);
> > -	if (!ASSERT_NULL(addr_1, "usdt_1_find_nop1_nop5_combo"))
> > +	addr_1 = find_instr(usdt_1, nop1_nop10_combo, 6);
> 
> Should this length parameter be updated to match the new array size? The
> nop1_nop10_combo array was increased to 11 bytes, but find_instr() is still
> being called with length 6.

should be 11

> 
> >  	addr_1 = find_instr(usdt_1, nop1, 1);
> >  	if (!ASSERT_OK_PTR(addr_1, "usdt_1_find_nop1"))
> >  		return;
> >  
> > -	/* usdt_2 USDT probe has nop,nop5 instructions combo */
> > -	addr_2 = find_instr(usdt_2, nop1_nop5_combo, 6);
> > -	if (!ASSERT_OK_PTR(addr_2, "usdt_2_find_nop1_nop5_combo"))
> > +	/* usdt_2 USDT probe has nop,nop10 instructions combo */
> > +	addr_2 = find_instr(usdt_2, nop1_nop10_combo, 6);
> 
> Could this cause the test to miss verifying the full sequence? It looks like
> it is still passing 6 instead of 11 for the updated nop1_nop10_combo array.

should be 11, will fix

jirka