Re: [RFC 0/1] BPF tracing for arm64 using fprobe

[KP Singh]

On Fri, Nov 18, 2022 at 7:52 PM Chris Mason <clm@xxxxxxxx> wrote:
>
> On 11/18/22 1:06 PM, Steven Rostedt wrote:
> > On Fri, 18 Nov 2022 12:44:00 -0500
> > Chris Mason <clm@xxxxxxxx> wrote:
> >

(adding this back here)

> >>>> On Fri, 18 Nov 2022 16:34:50 +0000
> >>>> Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >>>> FWIW, given that the aim here seems to be to expose all kernel internals to be
> >>>> overridden arbitrarily, I'm also concerned that there's a huge surface area for
> >>>> issues with maintainability, robustness/correctness, and security.
> >>>>

This is not all kernel internals, just the functions allowed for error
injection.

> >>>> I really don't want to be stuck in a position where someone argues that all
> >>>> kernel internal functions are ABI and need to stay around as-is to be hooked by
> >>>> eBPF, and I hope that we all agree that there are no guarantees on that front.
> >>>>

Yes, BPF provides no guarantee that kernel functions will remain
stable (similar to tracepoints and kprobes).

> >>>> Thanks,
> >>>> Mark.
> >>>>
> >>> My biggest concern is changing functionality of arbitrary functions by BPF.
> >>> I would much rather limit what functions BPF could change with some
> >>> annotation.
> >>>
> >>> int __bpf_modify foo()
> >>> {
> >>>     ...
> >>> }

This annotation already exists, i.e. ALLOW_ERROR_INJECTION

Users, with CONFIG_FUNCTION_ERROR_INJECTION, can already modify return
values of kernel functions using kprobes and the failure injection
framework [1] for functions annotated with ALLOW_ERROR_INJECTION.

BPF just provides another way to do the same thing with "modify
return" programs and this also respects the error injection list [2]
and users can *only* attach these programs to the functions annotated
with ALLOW_ERROR_INJECTION.

[1] https://www.kernel.org/doc/Documentation/fault-injection/fault-injection.txt
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/bpf/verifier.c?id=f4c4ca70dedc1bce8e7b1648e652aa9be1d3fcd7#n14948

> >>>
> >>>
> >>> That way if somethings not working, you can see directly in the code that
> >>> the function could be modified by a BPF program, instead of getting some
> >>> random bug report because a function returned an unexpected result that the
> >>> code of that function could never produce.
> >>>
> >>
> >> The good news is that BPF generally confines the function replacement
> >> through struct ops interfaces.
> >
> > What struct ops interfaces?
>
> https://lwn.net/Articles/811631/
>
> >
> >>   There are also explicit allow lists to
> >> limit functions where you can do return value overrides etc, so I think
> >
> > Where are these lists.
>
> Some of the original features:
>
> https://lwn.net/Articles/811631/

I think you meant: https://lwn.net/Articles/740146/ ?

>
> It has changed and expanded since then, but hopefully you get the idea.
>
> >
> >> it's fair to say these concerns are already baked in.  I'm sure they can
> >
> > How do I know that a function return was modified by BPF? If I'm debugging

You can list the BPF programs that are loaded in the kernel with

# bpftool prog list

Also, the BPF programs show up in call stacks when you are debugging.

> > something, is it obvious to the developer that is debugging an issue
> > (perhaps unaware of what BPF programs are loaded on the users machine),
> > that the return of a function was tweaked by BPF and that could be the
> > source of the bug?
> >
> >> be improved over the long term, but I don't think that's related to this
> >> set of functionality on ARM.

There are workloads and applications (e.g. https://kubearmor.io/) that
already use BPF Tracing and LSM programs and are currently blocked on
their ARM server deployments.

This may be obvious, but I want to reiterate that while the attachment
points are not UAPI and users have to tolerate kernel function
changes, they do expect the core loading and attachment mechanisms to
exist (i.e. the ability to use LSM and tracing programs).

> >
> > I disagree. These issues may have been added to x86, but perhaps we should
> > take a deeper look at them again before extending them to other
> > architectures.
>
> Honestly, I think a large scale architecture review of every BPF feature
> and decision over the last 10 years is just the wrong bar for this patch
> series.

+1

>
>  From my understanding, Mark and Florent have some changes planned
> that'll improve ftrace, livepatching, and bpf.  Lets talk about those,
> and tackle any long term improvements you'd like to make to BPF in other
> patch series.

+1

 - KP

>
> -chris
>