Re: [PATCHv6 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 default `USDT_NOP` macro in `usdt.h` was not updated to `nop10`, causing `usdt_2.c` to compile with a 6-byte NOP and breaking the selftest.
- [Medium] Libbpf's USDT attachment logic is unaware of `nop10`, preventing the IP shift for optimized uprobes and causing them to fall back to unoptimized traps.
- [Medium] The `check_detach` function only verifies that the first 5 bytes of the `nop10` instruction are restored upon uprobe detachment, leaving the remaining 5 bytes unverified.
--

commit b3e7e5fee8889d2f95596f44b09c53ec9e3d2d6c
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/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]
Should check_detach() verify the full 10 bytes?

Since the optimized uprobe modifies a 10-byte instruction sequence, only 
verifying the first 5 bytes might leave incomplete detachments unverified. If 
the detachment logic fails to correctly restore the last 5 bytes, it could lead
to a corrupted memory access when executing the NOP.

> 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
[ ... ]
> @@ -328,8 +328,13 @@ static void subtest_optimized_attach(void)
>  	/* nop stays on addr_2 address */
>  	ASSERT_EQ(*addr_2, 0x90, "nop");
>  
> -	/* call is on addr_2 + 1 address */
> -	ASSERT_EQ(*(addr_2 + 1), 0xe8, "call");
> +	/*
> +	 * lea -0x80(%rsp), %rsp
> +	 * call ...
> +	 */
> +	static unsigned char expected[] = { 0x48, 0x8d, 0x64, 0x24, 0x80, 0xe8 };
> +
> +	ASSERT_MEMEQ(addr_2 + 1, expected, sizeof(expected), "lea_and_call");

[Severity: Medium]
Does libbpf need to be updated to recognize the new nop10 sequence?

Looking at has_nop_combo() in tools/lib/bpf/usdt.c, it still explicitly
checks for the 6-byte nop combo:

    unsigned char nop_combo[6] = {
        0x90, 0x0f, 0x1f, 0x44, 0x00, 0x00 /* nop,nop5 */
    };

If libbpf encounters the new nop10 sequence, it won't advance the instruction
pointer and will fall back to an unoptimized trap instead of using lea and
call. This would cause the ASSERT_MEMEQ here to fail.

> 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]
Does tools/testing/selftests/bpf/usdt.h need to be updated to define the 
new 11-byte sequence for USDT_NOP?

The test in prog_tests/usdt.c searches for nop1_nop10_combo:

    addr_2 = find_instr(usdt_2, nop1_nop10_combo, 11);
    if (!ASSERT_OK_PTR(addr_2, "usdt_2_find_nop1_nop10_combo"))
        return;

Because USDT_NOP in usdt.h was not modified in this patch, it still 
generates the old 6-byte sequence. This will cause find_instr() to return 
NULL, resulting in the test failing.

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