mbox series

[bpf-next,v5,00/17] Introduce eBPF support for HID devices

Message ID 20220518205924.399291-1-benjamin.tissoires@redhat.com (mailing list archive)
Headers show
Series Introduce eBPF support for HID devices | expand

Message

Benjamin Tissoires May 18, 2022, 8:59 p.m. UTC
Hi,

And here comes the v5 of the HID-BPF series.

I managed to achive the same functionalities than v3 this time.
Handling per-device BPF program was "interesting" to say the least,
but I don't know if we can have a generic BPF way of handling such
situation.

The interesting bits is that now the BPF core changes are rather small,
and I am mostly using existing facilities.
I didn't managed to write selftests for the RET_PTR_TO_MEM kfunc,
because I can not call kmalloc while in a SEC("tc") program to match
what the other kfunc tests are doing.
And AFAICT, the most interesting bits would be to implement verifier
selftests, which are way out of my league, given that they are
implemented as plain bytecode.

The logic is the following (see also the last patch for some more
documentation):
- hid-bpf first preloads a BPF program in the kernel that does a few
  things:
   * find out which attach_btf_id are associated with our trace points
   * adds a bpf_tail_call() BPF program that I can use to "call" any
     other BPF program stored into a jump table
   * monitors the releases of struct bpf_prog, and when there are no
     other users than us, detach the bpf progs from the HID devices
- users then declare their tracepoints and then call
  hid_bpf_attach_prog() in a SEC("syscall") program
- hid-bpf then calls multiple time the bpf_tail_call() program with a
  different index in the jump table whenever there is an event coming
  from a matching HID device

Note that I am tempted to pin an "attach_hid_program" in the bpffs so
that users don't need to declare one, but I am afraid this will be one
more API to handle, so maybe not.

I am also wondering if I should not strip out hid_bpf_jmp_table of most
of its features and implement everything as a BPF program. This might
remove the need to add the kernel light skeleton implementations of map
modifications, and might also possibly be more re-usable for other
subsystems. But every plan I do in my head involves a lot of back and
forth between the kernel and BPF to achieve the same, which doesn't feel
right. The tricky part is the RCU list of programs that is stored in each
device and also the global state of the jump table.
Anyway, something to look for in a next version if there is a push for it.

FWIW, patch 1 is something I'd like to get merged sooner. With 2
colleagues, we are also working on supporting the "revoke" functionality
of a fd for USB and for hidraw. While hidraw can be emulated with the
current features, we need the syscall kfuncs for USB, because when we
revoke a USB access, we also need to kick out the user, and for that, we
need to actually execute code in the kernel from a userspace event.

Anyway, happy reviewing.

Cheers,
Benjamin

[Patch series based on commit 68084a136420 ("selftests/bpf: Fix building bpf selftests statically")
in the bpf-next tree]

Benjamin Tissoires (17):
  bpf/btf: also allow kfunc in tracing and syscall programs
  bpf/verifier: allow kfunc to return an allocated mem
  bpf: prepare for more bpf syscall to be used from kernel and user
    space.
  libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton
  HID: core: store the unique system identifier in hid_device
  HID: export hid_report_type to uapi
  HID: initial BPF implementation
  selftests/bpf: add tests for the HID-bpf initial implementation
  HID: bpf: allocate data memory for device_event BPF programs
  selftests/bpf/hid: add test to change the report size
  HID: bpf: introduce hid_hw_request()
  selftests/bpf: add tests for bpf_hid_hw_request
  HID: bpf: allow to change the report descriptor
  selftests/bpf: add report descriptor fixup tests
  samples/bpf: add new hid_mouse example
  selftests/bpf: Add a test for BPF_F_INSERT_HEAD
  Documentation: add HID-BPF docs

 Documentation/hid/hid-bpf.rst                 | 528 ++++++++++
 Documentation/hid/index.rst                   |   1 +
 drivers/hid/Kconfig                           |   2 +
 drivers/hid/Makefile                          |   2 +
 drivers/hid/bpf/Kconfig                       |  19 +
 drivers/hid/bpf/Makefile                      |  11 +
 drivers/hid/bpf/entrypoints/Makefile          |  88 ++
 drivers/hid/bpf/entrypoints/README            |   4 +
 drivers/hid/bpf/entrypoints/entrypoints.bpf.c |  78 ++
 .../hid/bpf/entrypoints/entrypoints.lskel.h   | 782 ++++++++++++++
 drivers/hid/bpf/hid_bpf_dispatch.c            | 565 ++++++++++
 drivers/hid/bpf/hid_bpf_dispatch.h            |  28 +
 drivers/hid/bpf/hid_bpf_jmp_table.c           | 587 +++++++++++
 drivers/hid/hid-core.c                        |  43 +-
 include/linux/btf.h                           |   7 +
 include/linux/hid.h                           |  29 +-
 include/linux/hid_bpf.h                       | 144 +++
 include/uapi/linux/hid.h                      |  12 +
 include/uapi/linux/hid_bpf.h                  |  25 +
 kernel/bpf/btf.c                              |  47 +-
 kernel/bpf/syscall.c                          |  10 +-
 kernel/bpf/verifier.c                         |  72 +-
 samples/bpf/.gitignore                        |   1 +
 samples/bpf/Makefile                          |  23 +
 samples/bpf/hid_mouse.bpf.c                   | 134 +++
 samples/bpf/hid_mouse.c                       | 157 +++
 tools/lib/bpf/skel_internal.h                 |  23 +
 tools/testing/selftests/bpf/config            |   3 +
 tools/testing/selftests/bpf/prog_tests/hid.c  | 990 ++++++++++++++++++
 tools/testing/selftests/bpf/progs/hid.c       | 222 ++++
 30 files changed, 4593 insertions(+), 44 deletions(-)
 create mode 100644 Documentation/hid/hid-bpf.rst
 create mode 100644 drivers/hid/bpf/Kconfig
 create mode 100644 drivers/hid/bpf/Makefile
 create mode 100644 drivers/hid/bpf/entrypoints/Makefile
 create mode 100644 drivers/hid/bpf/entrypoints/README
 create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.bpf.c
 create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.lskel.h
 create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.c
 create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.h
 create mode 100644 drivers/hid/bpf/hid_bpf_jmp_table.c
 create mode 100644 include/linux/hid_bpf.h
 create mode 100644 include/uapi/linux/hid_bpf.h
 create mode 100644 samples/bpf/hid_mouse.bpf.c
 create mode 100644 samples/bpf/hid_mouse.c
 create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c
 create mode 100644 tools/testing/selftests/bpf/progs/hid.c

