bpf: RFC for platform specific BPF helper addition

bpf: RFC for platform specific BPF helper addition

[Tero Kristo]

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.

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.

-Tero

Re: bpf: RFC for platform specific BPF helper addition

[Alexei Starovoitov]

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?

> 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.

Re: bpf: RFC for platform specific BPF helper addition

[Tero Kristo]

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.

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.

-Tero

Re: bpf: RFC for platform specific BPF helper addition

[Alexei Starovoitov]

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.
>
> 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.

Re: bpf: RFC for platform specific BPF helper addition

[Tero Kristo]

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.
>> 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

Re: bpf: RFC for platform specific BPF helper addition

[Alexei Starovoitov]

On Tue, Feb 28, 2023 at 1:45 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >>> 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.

There are arch specific selftests.
They can be skipped on other archs either via DENYLIST or at runtime.

Re: bpf: RFC for platform specific BPF helper addition

[Eduard Zingerman]

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
>