Re: [PATCH bpf-next v2 00/28] Introduce eBPF support for HID devices

On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires
[off-list ref] wrote:

Hi,

This is a followup of my v1 at [0].

The short summary of the previous cover letter and discussions is that
HID could benefit from BPF for the following use cases:

- simple fixup of report descriptor:
  benefits are faster development time and testing, with the produced
  bpf program being shipped in the kernel directly (the shipping part
  is *not* addressed here).

- Universal Stylus Interface:
  allows a user-space program to define its own kernel interface

- Surface Dial:
  somehow similar to the previous one except that userspace can decide
  to change the shape of the exported device

- firewall:
  still partly missing there, there is not yet interception of hidraw
  calls, but it's coming in a followup series, I promise

- tracing:
  well, tracing.


I tried to address as many comments as I could and here is the short log
of changes:

v2:
===

- split the series by subsystem (bpf, HID, libbpf, selftests and
  samples)

- Added an extra patch at the beginning to not require CAP_NET_ADMIN for
  BPF_PROG_TYPE_LIRC_MODE2 (please shout if this is wrong)

- made the bpf context attached to HID program of dynamic size:
  * the first 1 kB will be able to be addressed directly
  * the rest can be retrieved through bpf_hid_{set|get}_data
    (note that I am definitivey not happy with that API, because there
    is part of it in bits and other in bytes. ouch)

- added an extra patch to prevent non GPL HID bpf programs to be loaded
  of type BPF_PROG_TYPE_HID
  * same here, not really happy but I don't know where to put that check
    in verifier.c

- added a new flag BPF_F_INSERT_HEAD for BPF_LINK_CREATE syscall when in
  used with HID program types.
  * this flag is used for tracing, to be able to load a program before
    any others that might already have been inserted and that might
    change the data stream.

Cheers,
Benjamin

The set looks good so far. I will review the rest later.

[...]

A quick note about how we organize these patches. Maybe we can
merge some of these patches like:

  bpf: introduce hid program type
  bpf/hid: add a new attach type to change the report descriptor
  bpf/hid: add new BPF type to trigger commands from userspace

I guess the three can merge into one.

  HID: hook up with bpf
  HID: allow to change the report descriptor from an eBPF program
  HID: bpf: compute only the required buffer size for the device
  HID: bpf: only call hid_bpf_raw_event() if a ctx is available

I haven't read through all of them, but I guess they can probably merge
as well.

  libbpf: add HID program type and API
  libbpf: add new attach type BPF_HID_RDESC_FIXUP
  libbpf: add new attach type BPF_HID_USER_EVENT

There 3 can merge, and maybe also the one below

  libbpf: add handling for BPF_F_INSERT_HEAD in HID programs

  samples/bpf: add new hid_mouse example
  samples/bpf: add a report descriptor fixup
  samples/bpf: fix bpf_program__attach_hid() api change

Maybe it makes sense to merge these 3?

  bpf/hid: add hid_{get|set}_data helpers
  HID: bpf: implement hid_bpf_get|set_data
  bpf/hid: add bpf_hid_raw_request helper function
  HID: add implementation of bpf_hid_raw_request

We can have 1 or 2 patches for these helpers

  selftests/bpf: add tests for the HID-bpf initial implementation
  selftests/bpf: add report descriptor fixup tests
  selftests/bpf: add tests for hid_{get|set}_data helpers
  selftests/bpf: add test for user call of HID bpf programs
  selftests/bpf: hid: rely on uhid event to know if a test device is
    ready
  selftests/bpf: add tests for bpf_hid_hw_request
  selftests/bpf: Add a test for BPF_F_INSERT_HEAD

These selftests could also merge into 1 or 2 patches I guess.

I understand rearranging these patches may take quite some effort.
But I do feel that's a cleaner approach (from someone doesn't know
much about HID). If you really hate it that way, we can discuss...

Thanks,
Song