Re: [PATCH bpf-next v13 4/6] selftests/bpf: add selftest to check bpf_get_cpu_time_counter jit

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] The selftests rigidly enforce a broken x86-64 JIT implementation for `bpf_cpu_time_counter_to_ns()`.
- [Medium] Fragile tests dependent on hardware features will cause CI flakiness.
- [Low] Mismatched and inaccurate comments due to copy-pasting.
--

commit 01407c2c12560c811a29d35f221844615da2962d
Author: Vadim Fedorenko <vadim.fedorenko@linux.dev>

selftests/bpf: add selftest to check bpf_get_cpu_time_counter jit

This commit adds selftests for the JIT implementations of
bpf_get_cpu_time_counter() and bpf_cpu_time_counter_to_ns() on x86_64 and
arm64. It verifies the emitted assembly using __jited and __xlated macros.

> diff --git a/tools/testing/selftests/bpf/progs/verifier_cpu_cycles.c b/tools/testing/selftests/bpf/progs/verifier_cpu_cycles.c
> new file mode 100644
> index 0000000000000..26c02010ccf1f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/verifier_cpu_cycles.c
[ ... ]
> +SEC("syscall")
> +__arch_x86_64
> +/* program entry for bpf_rdtsc_jit_x86_64(), regular function prologue */
> +__jited("	endbr64")
> +__jited("	nopl	(%rax,%rax)")
> +__jited("	nopl	(%rax)")
> +__jited("	pushq	%rbp")
> +__jited("	movq	%rsp, %rbp")
> +__jited("	endbr64")
> +/* save RDX in R11 as it will be overwritten */
> +__jited("	movq	%rdx, %r11")
> +/* lfence may not be executed depending on cpu features */
> +__jited("	{{(lfence|)}}")
> +__jited("	rdtsc")

Will this test deterministically fail in environments where
sched_clock_stable() is false or the CPU lacks the TSC feature?

The test strictly asserts that rdtsc is emitted. However, the underlying
JIT falls back to a standard function call if those hardware conditions are
not met, which might cause test failures in some CI and virtualization
environments.

[ ... ]
> +SEC("syscall")
> +__arch_x86_64
> +/* program entry for bpf_rdtsc_jit_x86_64(), regular function prologue */

This isn't a bug, but does this comment reference the wrong function name
due to a copy-paste error?

> +__jited("	endbr64")
> +__jited("	nopl	(%rax,%rax)")
> +__jited("	nopl	(%rax)")
> +__jited("	pushq	%rbp")
> +__jited("	movq	%rsp, %rbp")
> +__jited("	endbr64")
> +/* save RDX in R11 as it will be overwritten */
> +__jited("	movabsq	$0x2a2a2a2a2a, %rdi")

This isn't a bug, but the comment mentions saving RDX in R11, while the
following instruction does not touch either of those registers.

> +__jited("	imulq	${{.*}}, %rdi, %rax")
> +__jited("	shrq	${{.*}}, %rax")

Does enforcing this specific JIT sequence codify broken math for large cycle
counts?

The x86 imulq instruction computes a 64-bit product, truncating the upper
bits. Since cycle counts are 64-bit and multipliers can be large, their
product often requires 128 bits. The truncation would result in silent
wraparound.

Additionally, using a 32-bit immediate with imulq sign-extends the value to
64 bits. If the multiplier has its most significant bit set, it will be
treated as a large negative number, corrupting the result.

Finally, the emitted sequence does not add the offset. On arm64, the test
expects a no-op since the counter returns absolute nanoseconds, but on
x86-64 without the offset, absolute counter conversions will return
incorrect values. Does this test assert a cross-architecture inconsistency?

> +__jited("	leave")
> +__naked int bpf_cyc2ns_jit_x86(void)

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