Comments

Christoph Hellwig May 19, 2022, 8:10 a.m. UTC | #1
> The logic is the following (see also the last patch for some more
> documentation):
> - hid-bpf first preloads a BPF program in the kernel that does a few
>   things:
>    * find out which attach_btf_id are associated with our trace points
>    * adds a bpf_tail_call() BPF program that I can use to "call" any
>      other BPF program stored into a jump table
>    * monitors the releases of struct bpf_prog, and when there are no
>      other users than us, detach the bpf progs from the HID devices
> - users then declare their tracepoints and then call
>   hid_bpf_attach_prog() in a SEC("syscall") program
> - hid-bpf then calls multiple time the bpf_tail_call() program with a
>   different index in the jump table whenever there is an event coming
>   from a matching HID device

So driver abstractions like UDI are now perfectly fine as long as they
are written using a hip new VM?

This whole idea seems like a bad idea, against the Linux spirit and
now actually useful - it is totally trivial to write a new HID
driver alreay, and if it isn't in some cases we need to fix that.

So a big fat NAK to the idea of using eBPF for actual driver logic.
Greg Kroah-Hartman May 19, 2022, 8:20 a.m. UTC | #2
On Thu, May 19, 2022 at 01:10:48AM -0700, Christoph Hellwig wrote:
> > The logic is the following (see also the last patch for some more
> > documentation):
> > - hid-bpf first preloads a BPF program in the kernel that does a few
> >   things:
> >    * find out which attach_btf_id are associated with our trace points
> >    * adds a bpf_tail_call() BPF program that I can use to "call" any
> >      other BPF program stored into a jump table
> >    * monitors the releases of struct bpf_prog, and when there are no
> >      other users than us, detach the bpf progs from the HID devices
> > - users then declare their tracepoints and then call
> >   hid_bpf_attach_prog() in a SEC("syscall") program
> > - hid-bpf then calls multiple time the bpf_tail_call() program with a
> >   different index in the jump table whenever there is an event coming
> >   from a matching HID device
> 
> So driver abstractions like UDI are now perfectly fine as long as they
> are written using a hip new VM?

Ugh, don't mention UDI, that's a bad flashback...

> This whole idea seems like a bad idea, against the Linux spirit and
> now actually useful - it is totally trivial to write a new HID
> driver alreay, and if it isn't in some cases we need to fix that.
> 
> So a big fat NAK to the idea of using eBPF for actual driver logic.

I thought the goal here was to move a lot of the quirk handling and
"fixup the broken HID decriptors in this device" out of kernel .c code
and into BPF code instead, which this patchset would allow.

So that would just be exception handling.  I don't think you can write a
real HID driver here at all, but I could be wrong as I have not read the
new patchset (older versions of this series could not do that.)

thanks,

greg k-h
Christoph Hellwig May 19, 2022, 8:38 a.m. UTC | #3
On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
> > are written using a hip new VM?
> 
> Ugh, don't mention UDI, that's a bad flashback...

But that is very much what we are doing here.

> I thought the goal here was to move a lot of the quirk handling and
> "fixup the broken HID decriptors in this device" out of kernel .c code
> and into BPF code instead, which this patchset would allow.
> 
> So that would just be exception handling.  I don't think you can write a
> real HID driver here at all, but I could be wrong as I have not read the
> new patchset (older versions of this series could not do that.)

And that "exception handling" is most of the driver.
Benjamin Tissoires May 19, 2022, 10:20 a.m. UTC | #4
On Thu, May 19, 2022 at 10:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
> > > are written using a hip new VM?
> >
> > Ugh, don't mention UDI, that's a bad flashback...
>
> But that is very much what we are doing here.
>
> > I thought the goal here was to move a lot of the quirk handling and
> > "fixup the broken HID decriptors in this device" out of kernel .c code
> > and into BPF code instead, which this patchset would allow.

Yes, quirks are a big motivation for this work. Right now half of the
HID drivers are less than 100 lines of code, and are just trivial
fixes (one byte in the report descriptor, one key mapping, etc...).
Using eBPF for those would simplify the process from the user point of
view: you drop a "firmware fix" as an eBPF program in your system and
you can continue working on your existing kernel.

