| Message ID | 20240621-hid_hw_req_bpf-v1-6-d7ab8b885a0b@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | HID: bpf_struct_ops, part 2 | expand |
On Fri, Jun 21, 2024 at 1:56 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > Same story than hid_hw_raw_requests: > > This allows to intercept and prevent or change the behavior of > hid_hw_output_report() from a bpf program. > > The intent is to solve a couple of use case: > - firewalling a HID device: a firewall can monitor who opens the hidraw > nodes and then prevent or allow access to write operations on that > hidraw node. > - change the behavior of a device and emulate a new HID feature request > > The hook is allowed to be run as sleepable so it can itself call > hid_hw_output_report(), which allows to "convert" one feature request into > another or even call the feature request on a different HID device on the > same physical device. > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > --- > > Here checkpatch complains about: > WARNING: use of RCU tasks trace is incorrect outside BPF or core RCU code > > However, we are jumping in BPF code, so I think this is correct, but I'd > like to have the opinion on the BPF folks. > --- > drivers/hid/bpf/hid_bpf_dispatch.c | 37 ++++++++++++++++++++++++++++++++---- > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > drivers/hid/hid-core.c | 10 ++++++++-- > drivers/hid/hidraw.c | 2 +- > include/linux/hid.h | 3 ++- > include/linux/hid_bpf.h | 24 ++++++++++++++++++++++- > 6 files changed, 68 insertions(+), 9 deletions(-) > > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c > index 8d6e08b7c42f..2a29a0625a3b 100644 > --- a/drivers/hid/bpf/hid_bpf_dispatch.c > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c > @@ -111,6 +111,38 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, > } > EXPORT_SYMBOL_GPL(dispatch_hid_bpf_raw_requests); > > +int dispatch_hid_bpf_output_report(struct hid_device *hdev, > + __u8 *buf, u32 size, __u64 source, > + bool from_bpf) > +{ > + struct hid_bpf_ctx_kern ctx_kern = { > + .ctx = { > + .hid = hdev, > + .allocated_size = size, > + .size = size, > + }, > + .data = buf, > + .from_bpf = from_bpf, > + }; > + struct hid_bpf_ops *e; > + int ret; > + > + rcu_read_lock_trace(); > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) { > + if (e->hid_hw_output_report) { > + ret = e->hid_hw_output_report(&ctx_kern.ctx, source); > + if (ret) > + goto out; > + } > + } > + ret = 0; > + > +out: > + rcu_read_unlock_trace(); same question. What protects prog_list ? list_for_each_entry_rcu() should be used within RCU CS if elements of that list are freed via call_rcu(). rcu_read_lock_trace() looks wrong here.
On Jun 21 2024, Alexei Starovoitov wrote: > On Fri, Jun 21, 2024 at 1:56 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > Same story than hid_hw_raw_requests: > > > > This allows to intercept and prevent or change the behavior of > > hid_hw_output_report() from a bpf program. > > > > The intent is to solve a couple of use case: > > - firewalling a HID device: a firewall can monitor who opens the hidraw > > nodes and then prevent or allow access to write operations on that > > hidraw node. > > - change the behavior of a device and emulate a new HID feature request > > > > The hook is allowed to be run as sleepable so it can itself call > > hid_hw_output_report(), which allows to "convert" one feature request into > > another or even call the feature request on a different HID device on the > > same physical device. > > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > > > --- > > > > Here checkpatch complains about: > > WARNING: use of RCU tasks trace is incorrect outside BPF or core RCU code > > > > However, we are jumping in BPF code, so I think this is correct, but I'd > > like to have the opinion on the BPF folks. > > --- > > drivers/hid/bpf/hid_bpf_dispatch.c | 37 ++++++++++++++++++++++++++++++++---- > > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > > drivers/hid/hid-core.c | 10 ++++++++-- > > drivers/hid/hidraw.c | 2 +- > > include/linux/hid.h | 3 ++- > > include/linux/hid_bpf.h | 24 ++++++++++++++++++++++- > > 6 files changed, 68 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c > > index 8d6e08b7c42f..2a29a0625a3b 100644 > > --- a/drivers/hid/bpf/hid_bpf_dispatch.c > > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c > > @@ -111,6 +111,38 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, > > } > > EXPORT_SYMBOL_GPL(dispatch_hid_bpf_raw_requests); > > > > +int dispatch_hid_bpf_output_report(struct hid_device *hdev, > > + __u8 *buf, u32 size, __u64 source, > > + bool from_bpf) > > +{ > > + struct hid_bpf_ctx_kern ctx_kern = { > > + .ctx = { > > + .hid = hdev, > > + .allocated_size = size, > > + .size = size, > > + }, > > + .data = buf, > > + .from_bpf = from_bpf, > > + }; > > + struct hid_bpf_ops *e; > > + int ret; > > + > > + rcu_read_lock_trace(); > > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) { > > + if (e->hid_hw_output_report) { > > + ret = e->hid_hw_output_report(&ctx_kern.ctx, source); > > + if (ret) > > + goto out; > > + } > > + } > > + ret = 0; > > + > > +out: > > + rcu_read_unlock_trace(); > > same question. re What is this for?: e->hid_hw_output_report might sleep, so using a plain rcu_read_lock() introduces warnings. > What protects prog_list ? I currently have a mutex in "struct hid_bpf" (prog_list_lock). I tried to take the lock instead of calling rcu_read_lock_trace() but while in e->hid_hw_output_report, we can call hid_bpf_hw_output_report exactly once, which leads to a deadlock as we are re-entering dispatch_hid_bpf_output_report() (same applies to hid_raw_request). > list_for_each_entry_rcu() should be used within RCU CS > if elements of that list are freed via call_rcu(). > rcu_read_lock_trace() looks wrong here. I'm not sure if I could use nested mutexes or if I should work with some other locking mechanism (or not take the lock when we are coming from bpf, but I would need to keep tabs on who actually called what). Anyway, thanks for having a look at it :) Cheers, Benjamin
On Fri, Jun 21, 2024 at 9:08 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > On Jun 21 2024, Alexei Starovoitov wrote: > > On Fri, Jun 21, 2024 at 1:56 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > Same story than hid_hw_raw_requests: > > > > > > This allows to intercept and prevent or change the behavior of > > > hid_hw_output_report() from a bpf program. > > > > > > The intent is to solve a couple of use case: > > > - firewalling a HID device: a firewall can monitor who opens the hidraw > > > nodes and then prevent or allow access to write operations on that > > > hidraw node. > > > - change the behavior of a device and emulate a new HID feature request > > > > > > The hook is allowed to be run as sleepable so it can itself call > > > hid_hw_output_report(), which allows to "convert" one feature request into > > > another or even call the feature request on a different HID device on the > > > same physical device. > > > > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > > > > > --- > > > > > > Here checkpatch complains about: > > > WARNING: use of RCU tasks trace is incorrect outside BPF or core RCU code > > > > > > However, we are jumping in BPF code, so I think this is correct, but I'd > > > like to have the opinion on the BPF folks. > > > --- > > > drivers/hid/bpf/hid_bpf_dispatch.c | 37 ++++++++++++++++++++++++++++++++---- > > > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > > > drivers/hid/hid-core.c | 10 ++++++++-- > > > drivers/hid/hidraw.c | 2 +- > > > include/linux/hid.h | 3 ++- > > > include/linux/hid_bpf.h | 24 ++++++++++++++++++++++- > > > 6 files changed, 68 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c > > > index 8d6e08b7c42f..2a29a0625a3b 100644 > > > --- a/drivers/hid/bpf/hid_bpf_dispatch.c > > > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c > > > @@ -111,6 +111,38 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, > > > } > > > EXPORT_SYMBOL_GPL(dispatch_hid_bpf_raw_requests); > > > > > > +int dispatch_hid_bpf_output_report(struct hid_device *hdev, > > > + __u8 *buf, u32 size, __u64 source, > > > + bool from_bpf) > > > +{ > > > + struct hid_bpf_ctx_kern ctx_kern = { > > > + .ctx = { > > > + .hid = hdev, > > > + .allocated_size = size, > > > + .size = size, > > > + }, > > > + .data = buf, > > > + .from_bpf = from_bpf, > > > + }; > > > + struct hid_bpf_ops *e; > > > + int ret; > > > + > > > + rcu_read_lock_trace(); > > > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) { > > > + if (e->hid_hw_output_report) { > > > + ret = e->hid_hw_output_report(&ctx_kern.ctx, source); > > > + if (ret) > > > + goto out; > > > + } > > > + } > > > + ret = 0; > > > + > > > +out: > > > + rcu_read_unlock_trace(); > > > > same question. > > re What is this for?: > > e->hid_hw_output_report might sleep, so using a plain rcu_read_lock() > introduces warnings. Ok, but just replacing rcu_read_lock() with rcu_read_lock_trace() doesn't fix it. rcu and rcu_tasks_trace are different. If you're using call_rcu to wait for GP to free an element in that list the thing will go wrong. If you really need rcu life times here use srcu. It's a much better fit. There will be srcu_read_lock() here, paired with call_srcu().
On Jun 21 2024, Alexei Starovoitov wrote: > On Fri, Jun 21, 2024 at 9:08 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > On Jun 21 2024, Alexei Starovoitov wrote: > > > On Fri, Jun 21, 2024 at 1:56 AM Benjamin Tissoires <bentiss@kernel.org> wrote: > > > > > > > > Same story than hid_hw_raw_requests: > > > > > > > > This allows to intercept and prevent or change the behavior of > > > > hid_hw_output_report() from a bpf program. > > > > > > > > The intent is to solve a couple of use case: > > > > - firewalling a HID device: a firewall can monitor who opens the hidraw > > > > nodes and then prevent or allow access to write operations on that > > > > hidraw node. > > > > - change the behavior of a device and emulate a new HID feature request > > > > > > > > The hook is allowed to be run as sleepable so it can itself call > > > > hid_hw_output_report(), which allows to "convert" one feature request into > > > > another or even call the feature request on a different HID device on the > > > > same physical device. > > > > > > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > > > > > > > --- > > > > > > > > Here checkpatch complains about: > > > > WARNING: use of RCU tasks trace is incorrect outside BPF or core RCU code > > > > > > > > However, we are jumping in BPF code, so I think this is correct, but I'd > > > > like to have the opinion on the BPF folks. > > > > --- > > > > drivers/hid/bpf/hid_bpf_dispatch.c | 37 ++++++++++++++++++++++++++++++++---- > > > > drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + > > > > drivers/hid/hid-core.c | 10 ++++++++-- > > > > drivers/hid/hidraw.c | 2 +- > > > > include/linux/hid.h | 3 ++- > > > > include/linux/hid_bpf.h | 24 ++++++++++++++++++++++- > > > > 6 files changed, 68 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c > > > > index 8d6e08b7c42f..2a29a0625a3b 100644 > > > > --- a/drivers/hid/bpf/hid_bpf_dispatch.c > > > > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c > > > > @@ -111,6 +111,38 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, > > > > } > > > > EXPORT_SYMBOL_GPL(dispatch_hid_bpf_raw_requests); > > > > > > > > +int dispatch_hid_bpf_output_report(struct hid_device *hdev, > > > > + __u8 *buf, u32 size, __u64 source, > > > > + bool from_bpf) > > > > +{ > > > > + struct hid_bpf_ctx_kern ctx_kern = { > > > > + .ctx = { > > > > + .hid = hdev, > > > > + .allocated_size = size, > > > > + .size = size, > > > > + }, > > > > + .data = buf, > > > > + .from_bpf = from_bpf, > > > > + }; > > > > + struct hid_bpf_ops *e; > > > > + int ret; > > > > + > > > > + rcu_read_lock_trace(); > > > > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) { > > > > + if (e->hid_hw_output_report) { > > > > + ret = e->hid_hw_output_report(&ctx_kern.ctx, source); > > > > + if (ret) > > > > + goto out; > > > > + } > > > > + } > > > > + ret = 0; > > > > + > > > > +out: > > > > + rcu_read_unlock_trace(); > > > > > > same question. > > > > re What is this for?: > > > > e->hid_hw_output_report might sleep, so using a plain rcu_read_lock() > > introduces warnings. > > Ok, but just replacing rcu_read_lock() with rcu_read_lock_trace() > doesn't fix it. > rcu and rcu_tasks_trace are different. > If you're using call_rcu to wait for GP to free an element in that > list the thing will go wrong. > > If you really need rcu life times here use srcu. It's a much better fit. > There will be srcu_read_lock() here, paired with call_srcu(). OK, thanks for the explanation. I'll work on this for v2 Cheers, Benjamin
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c index 8d6e08b7c42f..2a29a0625a3b 100644 --- a/drivers/hid/bpf/hid_bpf_dispatch.c +++ b/drivers/hid/bpf/hid_bpf_dispatch.c @@ -111,6 +111,38 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, } EXPORT_SYMBOL_GPL(dispatch_hid_bpf_raw_requests); +int dispatch_hid_bpf_output_report(struct hid_device *hdev, + __u8 *buf, u32 size, __u64 source, + bool from_bpf) +{ + struct hid_bpf_ctx_kern ctx_kern = { + .ctx = { + .hid = hdev, + .allocated_size = size, + .size = size, + }, + .data = buf, + .from_bpf = from_bpf, + }; + struct hid_bpf_ops *e; + int ret; + + rcu_read_lock_trace(); + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) { + if (e->hid_hw_output_report) { + ret = e->hid_hw_output_report(&ctx_kern.ctx, source); + if (ret) + goto out; + } + } + ret = 0; + +out: + rcu_read_unlock_trace(); + return ret; +} +EXPORT_SYMBOL_GPL(dispatch_hid_bpf_output_report); + u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *size) { int ret; @@ -441,10 +473,7 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz) if (!dma_data) return -ENOMEM; - ret = hid_ops->hid_hw_output_report(hdev, - dma_data, - size, - (__u64)ctx); + ret = hid_ops->hid_hw_output_report(hdev, dma_data, size, (__u64)ctx, true); kfree(dma_data); return ret; diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c index 93c824ba6a65..71143a65a99c 100644 --- a/drivers/hid/bpf/hid_bpf_struct_ops.c +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c @@ -45,6 +45,7 @@ static int hid_bpf_ops_check_member(const struct btf_type *t, switch (moff) { case offsetof(struct hid_bpf_ops, hid_rdesc_fixup): case offsetof(struct hid_bpf_ops, hid_hw_request): + case offsetof(struct hid_bpf_ops, hid_hw_output_report): break; default: if (prog->sleepable) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 0164aacf07ac..5a5fa4a32cbc 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2445,9 +2445,11 @@ int hid_hw_raw_request(struct hid_device *hdev, } EXPORT_SYMBOL_GPL(hid_hw_raw_request); -int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 source) +int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 source, + bool from_bpf) { unsigned int max_buffer_size = HID_MAX_BUFFER_SIZE; + int ret; if (hdev->ll_driver->max_buffer_size) max_buffer_size = hdev->ll_driver->max_buffer_size; @@ -2455,6 +2457,10 @@ int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 if (len < 1 || len > max_buffer_size || !buf) return -EINVAL; + ret = dispatch_hid_bpf_output_report(hdev, buf, len, source, from_bpf); + if (ret) + return ret; + if (hdev->ll_driver->output_report) return hdev->ll_driver->output_report(hdev, buf, len); @@ -2472,7 +2478,7 @@ int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 */ int hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len) { - return __hid_hw_output_report(hdev, buf, len, 0); + return __hid_hw_output_report(hdev, buf, len, 0, false); } EXPORT_SYMBOL_GPL(hid_hw_output_report); diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c index 4ba3131de614..c2396916cdaa 100644 --- a/drivers/hid/hidraw.c +++ b/drivers/hid/hidraw.c @@ -140,7 +140,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer, if ((report_type == HID_OUTPUT_REPORT) && !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP)) { - ret = __hid_hw_output_report(dev, buf, count, (__u64)file); + ret = __hid_hw_output_report(dev, buf, count, (__u64)file, false); /* * compatibility with old implementation of USB-HID and I2C-HID: * if the device does not support receiving output reports, diff --git a/include/linux/hid.h b/include/linux/hid.h index 24d0d7c0bd33..1533c9dcd3a6 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1130,7 +1130,8 @@ int __hid_hw_raw_request(struct hid_device *hdev, size_t len, enum hid_report_type rtype, enum hid_class_request reqtype, __u64 source, bool from_bpf); -int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 source); +int __hid_hw_output_report(struct hid_device *hdev, __u8 *buf, size_t len, __u64 source, + bool from_bpf); int hid_hw_raw_request(struct hid_device *hdev, unsigned char reportnum, __u8 *buf, size_t len, enum hid_report_type rtype, diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h index bb6cc5c7c705..3872c6fac62b 100644 --- a/include/linux/hid_bpf.h +++ b/include/linux/hid_bpf.h @@ -69,7 +69,7 @@ struct hid_ops { enum hid_class_request reqtype, __u64 source, bool from_bpf); int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len, - __u64 source); + __u64 source, bool from_bpf); int (*hid_input_report)(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, int interrupt, u64 source); struct module *owner; @@ -152,6 +152,24 @@ struct hid_bpf_ops { enum hid_report_type rtype, enum hid_class_request reqtype, __u64 source); + /** + * @hid_hw_output_report: called whenever a hid_hw_output_report() call is emitted + * on the HID device + * + * It has the following arguments: + * + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx + * ``source``: a u64 referring to a uniq but identifiable source. If %0, the + * kernel itself emitted that call. For hidraw, ``source`` is set + * to the associated ``struct file *``. + * + * Return: %0 to keep processing the request by hid-core; any other value + * stops hid-core from processing that event. A positive value should be + * returned with the number of bytes written to the device; a negative error + * code interrupts the processing of this call. + */ + int (*hid_hw_output_report)(struct hid_bpf_ctx *ctx, __u64 source); + /* private: do not show up in the docs */ struct hid_device *hdev; @@ -179,6 +197,8 @@ int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, u32 size, enum hid_report_type rtype, enum hid_class_request reqtype, __u64 source, bool from_bpf); +int dispatch_hid_bpf_output_report(struct hid_device *hdev, __u8 *buf, u32 size, + __u64 source, bool from_bpf); int hid_bpf_connect_device(struct hid_device *hdev); void hid_bpf_disconnect_device(struct hid_device *hdev); void hid_bpf_destroy_device(struct hid_device *hid); @@ -193,6 +213,8 @@ static inline int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, u32 size, enum hid_report_type rtype, enum hid_class_request reqtype, u64 source, bool from_bpf) { return 0; } +static inline int dispatch_hid_bpf_output_report(struct hid_device *hdev, __u8 *buf, u32 size, + __u64 source, bool from_bpf) { return 0; } 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) {}
Same story than hid_hw_raw_requests: This allows to intercept and prevent or change the behavior of hid_hw_output_report() from a bpf program. The intent is to solve a couple of use case: - firewalling a HID device: a firewall can monitor who opens the hidraw nodes and then prevent or allow access to write operations on that hidraw node. - change the behavior of a device and emulate a new HID feature request The hook is allowed to be run as sleepable so it can itself call hid_hw_output_report(), which allows to "convert" one feature request into another or even call the feature request on a different HID device on the same physical device. Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- Here checkpatch complains about: WARNING: use of RCU tasks trace is incorrect outside BPF or core RCU code However, we are jumping in BPF code, so I think this is correct, but I'd like to have the opinion on the BPF folks. --- drivers/hid/bpf/hid_bpf_dispatch.c | 37 ++++++++++++++++++++++++++++++++---- drivers/hid/bpf/hid_bpf_struct_ops.c | 1 + drivers/hid/hid-core.c | 10 ++++++++-- drivers/hid/hidraw.c | 2 +- include/linux/hid.h | 3 ++- include/linux/hid_bpf.h | 24 ++++++++++++++++++++++- 6 files changed, 68 insertions(+), 9 deletions(-)