| Message ID | 20240903-hid-bpf-hid-generic-v1-6-9511a565b2da@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: bpf: add a new hook to control hid-generic | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Sep 03, 2024 at 01:14:36AM +0900, Benjamin Tissoires wrote: > We make struct hid_device_id writeable and use the .driver_data field > of hid-generic as the connect mask. I think this needs to be spelled out a bit more: for this to work the driver *must* be hid-generic, otherwise this doesn't work. But I'm a bit confused why we have a custom fields for force/ignore driver but whether the device is connected (and thus uses the driver) is hidden in an effectively undocumented private field of one specific driver. Wouldn't it be easier to add another boolean (or enum entry, see my other comment) to hid_bpf_driver? This way *how* it happens is hidden from the API as well - you say "hidraw only please" and the kernel does the rest (through hid-generic or otherwise). Cheers, Peter > > This way, we can control from a HID-BPF program if a device needs to > be exported through hidraw and/or hid-input mainly. > > This is useful in case we want to have a third party program that directly > talks to the hidraw node and we don't want regular input events to be > emitted. This third party program can load a BPF program that instructs > hid-generic to rebind on the device with hidraw only and then open the > hidraw node itself. > > When the application is closed, the BPF program is unloaded and the normal > driver takes back the control of the device. > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > --- > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > drivers/hid/hid-core.c | 14 ++++++++------ > drivers/hid/hid-generic.c | 5 +++-- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c > index 1e13a22f73a1..bb755edd02f0 100644 > --- a/drivers/hid/bpf/hid_bpf_struct_ops.c > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c > @@ -80,6 +80,7 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log, > WRITE_RANGE(hid_device, name, true), > WRITE_RANGE(hid_device, uniq, true), > WRITE_RANGE(hid_device, phys, true), > + WRITE_RANGE(hid_device_id, driver_data, false), > WRITE_RANGE(hid_bpf_driver, force_driver, false), > WRITE_RANGE(hid_bpf_driver, ignore_driver, false), > }; > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 7845f0a789ec..2bd279b23aa4 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -2637,15 +2637,17 @@ EXPORT_SYMBOL_GPL(hid_compare_device_paths); > > static bool hid_check_device_match(struct hid_device *hdev, > struct hid_driver *hdrv, > - const struct hid_device_id **id) > + struct hid_device_id *id) > { > + const struct hid_device_id *_id = hid_match_device(hdev, hdrv); > int ret; > > - *id = hid_match_device(hdev, hdrv); > - if (!*id) > + if (!_id) > return false; > > - ret = call_hid_bpf_driver_probe(hdev, hdrv, *id); > + memcpy(id, _id, sizeof(*id)); > + > + ret = call_hid_bpf_driver_probe(hdev, hdrv, id); > if (ret) > return ret > 0; > > @@ -2662,7 +2664,7 @@ static bool hid_check_device_match(struct hid_device *hdev, > > static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) > { > - const struct hid_device_id *id; > + struct hid_device_id id; > int ret; > > if (!hid_check_device_match(hdev, hdrv, &id)) > @@ -2677,7 +2679,7 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) > hdev->driver = hdrv; > > if (hdrv->probe) { > - ret = hdrv->probe(hdev, id); > + ret = hdrv->probe(hdev, &id); > } else { /* default probe */ > ret = hid_open_report(hdev); > if (!ret) > diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c > index f9db991d3c5a..5cd1f3a79a4b 100644 > --- a/drivers/hid/hid-generic.c > +++ b/drivers/hid/hid-generic.c > @@ -64,11 +64,12 @@ static int hid_generic_probe(struct hid_device *hdev, > if (ret) > return ret; > > - return hid_hw_start(hdev, HID_CONNECT_DEFAULT); > + return hid_hw_start(hdev, id->driver_data); > } > > static const struct hid_device_id hid_table[] = { > - { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) }, > + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID), > + .driver_data = HID_CONNECT_DEFAULT }, > { } > }; > MODULE_DEVICE_TABLE(hid, hid_table); > > -- > 2.46.0 >
On Sep 03 2024, Peter Hutterer wrote: > On Tue, Sep 03, 2024 at 01:14:36AM +0900, Benjamin Tissoires wrote: > > We make struct hid_device_id writeable and use the .driver_data field > > of hid-generic as the connect mask. > > I think this needs to be spelled out a bit more: for this to work the > driver *must* be hid-generic, otherwise this doesn't work. But I'm a bit > confused why we have a custom fields for force/ignore driver but > whether the device is connected (and thus uses the driver) is hidden in > an effectively undocumented private field of one specific driver. It's hid-generic only because that is the less intrusive approach. I'm not sure we want an override from BPF for any drivers, as suddenly you will get some harder-than-required bugs with drivers not exposing input when they should. > > Wouldn't it be easier to add another boolean (or enum entry, see my > other comment) to hid_bpf_driver? This way *how* it happens is hidden > from the API as well - you say "hidraw only please" and the kernel does > the rest (through hid-generic or otherwise). I thought at that, but again, given that I wanted to enable this only for hid-generic, it felt weird to have a field that works for just one driver. Also, after I sent the series, I realized that it was probably not great for Steam/SDL: today they basically set uaccess on the hidraw nodes, but now we are going to require some root permissions to disable the event node. I'll need to think more but we probably need something more like udev-hid-bpf where you can load the "disable event node on hidraw open" BPF once, and forget about it, and make this bpf recognize that SDL/Steam is opening the hidraw node, and therefore it needs to reconnect the device. But this is not possible to do with this series (and maybe not with the current infrastructure). Cheers, Benjamin > > Cheers, > Peter > > > > > This way, we can control from a HID-BPF program if a device needs to > > be exported through hidraw and/or hid-input mainly. > > > > This is useful in case we want to have a third party program that directly > > talks to the hidraw node and we don't want regular input events to be > > emitted. This third party program can load a BPF program that instructs > > hid-generic to rebind on the device with hidraw only and then open the > > hidraw node itself. > > > > When the application is closed, the BPF program is unloaded and the normal > > driver takes back the control of the device. > > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > --- > > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > > drivers/hid/hid-core.c | 14 ++++++++------ > > drivers/hid/hid-generic.c | 5 +++-- > > 3 files changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c > > index 1e13a22f73a1..bb755edd02f0 100644 > > --- a/drivers/hid/bpf/hid_bpf_struct_ops.c > > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c > > @@ -80,6 +80,7 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log, > > WRITE_RANGE(hid_device, name, true), > > WRITE_RANGE(hid_device, uniq, true), > > WRITE_RANGE(hid_device, phys, true), > > + WRITE_RANGE(hid_device_id, driver_data, false), > > WRITE_RANGE(hid_bpf_driver, force_driver, false), > > WRITE_RANGE(hid_bpf_driver, ignore_driver, false), > > }; > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > > index 7845f0a789ec..2bd279b23aa4 100644 > > --- a/drivers/hid/hid-core.c > > +++ b/drivers/hid/hid-core.c > > @@ -2637,15 +2637,17 @@ EXPORT_SYMBOL_GPL(hid_compare_device_paths); > > > > static bool hid_check_device_match(struct hid_device *hdev, > > struct hid_driver *hdrv, > > - const struct hid_device_id **id) > > + struct hid_device_id *id) > > { > > + const struct hid_device_id *_id = hid_match_device(hdev, hdrv); > > int ret; > > > > - *id = hid_match_device(hdev, hdrv); > > - if (!*id) > > + if (!_id) > > return false; > > > > - ret = call_hid_bpf_driver_probe(hdev, hdrv, *id); > > + memcpy(id, _id, sizeof(*id)); > > + > > + ret = call_hid_bpf_driver_probe(hdev, hdrv, id); > > if (ret) > > return ret > 0; > > > > @@ -2662,7 +2664,7 @@ static bool hid_check_device_match(struct hid_device *hdev, > > > > static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) > > { > > - const struct hid_device_id *id; > > + struct hid_device_id id; > > int ret; > > > > if (!hid_check_device_match(hdev, hdrv, &id)) > > @@ -2677,7 +2679,7 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) > > hdev->driver = hdrv; > > > > if (hdrv->probe) { > > - ret = hdrv->probe(hdev, id); > > + ret = hdrv->probe(hdev, &id); > > } else { /* default probe */ > > ret = hid_open_report(hdev); > > if (!ret) > > diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c > > index f9db991d3c5a..5cd1f3a79a4b 100644 > > --- a/drivers/hid/hid-generic.c > > +++ b/drivers/hid/hid-generic.c > > @@ -64,11 +64,12 @@ static int hid_generic_probe(struct hid_device *hdev, > > if (ret) > > return ret; > > > > - return hid_hw_start(hdev, HID_CONNECT_DEFAULT); > > + return hid_hw_start(hdev, id->driver_data); > > } > > > > static const struct hid_device_id hid_table[] = { > > - { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) }, > > + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID), > > + .driver_data = HID_CONNECT_DEFAULT }, > > { } > > }; > > MODULE_DEVICE_TABLE(hid, hid_table); > > > > -- > > 2.46.0 > >
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c index 1e13a22f73a1..bb755edd02f0 100644 --- a/drivers/hid/bpf/hid_bpf_struct_ops.c +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c @@ -80,6 +80,7 @@ static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log, WRITE_RANGE(hid_device, name, true), WRITE_RANGE(hid_device, uniq, true), WRITE_RANGE(hid_device, phys, true), + WRITE_RANGE(hid_device_id, driver_data, false), WRITE_RANGE(hid_bpf_driver, force_driver, false), WRITE_RANGE(hid_bpf_driver, ignore_driver, false), }; diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 7845f0a789ec..2bd279b23aa4 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2637,15 +2637,17 @@ EXPORT_SYMBOL_GPL(hid_compare_device_paths); static bool hid_check_device_match(struct hid_device *hdev, struct hid_driver *hdrv, - const struct hid_device_id **id) + struct hid_device_id *id) { + const struct hid_device_id *_id = hid_match_device(hdev, hdrv); int ret; - *id = hid_match_device(hdev, hdrv); - if (!*id) + if (!_id) return false; - ret = call_hid_bpf_driver_probe(hdev, hdrv, *id); + memcpy(id, _id, sizeof(*id)); + + ret = call_hid_bpf_driver_probe(hdev, hdrv, id); if (ret) return ret > 0; @@ -2662,7 +2664,7 @@ static bool hid_check_device_match(struct hid_device *hdev, static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) { - const struct hid_device_id *id; + struct hid_device_id id; int ret; if (!hid_check_device_match(hdev, hdrv, &id)) @@ -2677,7 +2679,7 @@ static int __hid_device_probe(struct hid_device *hdev, struct hid_driver *hdrv) hdev->driver = hdrv; if (hdrv->probe) { - ret = hdrv->probe(hdev, id); + ret = hdrv->probe(hdev, &id); } else { /* default probe */ ret = hid_open_report(hdev); if (!ret) diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c index f9db991d3c5a..5cd1f3a79a4b 100644 --- a/drivers/hid/hid-generic.c +++ b/drivers/hid/hid-generic.c @@ -64,11 +64,12 @@ static int hid_generic_probe(struct hid_device *hdev, if (ret) return ret; - return hid_hw_start(hdev, HID_CONNECT_DEFAULT); + return hid_hw_start(hdev, id->driver_data); } static const struct hid_device_id hid_table[] = { - { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) }, + { HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID), + .driver_data = HID_CONNECT_DEFAULT }, { } }; MODULE_DEVICE_TABLE(hid, hid_table);
We make struct hid_device_id writeable and use the .driver_data field of hid-generic as the connect mask. This way, we can control from a HID-BPF program if a device needs to be exported through hidraw and/or hid-input mainly. This is useful in case we want to have a third party program that directly talks to the hidraw node and we don't want regular input events to be emitted. This third party program can load a BPF program that instructs hid-generic to rebind on the device with hidraw only and then open the hidraw node itself. When the application is closed, the BPF program is unloaded and the normal driver takes back the control of the device. Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + drivers/hid/hid-core.c | 14 ++++++++------ drivers/hid/hid-generic.c | 5 +++-- 3 files changed, 12 insertions(+), 8 deletions(-)