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

[Benjamin Tissoires]

Hi Song,

Thanks a lot for the review.

I'll comment on the review in more details next week, but I have a
quick question here:

On Sat, Mar 5, 2022 at 2:14 AM Song Liu <song@xxxxxxxxxx> wrote:
>
> On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> 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:

Just to be sure we are talking about the same thing: you mean squash
the patch together?

>
> >   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.

There are certainly patches that we could squash together (3 and 4
from this list into the previous ones), but I'd like to keep some sort
of granularity here to not have a patch bomb that gets harder to come
back later.

>
> >   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

Yeah, the libbpf changes are small enough to not really justify
separate patches.

>
> >   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?

Sure, why not.

>
> >   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

OK, the patches should be self-contained enough.

>
> >   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'd still like to link them to the granularity of the bpf changes, so
I can refer a selftest change to a specific commit/functionality
added. But that's just my personal taste, and I can be convinced
otherwise. This should give us maybe 4 patches instead of 7.

>
> 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...
>

No worries. I don't mind iterating on the series. IIRC I already
rewrote it twice from scratch, and that's when the selftests I
introduced in the second rewrite were tremendously helpful :) And
honestly I don't think it'll be too much effort to reorder/squash the
patches given that the v2 is *very* granular.

Anyway, I prefer having the reviewers happy so we can have a solid
rock API from day 1 than keeping it obscure for everyone and having to
deal with design issues forever. So if it takes 10 or 20 revisions to
have everybody on the same page, that's fine with me (not that I want
to have that many revisions, just that I won't be afraid of the
bikeshedding we might have at some point).

Cheers,
Benjamin