| Message ID | 20220318161528.1531164-1-benjamin.tissoires@redhat.com |
|---|---|
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > LIRC_MODE2 does not really need net_admin capability, but only sys_admin. > > Extract a new helper for it, it will be also used for the HID bpf > implementation. > > Cc: Sean Young <sean@mess.org> > Acked-by: Sean Young <sean@mess.org> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Acked-by: Song Liu <songliubraving@fb.com> > > --- > > changes in v3: > - dropped BPF_PROG_TYPE_EXT from the new helper > > new in v2 > --- > kernel/bpf/syscall.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 9beb585be5a6..b88688264ad0 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -2165,7 +2165,6 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) > case BPF_PROG_TYPE_LWT_SEG6LOCAL: > case BPF_PROG_TYPE_SK_SKB: > case BPF_PROG_TYPE_SK_MSG: > - case BPF_PROG_TYPE_LIRC_MODE2: > case BPF_PROG_TYPE_FLOW_DISSECTOR: > case BPF_PROG_TYPE_CGROUP_DEVICE: > case BPF_PROG_TYPE_CGROUP_SOCK: > @@ -2202,6 +2201,16 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type) > } > } > > +static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > +{ > + switch (prog_type) { > + case BPF_PROG_TYPE_LIRC_MODE2: > + return true; > + default: > + return false; > + } > +} > + > /* last field in 'union bpf_attr' used by this command */ > #define BPF_PROG_LOAD_LAST_FIELD core_relo_rec_size > > @@ -2252,6 +2261,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr) > return -EPERM; > if (is_perfmon_prog_type(type) && !perfmon_capable()) > return -EPERM; > + if (is_sys_admin_prog_type(type) && !capable(CAP_SYS_ADMIN)) > + return -EPERM; > > /* attach_prog_fd/attach_btf_obj_fd can specify fd of either bpf_prog > * or btf, we need to check which one it is > -- > 2.35.1 >
On Fri, Mar 18, 2022 at 9:16 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > [...] > > diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h > new file mode 100644 > index 000000000000..9c8dbd389995 > --- /dev/null > +++ b/include/linux/bpf-hid.h > [...] > + > +struct hid_bpf_ctx_kern { > + enum hid_bpf_event type; /* read-only */ > + struct hid_device *hdev; /* read-only */ > + > + u16 size; /* used size in data (RW) */ > + u8 *data; /* data buffer (RW) */ > + u32 allocated_size; /* allocated size of data (RO) */ Why u16 size vs. u32 allocated_size? Also, maybe shuffle the members to remove some holes? > + > + s32 retval; /* in use when BPF_HID_ATTACH_USER_EVENT (RW) */ > +}; > + [...] > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h We need to mirror these changes to tools/include/uapi/linux/bpf.h. > index 99fab54ae9c0..0e8438e93768 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -952,6 +952,7 @@ enum bpf_prog_type { > BPF_PROG_TYPE_LSM, > BPF_PROG_TYPE_SK_LOOKUP, > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */ > + BPF_PROG_TYPE_HID, > }; [...] > + > /* When BPF ldimm64's insn[0].src_reg != 0 then this can have > * the following extensions: > * > @@ -5129,6 +5145,16 @@ union bpf_attr { > * The **hash_algo** is returned on success, > * **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if > * invalid arguments are passed. > + * > + * void *bpf_hid_get_data(void *ctx, u64 offset, u64 size) > + * Description > + * Returns a pointer to the data associated with context at the given > + * offset and size (in bytes). > + * > + * Note: the returned pointer is refcounted and must be dereferenced > + * by a call to bpf_hid_discard; > + * Return > + * The pointer to the data. On error, a null value is returned. Please use annotations like *size*, **NULL**. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -5325,6 +5351,7 @@ union bpf_attr { > FN(copy_from_user_task), \ > FN(skb_set_tstamp), \ > FN(ima_file_hash), \ > + FN(hid_get_data), \ > /* */ > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > @@ -5925,6 +5952,10 @@ struct bpf_link_info { > struct { > __u32 ifindex; > } xdp; > + struct { > + __s32 hidraw_number; > + __u32 attach_type; > + } hid; > }; > } __attribute__((aligned(8))); > > diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h > new file mode 100644 > index 000000000000..64a8b9dd8809 > --- /dev/null > +++ b/include/uapi/linux/bpf_hid.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */ > + > +/* > + * HID BPF public headers > + * > + * Copyright (c) 2022 Benjamin Tissoires > + */ > + > +#ifndef _UAPI__LINUX_BPF_HID_H__ > +#define _UAPI__LINUX_BPF_HID_H__ > + > +#include <linux/types.h> > + > +enum hid_bpf_event { > + HID_BPF_UNDEF = 0, > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ > + HID_BPF_RDESC_FIXUP, /* ................... BPF_HID_RDESC_FIXUP */ > + HID_BPF_USER_EVENT, /* ................... BPF_HID_USER_EVENT */ Why don't we have a DRIVER_EVENT type here? > [...] > + > +BPF_CALL_3(bpf_hid_get_data, struct hid_bpf_ctx_kern*, ctx, u64, offset, u64, size) > +{ > + if (!size) > + return 0UL; > + > + if (offset + size > ctx->allocated_size) > + return 0UL; > + > + return (unsigned long)(ctx->data + offset); > +} > + > +static const struct bpf_func_proto bpf_hid_get_data_proto = { > + .func = bpf_hid_get_data, > + .gpl_only = true, > + .ret_type = RET_PTR_TO_ALLOC_MEM_OR_NULL, > + .arg1_type = ARG_PTR_TO_CTX, > + .arg2_type = ARG_ANYTHING, > + .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO, I think we should use ARG_CONST_SIZE or ARG_CONST_SIZE_OR_ZERO? > +}; > + > +static const struct bpf_func_proto * > +hid_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > +{ > + switch (func_id) { > + case BPF_FUNC_hid_get_data: > + return &bpf_hid_get_data_proto; > + default: > + return bpf_base_func_proto(func_id); > + } > +} [...] > + > +static int hid_bpf_prog_test_run(struct bpf_prog *prog, > + const union bpf_attr *attr, > + union bpf_attr __user *uattr) > +{ > + struct hid_device *hdev = NULL; > + struct bpf_prog_array *progs; > + bool valid_prog = false; > + int i; > + int target_fd, ret; > + void __user *data_out = u64_to_user_ptr(attr->test.data_out); > + void __user *data_in = u64_to_user_ptr(attr->test.data_in); > + u32 user_size_in = attr->test.data_size_in; > + u32 user_size_out = attr->test.data_size_out; > + u32 allocated_size = max(user_size_in, user_size_out); > + struct hid_bpf_ctx_kern ctx = { > + .type = HID_BPF_USER_EVENT, > + .allocated_size = allocated_size, > + }; > + > + if (!hid_hooks.hdev_from_fd) > + return -EOPNOTSUPP; > + > + if (attr->test.ctx_size_in != sizeof(int)) > + return -EINVAL; ctx_size_in is always 4 bytes? > + > + if (allocated_size > HID_MAX_BUFFER_SIZE) > + return -E2BIG; > + > + if (copy_from_user(&target_fd, (void *)attr->test.ctx_in, attr->test.ctx_size_in)) > + return -EFAULT; > + > + hdev = hid_hooks.hdev_from_fd(target_fd); > + if (IS_ERR(hdev)) > + return PTR_ERR(hdev); > + > + if (allocated_size) { > + ctx.data = kzalloc(allocated_size, GFP_KERNEL); > + if (!ctx.data) > + return -ENOMEM; > + > + ctx.allocated_size = allocated_size; > + } > + ctx.hdev = hdev; > + > + ret = mutex_lock_interruptible(&bpf_hid_mutex); > + if (ret) > + return ret; > + > + /* check if the given program is of correct type and registered */ > + progs = rcu_dereference_protected(hdev->bpf.run_array[BPF_HID_ATTACH_USER_EVENT], > + lockdep_is_held(&bpf_hid_mutex)); > + if (!progs) { > + ret = -EFAULT; > + goto unlock; > + } > + > + for (i = 0; i < bpf_prog_array_length(progs); i++) { > + if (progs->items[i].prog == prog) { > + valid_prog = true; > + break; > + } > + } > + > + if (!valid_prog) { > + ret = -EINVAL; > + goto unlock; > + } > + > + /* copy data_in from userspace */ > + if (user_size_in) { > + if (copy_from_user(ctx.data, data_in, user_size_in)) { > + ret = -EFAULT; > + goto unlock; > + } > + > + ctx.size = user_size_in; > + } > + > + migrate_disable(); > + > + ret = bpf_prog_run(prog, &ctx); > + > + migrate_enable(); > + > + if (user_size_out && data_out) { > + user_size_out = min3(user_size_out, (u32)ctx.size, allocated_size); > + > + if (copy_to_user(data_out, ctx.data, user_size_out)) { > + ret = -EFAULT; > + goto unlock; > + } > + > + if (copy_to_user(&uattr->test.data_size_out, > + &user_size_out, > + sizeof(user_size_out))) { > + ret = -EFAULT; > + goto unlock; > + } > + } > + > + if (copy_to_user(&uattr->test.retval, &ctx.retval, sizeof(ctx.retval))) > + ret = -EFAULT; > + > +unlock: > + kfree(ctx.data); > + > + mutex_unlock(&bpf_hid_mutex); > + return ret; > +} > + > +const struct bpf_prog_ops hid_prog_ops = { > + .test_run = hid_bpf_prog_test_run, > +}; > + > +int bpf_hid_init(struct hid_device *hdev) > +{ > + int type; > + > + for (type = 0; type < MAX_BPF_HID_ATTACH_TYPE; type++) > + INIT_LIST_HEAD(&hdev->bpf.links[type]); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(bpf_hid_init); > + > +void bpf_hid_exit(struct hid_device *hdev) > +{ > + enum bpf_hid_attach_type type; > + struct bpf_hid_link *hid_link; > + > + mutex_lock(&bpf_hid_mutex); > + for (type = 0; type < MAX_BPF_HID_ATTACH_TYPE; type++) { > + bpf_hid_run_array_detach(hdev, type); > + list_for_each_entry(hid_link, &hdev->bpf.links[type], node) { > + hid_link->hdev = NULL; /* auto-detach link */ > + } > + } > + mutex_unlock(&bpf_hid_mutex); > +} > +EXPORT_SYMBOL_GPL(bpf_hid_exit); > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index b88688264ad0..d1c05011e5ab 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3,6 +3,7 @@ > */ > #include <linux/bpf.h> > #include <linux/bpf-cgroup.h> > +#include <linux/bpf-hid.h> > #include <linux/bpf_trace.h> > #include <linux/bpf_lirc.h> > #include <linux/bpf_verifier.h> > @@ -2205,6 +2206,7 @@ static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) > { > switch (prog_type) { > case BPF_PROG_TYPE_LIRC_MODE2: > + case BPF_PROG_TYPE_HID: > return true; > default: > return false; > @@ -3199,6 +3201,11 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) > return BPF_PROG_TYPE_SK_LOOKUP; > case BPF_XDP: > return BPF_PROG_TYPE_XDP; > + case BPF_HID_DEVICE_EVENT: > + case BPF_HID_RDESC_FIXUP: > + case BPF_HID_USER_EVENT: > + case BPF_HID_DRIVER_EVENT: > + return BPF_PROG_TYPE_HID; > default: > return BPF_PROG_TYPE_UNSPEC; > } > @@ -3342,6 +3349,11 @@ static int bpf_prog_query(const union bpf_attr *attr, > case BPF_SK_MSG_VERDICT: > case BPF_SK_SKB_VERDICT: > return sock_map_bpf_prog_query(attr, uattr); > + case BPF_HID_DEVICE_EVENT: > + case BPF_HID_RDESC_FIXUP: > + case BPF_HID_USER_EVENT: > + case BPF_HID_DRIVER_EVENT: > + return bpf_hid_prog_query(attr, uattr); > default: > return -EINVAL; > } > @@ -4336,6 +4348,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr) > ret = bpf_perf_link_attach(attr, prog); > break; > #endif > + case BPF_PROG_TYPE_HID: > + return bpf_hid_link_create(attr, prog); > default: > ret = -EINVAL; > } > -- > 2.35.1 >
On Fri, Mar 18, 2022 at 9:18 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Export implement() outside of hid-core.c and use this and Maybe rename implement() to something that makes sense? > hid_field_extract() to implement the helprs for hid-bpf. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > changes in v3: > - renamed hid_{get|set}_data into hid_{get|set}_bits > > changes in v2: > - split the series by bpf/libbpf/hid/selftests and samples > - allow for n > 32, by relying on memcpy > --- > drivers/hid/hid-bpf.c | 29 +++++++++++++++++++++++++++++ > drivers/hid/hid-core.c | 4 ++-- > include/linux/hid.h | 2 ++ > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c > index 45c87ff47324..650dd5e54919 100644 > --- a/drivers/hid/hid-bpf.c > +++ b/drivers/hid/hid-bpf.c > @@ -122,6 +122,33 @@ static void hid_bpf_array_detach(struct hid_device *hdev, enum bpf_hid_attach_ty > } > } > > +static int hid_bpf_get_bits(struct hid_device *hdev, u8 *buf, size_t buf_size, u64 offset, u32 n, > + u32 *data) > +{ > + if (n > 32) > + return -EINVAL; > + > + if (((offset + n) >> 3) >= buf_size) > + return -E2BIG; > + > + *data = hid_field_extract(hdev, buf, offset, n); > + return n; > +} > + > +static int hid_bpf_set_bits(struct hid_device *hdev, u8 *buf, size_t buf_size, u64 offset, u32 n, > + u32 data) > +{ > + if (n > 32) > + return -EINVAL; > + > + if (((offset + n) >> 3) >= buf_size) > + return -E2BIG; > + > + /* data must be a pointer to a u32 */ > + implement(hdev, buf, offset, n, data); > + return n; > +} > + > static int hid_bpf_run_progs(struct hid_device *hdev, struct hid_bpf_ctx_kern *ctx) > { > enum bpf_hid_attach_type type; > @@ -223,6 +250,8 @@ int __init hid_bpf_module_init(void) > .pre_link_attach = hid_bpf_pre_link_attach, > .post_link_attach = hid_bpf_post_link_attach, > .array_detach = hid_bpf_array_detach, > + .hid_get_bits = hid_bpf_get_bits, > + .hid_set_bits = hid_bpf_set_bits, > }; > > bpf_hid_set_hooks(&hooks); > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c > index 3182c39db006..4f669dcddc08 100644 > --- a/drivers/hid/hid-core.c > +++ b/drivers/hid/hid-core.c > @@ -1416,8 +1416,8 @@ static void __implement(u8 *report, unsigned offset, int n, u32 value) > } > } > > -static void implement(const struct hid_device *hid, u8 *report, > - unsigned offset, unsigned n, u32 value) > +void implement(const struct hid_device *hid, u8 *report, unsigned int offset, unsigned int n, > + u32 value) > { > if (unlikely(n > 32)) { > hid_warn(hid, "%s() called with n (%d) > 32! (%s)\n", > diff --git a/include/linux/hid.h b/include/linux/hid.h > index 66d949d10b78..7454e844324c 100644 > --- a/include/linux/hid.h > +++ b/include/linux/hid.h > @@ -944,6 +944,8 @@ bool hid_compare_device_paths(struct hid_device *hdev_a, > s32 hid_snto32(__u32 value, unsigned n); > __u32 hid_field_extract(const struct hid_device *hid, __u8 *report, > unsigned offset, unsigned n); > +void implement(const struct hid_device *hid, u8 *report, unsigned int offset, unsigned int n, > + u32 value); > > #ifdef CONFIG_PM > int hid_driver_suspend(struct hid_device *hdev, pm_message_t state); > -- > 2.35.1 >
On Mon, Mar 21, 2022 at 9:07 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > Hi Song, > > many thanks for the quick response. > > On Fri, Mar 18, 2022 at 9:48 PM Song Liu <song@kernel.org> wrote: [...] > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > We need to mirror these changes to tools/include/uapi/linux/bpf.h. > > OK. I did that in patch 4/17 but I can bring in the changes there too. Let's keep changes to the two files in the same patch. This will make sure they are back ported together. [...] > > > +enum hid_bpf_event { > > > + HID_BPF_UNDEF = 0, > > > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ > > > + HID_BPF_RDESC_FIXUP, /* ................... BPF_HID_RDESC_FIXUP */ > > > + HID_BPF_USER_EVENT, /* ................... BPF_HID_USER_EVENT */ > > > > Why don't we have a DRIVER_EVENT type here? > > For driver event, I want to have a little bit more of information > which tells which event we have: > - HID_BPF_DRIVER_PROBE > - HID_BPF_DRIVER_SUSPEND > - HID_BPF_DRIVER_RAW_REQUEST > - HID_BPF_DRIVER_RAW_REQUEST_ANSWER > - etc... > > However, I am not entirely sure on the implementation of all of those, > so I left them aside for now. > > I'll work on that for v4. This set is already pretty big. I guess we can add them in a follow-up set. > > > > > > > > [...] > > > + > > > +BPF_CALL_3(bpf_hid_get_data, struct hid_bpf_ctx_kern*, ctx, u64, offset, u64, size) > > > +{ > > > + if (!size) > > > + return 0UL; > > > + > > > + if (offset + size > ctx->allocated_size) > > > + return 0UL; > > > + > > > + return (unsigned long)(ctx->data + offset); > > > +} > > > + > > > +static const struct bpf_func_proto bpf_hid_get_data_proto = { > > > + .func = bpf_hid_get_data, > > > + .gpl_only = true, > > > + .ret_type = RET_PTR_TO_ALLOC_MEM_OR_NULL, > > > + .arg1_type = ARG_PTR_TO_CTX, > > > + .arg2_type = ARG_ANYTHING, > > > + .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO, > > > > I think we should use ARG_CONST_SIZE or ARG_CONST_SIZE_OR_ZERO? > > I initially tried this with ARG_CONST_SIZE_OR_ZERO but it doesn't work > for 2 reasons: > - we need to pair the argument ARG_CONST_SIZE_* with a pointer to a > memory just before, which doesn't really make sense here > - ARG_CONST_SIZE_* isn't handled in the same way > ARG_CONST_ALLOC_SIZE_OR_ZERO is. The latter tells the verifier that > the given size is the available size of the returned > PTR_TO_ALLOC_MEM_OR_NULL, which is exactly what we want. I misread the logic initially. It makes sense now. > > > > > > +}; > > > + > > > +static const struct bpf_func_proto * > > > +hid_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > > +{ > > > + switch (func_id) { > > > + case BPF_FUNC_hid_get_data: > > > + return &bpf_hid_get_data_proto; > > > + default: > > > + return bpf_base_func_proto(func_id); > > > + } > > > +} > > [...] > > > + > > > +static int hid_bpf_prog_test_run(struct bpf_prog *prog, > > > + const union bpf_attr *attr, > > > + union bpf_attr __user *uattr) > > > +{ > > > + struct hid_device *hdev = NULL; > > > + struct bpf_prog_array *progs; > > > + bool valid_prog = false; > > > + int i; > > > + int target_fd, ret; > > > + void __user *data_out = u64_to_user_ptr(attr->test.data_out); > > > + void __user *data_in = u64_to_user_ptr(attr->test.data_in); > > > + u32 user_size_in = attr->test.data_size_in; > > > + u32 user_size_out = attr->test.data_size_out; > > > + u32 allocated_size = max(user_size_in, user_size_out); > > > + struct hid_bpf_ctx_kern ctx = { > > > + .type = HID_BPF_USER_EVENT, > > > + .allocated_size = allocated_size, > > > + }; > > > + > > > + if (!hid_hooks.hdev_from_fd) > > > + return -EOPNOTSUPP; > > > + > > > + if (attr->test.ctx_size_in != sizeof(int)) > > > + return -EINVAL; > > > > ctx_size_in is always 4 bytes? > > Yes. Basically what I had in mind is that the "ctx" for > user_prog_test_run is the file descriptor to the sysfs that represent > the HID device. > This seemed to me to be the easiest to handle for users. > > I'm open to suggestions though. How about we use data_in? ctx for test_run usually means the program ctx, which is struct hid_bpf_ctx here. Thanks, Song
Hi Benjamin, I tested this iteration of the set, and I faced couple of problems with it. 1) There were some conflicts as I could not figure out the correct kernel commit on which to apply the series on. I applied this on top of last weeks bpf-next (see below) with some local merge fixes. commit 2af7e566a8616c278e1d7287ce86cd3900bed943 (bpf-next/master, bpf-next/for-next) Author: Saeed Mahameed <saeedm@nvidia.com> Date: Tue Mar 22 10:22:24 2022 -0700 net/mlx5e: Fix build warning, detected write beyond size of field 2) hid_is_valid_access() causes some trouble and it rejects pretty much every BPF program which tries to use ctx->retval. This appears to be because prog->expected_attach_type is not populated, I had to apply below local tweak to overcome this problem: diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c index 30a62e8e0f0a..bf64411e6e9b 100644 --- a/kernel/bpf/hid.c +++ b/kernel/bpf/hid.c @@ -180,8 +180,7 @@ static bool hid_is_valid_access(int off, int size, case offsetof(struct hid_bpf_ctx, retval): if (size != size_default) return false; - return (prog->expected_attach_type == BPF_HID_USER_EVENT || - prog->expected_attach_type == BPF_HID_DRIVER_EVENT); + return true; default: if (size != size_default) return false; Proper fix would probably be to actually populate the expected_attach_type, but I could not figure out quickly where this should be done, or whether it is actually done on some other base commit. With those, for the whole series: Tested-by: Tero Kristo <tero.kristo@linux.intel.com> On 18/03/2022 18:15, Benjamin Tissoires wrote: > Hi, > > This is a followup of my v1 at [0] and v2 at [1]. > > The short summary of the previous cover letter and discussions is that > HID could benefit from BPF for the following use cases: > > - simple fixup of report descriptor: > benefits are faster development time and testing, with the produced > bpf program being shipped in the kernel directly (the shipping part > is *not* addressed here). > > - Universal Stylus Interface: > allows a user-space program to define its own kernel interface > > - Surface Dial: > somehow similar to the previous one except that userspace can decide > to change the shape of the exported device > > - firewall: > still partly missing there, there is not yet interception of hidraw > calls, but it's coming in a followup series, I promise > > - tracing: > well, tracing. > > > I think I addressed the comments from the previous version, but there are > a few things I'd like to note here: > > - I did not take the various rev-by and tested-by (thanks a lot for those) > because the uapi changed significantly in v3, so I am not very confident > in taking those rev-by blindly > > - I mentioned in my discussion with Song that I'll put a summary of the uapi > in the cover letter, but I ended up adding a (long) file in the Documentation > directory. So please maybe start by reading 17/17 to have an overview of > what I want to achieve > > - I added in the libbpf and bpf the new type BPF_HID_DRIVER_EVENT, even though > I don't have a user of it right now in the kernel. I wanted to have them in > the docs, but we might not want to have them ready here. > In terms of code, it just means that we can attach such programs types > but that they will never get triggered. > > Anyway, I have been mulling on this for the past 2 weeks, and I think that > maybe sharing this now is better than me just starring at the code over and > over. > > > Short summary of changes: > > v3: > === > > - squashed back together most of the libbpf and bpf changes into bigger > commits that give a better overview of the whole interactions > > - reworked the user API to not expose .data as a directly accessible field > from the context, but instead forces everyone to use hid_bpf_get_data (or > get/set_bits) > > - added BPF_HID_DRIVER_EVENT (see note above) > > - addressed the various nitpicks from v2 > > - added a big Documentation file (and so adding now the doc maintainers to the > long list of recipients) > > v2: > === > > - split the series by subsystem (bpf, HID, libbpf, selftests and > samples) > > - Added an extra patch at the beginning to not require CAP_NET_ADMIN for > BPF_PROG_TYPE_LIRC_MODE2 (please shout if this is wrong) > > - made the bpf context attached to HID program of dynamic size: > * the first 1 kB will be able to be addressed directly > * the rest can be retrieved through bpf_hid_{set|get}_data > (note that I am definitivey not happy with that API, because there > is part of it in bits and other in bytes. ouch) > > - added an extra patch to prevent non GPL HID bpf programs to be loaded > of type BPF_PROG_TYPE_HID > * same here, not really happy but I don't know where to put that check > in verifier.c > > - added a new flag BPF_F_INSERT_HEAD for BPF_LINK_CREATE syscall when in > used with HID program types. > * this flag is used for tracing, to be able to load a program before > any others that might already have been inserted and that might > change the data stream. > > Cheers, > Benjamin > > > > [0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@redhat.com/T/#t > [1] https://lore.kernel.org/linux-input/20220304172852.274126-1-benjamin.tissoires@redhat.com/T/#t > > > Benjamin Tissoires (17): > bpf: add new is_sys_admin_prog_type() helper > bpf: introduce hid program type > bpf/verifier: prevent non GPL programs to be loaded against HID > libbpf: add HID program type and API > HID: hook up with bpf > HID: allow to change the report descriptor from an eBPF program > selftests/bpf: add tests for the HID-bpf initial implementation > selftests/bpf: add report descriptor fixup tests > selftests/bpf: Add a test for BPF_F_INSERT_HEAD > selftests/bpf: add test for user call of HID bpf programs > samples/bpf: add new hid_mouse example > bpf/hid: add more HID helpers > HID: bpf: implement hid_bpf_get|set_bits > HID: add implementation of bpf_hid_raw_request > selftests/bpf: add tests for hid_{get|set}_bits helpers > selftests/bpf: add tests for bpf_hid_hw_request > Documentation: add HID-BPF docs > > Documentation/hid/hid-bpf.rst | 444 +++++++++++ > Documentation/hid/index.rst | 1 + > drivers/hid/Makefile | 1 + > drivers/hid/hid-bpf.c | 328 ++++++++ > drivers/hid/hid-core.c | 34 +- > include/linux/bpf-hid.h | 127 +++ > include/linux/bpf_types.h | 4 + > include/linux/hid.h | 36 +- > include/uapi/linux/bpf.h | 67 ++ > include/uapi/linux/bpf_hid.h | 71 ++ > include/uapi/linux/hid.h | 10 + > kernel/bpf/Makefile | 3 + > kernel/bpf/btf.c | 1 + > kernel/bpf/hid.c | 728 +++++++++++++++++ > kernel/bpf/syscall.c | 27 +- > kernel/bpf/verifier.c | 7 + > samples/bpf/.gitignore | 1 + > samples/bpf/Makefile | 4 + > samples/bpf/hid_mouse_kern.c | 117 +++ > samples/bpf/hid_mouse_user.c | 129 +++ > tools/include/uapi/linux/bpf.h | 67 ++ > tools/lib/bpf/libbpf.c | 23 +- > tools/lib/bpf/libbpf.h | 2 + > tools/lib/bpf/libbpf.map | 1 + > tools/testing/selftests/bpf/config | 3 + > tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++ > tools/testing/selftests/bpf/progs/hid.c | 205 +++++ > 27 files changed, 3204 insertions(+), 25 deletions(-) > create mode 100644 Documentation/hid/hid-bpf.rst > create mode 100644 drivers/hid/hid-bpf.c > create mode 100644 include/linux/bpf-hid.h > create mode 100644 include/uapi/linux/bpf_hid.h > create mode 100644 kernel/bpf/hid.c > create mode 100644 samples/bpf/hid_mouse_kern.c > create mode 100644 samples/bpf/hid_mouse_user.c > create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c > create mode 100644 tools/testing/selftests/bpf/progs/hid.c >
Hi Tero, On Tue, Mar 29, 2022 at 3:04 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: > > Hi Benjamin, > > I tested this iteration of the set, and I faced couple of problems with it. > > 1) There were some conflicts as I could not figure out the correct > kernel commit on which to apply the series on. I applied this on top of > last weeks bpf-next (see below) with some local merge fixes. Right, there was a new conflict in bpf-next, but you managed it :) > > commit 2af7e566a8616c278e1d7287ce86cd3900bed943 (bpf-next/master, > bpf-next/for-next) > Author: Saeed Mahameed <saeedm@nvidia.com> > Date: Tue Mar 22 10:22:24 2022 -0700 > > net/mlx5e: Fix build warning, detected write beyond size of field > > 2) hid_is_valid_access() causes some trouble and it rejects pretty much > every BPF program which tries to use ctx->retval. This appears to be > because prog->expected_attach_type is not populated, I had to apply > below local tweak to overcome this problem: > > diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c > index 30a62e8e0f0a..bf64411e6e9b 100644 > --- a/kernel/bpf/hid.c > +++ b/kernel/bpf/hid.c > @@ -180,8 +180,7 @@ static bool hid_is_valid_access(int off, int size, > case offsetof(struct hid_bpf_ctx, retval): > if (size != size_default) > return false; > - return (prog->expected_attach_type == BPF_HID_USER_EVENT || > - prog->expected_attach_type == BPF_HID_DRIVER_EVENT); > + return true; > default: > if (size != size_default) > return false; > > Proper fix would probably be to actually populate the > expected_attach_type, but I could not figure out quickly where this > should be done, or whether it is actually done on some other base commit. Hmm, this is not what I would have expected. Anyway, "return true" would be a valid solution too, but... > > With those, for the whole series: > > Tested-by: Tero Kristo <tero.kristo@linux.intel.com> Thanks a lot. Unfortunately, if you saw the discussion with Alexei in patch 6/17, you'll see that there is a push toward a slightly different implementation. I had a meeting with Alexei, and a few other BPF folks yesterday, and they convinced me that this series is implementing a BPF feature the "old way", and that we should aim at having HID using standard BPF facilities instead of having HID messing up with bpf-core. This will be beneficial in the long term as we won't depend on BPF to be able to add new UAPI, being BPF calls or functions. I'll reply in more detail on 6/17. Cheers, Benjamin > > On 18/03/2022 18:15, Benjamin Tissoires wrote: > > Hi, > > > > This is a followup of my v1 at [0] and v2 at [1]. > > > > The short summary of the previous cover letter and discussions is that > > HID could benefit from BPF for the following use cases: > > > > - simple fixup of report descriptor: > > benefits are faster development time and testing, with the produced > > bpf program being shipped in the kernel directly (the shipping part > > is *not* addressed here). > > > > - Universal Stylus Interface: > > allows a user-space program to define its own kernel interface > > > > - Surface Dial: > > somehow similar to the previous one except that userspace can decide > > to change the shape of the exported device > > > > - firewall: > > still partly missing there, there is not yet interception of hidraw > > calls, but it's coming in a followup series, I promise > > > > - tracing: > > well, tracing. > > > > > > I think I addressed the comments from the previous version, but there are > > a few things I'd like to note here: > > > > - I did not take the various rev-by and tested-by (thanks a lot for those) > > because the uapi changed significantly in v3, so I am not very confident > > in taking those rev-by blindly > > > > - I mentioned in my discussion with Song that I'll put a summary of the uapi > > in the cover letter, but I ended up adding a (long) file in the Documentation > > directory. So please maybe start by reading 17/17 to have an overview of > > what I want to achieve > > > > - I added in the libbpf and bpf the new type BPF_HID_DRIVER_EVENT, even though > > I don't have a user of it right now in the kernel. I wanted to have them in > > the docs, but we might not want to have them ready here. > > In terms of code, it just means that we can attach such programs types > > but that they will never get triggered. > > > > Anyway, I have been mulling on this for the past 2 weeks, and I think that > > maybe sharing this now is better than me just starring at the code over and > > over. > > > > > > Short summary of changes: > > > > v3: > > === > > > > - squashed back together most of the libbpf and bpf changes into bigger > > commits that give a better overview of the whole interactions > > > > - reworked the user API to not expose .data as a directly accessible field > > from the context, but instead forces everyone to use hid_bpf_get_data (or > > get/set_bits) > > > > - added BPF_HID_DRIVER_EVENT (see note above) > > > > - addressed the various nitpicks from v2 > > > > - added a big Documentation file (and so adding now the doc maintainers to the > > long list of recipients) > > > > v2: > > === > > > > - split the series by subsystem (bpf, HID, libbpf, selftests and > > samples) > > > > - Added an extra patch at the beginning to not require CAP_NET_ADMIN for > > BPF_PROG_TYPE_LIRC_MODE2 (please shout if this is wrong) > > > > - made the bpf context attached to HID program of dynamic size: > > * the first 1 kB will be able to be addressed directly > > * the rest can be retrieved through bpf_hid_{set|get}_data > > (note that I am definitivey not happy with that API, because there > > is part of it in bits and other in bytes. ouch) > > > > - added an extra patch to prevent non GPL HID bpf programs to be loaded > > of type BPF_PROG_TYPE_HID > > * same here, not really happy but I don't know where to put that check > > in verifier.c > > > > - added a new flag BPF_F_INSERT_HEAD for BPF_LINK_CREATE syscall when in > > used with HID program types. > > * this flag is used for tracing, to be able to load a program before > > any others that might already have been inserted and that might > > change the data stream. > > > > Cheers, > > Benjamin > > > > > > > > [0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@redhat.com/T/#t > > [1] https://lore.kernel.org/linux-input/20220304172852.274126-1-benjamin.tissoires@redhat.com/T/#t > > > > > > Benjamin Tissoires (17): > > bpf: add new is_sys_admin_prog_type() helper > > bpf: introduce hid program type > > bpf/verifier: prevent non GPL programs to be loaded against HID > > libbpf: add HID program type and API > > HID: hook up with bpf > > HID: allow to change the report descriptor from an eBPF program > > selftests/bpf: add tests for the HID-bpf initial implementation > > selftests/bpf: add report descriptor fixup tests > > selftests/bpf: Add a test for BPF_F_INSERT_HEAD > > selftests/bpf: add test for user call of HID bpf programs > > samples/bpf: add new hid_mouse example > > bpf/hid: add more HID helpers > > HID: bpf: implement hid_bpf_get|set_bits > > HID: add implementation of bpf_hid_raw_request > > selftests/bpf: add tests for hid_{get|set}_bits helpers > > selftests/bpf: add tests for bpf_hid_hw_request > > Documentation: add HID-BPF docs > > > > Documentation/hid/hid-bpf.rst | 444 +++++++++++ > > Documentation/hid/index.rst | 1 + > > drivers/hid/Makefile | 1 + > > drivers/hid/hid-bpf.c | 328 ++++++++ > > drivers/hid/hid-core.c | 34 +- > > include/linux/bpf-hid.h | 127 +++ > > include/linux/bpf_types.h | 4 + > > include/linux/hid.h | 36 +- > > include/uapi/linux/bpf.h | 67 ++ > > include/uapi/linux/bpf_hid.h | 71 ++ > > include/uapi/linux/hid.h | 10 + > > kernel/bpf/Makefile | 3 + > > kernel/bpf/btf.c | 1 + > > kernel/bpf/hid.c | 728 +++++++++++++++++ > > kernel/bpf/syscall.c | 27 +- > > kernel/bpf/verifier.c | 7 + > > samples/bpf/.gitignore | 1 + > > samples/bpf/Makefile | 4 + > > samples/bpf/hid_mouse_kern.c | 117 +++ > > samples/bpf/hid_mouse_user.c | 129 +++ > > tools/include/uapi/linux/bpf.h | 67 ++ > > tools/lib/bpf/libbpf.c | 23 +- > > tools/lib/bpf/libbpf.h | 2 + > > tools/lib/bpf/libbpf.map | 1 + > > tools/testing/selftests/bpf/config | 3 + > > tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++ > > tools/testing/selftests/bpf/progs/hid.c | 205 +++++ > > 27 files changed, 3204 insertions(+), 25 deletions(-) > > create mode 100644 Documentation/hid/hid-bpf.rst > > create mode 100644 drivers/hid/hid-bpf.c > > create mode 100644 include/linux/bpf-hid.h > > create mode 100644 include/uapi/linux/bpf_hid.h > > create mode 100644 kernel/bpf/hid.c > > create mode 100644 samples/bpf/hid_mouse_kern.c > > create mode 100644 samples/bpf/hid_mouse_user.c > > create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c > > create mode 100644 tools/testing/selftests/bpf/progs/hid.c > > >
Hi, This is a followup of my v1 at [0] and v2 at [1]. The short summary of the previous cover letter and discussions is that HID could benefit from BPF for the following use cases: - simple fixup of report descriptor: benefits are faster development time and testing, with the produced bpf program being shipped in the kernel directly (the shipping part is *not* addressed here). - Universal Stylus Interface: allows a user-space program to define its own kernel interface - Surface Dial: somehow similar to the previous one except that userspace can decide to change the shape of the exported device - firewall: still partly missing there, there is not yet interception of hidraw calls, but it's coming in a followup series, I promise - tracing: well, tracing. I think I addressed the comments from the previous version, but there are a few things I'd like to note here: - I did not take the various rev-by and tested-by (thanks a lot for those) because the uapi changed significantly in v3, so I am not very confident in taking those rev-by blindly - I mentioned in my discussion with Song that I'll put a summary of the uapi in the cover letter, but I ended up adding a (long) file in the Documentation directory. So please maybe start by reading 17/17 to have an overview of what I want to achieve - I added in the libbpf and bpf the new type BPF_HID_DRIVER_EVENT, even though I don't have a user of it right now in the kernel. I wanted to have them in the docs, but we might not want to have them ready here. In terms of code, it just means that we can attach such programs types but that they will never get triggered. Anyway, I have been mulling on this for the past 2 weeks, and I think that maybe sharing this now is better than me just starring at the code over and over. Short summary of changes: v3: === - squashed back together most of the libbpf and bpf changes into bigger commits that give a better overview of the whole interactions - reworked the user API to not expose .data as a directly accessible field from the context, but instead forces everyone to use hid_bpf_get_data (or get/set_bits) - added BPF_HID_DRIVER_EVENT (see note above) - addressed the various nitpicks from v2 - added a big Documentation file (and so adding now the doc maintainers to the long list of recipients) v2: === - split the series by subsystem (bpf, HID, libbpf, selftests and samples) - Added an extra patch at the beginning to not require CAP_NET_ADMIN for BPF_PROG_TYPE_LIRC_MODE2 (please shout if this is wrong) - made the bpf context attached to HID program of dynamic size: * the first 1 kB will be able to be addressed directly * the rest can be retrieved through bpf_hid_{set|get}_data (note that I am definitivey not happy with that API, because there is part of it in bits and other in bytes. ouch) - added an extra patch to prevent non GPL HID bpf programs to be loaded of type BPF_PROG_TYPE_HID * same here, not really happy but I don't know where to put that check in verifier.c - added a new flag BPF_F_INSERT_HEAD for BPF_LINK_CREATE syscall when in used with HID program types. * this flag is used for tracing, to be able to load a program before any others that might already have been inserted and that might change the data stream. Cheers, Benjamin [0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@redhat.com/T/#t [1] https://lore.kernel.org/linux-input/20220304172852.274126-1-benjamin.tissoires@redhat.com/T/#t Benjamin Tissoires (17): bpf: add new is_sys_admin_prog_type() helper bpf: introduce hid program type bpf/verifier: prevent non GPL programs to be loaded against HID libbpf: add HID program type and API HID: hook up with bpf HID: allow to change the report descriptor from an eBPF program selftests/bpf: add tests for the HID-bpf initial implementation selftests/bpf: add report descriptor fixup tests selftests/bpf: Add a test for BPF_F_INSERT_HEAD selftests/bpf: add test for user call of HID bpf programs samples/bpf: add new hid_mouse example bpf/hid: add more HID helpers HID: bpf: implement hid_bpf_get|set_bits HID: add implementation of bpf_hid_raw_request selftests/bpf: add tests for hid_{get|set}_bits helpers selftests/bpf: add tests for bpf_hid_hw_request Documentation: add HID-BPF docs Documentation/hid/hid-bpf.rst | 444 +++++++++++ Documentation/hid/index.rst | 1 + drivers/hid/Makefile | 1 + drivers/hid/hid-bpf.c | 328 ++++++++ drivers/hid/hid-core.c | 34 +- include/linux/bpf-hid.h | 127 +++ include/linux/bpf_types.h | 4 + include/linux/hid.h | 36 +- include/uapi/linux/bpf.h | 67 ++ include/uapi/linux/bpf_hid.h | 71 ++ include/uapi/linux/hid.h | 10 + kernel/bpf/Makefile | 3 + kernel/bpf/btf.c | 1 + kernel/bpf/hid.c | 728 +++++++++++++++++ kernel/bpf/syscall.c | 27 +- kernel/bpf/verifier.c | 7 + samples/bpf/.gitignore | 1 + samples/bpf/Makefile | 4 + samples/bpf/hid_mouse_kern.c | 117 +++ samples/bpf/hid_mouse_user.c | 129 +++ tools/include/uapi/linux/bpf.h | 67 ++ tools/lib/bpf/libbpf.c | 23 +- tools/lib/bpf/libbpf.h | 2 + tools/lib/bpf/libbpf.map | 1 + tools/testing/selftests/bpf/config | 3 + tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 205 +++++ 27 files changed, 3204 insertions(+), 25 deletions(-) create mode 100644 Documentation/hid/hid-bpf.rst create mode 100644 drivers/hid/hid-bpf.c create mode 100644 include/linux/bpf-hid.h create mode 100644 include/uapi/linux/bpf_hid.h create mode 100644 kernel/bpf/hid.c create mode 100644 samples/bpf/hid_mouse_kern.c create mode 100644 samples/bpf/hid_mouse_user.c create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c create mode 100644 tools/testing/selftests/bpf/progs/hid.c