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

[sashiko-bot@kernel.org]

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.

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

> +	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.

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

> +	if (!ASSERT_OK_PTR(addr_2, "usdt_2_find_nop1_nop10_combo"))
>  		return;

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