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