| Message ID | 20220518205924.399291-1-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
> 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.
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
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.
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
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
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
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
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
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.
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!
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 >
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 > > >
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