| Message ID | 20220304172852.274126-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Fri, Mar 04, 2022 at 06:28:27PM +0100, Benjamin Tissoires wrote: > Now that BPF can be compatible with HID, add the capability into HID. > drivers/hid/hid-bpf.c takes care of the glue between bpf and HID, and > hid-core can then inject any incoming event from the device into a BPF > program to filter/analyze it. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Fri, Mar 04, 2022 at 06:28:31PM +0100, Benjamin Tissoires wrote: > The report descriptor is the dictionary of the HID protocol specific > to the given device. > Changing it is a common habit in the HID world, and making that feature > accessible from eBPF allows to fix devices without having to install a > new kernel. > > However, the report descriptor is supposed to be static on a device. > To be able to change it, we need to reconnect the device at the HID > level. > So whenever the report descriptor program type is attached or detached, > we call on a hook on HID to notify it that there is something to be > done. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Fri, Mar 04, 2022 at 06:28:34PM +0100, Benjamin Tissoires wrote: > Simple report descriptor override in HID: replace part of the report > descriptor from a static definition in the bpf kernel program. > > Note that this test should be run last because we disconnect/reconnect > the device, meaning that it changes the overall uhid device. You might want to add that in a comment right before: > @@ -329,6 +395,9 @@ void serial_test_hid_bpf(void) > err = test_hid_raw_event(hid_skel, uhid_fd, sysfs_fd); > ASSERT_OK(err, "hid"); > > + err = test_rdesc_fixup(hid_skel, uhid_fd, sysfs_fd); > + ASSERT_OK(err, "hid_rdesc_fixup"); There, so that you don't add another test after this later on in 5 years. Anyway, just a nit: Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > Extract a new helper for it, it will be also used for the HID bpf > implementation. > > Cc: Sean Young <sean@mess.org> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > new in v2 > --- > kernel/bpf/syscall.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index db402ebc5570..cc570891322b 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > case BPF_PROG_TYPE_SK_SKB: > case BPF_PROG_TYPE_SK_MSG: > - case BPF_PROG_TYPE_LIRC_MODE2: > case BPF_PROG_TYPE_FLOW_DISSECTOR: > case BPF_PROG_TYPE_CGROUP_DEVICE: > case BPF_PROG_TYPE_CGROUP_SOCK: > @@ -2202,6 +2201,17 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > } > } > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > +{ > + switch (prog_type) { > + case BPF_PROG_TYPE_LIRC_MODE2: > + case BPF_PROG_TYPE_EXT: /* extends any prog */ > + return true; > + default: > + return false; > + } > +} I am not sure whether we should do this. This is a behavior change, that may break some user space. Also, BPF_PROG_TYPE_EXT is checked in is_perfmon_prog_type(), and this change will make that case useless. Thanks, Song [...]
On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> 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
On Sat, Mar 5, 2022 at 12:12 AM Song Liu <song@kernel.org> wrote: > > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > > > Extract a new helper for it, it will be also used for the HID bpf > > implementation. > > > > Cc: Sean Young <sean@mess.org> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > new in v2 > > --- > > kernel/bpf/syscall.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index db402ebc5570..cc570891322b 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > > case BPF_PROG_TYPE_SK_SKB: > > case BPF_PROG_TYPE_SK_MSG: > > - case BPF_PROG_TYPE_LIRC_MODE2: > > case BPF_PROG_TYPE_FLOW_DISSECTOR: > > case BPF_PROG_TYPE_CGROUP_DEVICE: > > case BPF_PROG_TYPE_CGROUP_SOCK: > > @@ -2202,6 +2201,17 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > > } > > } > > > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > > +{ > > + switch (prog_type) { > > + case BPF_PROG_TYPE_LIRC_MODE2: > > + case BPF_PROG_TYPE_EXT: /* extends any prog */ > > + return true; > > + default: > > + return false; > > + } > > +} > > I am not sure whether we should do this. This is a behavior change, that may > break some user space. Also, BPF_PROG_TYPE_EXT is checked in > is_perfmon_prog_type(), and this change will make that case useless. Sure, I can drop it from v3 and make this function appear for HID only. Regarding BPF_PROG_TYPE_EXT, it was already in both is_net_admin_prog_type() and is_perfmon_prog_type(), so I duplicated it here, but I agree, given that it's already in the first function there, CPA_SYS_ADMIN is already checked. Cheers, Benjamin > > Thanks, > Song > > [...] >
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@kernel.org> wrote: > > On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> 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
On Sat, Mar 05, 2022 at 11:07:04AM +0100, Benjamin Tissoires wrote: > On Sat, Mar 5, 2022 at 12:12 AM Song Liu <song@kernel.org> wrote: > > > > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > > > > > Extract a new helper for it, it will be also used for the HID bpf > > > implementation. > > > > > > Cc: Sean Young <sean@mess.org> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > --- > > > > > > new in v2 > > > --- > > > kernel/bpf/syscall.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > index db402ebc5570..cc570891322b 100644 > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > > > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > > > case BPF_PROG_TYPE_SK_SKB: > > > case BPF_PROG_TYPE_SK_MSG: > > > - case BPF_PROG_TYPE_LIRC_MODE2: > > > case BPF_PROG_TYPE_FLOW_DISSECTOR: > > > case BPF_PROG_TYPE_CGROUP_DEVICE: > > > case BPF_PROG_TYPE_CGROUP_SOCK: > > > @@ -2202,6 +2201,17 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > > > } > > > } > > > > > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > > > +{ > > > + switch (prog_type) { > > > + case BPF_PROG_TYPE_LIRC_MODE2: > > > + case BPF_PROG_TYPE_EXT: /* extends any prog */ > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > > I am not sure whether we should do this. This is a behavior change, that may > > break some user space. Also, BPF_PROG_TYPE_EXT is checked in > > is_perfmon_prog_type(), and this change will make that case useless. > > Sure, I can drop it from v3 and make this function appear for HID only. For BPF_PROG_TYPE_LIRC_MODE2, I don't think this change will break userspace. This is called from ir-keytable(1) which is called from udev. It should have all the necessary permissions. In addition, the vast majority IR decoders are non-bpf. bpf ir decoders have very few users at the moment. I am working on completely new userspace tooling which will make extensive use of bpf ir decoding with full lircd and IRP compatibility, but this is not finished yet (see https://github.com/seanyoung/cir). Thanks Sean
On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > > > > > > 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? Right, squash some patches 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. Totally agreed with the granularity of patches. I am not a big fan of patch bombs either. :) I guess the problem I have with the current version is that I don't have a big picture of the design while reading through relatively big patches. A overview with the following information in the cover letter would be really help here: 1. How different types of programs are triggered (IRQ, user input, etc.); 2. What are the operations and/or outcomes of these programs; 3. How would programs of different types (or attach types) interact with each other (via bpf maps? chaining?) 4. What's the new uapi; 5. New helpers and other logistics Sometimes, I find the changes to uapi are the key for me to understand the patches, and I would like to see one or two patches with all the UAPI changes (i.e. bpf_hid_attach_type). However, that may or may not apply to this set due to granularity concerns. Does this make sense? Thanks, Song > > > > > > 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 >
On Sat, Mar 5, 2022 at 2:07 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Sat, Mar 5, 2022 at 12:12 AM Song Liu <song@kernel.org> wrote: > > > > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > > > > > Extract a new helper for it, it will be also used for the HID bpf > > > implementation. > > > > > > Cc: Sean Young <sean@mess.org> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > --- > > > > > > new in v2 > > > --- > > > kernel/bpf/syscall.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > index db402ebc5570..cc570891322b 100644 > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > > > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > > > case BPF_PROG_TYPE_SK_SKB: > > > case BPF_PROG_TYPE_SK_MSG: > > > - case BPF_PROG_TYPE_LIRC_MODE2: > > > case BPF_PROG_TYPE_FLOW_DISSECTOR: > > > case BPF_PROG_TYPE_CGROUP_DEVICE: > > > case BPF_PROG_TYPE_CGROUP_SOCK: > > > @@ -2202,6 +2201,17 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > > > } > > > } > > > > > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > > > +{ > > > + switch (prog_type) { > > > + case BPF_PROG_TYPE_LIRC_MODE2: > > > + case BPF_PROG_TYPE_EXT: /* extends any prog */ > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > > I am not sure whether we should do this. This is a behavior change, that may > > break some user space. Also, BPF_PROG_TYPE_EXT is checked in > > is_perfmon_prog_type(), and this change will make that case useless. > > Sure, I can drop it from v3 and make this function appear for HID only. > > Regarding BPF_PROG_TYPE_EXT, it was already in both > is_net_admin_prog_type() and is_perfmon_prog_type(), so I duplicated > it here, but I agree, given that it's already in the first function > there, CPA_SYS_ADMIN is already checked. I think with current code, a user with CAP_BPF, CAP_NET_ADMIN, and CAP_PERFMON (but not CAP_SYS_ADMIN) can load programs of type BPF_PROG_TYPE_EXT. But after the patch, the same user will not be able to do it. Did I misread it? It is not a common case though.
On Sat, Mar 5, 2022 at 8:58 AM Sean Young <sean@mess.org> wrote: > > On Sat, Mar 05, 2022 at 11:07:04AM +0100, Benjamin Tissoires wrote: > > On Sat, Mar 5, 2022 at 12:12 AM Song Liu <song@kernel.org> wrote: > > > > > > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > > > > > > > Extract a new helper for it, it will be also used for the HID bpf > > > > implementation. > > > > > > > > Cc: Sean Young <sean@mess.org> > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > > > --- > > > > > > > > new in v2 > > > > --- > > > > kernel/bpf/syscall.c | 14 +++++++++++++- > > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > > index db402ebc5570..cc570891322b 100644 > > > > --- a/kernel/bpf/syscall.c > > > > +++ b/kernel/bpf/syscall.c > > > > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > > > > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > > > > case BPF_PROG_TYPE_SK_SKB: > > > > case BPF_PROG_TYPE_SK_MSG: > > > > - case BPF_PROG_TYPE_LIRC_MODE2: > > > > case BPF_PROG_TYPE_FLOW_DISSECTOR: > > > > case BPF_PROG_TYPE_CGROUP_DEVICE: > > > > case BPF_PROG_TYPE_CGROUP_SOCK: > > > > @@ -2202,6 +2201,17 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > > > > } > > > > } > > > > > > > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > > > > +{ > > > > + switch (prog_type) { > > > > + case BPF_PROG_TYPE_LIRC_MODE2: > > > > + case BPF_PROG_TYPE_EXT: /* extends any prog */ > > > > + return true; > > > > + default: > > > > + return false; > > > > + } > > > > +} > > > > > > I am not sure whether we should do this. This is a behavior change, that may > > > break some user space. Also, BPF_PROG_TYPE_EXT is checked in > > > is_perfmon_prog_type(), and this change will make that case useless. > > > > Sure, I can drop it from v3 and make this function appear for HID only. > > For BPF_PROG_TYPE_LIRC_MODE2, I don't think this change will break userspace. > This is called from ir-keytable(1) which is called from udev. It should have > all the necessary permissions. > > In addition, the vast majority IR decoders are non-bpf. bpf ir decoders have > very few users at the moment. > > I am working on completely new userspace tooling which will make extensive > use of bpf ir decoding with full lircd and IRP compatibility, but this is not > finished yet (see https://github.com/seanyoung/cir). Thanks for these information. I guess change for BPF_PROG_TYPE_LIRC_MODE2 is ok then. Would you mind ack or review this change (either current version or a later version)? Thanks, Song
On Mon, Mar 7, 2022 at 7:12 PM Song Liu <song@kernel.org> wrote: > > On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > 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? > > Right, squash some patches 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. > > Totally agreed with the granularity of patches. I am not a big fan of patch > bombs either. :) > > I guess the problem I have with the current version is that I don't have a > big picture of the design while reading through relatively big patches. A > overview with the following information in the cover letter would be really > help here: > 1. How different types of programs are triggered (IRQ, user input, etc.); > 2. What are the operations and/or outcomes of these programs; > 3. How would programs of different types (or attach types) interact > with each other (via bpf maps? chaining?) > 4. What's the new uapi; > 5. New helpers and other logistics > > Sometimes, I find the changes to uapi are the key for me to understand the > patches, and I would like to see one or two patches with all the UAPI > changes (i.e. bpf_hid_attach_type). However, that may or may not apply to > this set due to granularity concerns. > > Does this make sense? > It definitely does. And as I read that, I realized that if I manage to get such a clear depiction of what HID-BPF is, it would certainly be a good idea to paste that in a file into the Documentation directory as well :) I think you have a slightly better picture now with the exchanges we are having on the individual patches, but I'll try to come out with that description in the cover letter for v3. Cheers, Benjamin
Hi Benjamin, On 04/03/2022 19:28, Benjamin Tissoires 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 I posted a couple of comments to the series, but other than that for the whole series you can use: Reviewed-by: Tero Kristo <tero.kristo@linux.intel.com> Tested-by: Tero Kristo <tero.kristo@linux.intel.com> I did test this with my USI-BPF program + userspace code, they work with few minor updates compared to previous version. -Tero > > > > [0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@redhat.com/T/#t > > > Benjamin Tissoires (28): > bpf: add new is_sys_admin_prog_type() helper > bpf: introduce hid program type > HID: hook up with bpf > libbpf: add HID program type and API > selftests/bpf: add tests for the HID-bpf initial implementation > samples/bpf: add new hid_mouse example > bpf/hid: add a new attach type to change the report descriptor > HID: allow to change the report descriptor from an eBPF program > libbpf: add new attach type BPF_HID_RDESC_FIXUP > selftests/bpf: add report descriptor fixup tests > samples/bpf: add a report descriptor fixup > bpf/hid: add hid_{get|set}_data helpers > HID: bpf: implement hid_bpf_get|set_data > selftests/bpf: add tests for hid_{get|set}_data helpers > bpf/hid: add new BPF type to trigger commands from userspace > libbpf: add new attach type BPF_HID_USER_EVENT > 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 > bpf/hid: add bpf_hid_raw_request helper function > HID: add implementation of bpf_hid_raw_request > selftests/bpf: add tests for bpf_hid_hw_request > bpf/verifier: prevent non GPL programs to be loaded against HID > HID: bpf: compute only the required buffer size for the device > HID: bpf: only call hid_bpf_raw_event() if a ctx is available > bpf/hid: Add a flag to add the program at the beginning of the list > libbpf: add handling for BPF_F_INSERT_HEAD in HID programs > selftests/bpf: Add a test for BPF_F_INSERT_HEAD > samples/bpf: fix bpf_program__attach_hid() api change > > drivers/hid/Makefile | 1 + > drivers/hid/hid-bpf.c | 361 +++++++++ > drivers/hid/hid-core.c | 34 +- > include/linux/bpf-hid.h | 129 +++ > include/linux/bpf_types.h | 4 + > include/linux/hid.h | 25 + > include/uapi/linux/bpf.h | 59 ++ > include/uapi/linux/bpf_hid.h | 50 ++ > kernel/bpf/Makefile | 3 + > kernel/bpf/hid.c | 652 +++++++++++++++ > kernel/bpf/syscall.c | 26 +- > kernel/bpf/verifier.c | 7 + > samples/bpf/.gitignore | 1 + > samples/bpf/Makefile | 4 + > samples/bpf/hid_mouse_kern.c | 91 +++ > samples/bpf/hid_mouse_user.c | 129 +++ > tools/include/uapi/linux/bpf.h | 59 ++ > tools/lib/bpf/libbpf.c | 22 +- > tools/lib/bpf/libbpf.h | 2 + > tools/lib/bpf/libbpf.map | 1 + > tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++ > tools/testing/selftests/bpf/progs/hid.c | 216 +++++ > 22 files changed, 2649 insertions(+), 15 deletions(-) > create mode 100644 drivers/hid/hid-bpf.c > create mode 100644 include/linux/bpf-hid.h > create mode 100644 include/uapi/linux/bpf_hid.h > create mode 100644 kernel/bpf/hid.c > create mode 100644 samples/bpf/hid_mouse_kern.c > create mode 100644 samples/bpf/hid_mouse_user.c > create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c > create mode 100644 tools/testing/selftests/bpf/progs/hid.c >
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 [0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@redhat.com/T/#t Benjamin Tissoires (28): bpf: add new is_sys_admin_prog_type() helper bpf: introduce hid program type HID: hook up with bpf libbpf: add HID program type and API selftests/bpf: add tests for the HID-bpf initial implementation samples/bpf: add new hid_mouse example bpf/hid: add a new attach type to change the report descriptor HID: allow to change the report descriptor from an eBPF program libbpf: add new attach type BPF_HID_RDESC_FIXUP selftests/bpf: add report descriptor fixup tests samples/bpf: add a report descriptor fixup bpf/hid: add hid_{get|set}_data helpers HID: bpf: implement hid_bpf_get|set_data selftests/bpf: add tests for hid_{get|set}_data helpers bpf/hid: add new BPF type to trigger commands from userspace libbpf: add new attach type BPF_HID_USER_EVENT 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 bpf/hid: add bpf_hid_raw_request helper function HID: add implementation of bpf_hid_raw_request selftests/bpf: add tests for bpf_hid_hw_request bpf/verifier: prevent non GPL programs to be loaded against HID HID: bpf: compute only the required buffer size for the device HID: bpf: only call hid_bpf_raw_event() if a ctx is available bpf/hid: Add a flag to add the program at the beginning of the list libbpf: add handling for BPF_F_INSERT_HEAD in HID programs selftests/bpf: Add a test for BPF_F_INSERT_HEAD samples/bpf: fix bpf_program__attach_hid() api change drivers/hid/Makefile | 1 + drivers/hid/hid-bpf.c | 361 +++++++++ drivers/hid/hid-core.c | 34 +- include/linux/bpf-hid.h | 129 +++ include/linux/bpf_types.h | 4 + include/linux/hid.h | 25 + include/uapi/linux/bpf.h | 59 ++ include/uapi/linux/bpf_hid.h | 50 ++ kernel/bpf/Makefile | 3 + kernel/bpf/hid.c | 652 +++++++++++++++ kernel/bpf/syscall.c | 26 +- kernel/bpf/verifier.c | 7 + samples/bpf/.gitignore | 1 + samples/bpf/Makefile | 4 + samples/bpf/hid_mouse_kern.c | 91 +++ samples/bpf/hid_mouse_user.c | 129 +++ tools/include/uapi/linux/bpf.h | 59 ++ tools/lib/bpf/libbpf.c | 22 +- tools/lib/bpf/libbpf.h | 2 + tools/lib/bpf/libbpf.map | 1 + tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 216 +++++ 22 files changed, 2649 insertions(+), 15 deletions(-) create mode 100644 drivers/hid/hid-bpf.c create mode 100644 include/linux/bpf-hid.h create mode 100644 include/uapi/linux/bpf_hid.h create mode 100644 kernel/bpf/hid.c create mode 100644 samples/bpf/hid_mouse_kern.c create mode 100644 samples/bpf/hid_mouse_user.c create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c create mode 100644 tools/testing/selftests/bpf/progs/hid.c