| Message ID | 20220318161528.1531164-7-benjamin.tissoires@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Fri, Mar 18, 2022 at 9:17 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Make use of BPF_HID_ATTACH_RDESC_FIXUP so we can trigger an rdesc fixup > in the bpf world. > > Whenever the program gets attached/detached, the device is reconnected > meaning that userspace will see it disappearing and reappearing with > the new report descriptor. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > changes in v3: > - ensure the ctx.size is properly bounded by allocated size > - s/link_attached/post_link_attach/ > - removed the switch statement with only one case > > changes in v2: > - split the series by bpf/libbpf/hid/selftests and samples > --- > drivers/hid/hid-bpf.c | 62 ++++++++++++++++++++++++++++++++++++++++++ > drivers/hid/hid-core.c | 3 +- > include/linux/hid.h | 6 ++++ > 3 files changed, 70 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > index 5060ebcb9979..45c87ff47324 100644 > --- a/drivers/hid/hid-bpf.c > +++ b/drivers/hid/hid-bpf.c > @@ -50,6 +50,14 @@ static struct hid_device *hid_bpf_fd_to_hdev(int fd) > return hdev; > } > > +static int hid_reconnect(struct hid_device *hdev) > +{ > + if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) > + return device_reprobe(&hdev->dev); > + > + return 0; > +} > + > static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > { > int err = 0; > @@ -92,6 +100,12 @@ static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_ > return err; > } > > +static void hid_bpf_post_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > +{ > + if (type == BPF_HID_ATTACH_RDESC_FIXUP) > + hid_reconnect(hdev); > +} > + > static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_type type) > { > switch (type) { > @@ -99,6 +113,9 @@ static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_ty > kfree(hdev->bpf.device_data); > hdev->bpf.device_data = NULL; > break; > + case BPF_HID_ATTACH_RDESC_FIXUP: > + hid_reconnect(hdev); > + break; > default: > /* do nothing */ > break; > @@ -116,6 +133,9 @@ static int hid_bpf_run_progs(struct hid_device *hdev, struct hid_bpf_ctx_kern *c > case HID_BPF_DEVICE_EVENT: > type = BPF_HID_ATTACH_DEVICE_EVENT; > break; > + case HID_BPF_RDESC_FIXUP: > + type = BPF_HID_ATTACH_RDESC_FIXUP; > + break; > default: > return -EINVAL; > } > @@ -155,11 +175,53 @@ u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) > return ctx.data; > } > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > +{ > + int ret; > + struct hid_bpf_ctx_kern ctx = { > + .type = HID_BPF_RDESC_FIXUP, > + .hdev = hdev, > + .size = *size, > + }; > + > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) Do we need to lock bpf_hid_mutex before calling bpf_hid_link_empty()? (or maybe we already did?) > + goto ignore_bpf; > + > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > + if (!ctx.data) > + goto ignore_bpf; > + > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > + > + ret = hid_bpf_run_progs(hdev, &ctx); > + if (ret) > + goto ignore_bpf; > + > + if (ctx.size > ctx.allocated_size) > + goto ignore_bpf; > + > + *size = ctx.size; > + > + if (*size) { > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > + } else { > + rdesc = NULL; > + kfree(ctx.data); > + } > + > + return rdesc; > + > + ignore_bpf: > + kfree(ctx.data); > + return kmemdup(rdesc, *size, GFP_KERNEL); > +} > + > int __init hid_bpf_module_init(void) > { > struct bpf_hid_hooks hooks = { > .hdev_from_fd = hid_bpf_fd_to_hdev, > .pre_link_attach = hid_bpf_pre_link_attach, > + .post_link_attach = hid_bpf_post_link_attach, > .array_detach = hid_bpf_array_detach, > }; > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 937fab7eb9c6..3182c39db006 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > return -ENODEV; > size = device->dev_rsize; > > - buf = kmemdup(start, size, GFP_KERNEL); > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > + buf = hid_bpf_report_fixup(device, start, &size); > if (buf == NULL) > return -ENOMEM; > > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 8fd79011f461..66d949d10b78 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -1213,10 +1213,16 @@ do { \ > > #ifdef CONFIG_BPF > u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size); > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size); > int hid_bpf_module_init(void); > void hid_bpf_module_exit(void); > #else > static inline u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size) { return rd; } > +static inline u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, > + unsigned int *size) > +{ > + return kmemdup(rdesc, *size, GFP_KERNEL); > +} > static inline int hid_bpf_module_init(void) { return 0; } > static inline void hid_bpf_module_exit(void) {} > #endif > -- > 2.35.1 >
On Fri, Mar 18, 2022 at 10:10 PM Song Liu <song@kernel.org> wrote: > > On Fri, Mar 18, 2022 at 9:17 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > Make use of BPF_HID_ATTACH_RDESC_FIXUP so we can trigger an rdesc fixup > > in the bpf world. > > > > Whenever the program gets attached/detached, the device is reconnected > > meaning that userspace will see it disappearing and reappearing with > > the new report descriptor. > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > --- > > > > changes in v3: > > - ensure the ctx.size is properly bounded by allocated size > > - s/link_attached/post_link_attach/ > > - removed the switch statement with only one case > > > > changes in v2: > > - split the series by bpf/libbpf/hid/selftests and samples > > --- > > drivers/hid/hid-bpf.c | 62 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/hid/hid-core.c | 3 +- > > include/linux/hid.h | 6 ++++ > > 3 files changed, 70 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > > index 5060ebcb9979..45c87ff47324 100644 > > --- a/drivers/hid/hid-bpf.c > > +++ b/drivers/hid/hid-bpf.c > > @@ -50,6 +50,14 @@ static struct hid_device *hid_bpf_fd_to_hdev(int fd) > > return hdev; > > } > > > > +static int hid_reconnect(struct hid_device *hdev) > > +{ > > + if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) > > + return device_reprobe(&hdev->dev); > > + > > + return 0; > > +} > > + > > static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > { > > int err = 0; > > @@ -92,6 +100,12 @@ static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_ > > return err; > > } > > > > +static void hid_bpf_post_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > +{ > > + if (type == BPF_HID_ATTACH_RDESC_FIXUP) > > + hid_reconnect(hdev); > > +} > > + > > static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > { > > switch (type) { > > @@ -99,6 +113,9 @@ static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_ty > > kfree(hdev->bpf.device_data); > > hdev->bpf.device_data = NULL; > > break; > > + case BPF_HID_ATTACH_RDESC_FIXUP: > > + hid_reconnect(hdev); > > + break; > > default: > > /* do nothing */ > > break; > > @@ -116,6 +133,9 @@ static int hid_bpf_run_progs(struct hid_device *hdev, struct hid_bpf_ctx_kern *c > > case HID_BPF_DEVICE_EVENT: > > type = BPF_HID_ATTACH_DEVICE_EVENT; > > break; > > + case HID_BPF_RDESC_FIXUP: > > + type = BPF_HID_ATTACH_RDESC_FIXUP; > > + break; > > default: > > return -EINVAL; > > } > > @@ -155,11 +175,53 @@ u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) > > return ctx.data; > > } > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > +{ > > + int ret; > > + struct hid_bpf_ctx_kern ctx = { > > + .type = HID_BPF_RDESC_FIXUP, > > + .hdev = hdev, > > + .size = *size, > > + }; > > + > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > Do we need to lock bpf_hid_mutex before calling bpf_hid_link_empty()? > (or maybe we > already did?) The mutex is not locked before this call, indeed. However, bpf_hid_link_empty() is an inlined function that just calls in the end list_empty(). Given that all the list heads are created just once for the entire life of the HID device, I *think* this is thread safe and does not require mutex locking. (I might be wrong) So when first plugging in the device, if there is a fighting process that attempts to add a program, if the program managed to insert itself before we enter this code, then the list won't be empty and we will execute BPF_PROG_RUN_ARRAY(), and if not, well, we ignore it and wait for reconnect(). But now I am starting to wonder if I need to also protect BPF_PROG_RUN_ARRAY() under bpf_hid_mutex... Cheers, Benjamin > > > > + goto ignore_bpf; > > + > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > + if (!ctx.data) > > + goto ignore_bpf; > > + > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > + > > + ret = hid_bpf_run_progs(hdev, &ctx); > > + if (ret) > > + goto ignore_bpf; > > + > > + if (ctx.size > ctx.allocated_size) > > + goto ignore_bpf; > > + > > + *size = ctx.size; > > + > > + if (*size) { > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > + } else { > > + rdesc = NULL; > > + kfree(ctx.data); > > + } > > + > > + return rdesc; > > + > > + ignore_bpf: > > + kfree(ctx.data); > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > +} > > + > > int __init hid_bpf_module_init(void) > > { > > struct bpf_hid_hooks hooks = { > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > .pre_link_attach = hid_bpf_pre_link_attach, > > + .post_link_attach = hid_bpf_post_link_attach, > > .array_detach = hid_bpf_array_detach, > > }; > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 937fab7eb9c6..3182c39db006 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > return -ENODEV; > > size = device->dev_rsize; > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > + buf = hid_bpf_report_fixup(device, start, &size); > > if (buf == NULL) > > return -ENOMEM; > > > > diff --git a/include/linux/hid.h b/include/linux/hid.h > > index 8fd79011f461..66d949d10b78 100644 > > --- a/include/linux/hid.h > > +++ b/include/linux/hid.h > > @@ -1213,10 +1213,16 @@ do { \ > > > > #ifdef CONFIG_BPF > > u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size); > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size); > > int hid_bpf_module_init(void); > > void hid_bpf_module_exit(void); > > #else > > static inline u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size) { return rd; } > > +static inline u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, > > + unsigned int *size) > > +{ > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > +} > > static inline int hid_bpf_module_init(void) { return 0; } > > static inline void hid_bpf_module_exit(void) {} > > #endif > > -- > > 2.35.1 > > >
On Mon, Mar 21, 2022 at 9:20 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Fri, Mar 18, 2022 at 10:10 PM Song Liu <song@kernel.org> wrote: > > > > On Fri, Mar 18, 2022 at 9:17 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Make use of BPF_HID_ATTACH_RDESC_FIXUP so we can trigger an rdesc fixup > > > in the bpf world. > > > > > > Whenever the program gets attached/detached, the device is reconnected > > > meaning that userspace will see it disappearing and reappearing with > > > the new report descriptor. > > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > > > --- > > > > > > changes in v3: > > > - ensure the ctx.size is properly bounded by allocated size > > > - s/link_attached/post_link_attach/ > > > - removed the switch statement with only one case > > > > > > changes in v2: > > > - split the series by bpf/libbpf/hid/selftests and samples > > > --- > > > drivers/hid/hid-bpf.c | 62 ++++++++++++++++++++++++++++++++++++++++++ > > > drivers/hid/hid-core.c | 3 +- > > > include/linux/hid.h | 6 ++++ > > > 3 files changed, 70 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > > > index 5060ebcb9979..45c87ff47324 100644 > > > --- a/drivers/hid/hid-bpf.c > > > +++ b/drivers/hid/hid-bpf.c > > > @@ -50,6 +50,14 @@ static struct hid_device *hid_bpf_fd_to_hdev(int fd) > > > return hdev; > > > } > > > > > > +static int hid_reconnect(struct hid_device *hdev) > > > +{ > > > + if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) > > > + return device_reprobe(&hdev->dev); > > > + > > > + return 0; > > > +} > > > + > > > static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > > { > > > int err = 0; > > > @@ -92,6 +100,12 @@ static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_ > > > return err; > > > } > > > > > > +static void hid_bpf_post_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > > +{ > > > + if (type == BPF_HID_ATTACH_RDESC_FIXUP) > > > + hid_reconnect(hdev); > > > +} > > > + > > > static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_type type) > > > { > > > switch (type) { > > > @@ -99,6 +113,9 @@ static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_ty > > > kfree(hdev->bpf.device_data); > > > hdev->bpf.device_data = NULL; > > > break; > > > + case BPF_HID_ATTACH_RDESC_FIXUP: > > > + hid_reconnect(hdev); > > > + break; > > > default: > > > /* do nothing */ > > > break; > > > @@ -116,6 +133,9 @@ static int hid_bpf_run_progs(struct hid_device *hdev, struct hid_bpf_ctx_kern *c > > > case HID_BPF_DEVICE_EVENT: > > > type = BPF_HID_ATTACH_DEVICE_EVENT; > > > break; > > > + case HID_BPF_RDESC_FIXUP: > > > + type = BPF_HID_ATTACH_RDESC_FIXUP; > > > + break; > > > default: > > > return -EINVAL; > > > } > > > @@ -155,11 +175,53 @@ u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) > > > return ctx.data; > > > } > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > > +{ > > > + int ret; > > > + struct hid_bpf_ctx_kern ctx = { > > > + .type = HID_BPF_RDESC_FIXUP, > > > + .hdev = hdev, > > > + .size = *size, > > > + }; > > > + > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > > > Do we need to lock bpf_hid_mutex before calling bpf_hid_link_empty()? > > (or maybe we > > already did?) > > The mutex is not locked before this call, indeed. > > However, bpf_hid_link_empty() is an inlined function that just calls > in the end list_empty(). Given that all the list heads are created > just once for the entire life of the HID device, I *think* this is > thread safe and does not require mutex locking. Hmm.. I guess you are right. > > (I might be wrong) > > So when first plugging in the device, if there is a fighting process > that attempts to add a program, if the program managed to insert > itself before we enter this code, then the list won't be empty and we > will execute BPF_PROG_RUN_ARRAY(), and if not, well, we ignore it and > wait for reconnect(). > > But now I am starting to wonder if I need to also protect > BPF_PROG_RUN_ARRAY() under bpf_hid_mutex... I think this is not necessary. Thanks, Song
Hi Alexei, On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > +{ > > + int ret; > > + struct hid_bpf_ctx_kern ctx = { > > + .type = HID_BPF_RDESC_FIXUP, > > + .hdev = hdev, > > + .size = *size, > > + }; > > + > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > + goto ignore_bpf; > > + > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > + if (!ctx.data) > > + goto ignore_bpf; > > + > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > + > > + ret = hid_bpf_run_progs(hdev, &ctx); > > + if (ret) > > + goto ignore_bpf; > > + > > + if (ctx.size > ctx.allocated_size) > > + goto ignore_bpf; > > + > > + *size = ctx.size; > > + > > + if (*size) { > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > + } else { > > + rdesc = NULL; > > + kfree(ctx.data); > > + } > > + > > + return rdesc; > > + > > + ignore_bpf: > > + kfree(ctx.data); > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > +} > > + > > int __init hid_bpf_module_init(void) > > { > > struct bpf_hid_hooks hooks = { > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > .pre_link_attach = hid_bpf_pre_link_attach, > > + .post_link_attach = hid_bpf_post_link_attach, > > .array_detach = hid_bpf_array_detach, > > }; > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 937fab7eb9c6..3182c39db006 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > return -ENODEV; > > size = device->dev_rsize; > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > + buf = hid_bpf_report_fixup(device, start, &size); > > Looking at this patch and the majority of other patches... > the code is doing a lot of work to connect HID side with bpf. > At the same time the evolution of the patch series suggests > that these hook points are not quite stable. More hooks and > helpers are being added. > It tells us that it's way too early to introduce a stable > interface between HID and bpf. I understand that you might be under the impression that the interface is changing a lot, but this is mostly due to my poor knowledge of all the arcanes of eBPF. The overall way HID-BPF works is to work on a single array, and we should pretty much be sorted out. There are a couple of helpers to be able to communicate with the device, but the API has been stable in the kernel for those for quite some time now. The variations in the hooks is mostly because I don't know what is the best representation we can use in eBPF for those, and the review process is changing that. > We suggest to use __weak global functions and unstable kfunc helpers > to achieve the same goal. > This way HID side and bpf side can evolve without introducing > stable uapi burden. > For example this particular patch can be compressed to: > __weak int hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, > unsigned int *size) > { > return 0; > } > ALLOW_ERROR_INJECTION(ALLOW_ERROR_INJECTION, ERRNO); > > - buf = kmemdup(start, size, GFP_KERNEL); > + if (!hid_bpf_report_fixup(device, start, &size)) > + buf = kmemdup(start, size, GFP_KERNEL); > > Then bpf program can replace hid_bpf_report_fixup function and adjust its > return value while reading args. I appreciate the suggestion and gave it a try, but AFAICT this doesn't work for HID (please correct me if I am wrong): - I tried to use __weak to replace the ugly struct bpf_hid_hooks This struct is in place simply because the HID module can be compiled in as a kernel module and we might not have the symbols available from kernel/bpf when it is a separate module. Either I did something wrong, but it seems that when we load the module in the kernel, there is no magic that overrides the weak symbols from the ones from the modules. - for hid_bpf_report_fixup(), this would mean that a BPF program could overwrite the function This is great, but I need to have one program per device, not one globally defined function. I can not have a generic report_fixup in the system, simply because you might need 2 different functions for 2 different devices. We could solve that by auto-generating the bpf program based on which devices are available, but that would mean that users will see a reconnect of all of their input devices when they plug in a new one, and will also require them to have LLVM installed, which I do not want. - for stuff like hid_bpf_raw_event(), I want to have multiple programs attached to the various devices, and not necessarily the same across devices. This is basically the same as above, except that I need to chain programs. For instance, we could have a program that "fixes" one device, but I also want to attach a tracing program on top of it to monitor what is happening. > > Similar approach can be done with all other hooks. > > Once api between HID and bpf stabilizes we can replace nop functions > with writeable tracepoints to make things a bit more stable > while still allowing for change of the interface in the future. > > The amount of bpf specific code in HID core will be close to zero > while bpf can be used to flexibly tweak it. Again, I like the idea, but I clearly don't see where you want to go.
On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Alexei, > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > > +{ > > > + int ret; > > > + struct hid_bpf_ctx_kern ctx = { > > > + .type = HID_BPF_RDESC_FIXUP, > > > + .hdev = hdev, > > > + .size = *size, > > > + }; > > > + > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > > + goto ignore_bpf; > > > + > > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > > + if (!ctx.data) > > > + goto ignore_bpf; > > > + > > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > > + > > > + ret = hid_bpf_run_progs(hdev, &ctx); > > > + if (ret) > > > + goto ignore_bpf; > > > + > > > + if (ctx.size > ctx.allocated_size) > > > + goto ignore_bpf; > > > + > > > + *size = ctx.size; > > > + > > > + if (*size) { > > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > > + } else { > > > + rdesc = NULL; > > > + kfree(ctx.data); > > > + } > > > + > > > + return rdesc; > > > + > > > + ignore_bpf: > > > + kfree(ctx.data); > > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > > +} > > > + > > > int __init hid_bpf_module_init(void) > > > { > > > struct bpf_hid_hooks hooks = { > > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > > .pre_link_attach = hid_bpf_pre_link_attach, > > > + .post_link_attach = hid_bpf_post_link_attach, > > > .array_detach = hid_bpf_array_detach, > > > }; > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > index 937fab7eb9c6..3182c39db006 100644 > > > --- a/drivers/hid/hid-core.c > > > +++ b/drivers/hid/hid-core.c > > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > > return -ENODEV; > > > size = device->dev_rsize; > > > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > > + buf = hid_bpf_report_fixup(device, start, &size); > > > > Looking at this patch and the majority of other patches... > > the code is doing a lot of work to connect HID side with bpf. > > At the same time the evolution of the patch series suggests > > that these hook points are not quite stable. More hooks and > > helpers are being added. > > It tells us that it's way too early to introduce a stable > > interface between HID and bpf. > > I understand that you might be under the impression that the interface > is changing a lot, but this is mostly due to my poor knowledge of all > the arcanes of eBPF. > The overall way HID-BPF works is to work on a single array, and we > should pretty much be sorted out. There are a couple of helpers to be > able to communicate with the device, but the API has been stable in > the kernel for those for quite some time now. > > The variations in the hooks is mostly because I don't know what is the > best representation we can use in eBPF for those, and the review > process is changing that. I think such a big feature as this one, especially that most BPF folks are (probably) not familiar with the HID subsystem in the kernel, would benefit from a bit of live discussion during BPF office hours. Do you think you can give a short overview of what you are trying to achieve with some background context on HID specifics at one of the next BPF office hours? We have a meeting scheduled every week on Thursday, 9am Pacific time. But people need to put their topic onto the agenda, otherwise the meeting is cancelled. See [0] for spreadsheet and links to Zoom meeting, agenda, etc. [0] https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0AejEveU [...]
On Sun, Mar 27, 2022 at 11:57 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Fri, Mar 25, 2022 at 6:00 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > Hi Alexei, > > > > > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > > > > +{ > > > > > + int ret; > > > > > + struct hid_bpf_ctx_kern ctx = { > > > > > + .type = HID_BPF_RDESC_FIXUP, > > > > > + .hdev = hdev, > > > > > + .size = *size, > > > > > + }; > > > > > + > > > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > > > > + goto ignore_bpf; > > > > > + > > > > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > > > > + if (!ctx.data) > > > > > + goto ignore_bpf; > > > > > + > > > > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > > > > + > > > > > + ret = hid_bpf_run_progs(hdev, &ctx); > > > > > + if (ret) > > > > > + goto ignore_bpf; > > > > > + > > > > > + if (ctx.size > ctx.allocated_size) > > > > > + goto ignore_bpf; > > > > > + > > > > > + *size = ctx.size; > > > > > + > > > > > + if (*size) { > > > > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > > > > + } else { > > > > > + rdesc = NULL; > > > > > + kfree(ctx.data); > > > > > + } > > > > > + > > > > > + return rdesc; > > > > > + > > > > > + ignore_bpf: > > > > > + kfree(ctx.data); > > > > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > > > > +} > > > > > + > > > > > int __init hid_bpf_module_init(void) > > > > > { > > > > > struct bpf_hid_hooks hooks = { > > > > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > > > > .pre_link_attach = hid_bpf_pre_link_attach, > > > > > + .post_link_attach = hid_bpf_post_link_attach, > > > > > .array_detach = hid_bpf_array_detach, > > > > > }; > > > > > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > > > index 937fab7eb9c6..3182c39db006 100644 > > > > > --- a/drivers/hid/hid-core.c > > > > > +++ b/drivers/hid/hid-core.c > > > > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > > > > return -ENODEV; > > > > > size = device->dev_rsize; > > > > > > > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > > > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > > > > + buf = hid_bpf_report_fixup(device, start, &size); > > > > > > > > Looking at this patch and the majority of other patches... > > > > the code is doing a lot of work to connect HID side with bpf. > > > > At the same time the evolution of the patch series suggests > > > > that these hook points are not quite stable. More hooks and > > > > helpers are being added. > > > > It tells us that it's way too early to introduce a stable > > > > interface between HID and bpf. > > > > > > I understand that you might be under the impression that the interface > > > is changing a lot, but this is mostly due to my poor knowledge of all > > > the arcanes of eBPF. > > > The overall way HID-BPF works is to work on a single array, and we > > > should pretty much be sorted out. There are a couple of helpers to be > > > able to communicate with the device, but the API has been stable in > > > the kernel for those for quite some time now. > > > > > > The variations in the hooks is mostly because I don't know what is the > > > best representation we can use in eBPF for those, and the review > > > process is changing that. > > > > I think such a big feature as this one, especially that most BPF folks > > are (probably) not familiar with the HID subsystem in the kernel, > > would benefit from a bit of live discussion during BPF office hours. > > Do you think you can give a short overview of what you are trying to > > achieve with some background context on HID specifics at one of the > > next BPF office hours? We have a meeting scheduled every week on > > Thursday, 9am Pacific time. But people need to put their topic onto > > the agenda, otherwise the meeting is cancelled. See [0] for > > spreadsheet and links to Zoom meeting, agenda, etc. > > This sounds like a good idea. I just added my topic on the agenda and > will prepare some slides. > Great! Unfortunately I personally have a conflict this week and won't be able to attend, so I'll have to catch up somehow through word of mouth :( Next week's BPF office hours would be best, but I don't want to delay discussions just because of me. > Cheers, > Benjamin > > > > > [0] https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0AejEveU > > > > [...] > > >
On Mon, Mar 28, 2022 at 11:35 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Sun, Mar 27, 2022 at 11:57 PM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Fri, Mar 25, 2022 at 6:00 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > Hi Alexei, > > > > > > > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > > > > > +{ > > > > > > + int ret; > > > > > > + struct hid_bpf_ctx_kern ctx = { > > > > > > + .type = HID_BPF_RDESC_FIXUP, > > > > > > + .hdev = hdev, > > > > > > + .size = *size, > > > > > > + }; > > > > > > + > > > > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > > > > > + goto ignore_bpf; > > > > > > + > > > > > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > > > > > + if (!ctx.data) > > > > > > + goto ignore_bpf; > > > > > > + > > > > > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > > > > > + > > > > > > + ret = hid_bpf_run_progs(hdev, &ctx); > > > > > > + if (ret) > > > > > > + goto ignore_bpf; > > > > > > + > > > > > > + if (ctx.size > ctx.allocated_size) > > > > > > + goto ignore_bpf; > > > > > > + > > > > > > + *size = ctx.size; > > > > > > + > > > > > > + if (*size) { > > > > > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > > > > > + } else { > > > > > > + rdesc = NULL; > > > > > > + kfree(ctx.data); > > > > > > + } > > > > > > + > > > > > > + return rdesc; > > > > > > + > > > > > > + ignore_bpf: > > > > > > + kfree(ctx.data); > > > > > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > > > > > +} > > > > > > + > > > > > > int __init hid_bpf_module_init(void) > > > > > > { > > > > > > struct bpf_hid_hooks hooks = { > > > > > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > > > > > .pre_link_attach = hid_bpf_pre_link_attach, > > > > > > + .post_link_attach = hid_bpf_post_link_attach, > > > > > > .array_detach = hid_bpf_array_detach, > > > > > > }; > > > > > > > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > > > > index 937fab7eb9c6..3182c39db006 100644 > > > > > > --- a/drivers/hid/hid-core.c > > > > > > +++ b/drivers/hid/hid-core.c > > > > > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > > > > > return -ENODEV; > > > > > > size = device->dev_rsize; > > > > > > > > > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > > > > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > > > > > + buf = hid_bpf_report_fixup(device, start, &size); > > > > > > > > > > Looking at this patch and the majority of other patches... > > > > > the code is doing a lot of work to connect HID side with bpf. > > > > > At the same time the evolution of the patch series suggests > > > > > that these hook points are not quite stable. More hooks and > > > > > helpers are being added. > > > > > It tells us that it's way too early to introduce a stable > > > > > interface between HID and bpf. > > > > > > > > I understand that you might be under the impression that the interface > > > > is changing a lot, but this is mostly due to my poor knowledge of all > > > > the arcanes of eBPF. > > > > The overall way HID-BPF works is to work on a single array, and we > > > > should pretty much be sorted out. There are a couple of helpers to be > > > > able to communicate with the device, but the API has been stable in > > > > the kernel for those for quite some time now. > > > > > > > > The variations in the hooks is mostly because I don't know what is the > > > > best representation we can use in eBPF for those, and the review > > > > process is changing that. > > > > > > I think such a big feature as this one, especially that most BPF folks > > > are (probably) not familiar with the HID subsystem in the kernel, > > > would benefit from a bit of live discussion during BPF office hours. > > > Do you think you can give a short overview of what you are trying to > > > achieve with some background context on HID specifics at one of the > > > next BPF office hours? We have a meeting scheduled every week on > > > Thursday, 9am Pacific time. But people need to put their topic onto > > > the agenda, otherwise the meeting is cancelled. See [0] for > > > spreadsheet and links to Zoom meeting, agenda, etc. > > > > This sounds like a good idea. I just added my topic on the agenda and > > will prepare some slides. > > > > Great! Unfortunately I personally have a conflict this week and won't > be able to attend, so I'll have to catch up somehow through word of > mouth :( Next week's BPF office hours would be best, but I don't want > to delay discussions just because of me. OK. FWIW, I'll have slides publicly available once I'll do a final roundup on them. Hopefully that will give you enough context on HID to understand the problem. If there are too many conflicts we can surely delay by a week, but I would rather have the discussion happening sooner :/ Cheers, Benjamin > > > > > > > > [0] https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0AejEveU > > > > > > [...] > > > > > >
On Tue, Mar 29, 2022 at 3:53 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Mon, Mar 28, 2022 at 11:35 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Sun, Mar 27, 2022 at 11:57 PM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Fri, Mar 25, 2022 at 6:00 PM Andrii Nakryiko > > > <andrii.nakryiko@gmail.com> wrote: > > > > > > > > On Wed, Mar 23, 2022 at 9:08 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > Hi Alexei, > > > > > > > > > > On Tue, Mar 22, 2022 at 11:51 PM Alexei Starovoitov > > > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > > > > > On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires > > > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > > > > > > > > > > +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) > > > > > > > +{ > > > > > > > + int ret; > > > > > > > + struct hid_bpf_ctx_kern ctx = { > > > > > > > + .type = HID_BPF_RDESC_FIXUP, > > > > > > > + .hdev = hdev, > > > > > > > + .size = *size, > > > > > > > + }; > > > > > > > + > > > > > > > + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) > > > > > > > + goto ignore_bpf; > > > > > > > + > > > > > > > + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); > > > > > > > + if (!ctx.data) > > > > > > > + goto ignore_bpf; > > > > > > > + > > > > > > > + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; > > > > > > > + > > > > > > > + ret = hid_bpf_run_progs(hdev, &ctx); > > > > > > > + if (ret) > > > > > > > + goto ignore_bpf; > > > > > > > + > > > > > > > + if (ctx.size > ctx.allocated_size) > > > > > > > + goto ignore_bpf; > > > > > > > + > > > > > > > + *size = ctx.size; > > > > > > > + > > > > > > > + if (*size) { > > > > > > > + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); > > > > > > > + } else { > > > > > > > + rdesc = NULL; > > > > > > > + kfree(ctx.data); > > > > > > > + } > > > > > > > + > > > > > > > + return rdesc; > > > > > > > + > > > > > > > + ignore_bpf: > > > > > > > + kfree(ctx.data); > > > > > > > + return kmemdup(rdesc, *size, GFP_KERNEL); > > > > > > > +} > > > > > > > + > > > > > > > int __init hid_bpf_module_init(void) > > > > > > > { > > > > > > > struct bpf_hid_hooks hooks = { > > > > > > > .hdev_from_fd = hid_bpf_fd_to_hdev, > > > > > > > .pre_link_attach = hid_bpf_pre_link_attach, > > > > > > > + .post_link_attach = hid_bpf_post_link_attach, > > > > > > > .array_detach = hid_bpf_array_detach, > > > > > > > }; > > > > > > > > > > > > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > > > > > > index 937fab7eb9c6..3182c39db006 100644 > > > > > > > --- a/drivers/hid/hid-core.c > > > > > > > +++ b/drivers/hid/hid-core.c > > > > > > > @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) > > > > > > > return -ENODEV; > > > > > > > size = device->dev_rsize; > > > > > > > > > > > > > > - buf = kmemdup(start, size, GFP_KERNEL); > > > > > > > + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ > > > > > > > + buf = hid_bpf_report_fixup(device, start, &size); > > > > > > > > > > > > Looking at this patch and the majority of other patches... > > > > > > the code is doing a lot of work to connect HID side with bpf. > > > > > > At the same time the evolution of the patch series suggests > > > > > > that these hook points are not quite stable. More hooks and > > > > > > helpers are being added. > > > > > > It tells us that it's way too early to introduce a stable > > > > > > interface between HID and bpf. > > > > > > > > > > I understand that you might be under the impression that the interface > > > > > is changing a lot, but this is mostly due to my poor knowledge of all > > > > > the arcanes of eBPF. > > > > > The overall way HID-BPF works is to work on a single array, and we > > > > > should pretty much be sorted out. There are a couple of helpers to be > > > > > able to communicate with the device, but the API has been stable in > > > > > the kernel for those for quite some time now. > > > > > > > > > > The variations in the hooks is mostly because I don't know what is the > > > > > best representation we can use in eBPF for those, and the review > > > > > process is changing that. > > > > > > > > I think such a big feature as this one, especially that most BPF folks > > > > are (probably) not familiar with the HID subsystem in the kernel, > > > > would benefit from a bit of live discussion during BPF office hours. > > > > Do you think you can give a short overview of what you are trying to > > > > achieve with some background context on HID specifics at one of the > > > > next BPF office hours? We have a meeting scheduled every week on > > > > Thursday, 9am Pacific time. But people need to put their topic onto > > > > the agenda, otherwise the meeting is cancelled. See [0] for > > > > spreadsheet and links to Zoom meeting, agenda, etc. > > > > > > This sounds like a good idea. I just added my topic on the agenda and > > > will prepare some slides. > > > > > > > Great! Unfortunately I personally have a conflict this week and won't > > be able to attend, so I'll have to catch up somehow through word of > > mouth :( Next week's BPF office hours would be best, but I don't want > > to delay discussions just because of me. > > OK. FWIW, I'll have slides publicly available once I'll do a final > roundup on them. Hopefully that will give you enough context on HID to > understand the problem. > If there are too many conflicts we can surely delay by a week, but I > would rather have the discussion happening sooner :/ > Follow up on the discussion we had yesterday: - this patchset should be dropped in its current form, because it's the "old way" of extending BPF: The new goal is to extend the BPF core so we work around the limitations we find in HID so other subsystems can use the same approach. This is what Alexei was explaining in his answer in this thread. We need HID to solely declare which functions are replaced (by abusing SEC("fentry/function_name") - or fexit, fmod_ret), and also allow HID to declare its BPF API without having to change a single bit in the BPF core. This approach should allow other subsystems (USB???) to use a similar approach without having to mess up with BPF too much. - being able to attach a SEC("fentry/function") to a particular HID device requires some changes in the BPF core We can live without it, but it would be way cleaner to selectively attach those trampolines to only the selected devices without having to hand that decision to the BPF programmers - patch 12 and 13 (the bits access helper) should be dropped This should be done entirely in BPF, with a BPF helper as a .h that users include instead of having to maintain yet another UAPI - Daniel raised the question of a flag which tells other BPF that a bug is fixed In the case we drop in the filesystem a BPF program to fix a particular device, and then we include that same program in the kernel, how can we know that the feature is already fixed? It's still an open question. - including BPF objects in the kernel is definitively doable through lskel But again, the question is how do we attach those programs to a particular device? - to export a new UAPI BPF helper, we should rely on kfunc as explained in Alexei's email However, the current kfunc and ALLOW_ERROR_INJECTION API doesn't clearly allow to define which function is sleepable or not, making it currently hard to define that behavior. Once the BPF kAPI allows to mark kfunc and ALLOW_ERROR_INJECTION (_weak) functions to be sleepable or not, we will be able to define the BPF hooks directly in HID without having to touch the BPF core. - running a SEC("fentry/...") BPF program as a standalone from userspace through a syscall is possible - When defining a SEC("fentry/..."), all parameters are currently read-only we either need a hid_bpf_get_data() hooks where we can mark the output as being read-write, or we need to be able to tag the target function as read-write for (part of) its arguments (I would lean toward the extra helper that might be much easier to declare IMO). I think that's it from yesterday's discussion. Many thanks for listening to me and proposing a better solution. Now I have a lot to work on. Cheers, Benjamin
diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c index 5060ebcb9979..45c87ff47324 100644 --- a/drivers/hid/hid-bpf.c +++ b/drivers/hid/hid-bpf.c @@ -50,6 +50,14 @@ static struct hid_device *hid_bpf_fd_to_hdev(int fd) return hdev; } +static int hid_reconnect(struct hid_device *hdev) +{ + if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) + return device_reprobe(&hdev->dev); + + return 0; +} + static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) { int err = 0; @@ -92,6 +100,12 @@ static int hid_bpf_pre_link_attach(struct hid_device *hdev, enum bpf_hid_attach_ return err; } +static void hid_bpf_post_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type) +{ + if (type == BPF_HID_ATTACH_RDESC_FIXUP) + hid_reconnect(hdev); +} + static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_type type) { switch (type) { @@ -99,6 +113,9 @@ static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_ty kfree(hdev->bpf.device_data); hdev->bpf.device_data = NULL; break; + case BPF_HID_ATTACH_RDESC_FIXUP: + hid_reconnect(hdev); + break; default: /* do nothing */ break; @@ -116,6 +133,9 @@ static int hid_bpf_run_progs(struct hid_device *hdev, struct hid_bpf_ctx_kern *c case HID_BPF_DEVICE_EVENT: type = BPF_HID_ATTACH_DEVICE_EVENT; break; + case HID_BPF_RDESC_FIXUP: + type = BPF_HID_ATTACH_RDESC_FIXUP; + break; default: return -EINVAL; } @@ -155,11 +175,53 @@ u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) return ctx.data; } +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) +{ + int ret; + struct hid_bpf_ctx_kern ctx = { + .type = HID_BPF_RDESC_FIXUP, + .hdev = hdev, + .size = *size, + }; + + if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_RDESC_FIXUP)) + goto ignore_bpf; + + ctx.data = kmemdup(rdesc, HID_MAX_DESCRIPTOR_SIZE, GFP_KERNEL); + if (!ctx.data) + goto ignore_bpf; + + ctx.allocated_size = HID_MAX_DESCRIPTOR_SIZE; + + ret = hid_bpf_run_progs(hdev, &ctx); + if (ret) + goto ignore_bpf; + + if (ctx.size > ctx.allocated_size) + goto ignore_bpf; + + *size = ctx.size; + + if (*size) { + rdesc = krealloc(ctx.data, *size, GFP_KERNEL); + } else { + rdesc = NULL; + kfree(ctx.data); + } + + return rdesc; + + ignore_bpf: + kfree(ctx.data); + return kmemdup(rdesc, *size, GFP_KERNEL); +} + int __init hid_bpf_module_init(void) { struct bpf_hid_hooks hooks = { .hdev_from_fd = hid_bpf_fd_to_hdev, .pre_link_attach = hid_bpf_pre_link_attach, + .post_link_attach = hid_bpf_post_link_attach, .array_detach = hid_bpf_array_detach, }; diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 937fab7eb9c6..3182c39db006 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1213,7 +1213,8 @@ int hid_open_report(struct hid_device *device) return -ENODEV; size = device->dev_rsize; - buf = kmemdup(start, size, GFP_KERNEL); + /* hid_bpf_report_fixup() ensures we work on a copy of rdesc */ + buf = hid_bpf_report_fixup(device, start, &size); if (buf == NULL) return -ENOMEM; diff --git a/include/linux/hid.h b/include/linux/hid.h index 8fd79011f461..66d949d10b78 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1213,10 +1213,16 @@ do { \ #ifdef CONFIG_BPF u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size); +u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size); int hid_bpf_module_init(void); void hid_bpf_module_exit(void); #else static inline u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size) { return rd; } +static inline u8 *hid_bpf_report_fixup(struct hid_device *hdev, u8 *rdesc, + unsigned int *size) +{ + return kmemdup(rdesc, *size, GFP_KERNEL); +} static inline int hid_bpf_module_init(void) { return 0; } static inline void hid_bpf_module_exit(void) {} #endif
Make use of BPF_HID_ATTACH_RDESC_FIXUP so we can trigger an rdesc fixup in the bpf world. Whenever the program gets attached/detached, the device is reconnected meaning that userspace will see it disappearing and reappearing with the new report descriptor. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- changes in v3: - ensure the ctx.size is properly bounded by allocated size - s/link_attached/post_link_attach/ - removed the switch statement with only one case changes in v2: - split the series by bpf/libbpf/hid/selftests and samples --- drivers/hid/hid-bpf.c | 62 ++++++++++++++++++++++++++++++++++++++++++ drivers/hid/hid-core.c | 3 +- include/linux/hid.h | 6 ++++ 3 files changed, 70 insertions(+), 1 deletion(-)