The other important aspect is being able to do filtering on the event
streams themselves.
This would mean for instance that you allow some applications to have
access to part of the device features and you reject some of them. The
main use case I have is to prevent applications to switch a device
into its bootloader mode and mess up with the firmware.

> >
> > So that would just be exception handling.  I don't think you can write a
> > real HID driver here at all, but I could be wrong as I have not read the
> > new patchset (older versions of this series could not do that.)

Well, to be fair, yes and no.
HID-BPF can only talk HID, and so we only act on arrays of bytes. You
can mess up with the report descriptor or the events themselves, but
you don't have access to other kernel APIs.
So no, you can not write a HID-BPF driver that would manually create
LEDs sysfs endpoints, input endpoints and battery endpoints.

However, HID is very versatile in how you can describe a device. And
the kernel now supports a lot of those features. So if you really
want, you can entirely change the look of the device (its report
descriptor), and rely on hid-core to export those LEDs, inputs and
battery endpoints.

But we already have this available by making use of hidraw+uhid. This
involves userspace and there are already projects (for handling
Corsair keyboard for example) which are doing exactly that, with a big
security whole in the middle because the application is reading *all*
events as they are flowing.

One of the most important things here is that this work allows for
context driven behavior. We can now control how a device is behaving
depending on the actual application without having to design and
maintain forever kernel APIs.
For example, the Surface Dial is a puck that can have some haptic
feedback when you turn it. However, when you enable the haptic
feedback you have to reduce the resolution to one event every 5
degrees or the haptic feedback feels just wrong. But the device is
capable of sub-degrees of event notifications. Which means you want
the high resolution mode without haptic, and low res with haptic.

Of course, you can use some new FF capabilities to enable/disable
haptic, but we have nothing to change the resolution on the fly of a
HID device, so we'll likely have to create another kernel API through
a sysfs node or a kernel parameter. But then we need to teach
userspace to use it and this kernel API is not standard, so it won't
be used outside of this particular device.
BPF in that case allows the application which needs it to do the
changes it requires depending on the context. And when I say
application, it is mostly either the compositor or a daemon, not gimp.

>
> And that "exception handling" is most of the driver.
>

Well, it depends. If hardware makers would not make crappy decisions
based on the fact that it somehow works under Windows, we wouldn't
have to do anything to support those devices.
But for half of the drivers, we are doing dumb things to fix those
devices in the kernel.

On the other hand, we do have generic protocols in HID that can not be
replaced by BPF.
For the exercise, I tried to think about what it would take to rewrite
the multitouch logic in eBPF, and trust me, you don't want that. The
code would be a lot of spaghetti and would require access to many
kernel APIs to handle it properly.

Cheers,
Benjamin
Greg Kroah-Hartman May 19, 2022, 10:32 a.m. UTC | #5
On Thu, May 19, 2022 at 01:38:58AM -0700, Christoph Hellwig wrote:
> On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
> > > are written using a hip new VM?
> > 
> > Ugh, don't mention UDI, that's a bad flashback...
> 
> But that is very much what we are doing here.
> 
> > I thought the goal here was to move a lot of the quirk handling and
> > "fixup the broken HID decriptors in this device" out of kernel .c code
> > and into BPF code instead, which this patchset would allow.
> > 
> > So that would just be exception handling.  I don't think you can write a
> > real HID driver here at all, but I could be wrong as I have not read the
> > new patchset (older versions of this series could not do that.)
> 
> And that "exception handling" is most of the driver.

For a number of "small" drivers, yes, that's all there is as the
hardware is "broken" and needs to be fixed up in order to work properly
with the hid core code.  An example of that would be hid-samsung.c which
rewrites the descriptors to be sane and maps the mouse buttons properly.

But that's it, after initialization that driver gets out of the way and
doesn't actually control anything.  From what I can tell, this patchset
would allow us to write those "fixup the mappings and reports before the
HID driver takes over" into ebpf programs.

It would not replace "real" HID drivers like hid-rmi.c that has to
handle the events and do other "real" work here.

Or I could be reading this code all wrong, Benjamin?

But even if it would allow us to write HID drivers as ebpf, what is
wrong with that?  It's not a licensing issue (this api is only allowed
for GPL ebpf programs), it should allow us to move a bunch of in-kernel
drivers into smaller ebpf programs instead.

It's not like this ebpf HID driver would actually work on any other
operating system, right?  I guess Microsoft could create a gpl-licensed
ebpf HID layer as well?  As Windows allows vendors to do all of this
horrible HID fixups in userspace today anyway, I strongly doubt they
would go through the effort to add a new api like this for no valid
reason.

thanks,

greg k-h
Toke Høiland-Jørgensen May 19, 2022, 10:43 a.m. UTC | #6
Benjamin Tissoires <benjamin.tissoires@redhat.com> writes:

> On Thu, May 19, 2022 at 10:39 AM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
>> > > are written using a hip new VM?
>> >
>> > Ugh, don't mention UDI, that's a bad flashback...
>>
>> But that is very much what we are doing here.
>>
>> > I thought the goal here was to move a lot of the quirk handling and
>> > "fixup the broken HID decriptors in this device" out of kernel .c code
>> > and into BPF code instead, which this patchset would allow.
>
> Yes, quirks are a big motivation for this work. Right now half of the
> HID drivers are less than 100 lines of code, and are just trivial
> fixes (one byte in the report descriptor, one key mapping, etc...).
> Using eBPF for those would simplify the process from the user point of
> view: you drop a "firmware fix" as an eBPF program in your system and
> you can continue working on your existing kernel.

