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

On Tue, Nov 29, 2022 at 7:00 PM Florent Revest [off-list ref] wrote:

On Thu, Nov 24, 2022 at 4:50 PM Benjamin Tissoires
[off-list ref] wrote:

quoted

On Wed, Nov 23, 2022 at 9:14 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Wed, Nov 23, 2022 at 6:53 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

Hi Jon,

On Wed, Nov 23, 2022 at 2:25 PM Jon Hunter [off-list ref] wrote:

quoted

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.

Oh, OK. Thanks for the explanation.

quoted

quoted

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!

yay!

quoted

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

OK

    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)

Ok as well.

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 ?

That would be nice, but requires some sort of synchronization between
our 2 trees, so I'll take the 2 configs in the HID tree, and try to
submit a patch for the bpf tree with the macro. Then I can reattach to
that macro when it hits Linus'.

1: https://lore.kernel.org/lkml/20221121104403.1545f9b5@gandalf.local.home/ (local)

That thread is a little bit worrying to me. HID-BPF relies on
CONFIG_FUNCTION_ERROR_INJECTION, and I would definitely like to see
HID-BPF in production kernels. I don't really care about cloud
servers, but chromebooks are something I'd like to have enabled. We'll
see how this thread goes I guess.

Anyway, thanks a lot for the deep explanation and understanding of my code :)

Cheers,
Benjamin