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