How do you envision those BPF programs living, and how would they be
distributed? (In-tree / out of tree?)

-Toke
Benjamin Tissoires May 19, 2022, 11:46 a.m. UTC | #7
On Thu, May 19, 2022 at 12:32 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 19, 2022 at 01:38:58AM -0700, Christoph Hellwig wrote:
> > On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
> > > > are written using a hip new VM?
> > >
> > > Ugh, don't mention UDI, that's a bad flashback...
> >
> > But that is very much what we are doing here.
> >
> > > I thought the goal here was to move a lot of the quirk handling and
> > > "fixup the broken HID decriptors in this device" out of kernel .c code
> > > and into BPF code instead, which this patchset would allow.
> > >
> > > So that would just be exception handling.  I don't think you can write a
> > > real HID driver here at all, but I could be wrong as I have not read the
> > > new patchset (older versions of this series could not do that.)
> >
> > And that "exception handling" is most of the driver.
>
> For a number of "small" drivers, yes, that's all there is as the
> hardware is "broken" and needs to be fixed up in order to work properly
> with the hid core code.  An example of that would be hid-samsung.c which
> rewrites the descriptors to be sane and maps the mouse buttons properly.
>
> But that's it, after initialization that driver gets out of the way and
> doesn't actually control anything.  From what I can tell, this patchset
> would allow us to write those "fixup the mappings and reports before the
> HID driver takes over" into ebpf programs.
>
> It would not replace "real" HID drivers like hid-rmi.c that has to
> handle the events and do other "real" work here.
>
> Or I could be reading this code all wrong, Benjamin?

You get it right. hid-rmi is a good example of something that can not
sanely be done with eBPF.
We can do some filtering on the events (dropping one event, changing
one other), but anything outside that would not be possible. This
driver does a lot of scheduling, synchronisation, and various setup
that would require a lot of back and forth between userspace and
BPF/kernel, which makes it definitively not fit for a BPF
implementation.

>
> But even if it would allow us to write HID drivers as ebpf, what is
> wrong with that?  It's not a licensing issue (this api is only allowed
> for GPL ebpf programs), it should allow us to move a bunch of in-kernel
> drivers into smaller ebpf programs instead.

The one thing I also like with eBPF is that it is safe. When you
declare an array of bytes, the verifier enforces we don't go outside
of the boundaries.
I know rust is coming, but compared to plain C, that is much better,
even if more restrictive.
And it will also prevent some potential bugs where we have a report
fixup that writes outside of the reserved memory.

>
> It's not like this ebpf HID driver would actually work on any other
> operating system, right?  I guess Microsoft could create a gpl-licensed
> ebpf HID layer as well?  As Windows allows vendors to do all of this
> horrible HID fixups in userspace today anyway, I strongly doubt they
> would go through the effort to add a new api like this for no valid
> reason.

OTOH, managing to push Microsoft to implement HID-BPF would be some
achievement :) (just kidding).

Cheers,
Benjamin
Benjamin Tissoires May 19, 2022, 11:56 a.m. UTC | #8
On Thu, May 19, 2022 at 12:43 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Benjamin Tissoires <benjamin.tissoires@redhat.com> writes:
>
> > On Thu, May 19, 2022 at 10:39 AM Christoph Hellwig <hch@infradead.org> wrote:
> >>
> >> On Thu, May 19, 2022 at 10:20:35AM +0200, Greg KH wrote:
> >> > > are written using a hip new VM?
> >> >
> >> > Ugh, don't mention UDI, that's a bad flashback...
> >>
> >> But that is very much what we are doing here.
> >>
> >> > I thought the goal here was to move a lot of the quirk handling and
> >> > "fixup the broken HID decriptors in this device" out of kernel .c code
> >> > and into BPF code instead, which this patchset would allow.
> >
> > Yes, quirks are a big motivation for this work. Right now half of the
> > HID drivers are less than 100 lines of code, and are just trivial
> > fixes (one byte in the report descriptor, one key mapping, etc...).
> > Using eBPF for those would simplify the process from the user point of
> > view: you drop a "firmware fix" as an eBPF program in your system and
> > you can continue working on your existing kernel.
>
> How do you envision those BPF programs living, and how would they be
> distributed? (In-tree / out of tree?)
>

As Greg mentioned in his reply, report descriptors fixups don't do
much besides changing a memory buffer at probe time. So we can either
have udev load the program, pin it and forget about it, or we can also
have the kernel do that for us.

So I envision the distribution to be hybrid:
- for plain fixups where no userspace is required, we should
distribute those programs in the kernel itself, in-tree.
This series already implements pre-loading of BPF programs for the
core part of HID-BPF, but I plan on working on some automation of
pre-loading of these programs from the kernel itself when we need to
do so.

