lkml.org 
[lkml]   [2022]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH bpf-next v5 00/17] Introduce eBPF support for HID devices
    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
    > >
    >

    \
     
     \ /
      Last update: 2022-05-30 17:59    [W:2.032 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site