Currently, hid_bpf_rdesc_fixup() is called once the match between the HID device and the driver is done. This can be problematic in case the driver selected by the kernel would change the report descriptor after the fact.
To give a chance for hid_bpf_rdesc_fixup() to provide hints on to how to select a dedicated driver or not, move the call to that BPF hook earlier in the .probe() process, when we get the first match.
However, this means that we might get called more than once (typically once for hid-generic, and once for hid-vendor-specific). So we store the result of HID-BPF fixup in struct hid_device. If that new value is NULL, we need to use ->dev_rdesc.
In order to not grow struct hid_device, some fields are re-ordered. This was the output of pahole for the first 128 bytes: struct hid_device { __u8 * dev_rdesc; /* 0 8 */ unsigned int dev_rsize; /* 8 4 */
/* XXX 4 bytes hole, try to pack */
__u8 * rdesc; /* 16 8 */ unsigned int rsize; /* 24 4 */
/* XXX 4 bytes hole, try to pack */
struct hid_collection * collection; /* 32 8 */ unsigned int collection_size; /* 40 4 */ unsigned int maxcollection; /* 44 4 */ unsigned int maxapplication; /* 48 4 */ __u16 bus; /* 52 2 */ __u16 group; /* 54 2 */ __u32 vendor; /* 56 4 */ __u32 product; /* 60 4 */ /* --- cacheline 1 boundary (64 bytes) --- */ __u32 version; /* 64 4 */ enum hid_type type; /* 68 4 */ unsigned int country; /* 72 4 */
/* XXX 4 bytes hole, try to pack */
struct hid_report_enum report_enum[3]; /* 80 6216 */
Basically, we got three holes of 4 bytes. We can reorder things a little and makes those 3 holes a continuous 12 bytes hole, which can be replaced by the new pointer and the new unsigned int we need.
In terms of code allocation, when not using HID-BPF, we are back to kernel v6.2 in hid_open_report(). These multiple kmemdup() calls will be fixed in a later commit.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
---
new in v2 --- drivers/hid/bpf/hid_bpf_dispatch.c | 8 ++++++-- drivers/hid/hid-core.c | 17 +++++++++++++---- include/linux/hid.h | 18 ++++++++++-------- include/linux/hid_bpf.h | 9 ++------- 4 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c index 8420c227e21b..58839c125fba 100644 --- a/drivers/hid/bpf/hid_bpf_dispatch.c +++ b/drivers/hid/bpf/hid_bpf_dispatch.c @@ -183,7 +183,7 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, const u8 *rdesc, unsigned
ignore_bpf: kfree(ctx_kern.data); - return kmemdup(rdesc, *size, GFP_KERNEL); + return NULL; } EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
@@ -260,8 +260,12 @@ int hid_bpf_allocate_event_data(struct hid_device *hdev)
int hid_bpf_reconnect(struct hid_device *hdev) { - if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) + if (!test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status)) { + kfree(hdev->bpf_rdesc); + hdev->bpf_rdesc = NULL; + hdev->bpf_rsize = 0; return device_reprobe(&hdev->dev); + }
return 0; } diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 311069588321..a5f5415571cb 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -708,6 +708,7 @@ void hiddev_free(struct kref *ref)
hid_close_report(hid); kfree(hid->dev_rdesc); + kfree(hid->bpf_rdesc); kfree(hid); }
@@ -1221,13 +1222,12 @@ int hid_open_report(struct hid_device *device) if (WARN_ON(device->status & HID_STAT_PARSED)) return -EBUSY;
- start = device->dev_rdesc; + start = device->bpf_rdesc ? device->bpf_rdesc : device->dev_rdesc; + size = device->bpf_rsize; if (WARN_ON(!start)) return -ENODEV; - size = device->dev_rsize;
- /* call_hid_bpf_rdesc_fixup() ensures we work on a copy of rdesc */ - buf = call_hid_bpf_rdesc_fixup(device, start, &size); + buf = kmemdup(start, size, GFP_KERNEL); if (buf == NULL) return -ENOMEM;
@@ -2659,6 +2659,12 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) const struct hid_device_id *id; int ret;
+ if (!hdev->bpf_rsize) { + hdev->bpf_rsize = hdev->dev_rsize; + hdev->bpf_rdesc = call_hid_bpf_rdesc_fixup(hdev, hdev->dev_rdesc, + &hdev->bpf_rsize); + } + if (!hid_check_device_match(hdev, hdrv, &id)) return -ENODEV;
@@ -2916,8 +2922,11 @@ static void hid_remove_device(struct hid_device *hdev) hdev->status &= ~HID_STAT_ADDED; } kfree(hdev->dev_rdesc); + kfree(hdev->bpf_rdesc); hdev->dev_rdesc = NULL; + hdev->bpf_rdesc = NULL; hdev->dev_rsize = 0; + hdev->bpf_rsize = 0; }
/** diff --git a/include/linux/hid.h b/include/linux/hid.h index 3a998fa3812d..d41fa18f1e03 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -599,15 +599,17 @@ enum hid_battery_status { struct hid_driver; struct hid_ll_driver;
-struct hid_device { /* device report descriptor */ - const __u8 *dev_rdesc; - unsigned dev_rsize; - const __u8 *rdesc; - unsigned rsize; +struct hid_device { + const __u8 *dev_rdesc; /* device report descriptor */ + const __u8 *bpf_rdesc; /* bpf modified report descriptor, if any */ + const __u8 *rdesc; /* currently used report descriptor */ + unsigned int dev_rsize; + unsigned int bpf_rsize; + unsigned int rsize; + unsigned int collection_size; /* Number of allocated hid_collections */ struct hid_collection *collection; /* List of HID collections */ - unsigned collection_size; /* Number of allocated hid_collections */ - unsigned maxcollection; /* Number of parsed collections */ - unsigned maxapplication; /* Number of applications */ + unsigned int maxcollection; /* Number of parsed collections */ + unsigned int maxapplication; /* Number of applications */ __u16 bus; /* BUS ID */ __u16 group; /* Report group */ __u32 vendor; /* Vendor ID */ diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h index 6a47223e6460..0290833544c8 100644 --- a/include/linux/hid_bpf.h +++ b/include/linux/hid_bpf.h @@ -228,13 +228,8 @@ static inline int hid_bpf_connect_device(struct hid_device *hdev) { return 0; } static inline void hid_bpf_disconnect_device(struct hid_device *hdev) {} static inline void hid_bpf_destroy_device(struct hid_device *hid) {} static inline int hid_bpf_device_init(struct hid_device *hid) { return 0; } -/* - * This specialized allocator has to be a macro for its allocations to be - * accounted separately (to have a separate alloc_tag). The typecast is - * intentional to enforce typesafety. - */ -#define call_hid_bpf_rdesc_fixup(_hdev, _rdesc, _size) \ - ((u8 *)kmemdup(_rdesc, *(_size), GFP_KERNEL)) +static inline u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, + unsigned int *size) { return NULL; }
#endif /* CONFIG_HID_BPF */