Re: [PATCH v4 02/10] user_events: Add minimal support for trace_event into ftrace

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue, 9 Nov 2021 11:08:44 -0800
Beau Belgrave <beaub@linux.microsoft.com> wrote:

> > > I'd like to know if I do fix them that the features like filtering will still
> > > be available to user_events or if it's better to just add flags to disable
> > > kernel filtering?
> > 
> > I would rather like that the filters will be available on the user_events.
> > 
> > My question is that you need to log the dynamic data or strings via user-
> > events or not. Since the other user-events, like SDT doesn't support the
> > string variables to trace, I guess that is not a high priority.
> > 
> > Moreover, since now we can use eprobes, if user event records the address of
> > user-string, the eprobes can fetch it.
> > 
> > So, my suggestion is implmenting with fixed-size parameters as the first step
> > and keep filter/histograms/eprobes available on the user-events.
> > If you find any performance issue, you can expand the user-events to support
> > dynamic (array) data and strings.
> > 
> 
> We need strings to be able to be emitted and recorded in eBPF, perf and
> ftrace. So I would rather go after a solution that lets us keep these in
> the ring buffers, even if it means a perf hit.

OK, my concern is based on the current implementation, so in that case
you can add some additional verification. That is good.

> Guess what's left is to where the best place to check is, checking in
> the filter with unsafe flags would let us keep most of the perf (we just
> check the undersize case, 1 branch). When these unsafe types are
> filtered then a perf tax is imposed to keep things safe.

I would like to keep verifying in writer side then we can ensure the
data on ring buffer (of perf and of ftrace) is sane. If you add the unsafe
flag, you have to change all the code which access the ring buffer, not only
the filter but also eprobes, histograms, perf-tools, and other user-space
tracing tools which reads the tracing buffer directly.

> It sounded like Steven wanted to think about this a bit, so I'll wait a
> bit before poking again for consensus :)
> 
> Do you have any strong feelings about where it goes?

I recommend you to start verifying the writer side, it should make the
change as small as possible. Unsafe flag idea may involve many other
tools. And it is not fundamentary required for user-events.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>