Re: [PATCH bpf-next v9 1/4] bpf: add bpf_get_cpu_time_counter kfunc

[Thomas Gleixner <tglx@linutronix.de>]

On Fri, Nov 22 2024 at 16:58, Vadim Fedorenko wrote:
> diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
> index de0f9e5f9f73..a549aea25f5f 100644
> --- a/arch/x86/net/bpf_jit_comp32.c
> +++ b/arch/x86/net/bpf_jit_comp32.c
> @@ -2094,6 +2094,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
>  			if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
>  				int err;
>  
> +				if (imm32 == BPF_CALL_IMM(bpf_get_cpu_time_counter)) {
> +					if (cpu_feature_enabled(X86_FEATURE_LFENCE_RDTSC))
> +						EMIT3(0x0F, 0xAE, 0xE8);
> +					EMIT2(0x0F, 0x31);

What guarantees that RDTSC is supported by the CPU?

Aside of that, if you want the read to be ordered, then you need to take
RDTSCP into account too.

> +#if IS_ENABLED(CONFIG_GENERIC_GETTIMEOFDAY)
> +__bpf_kfunc u64 bpf_get_cpu_time_counter(void)
> +{
> +	const struct vdso_data *vd = __arch_get_k_vdso_data();
> +
> +	vd = &vd[CS_RAW];
> +
> +	/* CS_RAW clock_mode translates to VDSO_CLOCKMODE_TSC on x86 and

How so?

vd->clock_mode is not guaranteed to be VDSO_CLOCKMODE_TSC or
VDSO_CLOCKMODE_ARCHTIMER. CS_RAW is the access to the raw (uncorrected)
time of the current clocksource. If the clock mode is not matching, then
you cannot access it.

> +	 * to VDSO_CLOCKMODE_ARCHTIMER on aarch64/risc-v. We cannot use
> +	 * vd->clock_mode directly because it brings possible access to
> +	 * pages visible by user-space only via vDSO.

How so? vd->clock_mode is kernel visible.

>        * But the constant value
> +	 * of 1 is exactly what we need - it works for any architecture and
> +	 * translates to reading of HW timecounter regardles of architecture.

It does not. Care to look at MIPS?

> +	 * We still have to provide vdso_data for some architectures to avoid
> +	 * NULL pointer dereference.
> +	 */
> +	return __arch_get_hw_counter(1, vd);

This is outright dangerous. __arch_get_hw_counter() is for VDSO usage
and not for in kernel usage. What guarantees you that the architecture
specific implementation does not need access to user only mappings.

Aside of that what guarantees that '1' is what you want and stays that
way forever? It's already broken on MIPS.

Thanks,

        tglx