Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] kselftests/uprobes: test unconditionally skips due to undefined FLT_V variable - [Medium] kselftests/uprobes: unaligned_nopfn site does not cross 8-byte boundary, invalidating test - [Low] kselftests/uprobes: 64-bit inline assembly causes build failure on 32-bit x86 --
commit af4e3ff18548d1ab18ec7d6c2b846c7b5c15b34e Author: Andi Kleen <[email protected]> Subject: ptwrite uprobes: Add kernel self tests This commit adds selftests for PTWRITE uprobes to exercise instruction punning, multinop patching, and probe lifecycle on x86 architectures. > diff --git a/tools/testing/selftests/uprobes/Makefile > b/tools/testing/selftests/uprobes/Makefile > new file mode 100644 > index 0000000000000..ee97442f8c9dc > --- /dev/null > +++ b/tools/testing/selftests/uprobes/Makefile > @@ -0,0 +1,14 @@ [ ... ] > +TEST_PROGS := run_ptw.sh > + > +ifneq ($(filter x86 x86_64,$(ARCH)),) > +TEST_GEN_FILES := ptw_probe > +else [Severity: Low] Will this fail to build on 32-bit x86 architectures? The Makefile enables test compilation for the x86 architecture filter, which includes 32-bit builds. However, ptw_probe.c uses strict 64-bit inline assembly like movabs, %rax, and %rsp. When compiled with a 32-bit toolchain, won't the assembler reject these instructions? > diff --git a/tools/testing/selftests/uprobes/ptw_probe.c > b/tools/testing/selftests/uprobes/ptw_probe.c > new file mode 100644 > index 0000000000000..b232fbd4d2aa0 > --- /dev/null > +++ b/tools/testing/selftests/uprobes/ptw_probe.c > @@ -0,0 +1,156 @@ [ ... ] > +static __attribute__((noipa)) uint64_t > +unaligned_nopfn(uint64_t a) > +{ > + asm volatile(".p2align 3, 0x90\n\t" > + "nop\n\t" > + ".globl nopfn_unaligned_site\n\t" > + "nopfn_unaligned_site:\n\t" > + "nop\n\tnop\n\tnop\n\tnop\n\tnop" ::: "memory"); > + return a * 13 + 5; > +} [Severity: Medium] Does this actually create an unaligned sequence that crosses an 8-byte boundary? The assembly code aligns to 8 bytes with .p2align 3 and adds a 1-byte nop, placing the start of the 5-byte target sequence at offset 1. Since offsets 1 through 5 fit perfectly within a single 8-byte word, the kernel's atomic patch path can handle it safely. Won't the kernel accept this probe instead of rejecting it, causing test 11 in run_ptw.sh to incorrectly report a failure when it expects the kernel to reject the probe? > diff --git a/tools/testing/selftests/uprobes/run_ptw.sh > b/tools/testing/selftests/uprobes/run_ptw.sh > new file mode 100755 > index 0000000000000..a16b7adeabb74 > --- /dev/null > +++ b/tools/testing/selftests/uprobes/run_ptw.sh > @@ -0,0 +1,219 @@ [ ... ] > +PUN_V=$(objdump -d "$BIN" | awk '/^[0-9a-f]+ <punfn>:/{print $1;exit}' | tr > -d ':') > +JCC_V=$(objdump -d "$BIN" | > + awk '/^[0-9a-f]+ <jcc8>:/ {f=1; next} f&&/jne/{print $1; exit}' | > + tr -d ':') > +NOP_V=$(objdump -d "$BIN" | awk '/^[0-9a-f]+ <nopfn_site>:/{print $1;exit}' > | tr -d ':') > +NOP5_V=$(objdump -d "$BIN" | awk '/^[0-9a-f]+ <nop5>:/{print $1;exit}' | tr > -d ':') > +UNOP_V=$(objdump -d "$BIN" | > + awk '/^[0-9a-f]+ <nopfn_unaligned_site>:/{print $1;exit}' | tr -d ':') > +RZ_V=$(objdump -d "$BIN" | awk '/^[0-9a-f]+ <rz_probe_site>:/{print > $1;exit}' | tr -d ':') > +if [ -z "$PUN_V" ] || [ -z "$JCC_V" ] || [ -z "$FLT_V" ] || > + [ -z "$NOP_V" ] || [ -z "$NOP5_V" ] || [ -z "$UNOP_V" ] || > + [ -z "$RZ_V" ]; then > + echo "1..0 # SKIP unable to resolve ptw_probe symbols" > + exit 0 > +fi [Severity: Medium] Is the FLT_V variable supposed to be defined or populated before this check? The script tests for the existence of an undefined variable FLT_V, which causes this condition to always evaluate to true. Doesn't this mean the entire ptwrite uprobe selftest unconditionally skips execution and reports 'unable to resolve ptw_probe symbols' on every run, resulting in no test coverage? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=11
