| Message ID | 20240607-hid_bpf_struct_ops-v2-0-3f95f4d02292@kernel.org |
|---|---|
| Headers | show |
| Series | HID: convert HID-BPF into using bpf_struct_ops | expand |
On Fri, Jun 7, 2024 at 8:28 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > +struct hid_bpf_ops { > + /* hid_id needs to stay first so we can easily change it > + * from userspace. > + */ > + int hid_id; > + u32 flags; > + > + /* private: internal use only */ > + struct list_head list; > + > + /* public: rest is public */ Didn't notice it before, but the above comments are misleading. The whole struct is private to the kernel and bpf prog, while registering, can only touch a handful. I'd drop "internal use" and "is public". It's not an uapi. > + > +/* fast path fields are put first to fill one cache line */ Also misleading. The whole struct fits one cache line. > + > + /** > + * @hid_device_event: called whenever an event is coming in from the device > + * > + * It has the following arguments: > + * > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx > + * > + * Return: %0 on success and keep processing; a positive > + * value to change the incoming size buffer; a negative > + * error code to interrupt the processing of this event > + * > + * Context: Interrupt context. > + */ > + int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type); > + > +/* control/slow paths put last */ > + > + /** > + * @hid_rdesc_fixup: called when the probe function parses the report descriptor > + * of the HID device > + * > + * It has the following arguments: > + * > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx > + * > + * Return: %0 on success and keep processing; a positive > + * value to change the incoming size buffer; a negative > + * error code to interrupt the processing of this device > + */ > + int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx); > + > + /* private: internal use only */ > + struct hid_device *hdev; > +} ____cacheline_aligned_in_smp; Such alignment is an overkill. I don't think you can measure the difference. > + > struct hid_bpf_prog_list { > u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV]; > u8 prog_cnt; > @@ -129,6 +188,10 @@ struct hid_bpf { > bool destroyed; /* prevents the assignment of any progs */ > > spinlock_t progs_lock; /* protects RCU update of progs */ > + > + struct hid_bpf_ops *rdesc_ops; > + struct list_head prog_list; > + struct mutex prog_list_lock; /* protects RCU update of prog_list */ mutex protects rcu update... sounds very odd. Just say that mutex protects prog_list update, because "RCU update" has a different meaning. RCU logic itself is what protects Update part of rcU. The rest looks good.
On Jun 07 2024, Alexei Starovoitov wrote: > On Fri, Jun 7, 2024 at 8:28 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > +struct hid_bpf_ops { > > + /* hid_id needs to stay first so we can easily change it > > + * from userspace. > > + */ > > + int hid_id; > > + u32 flags; > > + > > + /* private: internal use only */ > > + struct list_head list; > > + > > + /* public: rest is public */ > > Didn't notice it before, but the above comments are misleading. > The whole struct is private to the kernel and bpf prog, while > registering, can only touch a handful. > I'd drop "internal use" and "is public". It's not an uapi. Good point. The only purpose of this was to expose or not the fields in the doc, so I'll make it clear that this is the reason of "private/public". > > > + > > +/* fast path fields are put first to fill one cache line */ > > Also misleading. The whole struct fits one cache line. true :) > > > + > > + /** > > + * @hid_device_event: called whenever an event is coming in from the device > > + * > > + * It has the following arguments: > > + * > > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx > > + * > > + * Return: %0 on success and keep processing; a positive > > + * value to change the incoming size buffer; a negative > > + * error code to interrupt the processing of this event > > + * > > + * Context: Interrupt context. > > + */ > > + int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type); > > + > > +/* control/slow paths put last */ > > + > > + /** > > + * @hid_rdesc_fixup: called when the probe function parses the report descriptor > > + * of the HID device > > + * > > + * It has the following arguments: > > + * > > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx > > + * > > + * Return: %0 on success and keep processing; a positive > > + * value to change the incoming size buffer; a negative > > + * error code to interrupt the processing of this device > > + */ > > + int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx); > > + > > + /* private: internal use only */ > > + struct hid_device *hdev; > > +} ____cacheline_aligned_in_smp; > > Such alignment is an overkill. > I don't think you can measure the difference. ack > > > + > > struct hid_bpf_prog_list { > > u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV]; > > u8 prog_cnt; > > @@ -129,6 +188,10 @@ struct hid_bpf { > > bool destroyed; /* prevents the assignment of any progs */ > > > > spinlock_t progs_lock; /* protects RCU update of progs */ > > + > > + struct hid_bpf_ops *rdesc_ops; > > + struct list_head prog_list; > > + struct mutex prog_list_lock; /* protects RCU update of prog_list */ > > mutex protects rcu update... sounds very odd. > Just say that mutex protects prog_list update, because "RCU update" > has a different meaning. RCU logic itself is what protects Update part of rcU. Ack > > The rest looks good. Thanks for looking into it! Cheers, Benjamin
The purpose of this series is to rethink how HID-BPF is invoked. Currently it implies a jmp table, a prog fd bpf_map, a preloaded tracing bpf program and a lot of manual work for handling the bpf program lifetime and addition/removal. OTOH, bpf_struct_ops take care of most of the bpf handling leaving us with a simple list of ops pointers, and we can directly call the struct_ops program from the kernel as a regular function. The net gain right now is in term of code simplicity and lines of code removal (though is an API breakage), but udev-hid-bpf is able to handle such breakages. In the near future, we will be able to extend the HID-BPF struct_ops with entrypoints for hid_hw_raw_request() and hid_hw_output_report(), allowing for covering all of the initial use cases: - firewalling a HID device - fixing all of the HID device interactions (not just device events as it is right now). The matching user-space loader (udev-hid-bpf) MR is at https://gitlab.freedesktop.org/libevdev/udev-hid-bpf/-/merge_requests/86 I'll put it out of draft once this is merged. Cheers, Benjamin Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- Changes in v2: - drop HID_BPF_FLAGS enum and use BPF_F_BEFORE instead - fix .init_members to not open code member->offset - allow struct hid_device to be writeable from HID-BPF for its name, uniq and phys - Link to v1: https://lore.kernel.org/r/20240528-hid_bpf_struct_ops-v1-0-8c6663df27d8@kernel.org --- Benjamin Tissoires (16): HID: rename struct hid_bpf_ops into hid_ops HID: bpf: add hid_get/put_device() helpers HID: bpf: implement HID-BPF through bpf_struct_ops selftests/hid: convert the hid_bpf selftests with struct_ops HID: samples: convert the 2 HID-BPF samples into struct_ops HID: bpf: add defines for HID-BPF SEC in in-tree bpf fixes HID: bpf: convert in-tree fixes into struct_ops HID: bpf: remove tracing HID-BPF capability selftests/hid: add subprog call test Documentation: HID: amend HID-BPF for struct_ops Documentation: HID: add a small blurb on udev-hid-bpf HID: bpf: Artist24: remove unused variable HID: bpf: error on warnings when compiling bpf objects bpf: allow bpf helpers to be used into HID-BPF struct_ops HID: bpf: rework hid_bpf_ops_btf_struct_access HID: bpf: make part of struct hid_device writable Documentation/hid/hid-bpf.rst | 173 ++++--- drivers/hid/bpf/Makefile | 2 +- drivers/hid/bpf/entrypoints/Makefile | 93 ---- drivers/hid/bpf/entrypoints/README | 4 - drivers/hid/bpf/entrypoints/entrypoints.bpf.c | 25 - drivers/hid/bpf/entrypoints/entrypoints.lskel.h | 248 --------- drivers/hid/bpf/hid_bpf_dispatch.c | 266 +++------- drivers/hid/bpf/hid_bpf_dispatch.h | 12 +- drivers/hid/bpf/hid_bpf_jmp_table.c | 565 --------------------- drivers/hid/bpf/hid_bpf_struct_ops.c | 298 +++++++++++ drivers/hid/bpf/progs/FR-TEC__Raptor-Mach-2.bpf.c | 9 +- drivers/hid/bpf/progs/HP__Elite-Presenter.bpf.c | 6 +- drivers/hid/bpf/progs/Huion__Kamvas-Pro-19.bpf.c | 9 +- .../hid/bpf/progs/IOGEAR__Kaliber-MMOmentum.bpf.c | 6 +- drivers/hid/bpf/progs/Makefile | 2 +- .../hid/bpf/progs/Microsoft__XBox-Elite-2.bpf.c | 6 +- drivers/hid/bpf/progs/Wacom__ArtPen.bpf.c | 6 +- drivers/hid/bpf/progs/XPPen__Artist24.bpf.c | 10 +- drivers/hid/bpf/progs/XPPen__ArtistPro16Gen2.bpf.c | 24 +- drivers/hid/bpf/progs/hid_bpf.h | 5 + drivers/hid/hid-core.c | 6 +- include/linux/hid_bpf.h | 125 ++--- samples/hid/Makefile | 5 +- samples/hid/hid_bpf_attach.bpf.c | 18 - samples/hid/hid_bpf_attach.h | 14 - samples/hid/hid_mouse.bpf.c | 26 +- samples/hid/hid_mouse.c | 39 +- samples/hid/hid_surface_dial.bpf.c | 10 +- samples/hid/hid_surface_dial.c | 53 +- tools/testing/selftests/hid/hid_bpf.c | 100 +++- tools/testing/selftests/hid/progs/hid.c | 100 +++- .../testing/selftests/hid/progs/hid_bpf_helpers.h | 19 +- 32 files changed, 805 insertions(+), 1479 deletions(-) --- base-commit: 70ec81c2e2b4005465ad0d042e90b36087c36104 change-id: 20240513-hid_bpf_struct_ops-e3212a224555 Best regards,