Re: [PATCH bpf-next v13 6/6] bpf, arm64: Add JIT support for cpu time counter kfuncs

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] Division by zero kernel panic during JIT compilation due to direct read of cntfrq_el0.
- [High] Implicit sign-extension of the 32-bit multiplier corrupts nanosecond conversions.
- [Medium] Unconditional JIT inlining of timer read bypasses hardware errata workarounds.
- [Medium] Silent 64-bit math overflow for duration measurements exceeding 1 hour.
--

commit f282b55d9d2af467b0a51264b002bf14ff3648fc
Author: Puranjay Mohan <puranjay@kernel.org>

    bpf, arm64: Add JIT support for cpu time counter kfuncs

This commit adds ARM64 JIT inlining for the bpf_get_cpu_time_counter() and
bpf_cpu_time_counter_to_ns() kfuncs. It emits instructions to read the
architected timer directly and perform the nanosecond conversion.

> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> index 0816c40fc7af9..7da7507ab4315 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1571,10 +1573,54 @@ static int build_insn(const struct bpf_verifier_env *env, const struct bpf_insn
[ ... ]
> +		/* Inline kfunc bpf_get_cpu_time_counter() */
> +		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> +		    imm == BPF_CALL_IMM(bpf_get_cpu_time_counter) &&
> +		    bpf_jit_inlines_kfunc_call(imm)) {
> +			/*
> +			 * With ECV (ARMv8.6+), CNTVCTSS_EL0 is self-
> +			 * synchronizing — no ISB needed.  Without ECV,
> +			 * an ISB is required before reading CNTVCT_EL0
> +			 * to prevent speculative/out-of-order reads.
> +			 *
> +			 * Matches arch_timer_read_cntvct_el0().
> +			 */
> +			if (cpus_have_cap(ARM64_HAS_ECV)) {
> +				emit(A64_MRS_CNTVCTSS_EL0(r0), ctx);
> +			} else {
> +				emit(A64_ISB, ctx);
> +				emit(A64_MRS_CNTVCT_EL0(r0), ctx);
> +			}
> +			break;
> +		}

Does this unconditional JIT inlining bypass the hardware errata workarounds?

The generic C implementation of bpf_get_cpu_time_counter() uses
ktime_get_raw_fast_ns(), which safely respects the out-of-line workaround
framework (CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND). By blindly emitting the
MRS CNTVCT_EL0 (or CNTVCTSS_EL0) instructions, could BPF programs receive
unstable and corrupted timestamps on affected hardware?

> +		/* Inline kfunc bpf_cpu_time_counter_to_ns() */
> +		if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
> +		    imm == BPF_CALL_IMM(bpf_cpu_time_counter_to_ns) &&
> +		    bpf_jit_inlines_kfunc_call(imm)) {
> +			u32 freq = arch_timer_get_cntfrq();

Could this cause a division-by-zero kernel panic during JIT compilation?

On platforms with broken firmware, the cntfrq_el0 register is often left
uninitialized (0), and the kernel relies on a device tree fallback.
If arch_timer_get_cntfrq() returns 0, will clocks_calc_mult_shift() below
execute a do_div(tmp, freq) and crash the system during BPF program load?

> +			if (freq == NSEC_PER_SEC) {
> +				/* 1 GHz counter: 1 tick = 1 ns, identity */
> +				emit(A64_MOV(1, r0, r1), ctx);
> +			} else {
> +				u32 mult, shift;
> +
> +				clocks_calc_mult_shift(&mult, &shift, freq, NSEC_PER_SEC, 3600);
> +				emit_a64_mov_i(1, tmp, mult, ctx);

Does emit_a64_mov_i() implicitly sign-extend the 32-bit multiplier?

The emit_a64_mov_i() function takes a signed s32 value as its argument.
For systems with slower timer frequencies, clocks_calc_mult_shift() can
compute a large unsigned 32-bit multiplier whose most significant bit is
set. Will this cause the unsigned multiplier to be implicitly cast to a
negative number and sign-extended to 64 bits, corrupting the subsequent
multiplication?

> +				emit(A64_MUL(1, r0, r1, tmp), ctx);

Will this simple 64-bit multiplication silently overflow for duration
measurements exceeding one hour?

Since clocks_calc_mult_shift() is called with maxsec = 3600 (1 hour), the
multiplier is calculated such that delta_ticks * mult fits within 64 bits
up to that duration. For longer durations, could this simple 64-bit
multiplication wrap around, returning falsely small or corrupted nanosecond
durations, unlike the 128-bit math used in the generic kernel timekeeping?

> +				if (shift)
> +					emit(A64_LSR(1, r0, r0, shift), ctx);
> +			}
> +			break;
> +		}

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