We do this implementation in several steps to not have the CI failing:
- first (this patch), we add struct_ops while keeping the existing infra
available
- then we change the selftests, the examples and the existing in-tree
HID-BPF programs
- then we remove the existing trace points making old HID-BPF obsolete
There are a few advantages of struct_ops over tracing:
- compatibility with sleepable programs (for hid_hw_raw_request() in
a later patch)
- a lot simpler in the kernel: it's a simple rcu protected list
- we can add more parameters to the function called without much trouble
- the "attach" is now generic through BPF-core: the caller just needs to
set hid_id and flags before calling __load().
- all the BPF tough part is not handled in BPF-core through generic
processing
- hid_bpf_ctx is now only writable where it needs be
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
drivers/hid/bpf/Makefile | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 52 +++++++-
drivers/hid/bpf/hid_bpf_dispatch.h | 4 +
drivers/hid/bpf/hid_bpf_jmp_table.c | 3 +
drivers/hid/bpf/hid_bpf_struct_ops.c | 246 +++++++++++++++++++++++++++++++++++
include/linux/hid_bpf.h | 64 ++++++++-
6 files changed, 362 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/bpf/Makefile b/drivers/hid/bpf/Makefile
index cf55120cf7d6..1cb3f31e9335 100644
--- a/drivers/hid/bpf/Makefile
+++ b/drivers/hid/bpf/Makefile
@@ -8,4 +8,4 @@ LIBBPF_INCLUDE = $(srctree)/tools/lib
obj-$(CONFIG_HID_BPF) += hid_bpf.o
CFLAGS_hid_bpf_dispatch.o += -I$(LIBBPF_INCLUDE)
CFLAGS_hid_bpf_jmp_table.o += -I$(LIBBPF_INCLUDE)
-hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o
+hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o hid_bpf_struct_ops.o
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index c8bb79ce2354..7216c3c7713d 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -58,6 +58,7 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
},
.data = hdev->bpf.device_data,
};
+ struct hid_bpf_ops *e;
int ret;
if (type >= HID_REPORT_TYPES)
@@ -70,9 +71,25 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
memcpy(ctx_kern.data, data, *size);
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
+ if (e->hid_device_event) {
+ ret = e->hid_device_event(&ctx_kern.ctx, type);
+ if (ret < 0) {
+ rcu_read_unlock();
+ return ERR_PTR(ret);
+ }
+
+ if (ret)
+ ctx_kern.ctx.retval = ret;
+ }
+ }
+ rcu_read_unlock();
+
ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_DEVICE_EVENT, &ctx_kern);
if (ret < 0)
return ERR_PTR(ret);
+ ret = ctx_kern.ctx.retval;
if (ret) {
if (ret > ctx_kern.ctx.allocated_size)
@@ -122,7 +139,10 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
memcpy(ctx_kern.data, rdesc, min_t(unsigned int, *size, HID_MAX_DESCRIPTOR_SIZE));
- ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
+ if (hdev->bpf.rdesc_ops)
+ ret = hdev->bpf.rdesc_ops->hid_rdesc_fixup(&ctx_kern.ctx);
+ else
+ ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
if (ret < 0)
goto ignore_bpf;
@@ -150,7 +170,7 @@ static int device_match_id(struct device *dev, const void *id)
return hdev->id == *(int *)id;
}
-static struct hid_device *hid_get_device(unsigned int hid_id)
+struct hid_device *hid_get_device(unsigned int hid_id)
{
struct device *dev;
@@ -164,7 +184,7 @@ static struct hid_device *hid_get_device(unsigned int hid_id)
return to_hid_device(dev);
}
-static void hid_put_device(struct hid_device *hid)
+void hid_put_device(struct hid_device *hid)
{
put_device(&hid->dev);
}
@@ -205,7 +225,7 @@ static int __hid_bpf_allocate_data(struct hid_device *hdev, u8 **data, u32 *size
return 0;
}
-static int hid_bpf_allocate_event_data(struct hid_device *hdev)
+int hid_bpf_allocate_event_data(struct hid_device *hdev)
{
/* hdev->bpf.device_data is already allocated, abort */
if (hdev->bpf.device_data)
@@ -592,14 +612,22 @@ static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
int hid_bpf_connect_device(struct hid_device *hdev)
{
- struct hid_bpf_prog_list *prog_list;
+ bool need_to_allocate = false;
+ struct hid_bpf_ops *e;
rcu_read_lock();
- prog_list = rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]);
+ list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
+ if (e->hid_device_event) {
+ need_to_allocate = true;
+ break;
+ }
+ }
+ if (rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]))
+ need_to_allocate = true;
rcu_read_unlock();
/* only allocate BPF data if there are programs attached */
- if (!prog_list)
+ if (!need_to_allocate)
return 0;
return hid_bpf_allocate_event_data(hdev);
@@ -623,12 +651,15 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
hdev->bpf.destroyed = true;
__hid_bpf_destroy_device(hdev);
+ __hid_bpf_ops_destroy_device(hdev);
}
EXPORT_SYMBOL_GPL(hid_bpf_destroy_device);
void hid_bpf_device_init(struct hid_device *hdev)
{
spin_lock_init(&hdev->bpf.progs_lock);
+ INIT_LIST_HEAD(&hdev->bpf.prog_list);
+ mutex_init(&hdev->bpf.prog_list_lock);
}
EXPORT_SYMBOL_GPL(hid_bpf_device_init);
@@ -662,6 +693,13 @@ static int __init hid_bpf_init(void)
return 0;
}
+ /* register struct_ops kfuncs after we are sure we can load our preloaded bpf program */
+ err = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &hid_bpf_kfunc_set);
+ if (err) {
+ pr_warn("error while setting HID BPF tracing kfuncs: %d", err);
+ return 0;
+ }
+
/* register syscalls after we are sure we can load our preloaded bpf program */
err = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &hid_bpf_syscall_kfunc_set);
if (err) {
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
index fbe0639d09f2..e52c43d81650 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.h
+++ b/drivers/hid/bpf/hid_bpf_dispatch.h
@@ -10,12 +10,16 @@ struct hid_bpf_ctx_kern {
u8 *data;
};
+struct hid_device *hid_get_device(unsigned int hid_id);
+void hid_put_device(struct hid_device *hid);
+int hid_bpf_allocate_event_data(struct hid_device *hdev);
int hid_bpf_preload_skel(void);
void hid_bpf_free_links_and_skel(void);
int hid_bpf_get_prog_attach_type(struct bpf_prog *prog);
int __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type, int prog_fd,
struct bpf_prog *prog, __u32 flags);
void __hid_bpf_destroy_device(struct hid_device *hdev);
+void __hid_bpf_ops_destroy_device(struct hid_device *hdev);
int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
struct hid_bpf_ctx_kern *ctx_kern);
int hid_bpf_reconnect(struct hid_device *hdev);
diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
index aa8e1c79cdf5..8a54ba447718 100644
--- a/drivers/hid/bpf/hid_bpf_jmp_table.c
+++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
@@ -81,6 +81,9 @@ static int hid_bpf_program_count(struct hid_device *hdev,
if (type >= HID_BPF_PROG_TYPE_MAX)
return -EINVAL;
+ if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP && hdev->bpf.rdesc_ops)
+ n += 1;
+
FOR_ENTRIES(i, jmp_table.tail, jmp_table.head) {
struct hid_bpf_prog_entry *entry = &jmp_table.entries[i];
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
new file mode 100644
index 000000000000..be514a98e55b
--- /dev/null
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -0,0 +1,246 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * HID-BPF support for Linux
+ *
+ * Copyright (c) 2024 Benjamin Tissoires
+ */
+
+#include <linux/bitops.h>
+#include <linux/bpf_verifier.h>
+#include <linux/bpf.h>
+#include <linux/btf.h>
+#include <linux/btf_ids.h>
+#include <linux/filter.h>
+#include <linux/hid.h>
+#include <linux/hid_bpf.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include "hid_bpf_dispatch.h"
+
+static struct btf *hid_bpf_ops_btf;
+
+static int hid_bpf_ops_init(struct btf *btf)
+{
+ hid_bpf_ops_btf = btf;
+ return 0;
+}
+
+static bool hid_bpf_ops_is_valid_access(int off, int size,
+ enum bpf_access_type type,
+ const struct bpf_prog *prog,
+ struct bpf_insn_access_aux *info)
+{
+ return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
+}
+
+static int hid_bpf_ops_check_member(const struct btf_type *t,
+ const struct btf_member *member,
+ const struct bpf_prog *prog)
+{
+ u32 moff = __btf_member_bit_offset(t, member) / 8;
+
+ switch (moff) {
+ case offsetof(struct hid_bpf_ops, hid_rdesc_fixup):
+ break;
+ default:
+ if (prog->sleepable)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
+ const struct bpf_reg_state *reg,
+ int off, int size)
+{
+ const struct btf_type *state;
+ const struct btf_type *t;
+ s32 type_id;
+
+ type_id = btf_find_by_name_kind(reg->btf, "hid_bpf_ctx",
+ BTF_KIND_STRUCT);
+ if (type_id < 0)
+ return -EINVAL;
+
+ t = btf_type_by_id(reg->btf, reg->btf_id);
+ state = btf_type_by_id(reg->btf, type_id);
+ if (t != state) {
+ bpf_log(log, "only access to hid_bpf_ctx is supported\n");
+ return -EACCES;
+ }
+
+ /* out-of-bound access in hid_bpf_ctx */
+ if (off + size > sizeof(struct hid_bpf_ctx)) {
+ bpf_log(log, "write access at off %d with size %d\n", off, size);
+ return -EACCES;
+ }
+
+ if (off < offsetof(struct hid_bpf_ctx, retval)) {
+ bpf_log(log,
+ "write access at off %d with size %d on read-only part of hid_bpf_ctx\n",
+ off, size);
+ return -EACCES;
+ }
+
+ return NOT_INIT;
+}
+
+static const struct bpf_verifier_ops hid_bpf_verifier_ops = {
+ .is_valid_access = hid_bpf_ops_is_valid_access,
+ .btf_struct_access = hid_bpf_ops_btf_struct_access,
+};
+
+static int hid_bpf_ops_init_member(const struct btf_type *t,
+ const struct btf_member *member,
+ void *kdata, const void *udata)
+{
+ u32 flags;
+
+ switch (member->offset) {
+ case offsetof(struct hid_bpf_ops, hid_id) * 8:
+ /* For hid_id and flags fields, this function has to copy it
+ * and return 1 to indicate that the data has been handled by
+ * the struct_ops type, or the verifier will reject the map if
+ * the value of those fields is not zero.
+ */
+ ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
+ return 1;
+ case offsetof(struct hid_bpf_ops, flags) * 8:
+ flags = ((struct hid_bpf_ops *)udata)->flags;
+ if (flags & ~HID_BPF_FLAG_MASK)
+ return -EINVAL;
+ ((struct hid_bpf_ops *)kdata)->flags = flags;
+ return 1;
+ }
+ return 0;
+}
+
+static int hid_bpf_reg(void *kdata)
+{
+ struct hid_bpf_ops *ops = kdata;
+ struct hid_device *hdev;
+ int count, err = 0;
+
+ hdev = hid_get_device(ops->hid_id);
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+
+ ops->hdev = hdev;
+
+ mutex_lock(&hdev->bpf.prog_list_lock);
+
+ count = list_count_nodes(&hdev->bpf.prog_list);
+ if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
+ err = -E2BIG;
+ goto out_unlock;
+ }
+
+ if (ops->hid_rdesc_fixup) {
+ if (hdev->bpf.rdesc_ops) {
+ err = -EINVAL;
+ goto out_unlock;
+ }
+
+ hdev->bpf.rdesc_ops = ops;
+ }
+
+ if (ops->hid_device_event) {
+ err = hid_bpf_allocate_event_data(hdev);
+ if (err)
+ goto out_unlock;
+ }
+
+ if (ops->flags & HID_BPF_FLAG_INSERT_HEAD)
+ list_add_rcu(&ops->list, &hdev->bpf.prog_list);
+ else
+ list_add_tail_rcu(&ops->list, &hdev->bpf.prog_list);
+
+out_unlock:
+ mutex_unlock(&hdev->bpf.prog_list_lock);
+
+ if (err) {
+ if (hdev->bpf.rdesc_ops == ops)
+ hdev->bpf.rdesc_ops = NULL;
+ hid_put_device(hdev);
+ } else if (ops->hid_rdesc_fixup) {
+ hid_bpf_reconnect(hdev);
+ }
+
+ return err;
+}
+
+static void hid_bpf_unreg(void *kdata)
+{
+ struct hid_bpf_ops *ops = kdata;
+ struct hid_device *hdev;
+ bool reconnect = false;
+
+ hdev = ops->hdev;
+
+ /* check if __hid_bpf_ops_destroy_device() has been called */
+ if (!hdev)
+ return;
+
+ mutex_lock(&hdev->bpf.prog_list_lock);
+
+ list_del_rcu(&ops->list);
+
+ reconnect = hdev->bpf.rdesc_ops == ops;
+ if (reconnect)
+ hdev->bpf.rdesc_ops = NULL;
+
+ mutex_unlock(&hdev->bpf.prog_list_lock);
+
+ if (reconnect)
+ hid_bpf_reconnect(hdev);
+
+ hid_put_device(hdev);
+}
+
+static int __hid_bpf_device_event(struct hid_bpf_ctx *ctx, enum hid_report_type type)
+{
+ return 0;
+}
+
+static int __hid_bpf_rdesc_fixup(struct hid_bpf_ctx *ctx)
+{
+ return 0;
+}
+
+static struct hid_bpf_ops __bpf_hid_bpf_ops = {
+ .hid_device_event = __hid_bpf_device_event,
+ .hid_rdesc_fixup = __hid_bpf_rdesc_fixup,
+};
+
+static struct bpf_struct_ops bpf_hid_bpf_ops = {
+ .verifier_ops = &hid_bpf_verifier_ops,
+ .init = hid_bpf_ops_init,
+ .check_member = hid_bpf_ops_check_member,
+ .init_member = hid_bpf_ops_init_member,
+ .reg = hid_bpf_reg,
+ .unreg = hid_bpf_unreg,
+ .name = "hid_bpf_ops",
+ .cfi_stubs = &__bpf_hid_bpf_ops,
+ .owner = THIS_MODULE,
+};
+
+void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
+{
+ struct hid_bpf_ops *e;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
+ hid_put_device(hdev);
+ e->hdev = NULL;
+ }
+ rcu_read_unlock();
+}
+
+static int __init hid_bpf_struct_ops_init(void)
+{
+ return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
+}
+late_initcall(hid_bpf_struct_ops_init);
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index a66103618e6e..96495e977204 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -65,11 +65,12 @@ struct hid_bpf_ctx {
* @HID_BPF_FLAG_INSERT_HEAD: insert the given program before any other program
* currently attached to the device. This doesn't
* guarantee that this program will always be first
- * @HID_BPF_FLAG_MAX: sentinel value, not to be used by the callers
*/
enum hid_bpf_attach_flags {
HID_BPF_FLAG_NONE = 0,
HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
+
+ /* private: internal use only */
HID_BPF_FLAG_MAX,
};
@@ -112,6 +113,63 @@ struct hid_ops {
extern struct hid_ops *hid_ops;
+/**
+ * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
+ * programs to a HID device
+ * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
+ * cannot be changed after
+ * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
+ * Writeable only before ``load()``
+ */
+struct hid_bpf_ops {
+ /* hid_id needs to stay first so we can easily change it
+ * from userspace.
+ */
+ int hid_id;
+ u32 flags;
+
+ /* private: internal use only */
+ struct list_head list;
+
+ /* public: rest is public */
+
+/* fast path fields are put first to fill one cache line */
+
+ /**
+ * @hid_device_event: called whenever an event is coming in from the device
+ *
+ * It has the following arguments:
+ *
+ * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
+ *
+ * Return: %0 on success and keep processing; a positive
+ * value to change the incoming size buffer; a negative
+ * error code to interrupt the processing of this event
+ *
+ * Context: Interrupt context.
+ */
+ int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type);
+
+/* control/slow paths put last */
+
+ /**
+ * @hid_rdesc_fixup: called when the probe function parses the report descriptor
+ * of the HID device
+ *
+ * It has the following arguments:
+ *
+ * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
+ *
+ * Return: %0 on success and keep processing; a positive
+ * value to change the incoming size buffer; a negative
+ * error code to interrupt the processing of this device
+ */
+ int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
+
+ /* private: internal use only */
+ struct hid_device *hdev;
+} ____cacheline_aligned_in_smp;
+
struct hid_bpf_prog_list {
u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV];
u8 prog_cnt;
@@ -129,6 +187,10 @@ struct hid_bpf {
bool destroyed; /* prevents the assignment of any progs */
spinlock_t progs_lock; /* protects RCU update of progs */
+
+ struct hid_bpf_ops *rdesc_ops;
+ struct list_head prog_list;
+ struct mutex prog_list_lock; /* protects RCU update of prog_list */
};
/* specific HID-BPF link when a program is attached to a device */
--
2.44.0Hi Benjamin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 70ec81c2e2b4005465ad0d042e90b36087c36104]
url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/HID-rename-struct-hid_bpf_ops-into-hid_ops/20240528-212222
base: 70ec81c2e2b4005465ad0d042e90b36087c36104
patch link: https://lore.kernel.org/r/20240528-hid_bpf_struct_ops-v1-3-8c6663df27d8%40kernel.org
patch subject: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops
config: i386-buildonly-randconfig-002-20240531 (https://download.01.org/0day-ci/archive/20240531/202405312035.U1rZN04z-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240531/202405312035.U1rZN04z-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405312035.U1rZN04z-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/bpf_verifier.h:7:0,
from drivers/hid/bpf/hid_bpf_struct_ops.c:10:
drivers/hid/bpf/hid_bpf_struct_ops.c: In function 'hid_bpf_struct_ops_init':
>> include/linux/bpf.h:1844:50: warning: statement with no effect [-Wunused-value]
#define register_bpf_struct_ops(st_ops, type) ({ (void *)(st_ops); 0; })
^~~~~~~~~~~~~~~~
drivers/hid/bpf/hid_bpf_struct_ops.c:244:9: note: in expansion of macro 'register_bpf_struct_ops'
return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
^~~~~~~~~~~~~~~~~~~~~~~
vim +1844 include/linux/bpf.h
c196906d50e360d Hou Tao 2021-10-25 1834
c196906d50e360d Hou Tao 2021-10-25 1835 int bpf_struct_ops_test_run(struct bpf_prog *prog, const union bpf_attr *kattr,
c196906d50e360d Hou Tao 2021-10-25 1836 union bpf_attr __user *uattr);
c196906d50e360d Hou Tao 2021-10-25 1837 #endif
f6be98d19985411 Kui-Feng Lee 2024-01-19 1838 int bpf_struct_ops_desc_init(struct bpf_struct_ops_desc *st_ops_desc,
f6be98d19985411 Kui-Feng Lee 2024-01-19 1839 struct btf *btf,
f6be98d19985411 Kui-Feng Lee 2024-01-19 1840 struct bpf_verifier_log *log);
1338b93346587a2 Kui-Feng Lee 2024-01-19 1841 void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map);
1611603537a4b88 Kui-Feng Lee 2024-02-08 1842 void bpf_struct_ops_desc_release(struct bpf_struct_ops_desc *st_ops_desc);
27ae7997a66174c Martin KaFai Lau 2020-01-08 1843 #else
f6be98d19985411 Kui-Feng Lee 2024-01-19 @1844 #define register_bpf_struct_ops(st_ops, type) ({ (void *)(st_ops); 0; })
85d33df357b6346 Martin KaFai Lau 2020-01-08 1845 static inline bool bpf_try_module_get(const void *data, struct module *owner)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1846 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1847 return try_module_get(owner);
85d33df357b6346 Martin KaFai Lau 2020-01-08 1848 }
85d33df357b6346 Martin KaFai Lau 2020-01-08 1849 static inline void bpf_module_put(const void *data, struct module *owner)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1850 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1851 module_put(owner);
85d33df357b6346 Martin KaFai Lau 2020-01-08 1852 }
85d33df357b6346 Martin KaFai Lau 2020-01-08 1853 static inline int bpf_struct_ops_map_sys_lookup_elem(struct bpf_map *map,
85d33df357b6346 Martin KaFai Lau 2020-01-08 1854 void *key,
85d33df357b6346 Martin KaFai Lau 2020-01-08 1855 void *value)
85d33df357b6346 Martin KaFai Lau 2020-01-08 1856 {
85d33df357b6346 Martin KaFai Lau 2020-01-08 1857 return -EINVAL;
85d33df357b6346 Martin KaFai Lau 2020-01-08 1858 }
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1859 static inline int bpf_struct_ops_link_create(union bpf_attr *attr)
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1860 {
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1861 return -EOPNOTSUPP;
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1862 }
1338b93346587a2 Kui-Feng Lee 2024-01-19 1863 static inline void bpf_map_struct_ops_info_fill(struct bpf_map_info *info, struct bpf_map *map)
1338b93346587a2 Kui-Feng Lee 2024-01-19 1864 {
1338b93346587a2 Kui-Feng Lee 2024-01-19 1865 }
68b04864ca425d1 Kui-Feng Lee 2023-03-22 1866
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, May 28, 2024 at 6:15 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> We do this implementation in several steps to not have the CI failing:
> - first (this patch), we add struct_ops while keeping the existing infra
> available
> - then we change the selftests, the examples and the existing in-tree
> HID-BPF programs
> - then we remove the existing trace points making old HID-BPF obsolete
>
> There are a few advantages of struct_ops over tracing:
> - compatibility with sleepable programs (for hid_hw_raw_request() in
> a later patch)
> - a lot simpler in the kernel: it's a simple rcu protected list
> - we can add more parameters to the function called without much trouble
> - the "attach" is now generic through BPF-core: the caller just needs to
> set hid_id and flags before calling __load().
> - all the BPF tough part is not handled in BPF-core through generic
> processing
> - hid_bpf_ctx is now only writable where it needs be
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> drivers/hid/bpf/Makefile | 2 +-
> drivers/hid/bpf/hid_bpf_dispatch.c | 52 +++++++-
> drivers/hid/bpf/hid_bpf_dispatch.h | 4 +
> drivers/hid/bpf/hid_bpf_jmp_table.c | 3 +
> drivers/hid/bpf/hid_bpf_struct_ops.c | 246 +++++++++++++++++++++++++++++++++++
> include/linux/hid_bpf.h | 64 ++++++++-
> 6 files changed, 362 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/bpf/Makefile b/drivers/hid/bpf/Makefile
> index cf55120cf7d6..1cb3f31e9335 100644
> --- a/drivers/hid/bpf/Makefile
> +++ b/drivers/hid/bpf/Makefile
> @@ -8,4 +8,4 @@ LIBBPF_INCLUDE = $(srctree)/tools/lib
> obj-$(CONFIG_HID_BPF) += hid_bpf.o
> CFLAGS_hid_bpf_dispatch.o += -I$(LIBBPF_INCLUDE)
> CFLAGS_hid_bpf_jmp_table.o += -I$(LIBBPF_INCLUDE)
> -hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o
> +hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o hid_bpf_struct_ops.o
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index c8bb79ce2354..7216c3c7713d 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -58,6 +58,7 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
> },
> .data = hdev->bpf.device_data,
> };
> + struct hid_bpf_ops *e;
> int ret;
>
> if (type >= HID_REPORT_TYPES)
> @@ -70,9 +71,25 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
> memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
> memcpy(ctx_kern.data, data, *size);
>
> + rcu_read_lock();
> + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
In the typical case there will be only one prog per device, right?
The for_each is future proofing?
> + if (e->hid_device_event) {
> + ret = e->hid_device_event(&ctx_kern.ctx, type);
> + if (ret < 0) {
> + rcu_read_unlock();
> + return ERR_PTR(ret);
> + }
> +
> + if (ret)
> + ctx_kern.ctx.retval = ret;
> + }
> + }
> + rcu_read_unlock();
> +
> ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_DEVICE_EVENT, &ctx_kern);
> if (ret < 0)
> return ERR_PTR(ret);
> + ret = ctx_kern.ctx.retval;
>
> if (ret) {
> if (ret > ctx_kern.ctx.allocated_size)
> @@ -122,7 +139,10 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
>
> memcpy(ctx_kern.data, rdesc, min_t(unsigned int, *size, HID_MAX_DESCRIPTOR_SIZE));
>
> - ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
> + if (hdev->bpf.rdesc_ops)
> + ret = hdev->bpf.rdesc_ops->hid_rdesc_fixup(&ctx_kern.ctx);
> + else
> + ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
This is for backward compat?
I don't see it's being removed in the later patches.
> if (ret < 0)
> goto ignore_bpf;
>
> @@ -150,7 +170,7 @@ static int device_match_id(struct device *dev, const void *id)
> return hdev->id == *(int *)id;
> }
>
> -static struct hid_device *hid_get_device(unsigned int hid_id)
> +struct hid_device *hid_get_device(unsigned int hid_id)
> {
> struct device *dev;
>
> @@ -164,7 +184,7 @@ static struct hid_device *hid_get_device(unsigned int hid_id)
> return to_hid_device(dev);
> }
>
> -static void hid_put_device(struct hid_device *hid)
> +void hid_put_device(struct hid_device *hid)
> {
> put_device(&hid->dev);
> }
> @@ -205,7 +225,7 @@ static int __hid_bpf_allocate_data(struct hid_device *hdev, u8 **data, u32 *size
> return 0;
> }
>
> -static int hid_bpf_allocate_event_data(struct hid_device *hdev)
> +int hid_bpf_allocate_event_data(struct hid_device *hdev)
> {
> /* hdev->bpf.device_data is already allocated, abort */
> if (hdev->bpf.device_data)
> @@ -592,14 +612,22 @@ static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
>
> int hid_bpf_connect_device(struct hid_device *hdev)
> {
> - struct hid_bpf_prog_list *prog_list;
> + bool need_to_allocate = false;
> + struct hid_bpf_ops *e;
>
> rcu_read_lock();
> - prog_list = rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]);
> + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> + if (e->hid_device_event) {
> + need_to_allocate = true;
> + break;
> + }
> + }
> + if (rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]))
> + need_to_allocate = true;
> rcu_read_unlock();
>
> /* only allocate BPF data if there are programs attached */
> - if (!prog_list)
> + if (!need_to_allocate)
> return 0;
>
> return hid_bpf_allocate_event_data(hdev);
> @@ -623,12 +651,15 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
> hdev->bpf.destroyed = true;
>
> __hid_bpf_destroy_device(hdev);
> + __hid_bpf_ops_destroy_device(hdev);
> }
> EXPORT_SYMBOL_GPL(hid_bpf_destroy_device);
>
> void hid_bpf_device_init(struct hid_device *hdev)
> {
> spin_lock_init(&hdev->bpf.progs_lock);
> + INIT_LIST_HEAD(&hdev->bpf.prog_list);
> + mutex_init(&hdev->bpf.prog_list_lock);
> }
> EXPORT_SYMBOL_GPL(hid_bpf_device_init);
>
> @@ -662,6 +693,13 @@ static int __init hid_bpf_init(void)
> return 0;
> }
>
> + /* register struct_ops kfuncs after we are sure we can load our preloaded bpf program */
> + err = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &hid_bpf_kfunc_set);
> + if (err) {
> + pr_warn("error while setting HID BPF tracing kfuncs: %d", err);
> + return 0;
> + }
> +
> /* register syscalls after we are sure we can load our preloaded bpf program */
> err = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &hid_bpf_syscall_kfunc_set);
> if (err) {
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
> index fbe0639d09f2..e52c43d81650 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.h
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.h
> @@ -10,12 +10,16 @@ struct hid_bpf_ctx_kern {
> u8 *data;
> };
>
> +struct hid_device *hid_get_device(unsigned int hid_id);
> +void hid_put_device(struct hid_device *hid);
> +int hid_bpf_allocate_event_data(struct hid_device *hdev);
> int hid_bpf_preload_skel(void);
> void hid_bpf_free_links_and_skel(void);
> int hid_bpf_get_prog_attach_type(struct bpf_prog *prog);
> int __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type, int prog_fd,
> struct bpf_prog *prog, __u32 flags);
> void __hid_bpf_destroy_device(struct hid_device *hdev);
> +void __hid_bpf_ops_destroy_device(struct hid_device *hdev);
> int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
> struct hid_bpf_ctx_kern *ctx_kern);
> int hid_bpf_reconnect(struct hid_device *hdev);
> diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
> index aa8e1c79cdf5..8a54ba447718 100644
> --- a/drivers/hid/bpf/hid_bpf_jmp_table.c
> +++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
> @@ -81,6 +81,9 @@ static int hid_bpf_program_count(struct hid_device *hdev,
> if (type >= HID_BPF_PROG_TYPE_MAX)
> return -EINVAL;
>
> + if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP && hdev->bpf.rdesc_ops)
> + n += 1;
> +
> FOR_ENTRIES(i, jmp_table.tail, jmp_table.head) {
> struct hid_bpf_prog_entry *entry = &jmp_table.entries[i];
>
> diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> new file mode 100644
> index 000000000000..be514a98e55b
> --- /dev/null
> +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> @@ -0,0 +1,246 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * HID-BPF support for Linux
> + *
> + * Copyright (c) 2024 Benjamin Tissoires
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bpf_verifier.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> +#include <linux/btf_ids.h>
> +#include <linux/filter.h>
> +#include <linux/hid.h>
> +#include <linux/hid_bpf.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include "hid_bpf_dispatch.h"
> +
> +static struct btf *hid_bpf_ops_btf;
> +
> +static int hid_bpf_ops_init(struct btf *btf)
> +{
> + hid_bpf_ops_btf = btf;
> + return 0;
> +}
> +
> +static bool hid_bpf_ops_is_valid_access(int off, int size,
> + enum bpf_access_type type,
> + const struct bpf_prog *prog,
> + struct bpf_insn_access_aux *info)
> +{
> + return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
> +}
> +
> +static int hid_bpf_ops_check_member(const struct btf_type *t,
> + const struct btf_member *member,
> + const struct bpf_prog *prog)
> +{
> + u32 moff = __btf_member_bit_offset(t, member) / 8;
> +
this is good...
> + switch (moff) {
> + case offsetof(struct hid_bpf_ops, hid_rdesc_fixup):
> + break;
> + default:
> + if (prog->sleepable)
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
> + const struct bpf_reg_state *reg,
> + int off, int size)
> +{
> + const struct btf_type *state;
> + const struct btf_type *t;
> + s32 type_id;
> +
> + type_id = btf_find_by_name_kind(reg->btf, "hid_bpf_ctx",
> + BTF_KIND_STRUCT);
> + if (type_id < 0)
> + return -EINVAL;
> +
> + t = btf_type_by_id(reg->btf, reg->btf_id);
> + state = btf_type_by_id(reg->btf, type_id);
> + if (t != state) {
> + bpf_log(log, "only access to hid_bpf_ctx is supported\n");
> + return -EACCES;
> + }
> +
> + /* out-of-bound access in hid_bpf_ctx */
> + if (off + size > sizeof(struct hid_bpf_ctx)) {
> + bpf_log(log, "write access at off %d with size %d\n", off, size);
> + return -EACCES;
> + }
> +
> + if (off < offsetof(struct hid_bpf_ctx, retval)) {
> + bpf_log(log,
> + "write access at off %d with size %d on read-only part of hid_bpf_ctx\n",
> + off, size);
> + return -EACCES;
> + }
> +
> + return NOT_INIT;
> +}
> +
> +static const struct bpf_verifier_ops hid_bpf_verifier_ops = {
> + .is_valid_access = hid_bpf_ops_is_valid_access,
> + .btf_struct_access = hid_bpf_ops_btf_struct_access,
> +};
> +
> +static int hid_bpf_ops_init_member(const struct btf_type *t,
> + const struct btf_member *member,
> + void *kdata, const void *udata)
> +{
> + u32 flags;
> +
> + switch (member->offset) {
> + case offsetof(struct hid_bpf_ops, hid_id) * 8:
but here you're open coding it and adding ugly * 8
Just do:
moff = __btf_member_bit_offset(t, member) / 8;
switch (moff) {
case offsetof(struct tcp_congestion_ops, flags):
the way bpf_tcp_ca.c is doing?
Open code of ->offset is asking for trouble.
> + /* For hid_id and flags fields, this function has to copy it
> + * and return 1 to indicate that the data has been handled by
> + * the struct_ops type, or the verifier will reject the map if
> + * the value of those fields is not zero.
> + */
> + ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
> + return 1;
> + case offsetof(struct hid_bpf_ops, flags) * 8:
> + flags = ((struct hid_bpf_ops *)udata)->flags;
> + if (flags & ~HID_BPF_FLAG_MASK)
> + return -EINVAL;
> + ((struct hid_bpf_ops *)kdata)->flags = flags;
> + return 1;
> + }
> + return 0;
> +}
> +
> +static int hid_bpf_reg(void *kdata)
> +{
> + struct hid_bpf_ops *ops = kdata;
> + struct hid_device *hdev;
> + int count, err = 0;
> +
> + hdev = hid_get_device(ops->hid_id);
> + if (IS_ERR(hdev))
> + return PTR_ERR(hdev);
> +
> + ops->hdev = hdev;
> +
> + mutex_lock(&hdev->bpf.prog_list_lock);
> +
> + count = list_count_nodes(&hdev->bpf.prog_list);
> + if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
> + err = -E2BIG;
> + goto out_unlock;
> + }
> +
> + if (ops->hid_rdesc_fixup) {
> + if (hdev->bpf.rdesc_ops) {
> + err = -EINVAL;
> + goto out_unlock;
> + }
> +
> + hdev->bpf.rdesc_ops = ops;
> + }
> +
> + if (ops->hid_device_event) {
> + err = hid_bpf_allocate_event_data(hdev);
> + if (err)
> + goto out_unlock;
> + }
> +
> + if (ops->flags & HID_BPF_FLAG_INSERT_HEAD)
> + list_add_rcu(&ops->list, &hdev->bpf.prog_list);
> + else
> + list_add_tail_rcu(&ops->list, &hdev->bpf.prog_list);
Looks like future proofing, but I feel it's too little to materialize.
Take a look at include/linux/bpf_mprog.h
I suspect it might be useful here too.
> +
> +out_unlock:
> + mutex_unlock(&hdev->bpf.prog_list_lock);
> +
> + if (err) {
> + if (hdev->bpf.rdesc_ops == ops)
> + hdev->bpf.rdesc_ops = NULL;
> + hid_put_device(hdev);
> + } else if (ops->hid_rdesc_fixup) {
> + hid_bpf_reconnect(hdev);
> + }
> +
> + return err;
> +}
> +
> +static void hid_bpf_unreg(void *kdata)
> +{
> + struct hid_bpf_ops *ops = kdata;
> + struct hid_device *hdev;
> + bool reconnect = false;
> +
> + hdev = ops->hdev;
> +
> + /* check if __hid_bpf_ops_destroy_device() has been called */
> + if (!hdev)
> + return;
> +
> + mutex_lock(&hdev->bpf.prog_list_lock);
> +
> + list_del_rcu(&ops->list);
> +
> + reconnect = hdev->bpf.rdesc_ops == ops;
> + if (reconnect)
> + hdev->bpf.rdesc_ops = NULL;
> +
> + mutex_unlock(&hdev->bpf.prog_list_lock);
> +
> + if (reconnect)
> + hid_bpf_reconnect(hdev);
> +
> + hid_put_device(hdev);
> +}
> +
> +static int __hid_bpf_device_event(struct hid_bpf_ctx *ctx, enum hid_report_type type)
> +{
> + return 0;
> +}
> +
> +static int __hid_bpf_rdesc_fixup(struct hid_bpf_ctx *ctx)
> +{
> + return 0;
> +}
> +
> +static struct hid_bpf_ops __bpf_hid_bpf_ops = {
> + .hid_device_event = __hid_bpf_device_event,
> + .hid_rdesc_fixup = __hid_bpf_rdesc_fixup,
> +};
> +
> +static struct bpf_struct_ops bpf_hid_bpf_ops = {
> + .verifier_ops = &hid_bpf_verifier_ops,
> + .init = hid_bpf_ops_init,
> + .check_member = hid_bpf_ops_check_member,
> + .init_member = hid_bpf_ops_init_member,
> + .reg = hid_bpf_reg,
> + .unreg = hid_bpf_unreg,
> + .name = "hid_bpf_ops",
> + .cfi_stubs = &__bpf_hid_bpf_ops,
> + .owner = THIS_MODULE,
> +};
> +
> +void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
> +{
> + struct hid_bpf_ops *e;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> + hid_put_device(hdev);
> + e->hdev = NULL;
> + }
> + rcu_read_unlock();
> +}
> +
> +static int __init hid_bpf_struct_ops_init(void)
> +{
> + return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
> +}
> +late_initcall(hid_bpf_struct_ops_init);
> diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
> index a66103618e6e..96495e977204 100644
> --- a/include/linux/hid_bpf.h
> +++ b/include/linux/hid_bpf.h
> @@ -65,11 +65,12 @@ struct hid_bpf_ctx {
> * @HID_BPF_FLAG_INSERT_HEAD: insert the given program before any other program
> * currently attached to the device. This doesn't
> * guarantee that this program will always be first
> - * @HID_BPF_FLAG_MAX: sentinel value, not to be used by the callers
> */
> enum hid_bpf_attach_flags {
> HID_BPF_FLAG_NONE = 0,
> HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> +
> + /* private: internal use only */
> HID_BPF_FLAG_MAX,
> };
>
> @@ -112,6 +113,63 @@ struct hid_ops {
>
> extern struct hid_ops *hid_ops;
>
> +/**
> + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> + * programs to a HID device
> + * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
> + * cannot be changed after
> + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
> + * Writeable only before ``load()``
> + */
> +struct hid_bpf_ops {
> + /* hid_id needs to stay first so we can easily change it
> + * from userspace.
> + */
hmm.
patch 5 just does:
+ skel->struct_ops.mouse_invert->hid_id = hid_id;
I don't see a reason why it needs to be first.
> + int hid_id;
> + u32 flags;
> +
> + /* private: internal use only */
> + struct list_head list;
> +
> + /* public: rest is public */
> +
> +/* fast path fields are put first to fill one cache line */
> +
> + /**
> + * @hid_device_event: called whenever an event is coming in from the device
> + *
> + * It has the following arguments:
> + *
> + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> + *
> + * Return: %0 on success and keep processing; a positive
> + * value to change the incoming size buffer; a negative
> + * error code to interrupt the processing of this event
> + *
> + * Context: Interrupt context.
> + */
> + int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type);
> +
> +/* control/slow paths put last */
> +
> + /**
> + * @hid_rdesc_fixup: called when the probe function parses the report descriptor
> + * of the HID device
> + *
> + * It has the following arguments:
> + *
> + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> + *
> + * Return: %0 on success and keep processing; a positive
> + * value to change the incoming size buffer; a negative
> + * error code to interrupt the processing of this device
> + */
> + int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
It's fine since you want to keep the prog mostly unchanged,
but since the whole api is improving maybe it's time
to do something about hid_bpf_ctx in all callbacks?
Pass into prog what prog needs to see.
In the old api all progs had to have a common hid_bpf_ctx.
With struct-ops it can be fine tuned.
Overall it looks great.
hid_id did the trick :)
On May 28 2024, Alexei Starovoitov wrote:
> On Tue, May 28, 2024 at 6:15 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > We do this implementation in several steps to not have the CI failing:
> > - first (this patch), we add struct_ops while keeping the existing infra
> > available
> > - then we change the selftests, the examples and the existing in-tree
> > HID-BPF programs
> > - then we remove the existing trace points making old HID-BPF obsolete
> >
> > There are a few advantages of struct_ops over tracing:
> > - compatibility with sleepable programs (for hid_hw_raw_request() in
> > a later patch)
> > - a lot simpler in the kernel: it's a simple rcu protected list
> > - we can add more parameters to the function called without much trouble
> > - the "attach" is now generic through BPF-core: the caller just needs to
> > set hid_id and flags before calling __load().
> > - all the BPF tough part is not handled in BPF-core through generic
> > processing
> > - hid_bpf_ctx is now only writable where it needs be
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > drivers/hid/bpf/Makefile | 2 +-
> > drivers/hid/bpf/hid_bpf_dispatch.c | 52 +++++++-
> > drivers/hid/bpf/hid_bpf_dispatch.h | 4 +
> > drivers/hid/bpf/hid_bpf_jmp_table.c | 3 +
> > drivers/hid/bpf/hid_bpf_struct_ops.c | 246 +++++++++++++++++++++++++++++++++++
> > include/linux/hid_bpf.h | 64 ++++++++-
> > 6 files changed, 362 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hid/bpf/Makefile b/drivers/hid/bpf/Makefile
> > index cf55120cf7d6..1cb3f31e9335 100644
> > --- a/drivers/hid/bpf/Makefile
> > +++ b/drivers/hid/bpf/Makefile
> > @@ -8,4 +8,4 @@ LIBBPF_INCLUDE = $(srctree)/tools/lib
> > obj-$(CONFIG_HID_BPF) += hid_bpf.o
> > CFLAGS_hid_bpf_dispatch.o += -I$(LIBBPF_INCLUDE)
> > CFLAGS_hid_bpf_jmp_table.o += -I$(LIBBPF_INCLUDE)
> > -hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o
> > +hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_jmp_table.o hid_bpf_struct_ops.o
> > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> > index c8bb79ce2354..7216c3c7713d 100644
> > --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> > +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> > @@ -58,6 +58,7 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
> > },
> > .data = hdev->bpf.device_data,
> > };
> > + struct hid_bpf_ops *e;
> > int ret;
> >
> > if (type >= HID_REPORT_TYPES)
> > @@ -70,9 +71,25 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type
> > memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
> > memcpy(ctx_kern.data, data, *size);
> >
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
>
> In the typical case there will be only one prog per device, right?
90% of the time there is only one prog per device, but I need to have
more than one for the following reasons:
- when one prog is loaded, the output data gets modified, so to be able
to access the raw data from the device I need to add a "tracing" program
*before* the one that changes data to forward raw data to userspace
- we already have situation where for convenience multiple programs are
attached to one device (see drivers/hid/bpf/progs/XPPen__ArtistPro16Gen2.bpf.c)
- once firewalling will be in place, we will likely have one program for
the firewall and one for the device fix
> The for_each is future proofing?
>
> > + if (e->hid_device_event) {
> > + ret = e->hid_device_event(&ctx_kern.ctx, type);
> > + if (ret < 0) {
> > + rcu_read_unlock();
> > + return ERR_PTR(ret);
> > + }
> > +
> > + if (ret)
> > + ctx_kern.ctx.retval = ret;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_DEVICE_EVENT, &ctx_kern);
> > if (ret < 0)
> > return ERR_PTR(ret);
> > + ret = ctx_kern.ctx.retval;
> >
> > if (ret) {
> > if (ret > ctx_kern.ctx.allocated_size)
> > @@ -122,7 +139,10 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
> >
> > memcpy(ctx_kern.data, rdesc, min_t(unsigned int, *size, HID_MAX_DESCRIPTOR_SIZE));
> >
> > - ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
> > + if (hdev->bpf.rdesc_ops)
> > + ret = hdev->bpf.rdesc_ops->hid_rdesc_fixup(&ctx_kern.ctx);
> > + else
> > + ret = hid_bpf_prog_run(hdev, HID_BPF_PROG_TYPE_RDESC_FIXUP, &ctx_kern);
>
> This is for backward compat?
yes
> I don't see it's being removed in the later patches.
see patch 8/13, all the tracing capabilities are gone.
I made a backward compatibility step here so I do not break the
selftests while changing the API.
>
> > if (ret < 0)
> > goto ignore_bpf;
> >
> > @@ -150,7 +170,7 @@ static int device_match_id(struct device *dev, const void *id)
> > return hdev->id == *(int *)id;
> > }
> >
> > -static struct hid_device *hid_get_device(unsigned int hid_id)
> > +struct hid_device *hid_get_device(unsigned int hid_id)
> > {
> > struct device *dev;
> >
> > @@ -164,7 +184,7 @@ static struct hid_device *hid_get_device(unsigned int hid_id)
> > return to_hid_device(dev);
> > }
> >
> > -static void hid_put_device(struct hid_device *hid)
> > +void hid_put_device(struct hid_device *hid)
> > {
> > put_device(&hid->dev);
> > }
> > @@ -205,7 +225,7 @@ static int __hid_bpf_allocate_data(struct hid_device *hdev, u8 **data, u32 *size
> > return 0;
> > }
> >
> > -static int hid_bpf_allocate_event_data(struct hid_device *hdev)
> > +int hid_bpf_allocate_event_data(struct hid_device *hdev)
> > {
> > /* hdev->bpf.device_data is already allocated, abort */
> > if (hdev->bpf.device_data)
> > @@ -592,14 +612,22 @@ static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
> >
> > int hid_bpf_connect_device(struct hid_device *hdev)
> > {
> > - struct hid_bpf_prog_list *prog_list;
> > + bool need_to_allocate = false;
> > + struct hid_bpf_ops *e;
> >
> > rcu_read_lock();
> > - prog_list = rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]);
> > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> > + if (e->hid_device_event) {
> > + need_to_allocate = true;
> > + break;
> > + }
> > + }
> > + if (rcu_dereference(hdev->bpf.progs[HID_BPF_PROG_TYPE_DEVICE_EVENT]))
> > + need_to_allocate = true;
> > rcu_read_unlock();
> >
> > /* only allocate BPF data if there are programs attached */
> > - if (!prog_list)
> > + if (!need_to_allocate)
> > return 0;
> >
> > return hid_bpf_allocate_event_data(hdev);
> > @@ -623,12 +651,15 @@ void hid_bpf_destroy_device(struct hid_device *hdev)
> > hdev->bpf.destroyed = true;
> >
> > __hid_bpf_destroy_device(hdev);
> > + __hid_bpf_ops_destroy_device(hdev);
> > }
> > EXPORT_SYMBOL_GPL(hid_bpf_destroy_device);
> >
> > void hid_bpf_device_init(struct hid_device *hdev)
> > {
> > spin_lock_init(&hdev->bpf.progs_lock);
> > + INIT_LIST_HEAD(&hdev->bpf.prog_list);
> > + mutex_init(&hdev->bpf.prog_list_lock);
> > }
> > EXPORT_SYMBOL_GPL(hid_bpf_device_init);
> >
> > @@ -662,6 +693,13 @@ static int __init hid_bpf_init(void)
> > return 0;
> > }
> >
> > + /* register struct_ops kfuncs after we are sure we can load our preloaded bpf program */
> > + err = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, &hid_bpf_kfunc_set);
> > + if (err) {
> > + pr_warn("error while setting HID BPF tracing kfuncs: %d", err);
> > + return 0;
> > + }
> > +
> > /* register syscalls after we are sure we can load our preloaded bpf program */
> > err = register_btf_kfunc_id_set(BPF_PROG_TYPE_SYSCALL, &hid_bpf_syscall_kfunc_set);
> > if (err) {
> > diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
> > index fbe0639d09f2..e52c43d81650 100644
> > --- a/drivers/hid/bpf/hid_bpf_dispatch.h
> > +++ b/drivers/hid/bpf/hid_bpf_dispatch.h
> > @@ -10,12 +10,16 @@ struct hid_bpf_ctx_kern {
> > u8 *data;
> > };
> >
> > +struct hid_device *hid_get_device(unsigned int hid_id);
> > +void hid_put_device(struct hid_device *hid);
> > +int hid_bpf_allocate_event_data(struct hid_device *hdev);
> > int hid_bpf_preload_skel(void);
> > void hid_bpf_free_links_and_skel(void);
> > int hid_bpf_get_prog_attach_type(struct bpf_prog *prog);
> > int __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type, int prog_fd,
> > struct bpf_prog *prog, __u32 flags);
> > void __hid_bpf_destroy_device(struct hid_device *hdev);
> > +void __hid_bpf_ops_destroy_device(struct hid_device *hdev);
> > int hid_bpf_prog_run(struct hid_device *hdev, enum hid_bpf_prog_type type,
> > struct hid_bpf_ctx_kern *ctx_kern);
> > int hid_bpf_reconnect(struct hid_device *hdev);
> > diff --git a/drivers/hid/bpf/hid_bpf_jmp_table.c b/drivers/hid/bpf/hid_bpf_jmp_table.c
> > index aa8e1c79cdf5..8a54ba447718 100644
> > --- a/drivers/hid/bpf/hid_bpf_jmp_table.c
> > +++ b/drivers/hid/bpf/hid_bpf_jmp_table.c
> > @@ -81,6 +81,9 @@ static int hid_bpf_program_count(struct hid_device *hdev,
> > if (type >= HID_BPF_PROG_TYPE_MAX)
> > return -EINVAL;
> >
> > + if (type == HID_BPF_PROG_TYPE_RDESC_FIXUP && hdev->bpf.rdesc_ops)
> > + n += 1;
> > +
> > FOR_ENTRIES(i, jmp_table.tail, jmp_table.head) {
> > struct hid_bpf_prog_entry *entry = &jmp_table.entries[i];
> >
> > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > new file mode 100644
> > index 000000000000..be514a98e55b
> > --- /dev/null
> > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > @@ -0,0 +1,246 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * HID-BPF support for Linux
> > + *
> > + * Copyright (c) 2024 Benjamin Tissoires
> > + */
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/bpf_verifier.h>
> > +#include <linux/bpf.h>
> > +#include <linux/btf.h>
> > +#include <linux/btf_ids.h>
> > +#include <linux/filter.h>
> > +#include <linux/hid.h>
> > +#include <linux/hid_bpf.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> > +#include "hid_bpf_dispatch.h"
> > +
> > +static struct btf *hid_bpf_ops_btf;
> > +
> > +static int hid_bpf_ops_init(struct btf *btf)
> > +{
> > + hid_bpf_ops_btf = btf;
> > + return 0;
> > +}
> > +
> > +static bool hid_bpf_ops_is_valid_access(int off, int size,
> > + enum bpf_access_type type,
> > + const struct bpf_prog *prog,
> > + struct bpf_insn_access_aux *info)
> > +{
> > + return bpf_tracing_btf_ctx_access(off, size, type, prog, info);
> > +}
> > +
> > +static int hid_bpf_ops_check_member(const struct btf_type *t,
> > + const struct btf_member *member,
> > + const struct bpf_prog *prog)
> > +{
> > + u32 moff = __btf_member_bit_offset(t, member) / 8;
> > +
>
> this is good...
>
> > + switch (moff) {
> > + case offsetof(struct hid_bpf_ops, hid_rdesc_fixup):
> > + break;
> > + default:
> > + if (prog->sleepable)
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int hid_bpf_ops_btf_struct_access(struct bpf_verifier_log *log,
> > + const struct bpf_reg_state *reg,
> > + int off, int size)
> > +{
> > + const struct btf_type *state;
> > + const struct btf_type *t;
> > + s32 type_id;
> > +
> > + type_id = btf_find_by_name_kind(reg->btf, "hid_bpf_ctx",
> > + BTF_KIND_STRUCT);
> > + if (type_id < 0)
> > + return -EINVAL;
> > +
> > + t = btf_type_by_id(reg->btf, reg->btf_id);
> > + state = btf_type_by_id(reg->btf, type_id);
> > + if (t != state) {
> > + bpf_log(log, "only access to hid_bpf_ctx is supported\n");
> > + return -EACCES;
> > + }
> > +
> > + /* out-of-bound access in hid_bpf_ctx */
> > + if (off + size > sizeof(struct hid_bpf_ctx)) {
> > + bpf_log(log, "write access at off %d with size %d\n", off, size);
> > + return -EACCES;
> > + }
> > +
> > + if (off < offsetof(struct hid_bpf_ctx, retval)) {
> > + bpf_log(log,
> > + "write access at off %d with size %d on read-only part of hid_bpf_ctx\n",
> > + off, size);
> > + return -EACCES;
> > + }
> > +
> > + return NOT_INIT;
> > +}
> > +
> > +static const struct bpf_verifier_ops hid_bpf_verifier_ops = {
> > + .is_valid_access = hid_bpf_ops_is_valid_access,
> > + .btf_struct_access = hid_bpf_ops_btf_struct_access,
> > +};
> > +
> > +static int hid_bpf_ops_init_member(const struct btf_type *t,
> > + const struct btf_member *member,
> > + void *kdata, const void *udata)
> > +{
> > + u32 flags;
> > +
> > + switch (member->offset) {
> > + case offsetof(struct hid_bpf_ops, hid_id) * 8:
>
> but here you're open coding it and adding ugly * 8
> Just do:
> moff = __btf_member_bit_offset(t, member) / 8;
> switch (moff) {
> case offsetof(struct tcp_congestion_ops, flags):
>
> the way bpf_tcp_ca.c is doing?
> Open code of ->offset is asking for trouble.
heh, I just monkey-copied tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
Will change it :)
>
> > + /* For hid_id and flags fields, this function has to copy it
> > + * and return 1 to indicate that the data has been handled by
> > + * the struct_ops type, or the verifier will reject the map if
> > + * the value of those fields is not zero.
> > + */
> > + ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
> > + return 1;
> > + case offsetof(struct hid_bpf_ops, flags) * 8:
> > + flags = ((struct hid_bpf_ops *)udata)->flags;
> > + if (flags & ~HID_BPF_FLAG_MASK)
> > + return -EINVAL;
> > + ((struct hid_bpf_ops *)kdata)->flags = flags;
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +static int hid_bpf_reg(void *kdata)
> > +{
> > + struct hid_bpf_ops *ops = kdata;
> > + struct hid_device *hdev;
> > + int count, err = 0;
> > +
> > + hdev = hid_get_device(ops->hid_id);
> > + if (IS_ERR(hdev))
> > + return PTR_ERR(hdev);
> > +
> > + ops->hdev = hdev;
> > +
> > + mutex_lock(&hdev->bpf.prog_list_lock);
> > +
> > + count = list_count_nodes(&hdev->bpf.prog_list);
> > + if (count >= HID_BPF_MAX_PROGS_PER_DEV) {
> > + err = -E2BIG;
> > + goto out_unlock;
> > + }
> > +
> > + if (ops->hid_rdesc_fixup) {
> > + if (hdev->bpf.rdesc_ops) {
> > + err = -EINVAL;
> > + goto out_unlock;
> > + }
> > +
> > + hdev->bpf.rdesc_ops = ops;
> > + }
> > +
> > + if (ops->hid_device_event) {
> > + err = hid_bpf_allocate_event_data(hdev);
> > + if (err)
> > + goto out_unlock;
> > + }
> > +
> > + if (ops->flags & HID_BPF_FLAG_INSERT_HEAD)
> > + list_add_rcu(&ops->list, &hdev->bpf.prog_list);
> > + else
> > + list_add_tail_rcu(&ops->list, &hdev->bpf.prog_list);
>
> Looks like future proofing, but I feel it's too little to materialize.
again, the case already happens, so I need a list (or equivalent of) :)
> Take a look at include/linux/bpf_mprog.h
> I suspect it might be useful here too.
From the quick look of it this is interesting, but for plain struct
bpf_prog, right?
So I would have to adapt this include for struct_ops?
>
> > +
> > +out_unlock:
> > + mutex_unlock(&hdev->bpf.prog_list_lock);
> > +
> > + if (err) {
> > + if (hdev->bpf.rdesc_ops == ops)
> > + hdev->bpf.rdesc_ops = NULL;
> > + hid_put_device(hdev);
> > + } else if (ops->hid_rdesc_fixup) {
> > + hid_bpf_reconnect(hdev);
> > + }
> > +
> > + return err;
> > +}
> > +
> > +static void hid_bpf_unreg(void *kdata)
> > +{
> > + struct hid_bpf_ops *ops = kdata;
> > + struct hid_device *hdev;
> > + bool reconnect = false;
> > +
> > + hdev = ops->hdev;
> > +
> > + /* check if __hid_bpf_ops_destroy_device() has been called */
> > + if (!hdev)
> > + return;
> > +
> > + mutex_lock(&hdev->bpf.prog_list_lock);
> > +
> > + list_del_rcu(&ops->list);
> > +
> > + reconnect = hdev->bpf.rdesc_ops == ops;
> > + if (reconnect)
> > + hdev->bpf.rdesc_ops = NULL;
> > +
> > + mutex_unlock(&hdev->bpf.prog_list_lock);
> > +
> > + if (reconnect)
> > + hid_bpf_reconnect(hdev);
> > +
> > + hid_put_device(hdev);
> > +}
> > +
> > +static int __hid_bpf_device_event(struct hid_bpf_ctx *ctx, enum hid_report_type type)
> > +{
> > + return 0;
> > +}
> > +
> > +static int __hid_bpf_rdesc_fixup(struct hid_bpf_ctx *ctx)
> > +{
> > + return 0;
> > +}
> > +
> > +static struct hid_bpf_ops __bpf_hid_bpf_ops = {
> > + .hid_device_event = __hid_bpf_device_event,
> > + .hid_rdesc_fixup = __hid_bpf_rdesc_fixup,
> > +};
> > +
> > +static struct bpf_struct_ops bpf_hid_bpf_ops = {
> > + .verifier_ops = &hid_bpf_verifier_ops,
> > + .init = hid_bpf_ops_init,
> > + .check_member = hid_bpf_ops_check_member,
> > + .init_member = hid_bpf_ops_init_member,
> > + .reg = hid_bpf_reg,
> > + .unreg = hid_bpf_unreg,
> > + .name = "hid_bpf_ops",
> > + .cfi_stubs = &__bpf_hid_bpf_ops,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
> > +{
> > + struct hid_bpf_ops *e;
> > +
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> > + hid_put_device(hdev);
> > + e->hdev = NULL;
> > + }
> > + rcu_read_unlock();
> > +}
> > +
> > +static int __init hid_bpf_struct_ops_init(void)
> > +{
> > + return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
> > +}
> > +late_initcall(hid_bpf_struct_ops_init);
> > diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
> > index a66103618e6e..96495e977204 100644
> > --- a/include/linux/hid_bpf.h
> > +++ b/include/linux/hid_bpf.h
> > @@ -65,11 +65,12 @@ struct hid_bpf_ctx {
> > * @HID_BPF_FLAG_INSERT_HEAD: insert the given program before any other program
> > * currently attached to the device. This doesn't
> > * guarantee that this program will always be first
> > - * @HID_BPF_FLAG_MAX: sentinel value, not to be used by the callers
> > */
> > enum hid_bpf_attach_flags {
> > HID_BPF_FLAG_NONE = 0,
> > HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> > +
> > + /* private: internal use only */
> > HID_BPF_FLAG_MAX,
> > };
> >
> > @@ -112,6 +113,63 @@ struct hid_ops {
> >
> > extern struct hid_ops *hid_ops;
> >
> > +/**
> > + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> > + * programs to a HID device
> > + * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
> > + * cannot be changed after
> > + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
> > + * Writeable only before ``load()``
> > + */
> > +struct hid_bpf_ops {
> > + /* hid_id needs to stay first so we can easily change it
> > + * from userspace.
> > + */
>
> hmm.
> patch 5 just does:
> + skel->struct_ops.mouse_invert->hid_id = hid_id;
>
> I don't see a reason why it needs to be first.
See the selftests (and my external loader, udev-hid-bpf):
map = bpf_object__find_map_by_name(*self->skel->skeleton->obj,
programs[i].name + 4);
ASSERT_OK_PTR(map) TH_LOG("can not find struct_ops by name '%s'",
programs[i].name + 4);
/* hid_id is the first field of struct hid_bpf_ops */
ops_hid_id = bpf_map__initial_value(map, NULL);
ASSERT_OK_PTR(ops_hid_id) TH_LOG("unable to retrieve struct_ops data");
*ops_hid_id = self->hid_id;
Basically, I want to have to deal with .bpf.o and not the skeletons for
the generic case. I want to be able to drop a file in /lib/firmware/hid
and have udev-hid-bpf load it without having to recompile udev-hid-bpf.
I couldn't seem to be able to access the struct_ops BTF information from
libbpf, so I decided to have hid_id and flags first and consider this to
be ABI while I figure out how to get to the real offsets :)
>
> > + int hid_id;
> > + u32 flags;
> > +
> > + /* private: internal use only */
> > + struct list_head list;
> > +
> > + /* public: rest is public */
> > +
> > +/* fast path fields are put first to fill one cache line */
> > +
> > + /**
> > + * @hid_device_event: called whenever an event is coming in from the device
> > + *
> > + * It has the following arguments:
> > + *
> > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> > + *
> > + * Return: %0 on success and keep processing; a positive
> > + * value to change the incoming size buffer; a negative
> > + * error code to interrupt the processing of this event
> > + *
> > + * Context: Interrupt context.
> > + */
> > + int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type);
> > +
> > +/* control/slow paths put last */
> > +
> > + /**
> > + * @hid_rdesc_fixup: called when the probe function parses the report descriptor
> > + * of the HID device
> > + *
> > + * It has the following arguments:
> > + *
> > + * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> > + *
> > + * Return: %0 on success and keep processing; a positive
> > + * value to change the incoming size buffer; a negative
> > + * error code to interrupt the processing of this device
> > + */
> > + int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
>
> It's fine since you want to keep the prog mostly unchanged,
> but since the whole api is improving maybe it's time
> to do something about hid_bpf_ctx in all callbacks?
> Pass into prog what prog needs to see.
> In the old api all progs had to have a common hid_bpf_ctx.
> With struct-ops it can be fine tuned.
I agree, but I had a few cons:
- right now, I can recompile the same bpf.c with just a -D in the
cflags. Both types of program are compatible, which allows to write
the file once and recompile it for both pre/post struct_ops.
This will be very useful while HID-BPF struct_ops is not in mainline
distributions
- I need to have access to a few things: the struct hid_device, and the
incoming buffer. And I don't think I can teach the verifier about the
incoming buffer size without having to store it into a dedicated
struct, which results into the same hid_bpf_ctx...
- I like how hid_bpf_allocate_context() works with the same struct that
can be later used with the same functions hid_bpf_input_event() or
such (arguably I could return the struct hid_device directly)
I'll try to give it more thoughts :)
>
> Overall it looks great.
> hid_id did the trick :)
Thanks!
I agree, relying on struct_ops allowed to do a heavy cleanup in my code,
and TBH, even if I had only one callback, I think the gain is clear,
because it's much straightforward and you can also authorize some fine
grain write on your own structs (like changing the name of the
hid_device, or its flags, which is not part of this series, but which
will likely come later).
Cheers,
Benjamin
On Wed, May 29, 2024 at 12:38 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> > Take a look at include/linux/bpf_mprog.h
> > I suspect it might be useful here too.
>
> From the quick look of it this is interesting, but for plain struct
> bpf_prog, right?
>
> So I would have to adapt this include for struct_ops?
yeah. It needs work to be adapted to work with struct-ops.
I mentioned it mainly from uapi perspective.
BPF_F_BEFORE, BPF_F_AFTER, BPF_F_REPLACE.
Would be good to keep things consistent.
> > > @@ -112,6 +113,63 @@ struct hid_ops {
> > >
> > > extern struct hid_ops *hid_ops;
> > >
> > > +/**
> > > + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> > > + * programs to a HID device
> > > + * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
> > > + * cannot be changed after
> > > + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
> > > + * Writeable only before ``load()``
> > > + */
> > > +struct hid_bpf_ops {
> > > + /* hid_id needs to stay first so we can easily change it
> > > + * from userspace.
> > > + */
> >
> > hmm.
> > patch 5 just does:
> > + skel->struct_ops.mouse_invert->hid_id = hid_id;
> >
> > I don't see a reason why it needs to be first.
>
> See the selftests (and my external loader, udev-hid-bpf):
>
> map = bpf_object__find_map_by_name(*self->skel->skeleton->obj,
> programs[i].name + 4);
> ASSERT_OK_PTR(map) TH_LOG("can not find struct_ops by name '%s'",
> programs[i].name + 4);
>
> /* hid_id is the first field of struct hid_bpf_ops */
> ops_hid_id = bpf_map__initial_value(map, NULL);
> ASSERT_OK_PTR(ops_hid_id) TH_LOG("unable to retrieve struct_ops data");
>
> *ops_hid_id = self->hid_id;
>
> Basically, I want to have to deal with .bpf.o and not the skeletons for
> the generic case. I want to be able to drop a file in /lib/firmware/hid
> and have udev-hid-bpf load it without having to recompile udev-hid-bpf.
and with additional restriction of only one struct-ops prog per elf file,
right?
Otherwise multiple hid_bpf_ops will be in the same elf section and
the same map.
> I couldn't seem to be able to access the struct_ops BTF information from
> libbpf, so I decided to have hid_id and flags first and consider this to
> be ABI while I figure out how to get to the real offsets :)
I see. I guess it's ok for now.
© 2016 - 2025 Red Hat, Inc.