Date:   Mon, 30 May 2022 17:49:57 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Tero Kristo <tero.kristo@...ux.intel.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Jiri Kosina <jikos@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Dave Marchevsky <davemarchevsky@...com>,
        Joe Stringer <joe@...ium.io>, Jonathan Corbet <corbet@....net>,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>
Subject: Re: [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@...ux.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
> >
>

Powered by blists - more mailing lists