Re: bpf: RFC for platform specific BPF helper addition

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2023-02-28 at 11:45 +0200, Tero Kristo wrote:
> On 25/02/2023 02:01, Alexei Starovoitov wrote:
> > On Fri, Feb 24, 2023 at 3:49 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> > > 
> > > On 23/02/2023 19:46, Alexei Starovoitov wrote:
> > > > On Thu, Feb 23, 2023 at 5:23 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> > > > > Hi,
> > > > > 
> > > > > Some background first; on x86 platforms there is a free running TSC
> > > > > counter which can be used to generate extremely accurate profiling time
> > > > > stamps. Currently this can be used by BPF programs via hooking into perf
> > > > > subsystem and reading the value there; however this reduces the accuracy
> > > > > due to latency + jitter involved with long execution chain, and also the
> > > > > timebase gets converted into relative from the start of the execution of
> > > > > the program, instead of getting an absolute system level value.
> > > > Are you talking about rdtsc or some other counter?
> > > > Does it need an arch specific setup?
> > > Yes, this is rdtsc. TSC is setup automatically by the arch, but
> > > exporting it to BPF takes a few lines of arch specific code (I did use
> > > register_btf_kfunc_id_set() during init, under arch/x86/kernel/tsc.c.)
> > > > > Now, I do have a pretty trivial patch (under internal review atm. at
> > > > > Intel) that adds an x86 platform specific bpf helper that can directly
> > > > > read this timestamp counter without relying to perf subsystem hooks.
> > > > > 
> > > > > Do people have any feedback / insights on this list about addition of
> > > > > such platform specific BPF helper, basically thumbs up/down for adding
> > > > > such a thing? Currently I don't think there are any platform specific
> > > > > helpers in the kernel.
> > > > Right. That's one of the reasons we don't add new helpers anymore.
> > > > Please use kfunc instead. You can add it to:
> > > > arch/x86/net/bpf_jit_comp.c
> > > > like:
> > > > __bpf_kfunc u64 bpf_read_rdtsc(void)
> > > > { asm ("...
> > > > or to arch specific kernel module.
> > > > 
> > > > Make sure to add selftests when you submit a patch.
> Regarding this I got a follow up question, where would you recommend to 
> put selftests for such functionality? Any of the BPF selftests appear to 
> be generic currently.

Hi Tero,

You can take a look at these:
- tools/testing/selftests/bpf/prog_tests/get_func_ip_test.c
- tools/testing/selftests/bpf/prog_tests/test_bpf_syscall_macro.c

Pre-processor conditionals are used in both cases:

  #ifdef __x86_64__
  ...
  #endif

Thanks,
Eduard
> > > Ok, I can take a look at the selftest side if things nudge forward,
> > > however there is some internal pressure to ditch the whole idea of
> > > bpf_rdtsc() due to potential of side channel attacks by using BPF, and
> > > exploiting the accurate timer in the process. Any thoughts on that side?
> > > Using BPF requires root access nowadays so it is sort of on-par to
> > > out-of-tree kernel modules.
> > Can you elaborate on that security concern?
> > User space can do rdtsc, so not clear how doing the same in bpf prog
> > loaded by root makes any difference.
> > Unpriv bpf is pretty much non-existent.
> > bpf subsystem went root only long ago.
> 
> I am working on this internally with our security team atm., and the 
> initial assessment is that the concern may not be valid due to the 
> points you mention also.
> 
> -Tero
>