Ideally, the process would be:
* user reports a bug
* developer produces an eBPF program (and maybe compile it if the user
doesn't have LLVM)
* user tests/validates the fix without having to recompile anything
* developer drops the program in-tree
* some automated magic happens (still unclear exactly how to define
which HID device needs which eBPF program ATM)
* when the kernel sees this exact same device (BUS/VID/PID/INTERFACE)
it loads the fixup

- the other part of the hybrid solution is for when userspace is
heavily involved (because it exports a new dbus interface for that
particular feature on this device). We can not really automatically
preload the BPF program because we might not have the user in front of
it.
So in that case, the program would be hosted alongside the
application, out-of-the-tree, but given that to be able to call kernel
functions you need to be GPL, some public distribution of the sources
is required.

Cheers,
Benjamin
Alexei Starovoitov May 21, 2022, 12:18 a.m. UTC | #9
On Thu, May 19, 2022 at 4:56 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> As Greg mentioned in his reply, report descriptors fixups don't do
> much besides changing a memory buffer at probe time. So we can either
> have udev load the program, pin it and forget about it, or we can also
> have the kernel do that for us.
>
> So I envision the distribution to be hybrid:
> - for plain fixups where no userspace is required, we should
> distribute those programs in the kernel itself, in-tree.
> This series already implements pre-loading of BPF programs for the
> core part of HID-BPF, but I plan on working on some automation of
> pre-loading of these programs from the kernel itself when we need to
> do so.
>
> Ideally, the process would be:
> * user reports a bug
> * developer produces an eBPF program (and maybe compile it if the user
> doesn't have LLVM)
> * user tests/validates the fix without having to recompile anything
> * developer drops the program in-tree
> * some automated magic happens (still unclear exactly how to define
> which HID device needs which eBPF program ATM)
> * when the kernel sees this exact same device (BUS/VID/PID/INTERFACE)
> it loads the fixup
>
> - the other part of the hybrid solution is for when userspace is
> heavily involved (because it exports a new dbus interface for that
> particular feature on this device). We can not really automatically
> preload the BPF program because we might not have the user in front of
> it.
> So in that case, the program would be hosted alongside the
> application, out-of-the-tree, but given that to be able to call kernel
> functions you need to be GPL, some public distribution of the sources
> is required.

Agree with everything you've said earlier.
Just one additional comment:
By default the source code is embedded in bpf objects.
Here is an example.
$ bpftool prog dump jited id 3927008|head -50
void cwnd_event(long long unsigned int * ctx):
bpf_prog_9b9adc0a36a25303_cwnd_event:
; void BPF_STRUCT_OPS(cwnd_event, struct sock* sk, enum tcp_ca_event ev) {
   0:    nopl   0x0(%rax,%rax,1)
   5:    xchg   %ax,%ax
...
; switch (ev) {
  25:    mov    %r14d,%edi
  28:    add    $0xfffffffc,%edi
...
; ca->loss_cwnd = tp->snd_cwnd;
  4a:    mov    %edi,0x18(%r13)
  4e:    mov    $0x2,%edi
; tp->snd_ssthresh = max(tp->snd_cwnd >> 1U, 2U);
  53:    test   %rbx,%rbx
  56:    jne    0x000000000000005c

It's not the full source, of course, but good enough in practice
for a person to figure out what program is doing.
patchwork-bot+netdevbpf@kernel.org May 21, 2022, 2:40 a.m. UTC | #10
Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Wed, 18 May 2022 22:59:07 +0200 you wrote:
> Hi,
> 
> And here comes the v5 of the HID-BPF series.
> 
> I managed to achive the same functionalities than v3 this time.
> Handling per-device BPF program was "interesting" to say the least,
> but I don't know if we can have a generic BPF way of handling such
> situation.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v5,01/17] bpf/btf: also allow kfunc in tracing and syscall programs
    https://git.kernel.org/bpf/bpf-next/c/979497674e63
  - [bpf-next,v5,02/17] bpf/verifier: allow kfunc to return an allocated mem
    (no matching commit)
  - [bpf-next,v5,03/17] bpf: prepare for more bpf syscall to be used from kernel and user space.
    (no matching commit)
  - [bpf-next,v5,04/17] libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton
    (no matching commit)
  - [bpf-next,v5,05/17] HID: core: store the unique system identifier in hid_device
    (no matching commit)
  - [bpf-next,v5,06/17] HID: export hid_report_type to uapi
    (no matching commit)
  - [bpf-next,v5,07/17] HID: initial BPF implementation
    (no matching commit)
  - [bpf-next,v5,08/17] selftests/bpf: add tests for the HID-bpf initial implementation
    (no matching commit)
  - [bpf-next,v5,09/17] HID: bpf: allocate data memory for device_event BPF programs
    (no matching commit)
  - [bpf-next,v5,10/17] selftests/bpf/hid: add test to change the report size
    (no matching commit)
  - [bpf-next,v5,11/17] HID: bpf: introduce hid_hw_request()
    (no matching commit)
  - [bpf-next,v5,12/17] selftests/bpf: add tests for bpf_hid_hw_request
    (no matching commit)
  - [bpf-next,v5,13/17] HID: bpf: allow to change the report descriptor
    (no matching commit)
  - [bpf-next,v5,14/17] selftests/bpf: add report descriptor fixup tests
    (no matching commit)
  - [bpf-next,v5,15/17] samples/bpf: add new hid_mouse example
    (no matching commit)
  - [bpf-next,v5,16/17] selftests/bpf: Add a test for BPF_F_INSERT_HEAD
    (no matching commit)
  - [bpf-next,v5,17/17] Documentation: add HID-BPF docs
    (no matching commit)

You are awesome, thank you!
Tero Kristo May 27, 2022, 7:26 a.m. UTC | #11
Hi Benjamin,

I noticed a couple of issues with this series, but was able to 
fix/workaround them locally and got my USI program working with it.

1) You seem to be missing tools/include/uapi/linux/hid_bpf.h from index, 
I wasn't able to compile the selftests (or my own program) without 
adding this. It is included from 
tools/testing/selftests/bpf/prog_tests/hid.c: #include <linux/hid_bpf.h>

2) The limitation of needing to hardcode the size for hid_bpf_get_data() 
seems somewhat worrying, especially as the kernel side limits this to 
the ctx->allocated_size. I used a sufficiently large number for my 
purposes for now (256) which seems to work, but how should I handle my 
case where I basically need to read the whole input report and parse 
certain portions of it? How does the HID subsystem select the size of 
the ctx->allocated_size?

-Tero

On 18/05/2022 23:59, Benjamin Tissoires wrote:
> Hi,
>
> And here comes the v5 of the HID-BPF series.
>
> I managed to achive the same functionalities than v3 this time.
> Handling per-device BPF program was "interesting" to say the least,
> but I don't know if we can have a generic BPF way of handling such
> situation.
>
> The interesting bits is that now the BPF core changes are rather small,
> and I am mostly using existing facilities.
> I didn't managed to write selftests for the RET_PTR_TO_MEM kfunc,
> because I can not call kmalloc while in a SEC("tc") program to match
> what the other kfunc tests are doing.
> And AFAICT, the most interesting bits would be to implement verifier
> selftests, which are way out of my league, given that they are
> implemented as plain bytecode.
>
> The logic is the following (see also the last patch for some more
> documentation):
> - hid-bpf first preloads a BPF program in the kernel that does a few
>    things:
>     * find out which attach_btf_id are associated with our trace points
>     * adds a bpf_tail_call() BPF program that I can use to "call" any
>       other BPF program stored into a jump table
>     * monitors the releases of struct bpf_prog, and when there are no
>       other users than us, detach the bpf progs from the HID devices
> - users then declare their tracepoints and then call
>    hid_bpf_attach_prog() in a SEC("syscall") program
> - hid-bpf then calls multiple time the bpf_tail_call() program with a
>    different index in the jump table whenever there is an event coming
>    from a matching HID device
>
> Note that I am tempted to pin an "attach_hid_program" in the bpffs so
> that users don't need to declare one, but I am afraid this will be one
> more API to handle, so maybe not.
>
> I am also wondering if I should not strip out hid_bpf_jmp_table of most
> of its features and implement everything as a BPF program. This might
> remove the need to add the kernel light skeleton implementations of map
> modifications, and might also possibly be more re-usable for other
> subsystems. But every plan I do in my head involves a lot of back and
> forth between the kernel and BPF to achieve the same, which doesn't feel
> right. The tricky part is the RCU list of programs that is stored in each
> device and also the global state of the jump table.
> Anyway, something to look for in a next version if there is a push for it.
>
> FWIW, patch 1 is something I'd like to get merged sooner. With 2
> colleagues, we are also working on supporting the "revoke" functionality
> of a fd for USB and for hidraw. While hidraw can be emulated with the
> current features, we need the syscall kfuncs for USB, because when we
> revoke a USB access, we also need to kick out the user, and for that, we
> need to actually execute code in the kernel from a userspace event.
>
> Anyway, happy reviewing.
>
> Cheers,
> Benjamin
>
> [Patch series based on commit 68084a136420 ("selftests/bpf: Fix building bpf selftests statically")
> in the bpf-next tree]
>
> Benjamin Tissoires (17):
>    bpf/btf: also allow kfunc in tracing and syscall programs
>    bpf/verifier: allow kfunc to return an allocated mem
>    bpf: prepare for more bpf syscall to be used from kernel and user
>      space.
>    libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton
>    HID: core: store the unique system identifier in hid_device
>    HID: export hid_report_type to uapi
>    HID: initial BPF implementation
>    selftests/bpf: add tests for the HID-bpf initial implementation
>    HID: bpf: allocate data memory for device_event BPF programs
>    selftests/bpf/hid: add test to change the report size
>    HID: bpf: introduce hid_hw_request()
>    selftests/bpf: add tests for bpf_hid_hw_request
>    HID: bpf: allow to change the report descriptor
>    selftests/bpf: add report descriptor fixup tests
>    samples/bpf: add new hid_mouse example
>    selftests/bpf: Add a test for BPF_F_INSERT_HEAD
>    Documentation: add HID-BPF docs
>
>   Documentation/hid/hid-bpf.rst                 | 528 ++++++++++
>   Documentation/hid/index.rst                   |   1 +
>   drivers/hid/Kconfig                           |   2 +
>   drivers/hid/Makefile                          |   2 +
>   drivers/hid/bpf/Kconfig                       |  19 +
>   drivers/hid/bpf/Makefile                      |  11 +
>   drivers/hid/bpf/entrypoints/Makefile          |  88 ++
>   drivers/hid/bpf/entrypoints/README            |   4 +
>   drivers/hid/bpf/entrypoints/entrypoints.bpf.c |  78 ++
>   .../hid/bpf/entrypoints/entrypoints.lskel.h   | 782 ++++++++++++++
>   drivers/hid/bpf/hid_bpf_dispatch.c            | 565 ++++++++++
>   drivers/hid/bpf/hid_bpf_dispatch.h            |  28 +
>   drivers/hid/bpf/hid_bpf_jmp_table.c           | 587 +++++++++++
>   drivers/hid/hid-core.c                        |  43 +-
>   include/linux/btf.h                           |   7 +
>   include/linux/hid.h                           |  29 +-
>   include/linux/hid_bpf.h                       | 144 +++
>   include/uapi/linux/hid.h                      |  12 +
>   include/uapi/linux/hid_bpf.h                  |  25 +
>   kernel/bpf/btf.c                              |  47 +-
>   kernel/bpf/syscall.c                          |  10 +-
>   kernel/bpf/verifier.c                         |  72 +-
>   samples/bpf/.gitignore                        |   1 +
>   samples/bpf/Makefile                          |  23 +
>   samples/bpf/hid_mouse.bpf.c                   | 134 +++
>   samples/bpf/hid_mouse.c                       | 157 +++
>   tools/lib/bpf/skel_internal.h                 |  23 +
>   tools/testing/selftests/bpf/config            |   3 +
>   tools/testing/selftests/bpf/prog_tests/hid.c  | 990 ++++++++++++++++++
>   tools/testing/selftests/bpf/progs/hid.c       | 222 ++++
>   30 files changed, 4593 insertions(+), 44 deletions(-)
>   create mode 100644 Documentation/hid/hid-bpf.rst
>   create mode 100644 drivers/hid/bpf/Kconfig
>   create mode 100644 drivers/hid/bpf/Makefile
>   create mode 100644 drivers/hid/bpf/entrypoints/Makefile
>   create mode 100644 drivers/hid/bpf/entrypoints/README
>   create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.bpf.c
>   create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.lskel.h
>   create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.c
>   create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.h
>   create mode 100644 drivers/hid/bpf/hid_bpf_jmp_table.c
>   create mode 100644 include/linux/hid_bpf.h
>   create mode 100644 include/uapi/linux/hid_bpf.h
>   create mode 100644 samples/bpf/hid_mouse.bpf.c
>   create mode 100644 samples/bpf/hid_mouse.c
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c
>   create mode 100644 tools/testing/selftests/bpf/progs/hid.c
>
Benjamin Tissoires May 30, 2022, 3:49 p.m. UTC | #12
Hi Tero,

On Fri, May 27, 2022 at 9:26 AM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi Benjamin,
>
> I noticed a couple of issues with this series, but was able to
> fix/workaround them locally and got my USI program working with it.
>
> 1) You seem to be missing tools/include/uapi/linux/hid_bpf.h from index,
> I wasn't able to compile the selftests (or my own program) without
> adding this. It is included from
> tools/testing/selftests/bpf/prog_tests/hid.c: #include <linux/hid_bpf.h>

