Re: [PATCH v2 10/15] tools/rv: Add BPF monitors

[Nam Cao <namcao@linutronix.de>]

Gabriele Monaco <gmonaco@redhat.com> writes:
> Fair enough, I feel to be cautious at redefining what reactors are right now,
> though. Currently they are nothing but a function call to the set-up action.
> It's trivial to verify where the code goes without adding the tracing subsystem
> as a dependency in the path (typically important in some safety critical cases).
> Sure, handlers are called by the same tracing subsystem nonetheless, so it may
> not add much value.

I am thinking about dropping reactors entirely and only use
bpf_printk. I have never been a fan of the panic reactor, crashing the
system because of an RV violation is a bit much. And the printk reactor
would be replaced by bpf_printk, just that the messages would be in
tracing ring buffer instead of kernel log.

Or do you already have potential future reactors in mind?

> Let's leave reactors aside for a moment. What I am coming to terms with,
> instead, is that the current ringbuffer to userspace design isn't what we want.
> In kernel monitors communicate their status through tracepoints, perfectly
> integrating to tracing tools, BPF monitors use a custom buffer.
>
> Generating a tracepoint here is probably the way to go. bpf_printk does generate
> one, but as Tomas pointed out, hijacking that for everything (think of monitor
> events) might be a bit much.
> Nonetheless, we want RV BPF handlers to generate tracepoints, then attaching
> actions on error tracepoints (whether we call that reactors or not) can be done
> later.
>
> Summarising, we have 2 potential designs.
>
> 1. bpf_printk only:
>   bpf_printk("event: monitor: curr_state x event -> next_state")
>   bpf_printk("error: monitor: event not expected in curr_state")
>
> 2. new kfunc(s) calling a tracepoint: 
>   trace_event_da_bpf("monitor", "curr_state", "event", "next_state")
>   trace_error_da_bpf("monitor", "curr_state", "event")
>
> 1. is simpler and works mostly out of the box, depending on how powerful glob
> filters are, it can reach the same functionality as 2. Might risk to overload
> the shared bpf_printk event and perhaps have a higher filtering overhead.
> 2. is closest to the current in-kernel implementation and offers flexibility to
> easily filter the type of event (useful for DA, e.g. I want a backtrace only
> when going from state A to B). But it requires a series of kfuncs (e.g. error
> and event for each monitor class).
>
> In both cases a malicious actor could call the kfunc and make us believe there
> was a reaction (they cannot mess with the model though). If we own the kfuncs we
> can at least limit the availability (tracing-only and CAP_SYS_BOOT).
> This, unless we register BPF monitors in the kernel like in V1.
>
> I'm going to test each design once I get again to work on this.
>
> Thoughts?

I vote 1, much simpler (but maybe I just haven't quite got the issue). I
feel a bit ensure about adding RV kfuncs and would only go that route if
there is no other way.

Nam