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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch updates test expectations to look for an 11-byte NOP sequence but fails to update the default `USDT_NOP` in `usdt.h`, causing a guaranteed test failure.
- [Medium] `check_detach()` only verifies the first 5 bytes of the restored 10-byte NOP instruction, potentially masking an instruction stream corruption bug in the kernel.
--

commit 6e149e271696edf6debf9408dedc04512556082d
Author: Jiri Olsa <jolsa@kernel.org>
Subject: selftests/bpf: Change uprobe syscall tests to use nop10

This patch updates the uprobe syscall tests to reflect that optimized uprobes
now operate on top of 10-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..0bb37435ce97b 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 unsigned char nop10_prefix[] = { 0x66, 0x2e, 0x0f, 0x1f, 0x84 };
> +
>  	/* [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");

[Severity: Medium]
Does checking only the first 5 bytes mask potential kernel-side unpatching bugs?

The optimized uprobe logic patches a 10-byte sequence, but check_detach() only
verifies the first 5 bytes of the restored instruction. If the kernel's
unpatching logic fails to restore the remaining 5 bytes, they will still
contain part of the call instruction, leaving the execution stream corrupted.

Could we verify the full 10 bytes to ensure the instruction is correctly
and completely restored?

> 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
[ ... ]
> @@ -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, 11);
> +	if (!ASSERT_NULL(addr_1, "usdt_1_find_nop1_nop10_combo"))
>  		return;
>  
>  	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, 11);
> +	if (!ASSERT_OK_PTR(addr_2, "usdt_2_find_nop1_nop10_combo"))

[Severity: Medium]
Will this test fail because usdt.h wasn't updated?

It appears the default USDT_NOP in tools/testing/selftests/bpf/usdt.h for
x86_64 is still defined as a 6-byte instruction:

#elif defined(__x86_64__)
#define USDT_NOP                       .byte 0x90, 0x0f, 0x1f, 0x44, 0x00, 0x0 /* nop, nop5 */

Since usdt_2.c relies on this default macro, won't it still compile with the old
6-byte sequence, causing this find_instr() check for 11 bytes to return NULL?

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