Hmm... I initially thought that this would be "fixed" when the kernel
headers are properly installed, so I don't need to manually keep a
duplicate in the tools tree. But now that you mention it, I probably
need to do it the way you mention it.

>
> 2) The limitation of needing to hardcode the size for hid_bpf_get_data()
> seems somewhat worrying, especially as the kernel side limits this to
> the ctx->allocated_size. I used a sufficiently large number for my
> purposes for now (256) which seems to work, but how should I handle my
> case where I basically need to read the whole input report and parse
> certain portions of it? How does the HID subsystem select the size of
> the ctx->allocated_size?

The allocated size is based on the maximum size of the reports allowed
in the device. It is dynamically computed based on the report
descriptor.

I also had the exact same issue you mentioned (dynamically retrieve
the whole report), and that's why I added a couple of things:
- struct hid_bpf_ctx->allocated_size which gives the allocated size,
so you can use this as an upper bound in a for loop
- the allocated size is guaranteed to be a multiple of 64 bytes.

Which means you can have the following for loop:

for (i = 0; i * 64 < hid_ctx->allocated_size && i < 64; i++) {
  data = hid_bpf_get_data(hid_ctx, i * 64, 64);
  /* some more processing */
}

("i < 64" makes an upper bound of 4KB of data, which should be enough
in most cases).

