Re: [PATCH hid v12 03/15] HID: initial BPF implementation

[Florent Revest]

On Thu, Nov 24, 2022 at 4:50 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> On Wed, Nov 23, 2022 at 9:14 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Wed, Nov 23, 2022 at 6:53 AM Benjamin Tissoires
> > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > >
> > > Hi Jon,
> > >
> > > On Wed, Nov 23, 2022 at 2:25 PM Jon Hunter <jonathanh@xxxxxxxxxx> wrote:
> > > >
> > > > We have a kernel test that checks for new warning and error messages on
> > > > boot and with this change I am now seeing the following error message on
> > > > our Tegra platforms ...
> > > >
> > > >   WARNING KERN hid_bpf: error while preloading HID BPF dispatcher: -13
> > > >
> > > > I have a quick look at the code, but I can't say I am familiar with
> > > > this. So I wanted to ask if a way to fix this or avoid this? I see the
> > > > code returns 0, so one option would be to make this an informational or
> > > > debug print.
> > >
> > > I am not in favor of debug in that case, because I suspect it'll hide
> > > too much when getting a bug report. Informational could do, yes.
> > >
> > > However, before that, I'd like to dig a little bit more on why it is
> > > failing. I thought arm64 now has support of tracing bpf programs, so I
> > > would not expect this to fail.

We have BPF trampolines on arm64 already but no ftrace direct calls
right now. (so trampolines get jitted but don't get called eheh :)) So
indeed BPF tracing programs (fentry/fexit/fmod_ret) do not work on
arm64 at the moment.

> > Unfortunately the patches to add support for such tracing bpf progs got stuck.
> > Florent/Mark can probably share the latest status.

We are working on an implementation of ftrace direct calls that would
fit within the constraints of arm64 and play nice with the other users
of the ftrace call site. Hopefully we have a patch to share in the
next couple of weeks I'd say!

> Oh... I noticed Jon's config was lacking CONFIG_FTRACE. So should I
> also add a depends on CONFIG_FTRACE to enable HID-BPF?

If HID-BPF fundamentally depends on a fmod_ret program being attached
to function, it seems to me that it should depend on both:
    CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS (CONFIG_FTRACE or even
CONFIG_DYNAMIC_FTRACE aren't enough, there can be architectures that
do not support direct calls. here you noticed it on arm64 which
luckily should get fixed someday soon but there could be other
architectures with that issue too)
and
    CONFIG_FUNCTION_ERROR_INJECTION (since [1] error injection needs
to be explicitly opted-in, it's easy to miss it and fail to attach
fmod_ret programs in mysterious ways)

I'm thinking that maybe encoding these two dependencies in the
CONFIG_HID_BPF is leaking too much of the bpf tracing abstraction to
the user. Maybe the BPF Kconfig could provide "proxy" configs like
HAVE_BPF_FENTRY_FEXIT, HAVE_BPF_FMOD_RET (naming is hard) to expose
these dependencies better ?

1: https://lore.kernel.org/lkml/20221121104403.1545f9b5@xxxxxxxxxxxxxxxxxx/