Cheers,
Benjamin

>
> -Tero
>
> On 18/05/2022 23:59, Benjamin Tissoires wrote:
> > Hi,
> >
> > And here comes the v5 of the HID-BPF series.
> >
> > I managed to achive the same functionalities than v3 this time.
> > Handling per-device BPF program was "interesting" to say the least,
> > but I don't know if we can have a generic BPF way of handling such
> > situation.
> >
> > The interesting bits is that now the BPF core changes are rather small,
> > and I am mostly using existing facilities.
> > I didn't managed to write selftests for the RET_PTR_TO_MEM kfunc,
> > because I can not call kmalloc while in a SEC("tc") program to match
> > what the other kfunc tests are doing.
> > And AFAICT, the most interesting bits would be to implement verifier
> > selftests, which are way out of my league, given that they are
> > implemented as plain bytecode.
> >
> > The logic is the following (see also the last patch for some more
> > documentation):
> > - hid-bpf first preloads a BPF program in the kernel that does a few
> >    things:
> >     * find out which attach_btf_id are associated with our trace points
> >     * adds a bpf_tail_call() BPF program that I can use to "call" any
> >       other BPF program stored into a jump table
> >     * monitors the releases of struct bpf_prog, and when there are no
> >       other users than us, detach the bpf progs from the HID devices
> > - users then declare their tracepoints and then call
> >    hid_bpf_attach_prog() in a SEC("syscall") program
> > - hid-bpf then calls multiple time the bpf_tail_call() program with a
> >    different index in the jump table whenever there is an event coming
> >    from a matching HID device
> >
> > Note that I am tempted to pin an "attach_hid_program" in the bpffs so
> > that users don't need to declare one, but I am afraid this will be one
> > more API to handle, so maybe not.
> >
> > I am also wondering if I should not strip out hid_bpf_jmp_table of most
> > of its features and implement everything as a BPF program. This might
> > remove the need to add the kernel light skeleton implementations of map
> > modifications, and might also possibly be more re-usable for other
> > subsystems. But every plan I do in my head involves a lot of back and
> > forth between the kernel and BPF to achieve the same, which doesn't feel
> > right. The tricky part is the RCU list of programs that is stored in each
> > device and also the global state of the jump table.
> > Anyway, something to look for in a next version if there is a push for it.
> >
> > FWIW, patch 1 is something I'd like to get merged sooner. With 2
> > colleagues, we are also working on supporting the "revoke" functionality
> > of a fd for USB and for hidraw. While hidraw can be emulated with the
> > current features, we need the syscall kfuncs for USB, because when we
> > revoke a USB access, we also need to kick out the user, and for that, we
> > need to actually execute code in the kernel from a userspace event.
> >
> > Anyway, happy reviewing.
> >
> > Cheers,
> > Benjamin
> >
> > [Patch series based on commit 68084a136420 ("selftests/bpf: Fix building bpf selftests statically")
> > in the bpf-next tree]
> >
> > Benjamin Tissoires (17):
> >    bpf/btf: also allow kfunc in tracing and syscall programs
> >    bpf/verifier: allow kfunc to return an allocated mem
> >    bpf: prepare for more bpf syscall to be used from kernel and user
> >      space.
> >    libbpf: add map_get_fd_by_id and map_delete_elem in light skeleton
> >    HID: core: store the unique system identifier in hid_device
> >    HID: export hid_report_type to uapi
> >    HID: initial BPF implementation
> >    selftests/bpf: add tests for the HID-bpf initial implementation
> >    HID: bpf: allocate data memory for device_event BPF programs
> >    selftests/bpf/hid: add test to change the report size
> >    HID: bpf: introduce hid_hw_request()
> >    selftests/bpf: add tests for bpf_hid_hw_request
> >    HID: bpf: allow to change the report descriptor
> >    selftests/bpf: add report descriptor fixup tests
> >    samples/bpf: add new hid_mouse example
> >    selftests/bpf: Add a test for BPF_F_INSERT_HEAD
> >    Documentation: add HID-BPF docs
> >
> >   Documentation/hid/hid-bpf.rst                 | 528 ++++++++++
> >   Documentation/hid/index.rst                   |   1 +
> >   drivers/hid/Kconfig                           |   2 +
> >   drivers/hid/Makefile                          |   2 +
> >   drivers/hid/bpf/Kconfig                       |  19 +
> >   drivers/hid/bpf/Makefile                      |  11 +
> >   drivers/hid/bpf/entrypoints/Makefile          |  88 ++
> >   drivers/hid/bpf/entrypoints/README            |   4 +
> >   drivers/hid/bpf/entrypoints/entrypoints.bpf.c |  78 ++
> >   .../hid/bpf/entrypoints/entrypoints.lskel.h   | 782 ++++++++++++++
> >   drivers/hid/bpf/hid_bpf_dispatch.c            | 565 ++++++++++
> >   drivers/hid/bpf/hid_bpf_dispatch.h            |  28 +
> >   drivers/hid/bpf/hid_bpf_jmp_table.c           | 587 +++++++++++
> >   drivers/hid/hid-core.c                        |  43 +-
> >   include/linux/btf.h                           |   7 +
> >   include/linux/hid.h                           |  29 +-
> >   include/linux/hid_bpf.h                       | 144 +++
> >   include/uapi/linux/hid.h                      |  12 +
> >   include/uapi/linux/hid_bpf.h                  |  25 +
> >   kernel/bpf/btf.c                              |  47 +-
> >   kernel/bpf/syscall.c                          |  10 +-
> >   kernel/bpf/verifier.c                         |  72 +-
> >   samples/bpf/.gitignore                        |   1 +
> >   samples/bpf/Makefile                          |  23 +
> >   samples/bpf/hid_mouse.bpf.c                   | 134 +++
> >   samples/bpf/hid_mouse.c                       | 157 +++
> >   tools/lib/bpf/skel_internal.h                 |  23 +
> >   tools/testing/selftests/bpf/config            |   3 +
> >   tools/testing/selftests/bpf/prog_tests/hid.c  | 990 ++++++++++++++++++
> >   tools/testing/selftests/bpf/progs/hid.c       | 222 ++++
> >   30 files changed, 4593 insertions(+), 44 deletions(-)
> >   create mode 100644 Documentation/hid/hid-bpf.rst
> >   create mode 100644 drivers/hid/bpf/Kconfig
> >   create mode 100644 drivers/hid/bpf/Makefile
> >   create mode 100644 drivers/hid/bpf/entrypoints/Makefile
> >   create mode 100644 drivers/hid/bpf/entrypoints/README
> >   create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.bpf.c
> >   create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.lskel.h
> >   create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.c
> >   create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.h
> >   create mode 100644 drivers/hid/bpf/hid_bpf_jmp_table.c
> >   create mode 100644 include/linux/hid_bpf.h
> >   create mode 100644 include/uapi/linux/hid_bpf.h
> >   create mode 100644 samples/bpf/hid_mouse.bpf.c
> >   create mode 100644 samples/bpf/hid_mouse.c
> >   create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c
> >   create mode 100644 tools/testing/selftests/bpf/progs/hid.c
> >
>