Hi,
This is a followup of my v1 at [0].
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 tried to address as many comments as I could and here is the short log
of changes:
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/[email protected]/T/#t
Benjamin Tissoires (28):
bpf: add new is_sys_admin_prog_type() helper
bpf: introduce hid program type
HID: hook up with bpf
libbpf: add HID program type and API
selftests/bpf: add tests for the HID-bpf initial implementation
samples/bpf: add new hid_mouse example
bpf/hid: add a new attach type to change the report descriptor
HID: allow to change the report descriptor from an eBPF program
libbpf: add new attach type BPF_HID_RDESC_FIXUP
selftests/bpf: add report descriptor fixup tests
samples/bpf: add a report descriptor fixup
bpf/hid: add hid_{get|set}_data helpers
HID: bpf: implement hid_bpf_get|set_data
selftests/bpf: add tests for hid_{get|set}_data helpers
bpf/hid: add new BPF type to trigger commands from userspace
libbpf: add new attach type BPF_HID_USER_EVENT
selftests/bpf: add test for user call of HID bpf programs
selftests/bpf: hid: rely on uhid event to know if a test device is
ready
bpf/hid: add bpf_hid_raw_request helper function
HID: add implementation of bpf_hid_raw_request
selftests/bpf: add tests for bpf_hid_hw_request
bpf/verifier: prevent non GPL programs to be loaded against HID
HID: bpf: compute only the required buffer size for the device
HID: bpf: only call hid_bpf_raw_event() if a ctx is available
bpf/hid: Add a flag to add the program at the beginning of the list
libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
selftests/bpf: Add a test for BPF_F_INSERT_HEAD
samples/bpf: fix bpf_program__attach_hid() api change
drivers/hid/Makefile | 1 +
drivers/hid/hid-bpf.c | 361 +++++++++
drivers/hid/hid-core.c | 34 +-
include/linux/bpf-hid.h | 129 +++
include/linux/bpf_types.h | 4 +
include/linux/hid.h | 25 +
include/uapi/linux/bpf.h | 59 ++
include/uapi/linux/bpf_hid.h | 50 ++
kernel/bpf/Makefile | 3 +
kernel/bpf/hid.c | 652 +++++++++++++++
kernel/bpf/syscall.c | 26 +-
kernel/bpf/verifier.c | 7 +
samples/bpf/.gitignore | 1 +
samples/bpf/Makefile | 4 +
samples/bpf/hid_mouse_kern.c | 91 +++
samples/bpf/hid_mouse_user.c | 129 +++
tools/include/uapi/linux/bpf.h | 59 ++
tools/lib/bpf/libbpf.c | 22 +-
tools/lib/bpf/libbpf.h | 2 +
tools/lib/bpf/libbpf.map | 1 +
tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++
tools/testing/selftests/bpf/progs/hid.c | 216 +++++
22 files changed, 2649 insertions(+), 15 deletions(-)
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
--
2.35.1
This is just to hammer the obvious because I suspect you can not already
load a bpf HID program which is not GPL because all of the useful
functions are GPL only.
Anyway, this ensures that users are not tempted to bypass this requirement
and will allow us to ship tested BPF programs in the kernel without having
to aorry about the license.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
new in v2:
- Note: I placed this statement in check_attach_btf_id() to be local to
other similar checks (regarding LSM), however, I have no idea if this
is the correct place. Please shout at me if it isn't.
---
include/linux/bpf-hid.h | 8 ++++++++
kernel/bpf/hid.c | 12 ++++++++++++
kernel/bpf/verifier.c | 7 +++++++
3 files changed, 27 insertions(+)
diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
index bd548f6a4a26..3da1d0ecb9be 100644
--- a/include/linux/bpf-hid.h
+++ b/include/linux/bpf-hid.h
@@ -2,6 +2,7 @@
#ifndef _BPF_HID_H
#define _BPF_HID_H
+#include <linux/bpf_verifier.h>
#include <linux/mutex.h>
#include <uapi/linux/bpf.h>
#include <uapi/linux/bpf_hid.h>
@@ -71,6 +72,8 @@ int bpf_hid_prog_query(const union bpf_attr *attr,
union bpf_attr __user *uattr);
int bpf_hid_link_create(const union bpf_attr *attr,
struct bpf_prog *prog);
+int bpf_hid_verify_prog(struct bpf_verifier_log *vlog,
+ const struct bpf_prog *prog);
#else
static inline int bpf_hid_prog_query(const union bpf_attr *attr,
union bpf_attr __user *uattr)
@@ -83,6 +86,11 @@ static inline int bpf_hid_link_create(const union bpf_attr *attr,
{
return -EOPNOTSUPP;
}
+static inline int bpf_hid_verify_prog(struct bpf_verifier_log *vlog,
+ const struct bpf_prog *prog)
+{
+ return -EOPNOTSUPP;
+}
#endif
static inline bool bpf_hid_link_empty(struct bpf_hid *bpf,
diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c
index 653d10c0f4e6..b3dc1cd37a3e 100644
--- a/kernel/bpf/hid.c
+++ b/kernel/bpf/hid.c
@@ -37,6 +37,18 @@ void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks)
}
EXPORT_SYMBOL_GPL(bpf_hid_set_hooks);
+int bpf_hid_verify_prog(struct bpf_verifier_log *vlog,
+ const struct bpf_prog *prog)
+{
+ if (!prog->gpl_compatible) {
+ bpf_log(vlog,
+ "HID programs must have a GPL compatible license\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
BPF_CALL_5(bpf_hid_get_data, void*, ctx, u64, offset, u32, n, void*, data, u64, size)
{
struct hid_bpf_ctx *bpf_ctx = ctx;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a57db4b2803c..afec8fa1d674 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -21,6 +21,7 @@
#include <linux/perf_event.h>
#include <linux/ctype.h>
#include <linux/error-injection.h>
+#include <linux/bpf-hid.h>
#include <linux/bpf_lsm.h>
#include <linux/btf_ids.h>
@@ -14235,6 +14236,12 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
return check_struct_ops_btf_id(env);
+ if (prog->type == BPF_PROG_TYPE_HID) {
+ ret = bpf_hid_verify_prog(&env->log, prog);
+ if (ret < 0)
+ return ret;
+ }
+
if (prog->type != BPF_PROG_TYPE_TRACING &&
prog->type != BPF_PROG_TYPE_LSM &&
prog->type != BPF_PROG_TYPE_EXT)
--
2.35.1
Adds the SEC definiton for the user callbacks of HID bpf.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
---
tools/lib/bpf/libbpf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 192ef3901251..d1e305f760bb 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8682,6 +8682,7 @@ static const struct bpf_sec_def section_defs[] = {
SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
SEC_DEF("hid/rdesc_fixup", HID, BPF_HID_RDESC_FIXUP, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
+ SEC_DEF("hid/user_event", HID, BPF_HID_USER_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
};
#define MAX_TYPE_NAME_SIZE 32
--
2.35.1
the context is allocated the first time a program of type DEVICE_EVENT
is attached to the device. To not add too much jumps in the code for
the general device handling, call hid_bpf_raw_event() only if we know
that a program has been attached once during the life of the device.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
new in v2
---
drivers/hid/hid-core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index d0e015986e17..2b49f6064a40 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1751,10 +1751,13 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
u8 *cdata;
int ret = 0;
- data = hid_bpf_raw_event(hid, data, &size);
- if (IS_ERR(data)) {
- ret = PTR_ERR(data);
- goto out;
+ /* we pre-test if ctx is available here to cut the calls at the earliest */
+ if (hid->bpf.ctx) {
+ data = hid_bpf_raw_event(hid, data, &size);
+ if (IS_ERR(data)) {
+ ret = PTR_ERR(data);
+ goto out;
+ }
}
report = hid_get_report(report_enum, data);
--
2.35.1
HID-bpf program type are needing a new SEC.
To bind a hid-bpf program, we can rely on bpf_program__attach_fd()
so export a new function to the API.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
---
tools/lib/bpf/libbpf.c | 7 +++++++
tools/lib/bpf/libbpf.h | 2 ++
tools/lib/bpf/libbpf.map | 1 +
3 files changed, 10 insertions(+)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 81bf01d67671..356bbd3ad2c7 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8680,6 +8680,7 @@ static const struct bpf_sec_def section_defs[] = {
SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE),
SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
+ SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
};
#define MAX_TYPE_NAME_SIZE 32
@@ -10659,6 +10660,12 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
return bpf_program__attach_iter(prog, NULL);
}
+struct bpf_link *
+bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd)
+{
+ return bpf_program__attach_fd(prog, hid_fd, 0, "hid");
+}
+
struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
{
if (!prog->sec_def || !prog->sec_def->attach_fn)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index c8d8daad212e..f677ac0a9ede 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -529,6 +529,8 @@ struct bpf_iter_attach_opts {
LIBBPF_API struct bpf_link *
bpf_program__attach_iter(const struct bpf_program *prog,
const struct bpf_iter_attach_opts *opts);
+LIBBPF_API struct bpf_link *
+bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd);
/*
* Libbpf allows callers to adjust BPF programs before being loaded
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 47e70c9058d9..fdc6fa743953 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -424,6 +424,7 @@ LIBBPF_0.6.0 {
LIBBPF_0.7.0 {
global:
bpf_btf_load;
+ bpf_program__attach_hid;
bpf_program__expected_attach_type;
bpf_program__log_buf;
bpf_program__log_level;
--
2.35.1
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 <[email protected]>
Signed-off-by: Benjamin Tissoires <[email protected]>
---
new in v2
---
kernel/bpf/syscall.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index db402ebc5570..cc570891322b 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,17 @@ 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:
+ case BPF_PROG_TYPE_EXT: /* extends any prog */
+ 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 +2262,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
We have 2 cases of usage here:
- either n <= 32: we are addressing individual bits at the given offset
- either n > 32: we are using a memcpy to transmit the data to the caller,
meaning that we need to be byte-aligned.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
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 | 68 ++++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-core.c | 4 +--
include/linux/hid.h | 2 ++
3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c
index 510e24f4307c..8ae247fba5bc 100644
--- a/drivers/hid/hid-bpf.c
+++ b/drivers/hid/hid-bpf.c
@@ -105,6 +105,72 @@ static void hid_bpf_array_detached(struct hid_device *hdev, enum bpf_hid_attach_
}
}
+int hid_bpf_get_data(struct hid_device *hdev, u8 *buf, size_t buf_size, u64 offset, u32 n,
+ u8 *data, u64 data_size)
+{
+ u32 *value = (u32 *)data;
+
+ if (((offset + n) >> 3) >= buf_size)
+ return -E2BIG;
+
+ if (n <= 32) {
+ /* data must be a pointer to a u32 */
+ if (data_size != 4)
+ return -EINVAL;
+
+ *value = hid_field_extract(hdev, buf, offset, n);
+ return 4;
+ }
+
+ /* if n > 32, use memcpy, but ensure we are dealing with full bytes */
+ if ((n | offset) & 0x7)
+ return -EINVAL;
+
+ /* work on bytes now */
+ offset = offset >> 3;
+ n = n >> 3;
+
+ if (n > data_size)
+ return -EINVAL;
+
+ memcpy(data, buf + offset, n);
+
+ return n;
+}
+
+int hid_bpf_set_data(struct hid_device *hdev, u8 *buf, size_t buf_size, u64 offset, u32 n,
+ u8 *data, u64 data_size)
+{
+ u32 *value = (u32 *)data;
+
+ if (((offset + n) >> 3) >= buf_size)
+ return -E2BIG;
+
+ if (n <= 32) {
+ /* data must be a pointer to a u32 */
+ if (data_size != 4)
+ return -EINVAL;
+
+ implement(hdev, buf, offset, n, *value);
+ return 4;
+ }
+
+ /* if n > 32, use memcpy, but ensure we are dealing with full bytes */
+ if ((n | offset) & 0x7)
+ return -EINVAL;
+
+ /* work on bytes now */
+ offset = offset >> 3;
+ n = n >> 3;
+
+ if (n > data_size)
+ return -EINVAL;
+
+ memcpy(buf + offset, data, n);
+
+ return n;
+}
+
static int hid_bpf_run_progs(struct hid_device *hdev, enum bpf_hid_attach_type type,
struct hid_bpf_ctx *ctx, u8 *data, int size)
{
@@ -204,6 +270,8 @@ int __init hid_bpf_module_init(void)
.link_attach = hid_bpf_link_attach,
.link_attached = hid_bpf_link_attached,
.array_detached = hid_bpf_array_detached,
+ .hid_get_data = hid_bpf_get_data,
+ .hid_set_data = hid_bpf_set_data,
};
bpf_hid_set_hooks(&hooks);
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 0eb8189faaee..d3f4499ee4cd 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
Everything should be available in the selftest part of the tree, but
providing an example without uhid and hidraw will be more easy to
follow for users.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
---
samples/bpf/.gitignore | 1 +
samples/bpf/Makefile | 4 ++
samples/bpf/hid_mouse_kern.c | 66 ++++++++++++++++++
samples/bpf/hid_mouse_user.c | 129 +++++++++++++++++++++++++++++++++++
4 files changed, 200 insertions(+)
create mode 100644 samples/bpf/hid_mouse_kern.c
create mode 100644 samples/bpf/hid_mouse_user.c
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index 0e7bfdbff80a..65440bd618b2 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -2,6 +2,7 @@
cpustat
fds_example
hbm
+hid_mouse
ibumad
lathist
lwt_len_hist
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 38638845db9d..84ef458487df 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -60,6 +60,8 @@ tprogs-y += xdp_redirect_map
tprogs-y += xdp_redirect
tprogs-y += xdp_monitor
+tprogs-y += hid_mouse
+
# Libbpf dependencies
LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
LIBBPF_OUTPUT = $(abspath $(BPF_SAMPLES_PATH))/libbpf
@@ -124,6 +126,7 @@ xdp_redirect_cpu-objs := xdp_redirect_cpu_user.o $(XDP_SAMPLE)
xdp_redirect_map-objs := xdp_redirect_map_user.o $(XDP_SAMPLE)
xdp_redirect-objs := xdp_redirect_user.o $(XDP_SAMPLE)
xdp_monitor-objs := xdp_monitor_user.o $(XDP_SAMPLE)
+hid_mouse-objs := hid_mouse_user.o
# Tell kbuild to always build the programs
always-y := $(tprogs-y)
@@ -181,6 +184,7 @@ always-y += ibumad_kern.o
always-y += hbm_out_kern.o
always-y += hbm_edt_kern.o
always-y += xdpsock_kern.o
+always-y += hid_mouse_kern.o
ifeq ($(ARCH), arm)
# Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
diff --git a/samples/bpf/hid_mouse_kern.c b/samples/bpf/hid_mouse_kern.c
new file mode 100644
index 000000000000..c24a12e06b40
--- /dev/null
+++ b/samples/bpf/hid_mouse_kern.c
@@ -0,0 +1,66 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021 Benjamin Tissoires
+ */
+#include <linux/version.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/bpf_hid.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("hid/device_event")
+int hid_y_event(struct hid_bpf_ctx *ctx)
+{
+ s16 y;
+
+ bpf_printk("event: %02x size: %d", ctx->type, ctx->size);
+ bpf_printk("incoming event: %02x %02x %02x",
+ ctx->data[0],
+ ctx->data[1],
+ ctx->data[2]);
+ bpf_printk(" %02x %02x %02x",
+ ctx->data[3],
+ ctx->data[4],
+ ctx->data[5]);
+ bpf_printk(" %02x %02x %02x",
+ ctx->data[6],
+ ctx->data[7],
+ ctx->data[8]);
+
+ y = ctx->data[3] | (ctx->data[4] << 8);
+
+ y = -y;
+
+ ctx->data[3] = y & 0xFF;
+ ctx->data[4] = (y >> 8) & 0xFF;
+
+ bpf_printk("modified event: %02x %02x %02x",
+ ctx->data[0],
+ ctx->data[1],
+ ctx->data[2]);
+ bpf_printk(" %02x %02x %02x",
+ ctx->data[3],
+ ctx->data[4],
+ ctx->data[5]);
+ bpf_printk(" %02x %02x %02x",
+ ctx->data[6],
+ ctx->data[7],
+ ctx->data[8]);
+
+ return 0;
+}
+
+SEC("hid/device_event")
+int hid_x_event(struct hid_bpf_ctx *ctx)
+{
+ s16 x;
+
+ x = ctx->data[1] | (ctx->data[2] << 8);
+
+ x = -x;
+
+ ctx->data[1] = x & 0xFF;
+ ctx->data[2] = (x >> 8) & 0xFF;
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/hid_mouse_user.c b/samples/bpf/hid_mouse_user.c
new file mode 100644
index 000000000000..d4f37caca2fa
--- /dev/null
+++ b/samples/bpf/hid_mouse_user.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021 Benjamin Tissoires
+ */
+#include <linux/bpf.h>
+#include <linux/if_link.h>
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <sys/resource.h>
+
+#include "bpf_util.h"
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+static char *sysfs_path;
+static int sysfs_fd;
+static int prog_count;
+
+struct prog {
+ int fd;
+ struct bpf_link *link;
+ enum bpf_attach_type type;
+};
+
+static struct prog progs[10];
+
+static void int_exit(int sig)
+{
+ for (prog_count--; prog_count >= 0; prog_count--)
+ bpf_link__destroy(progs[prog_count].link);
+
+ close(sysfs_fd);
+ exit(0);
+}
+
+static void usage(const char *prog)
+{
+ fprintf(stderr,
+ "%s: %s /sys/bus/hid/devices/0BUS:0VID:0PID:00ID/uevent\n\n",
+ __func__, prog);
+}
+
+int main(int argc, char **argv)
+{
+ struct bpf_prog_info info = {};
+ __u32 info_len = sizeof(info);
+ const char *optstr = "";
+ struct bpf_object *obj;
+ struct bpf_program *prog;
+ int opt;
+ char filename[256];
+ int err;
+
+ while ((opt = getopt(argc, argv, optstr)) != -1) {
+ switch (opt) {
+ default:
+ usage(basename(argv[0]));
+ return 1;
+ }
+ }
+
+ if (optind == argc) {
+ usage(basename(argv[0]));
+ return 1;
+ }
+
+ sysfs_path = argv[optind];
+ if (!sysfs_path) {
+ perror("sysfs");
+ return 1;
+ }
+
+ snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+ obj = bpf_object__open_file(filename, NULL);
+ err = libbpf_get_error(obj);
+ if (err) {
+ fprintf(stderr, "ERROR: opening BPF object file failed\n");
+ obj = NULL;
+ err = 1;
+ goto cleanup;
+ }
+
+ /* load BPF program */
+ err = bpf_object__load(obj);
+ if (err) {
+ fprintf(stderr, "ERROR: loading BPF object file failed\n");
+ goto cleanup;
+ }
+
+ sysfs_fd = open(sysfs_path, O_RDONLY);
+
+ bpf_object__for_each_program(prog, obj) {
+ progs[prog_count].fd = bpf_program__fd(prog);
+ progs[prog_count].type = bpf_program__get_expected_attach_type(prog);
+ progs[prog_count].link = bpf_program__attach_hid(prog, sysfs_fd);
+ if (libbpf_get_error(progs[prog_count].link)) {
+ fprintf(stderr, "bpf_prog_attach: err=%m\n");
+ progs[prog_count].fd = 0;
+ progs[prog_count].link = NULL;
+ goto cleanup;
+ }
+ prog_count++;
+ }
+
+ signal(SIGINT, int_exit);
+ signal(SIGTERM, int_exit);
+
+ err = bpf_obj_get_info_by_fd(progs[0].fd, &info, &info_len);
+ if (err) {
+ printf("can't get prog info - %s\n", strerror(errno));
+ goto cleanup;
+ }
+
+ while (1)
+ ;
+
+ cleanup:
+ for (prog_count--; prog_count >= 0; prog_count--)
+ bpf_link__destroy(progs[prog_count].link);
+
+ bpf_object__close(obj);
+ return err;
+}
--
2.35.1
HID is a protocol that could benefit from using BPF too.
This patch implements a net-like use of BPF capability for HID.
Any incoming report coming from the device can be injected into a series
of BPF programs that can modify it or even discard it by setting the
size in the context to 0.
The kernel/bpf implementation is based on net-namespace.c, with only
the bpf_link part kept, there is no real points in keeping the
bpf_prog_{attach|detach} API.
The implementation here is only focusing on the bpf changes. The HID
changes that hooks onto this are coming in a separate patch.
Given that HID can be compiled in as a module, and the functions that
kernel/bpf/hid.c needs to call in hid.ko are exported in struct hid_hooks.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
- unsigned long -> __u16 in uapi/linux/bpf_hid.h
- change the bpf_ctx to be of variable size, with a min of 1024 bytes
- make this 1 kB available directly from bpf program, the rest will
need a helper
- add some more doc comments in uapi
---
include/linux/bpf-hid.h | 108 ++++++++
include/linux/bpf_types.h | 4 +
include/linux/hid.h | 5 +
include/uapi/linux/bpf.h | 7 +
include/uapi/linux/bpf_hid.h | 39 +++
kernel/bpf/Makefile | 3 +
kernel/bpf/hid.c | 437 +++++++++++++++++++++++++++++++++
kernel/bpf/syscall.c | 8 +
tools/include/uapi/linux/bpf.h | 7 +
9 files changed, 618 insertions(+)
create mode 100644 include/linux/bpf-hid.h
create mode 100644 include/uapi/linux/bpf_hid.h
create mode 100644 kernel/bpf/hid.c
diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
new file mode 100644
index 000000000000..3cda78051b5f
--- /dev/null
+++ b/include/linux/bpf-hid.h
@@ -0,0 +1,108 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _BPF_HID_H
+#define _BPF_HID_H
+
+#include <linux/mutex.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/bpf_hid.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+
+struct bpf_prog;
+struct bpf_prog_array;
+struct hid_device;
+
+enum bpf_hid_attach_type {
+ BPF_HID_ATTACH_INVALID = -1,
+ BPF_HID_ATTACH_DEVICE_EVENT = 0,
+ MAX_BPF_HID_ATTACH_TYPE
+};
+
+struct bpf_hid {
+ struct hid_bpf_ctx *ctx;
+
+ /* Array of programs to run compiled from links */
+ struct bpf_prog_array __rcu *run_array[MAX_BPF_HID_ATTACH_TYPE];
+ struct list_head links[MAX_BPF_HID_ATTACH_TYPE];
+};
+
+static inline enum bpf_hid_attach_type
+to_bpf_hid_attach_type(enum bpf_attach_type attach_type)
+{
+ switch (attach_type) {
+ case BPF_HID_DEVICE_EVENT:
+ return BPF_HID_ATTACH_DEVICE_EVENT;
+ default:
+ return BPF_HID_ATTACH_INVALID;
+ }
+}
+
+static inline struct hid_bpf_ctx *bpf_hid_allocate_ctx(struct hid_device *hdev,
+ size_t data_size)
+{
+ struct hid_bpf_ctx *ctx;
+
+ /* ensure data_size is between min and max */
+ data_size = clamp_val(data_size,
+ HID_BPF_MIN_BUFFER_SIZE,
+ HID_BPF_MAX_BUFFER_SIZE);
+
+ ctx = kzalloc(sizeof(*ctx) + data_size, GFP_KERNEL);
+ if (!ctx)
+ return ERR_PTR(-ENOMEM);
+
+ ctx->hdev = hdev;
+ ctx->allocated_size = data_size;
+
+ return ctx;
+}
+
+union bpf_attr;
+struct bpf_prog;
+
+#if IS_ENABLED(CONFIG_HID)
+int bpf_hid_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr);
+int bpf_hid_link_create(const union bpf_attr *attr,
+ struct bpf_prog *prog);
+#else
+static inline int bpf_hid_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int bpf_hid_link_create(const union bpf_attr *attr,
+ struct bpf_prog *prog)
+{
+ return -EOPNOTSUPP;
+}
+#endif
+
+static inline bool bpf_hid_link_empty(struct bpf_hid *bpf,
+ enum bpf_hid_attach_type type)
+{
+ return list_empty(&bpf->links[type]);
+}
+
+struct bpf_hid_hooks {
+ struct hid_device *(*hdev_from_fd)(int fd);
+ int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
+ void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
+};
+
+#ifdef CONFIG_BPF
+int bpf_hid_init(struct hid_device *hdev);
+void bpf_hid_exit(struct hid_device *hdev);
+void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks);
+#else
+static inline int bpf_hid_init(struct hid_device *hdev)
+{
+ return 0;
+}
+
+static inline void bpf_hid_exit(struct hid_device *hdev) {}
+static inline void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks) {}
+#endif
+
+#endif /* _BPF_HID_H */
diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
index 48a91c51c015..1509862aacc4 100644
--- a/include/linux/bpf_types.h
+++ b/include/linux/bpf_types.h
@@ -76,6 +76,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_EXT, bpf_extension,
BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
void *, void *)
#endif /* CONFIG_BPF_LSM */
+#if IS_ENABLED(CONFIG_HID)
+BPF_PROG_TYPE(BPF_PROG_TYPE_HID, hid,
+ __u32, u32)
+#endif
#endif
BPF_PROG_TYPE(BPF_PROG_TYPE_SYSCALL, bpf_syscall,
void *, void *)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7487b0586fe6..56f6f4ad45a7 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -15,6 +15,7 @@
#include <linux/bitops.h>
+#include <linux/bpf-hid.h>
#include <linux/types.h>
#include <linux/slab.h>
#include <linux/list.h>
@@ -639,6 +640,10 @@ struct hid_device { /* device report descriptor */
struct list_head debug_list;
spinlock_t debug_list_lock;
wait_queue_head_t debug_wait;
+
+#ifdef CONFIG_BPF
+ struct bpf_hid bpf;
+#endif
};
#define to_hid_device(pdev) \
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index afe3d0d7f5f2..5978b92cacd3 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,
};
enum bpf_attach_type {
@@ -997,6 +998,7 @@ enum bpf_attach_type {
BPF_SK_REUSEPORT_SELECT,
BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
BPF_PERF_EVENT,
+ BPF_HID_DEVICE_EVENT,
__MAX_BPF_ATTACH_TYPE
};
@@ -1011,6 +1013,7 @@ enum bpf_link_type {
BPF_LINK_TYPE_NETNS = 5,
BPF_LINK_TYPE_XDP = 6,
BPF_LINK_TYPE_PERF_EVENT = 7,
+ BPF_LINK_TYPE_HID = 8,
MAX_BPF_LINK_TYPE,
};
@@ -5870,6 +5873,10 @@ struct bpf_link_info {
struct {
__u32 ifindex;
} xdp;
+ struct {
+ __s32 hidraw_ino;
+ __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..975ca5bd526f
--- /dev/null
+++ b/include/uapi/linux/bpf_hid.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
+
+/*
+ * HID BPF public headers
+ *
+ * Copyright (c) 2021 Benjamin Tissoires
+ */
+
+#ifndef _UAPI__LINUX_BPF_HID_H__
+#define _UAPI__LINUX_BPF_HID_H__
+
+#include <linux/types.h>
+
+/*
+ * The first 1024 bytes are available directly in the bpf programs.
+ * To access the rest of the data (if allocated_size is bigger
+ * than 1024, you need to use bpf_hid_ helpers.
+ */
+#define HID_BPF_MIN_BUFFER_SIZE 1024
+#define HID_BPF_MAX_BUFFER_SIZE 16384 /* in sync with HID_MAX_BUFFER_SIZE */
+
+struct hid_device;
+
+enum hid_bpf_event {
+ HID_BPF_UNDEF = 0,
+ HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */
+};
+
+struct hid_bpf_ctx {
+ enum hid_bpf_event type; /* read-only */
+ __u16 allocated_size; /* the allocated size of data below (RO) */
+ struct hid_device *hdev; /* read-only */
+
+ __u16 size; /* used size in data (RW) */
+ __u8 data[]; /* data buffer (RW) */
+};
+
+#endif /* _UAPI__LINUX_BPF_HID_H__ */
+
diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index c1a9be6a4b9f..8d5619d3d7e5 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -35,6 +35,9 @@ ifeq ($(CONFIG_BPF_JIT),y)
obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o
obj-${CONFIG_BPF_LSM} += bpf_lsm.o
endif
+ifneq ($(CONFIG_HID),)
+obj-$(CONFIG_BPF_SYSCALL) += hid.o
+endif
obj-$(CONFIG_BPF_PRELOAD) += preload/
obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c
new file mode 100644
index 000000000000..db7f75a0a812
--- /dev/null
+++ b/kernel/bpf/hid.c
@@ -0,0 +1,437 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * based on kernel/bpf/net-namespace.c
+ */
+
+#include <linux/bpf.h>
+#include <linux/bpf-hid.h>
+#include <linux/filter.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+
+/*
+ * Functions to manage BPF programs attached to hid
+ */
+
+struct bpf_hid_link {
+ struct bpf_link link;
+ enum bpf_attach_type type;
+ enum bpf_hid_attach_type hid_type;
+
+ /* Must be accessed with bpf_hid_mutex held. */
+ struct hid_device *hdev;
+ struct list_head node; /* node in list of links attached to hid */
+};
+
+/* Protects updates to bpf_hid */
+DEFINE_MUTEX(bpf_hid_mutex);
+
+static struct bpf_hid_hooks hid_hooks = {0};
+
+void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks)
+{
+ if (hooks)
+ hid_hooks = *hooks;
+ else
+ memset(&hid_hooks, 0, sizeof(hid_hooks));
+}
+EXPORT_SYMBOL_GPL(bpf_hid_set_hooks);
+
+static const struct bpf_func_proto *
+hid_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
+{
+ switch (func_id) {
+ default:
+ return bpf_base_func_proto(func_id);
+ }
+}
+
+static bool hid_is_valid_access(int off, int size,
+ enum bpf_access_type access_type,
+ const struct bpf_prog *prog,
+ struct bpf_insn_access_aux *info)
+{
+ /* everything not in ctx is prohibited */
+ if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE)
+ return false;
+
+ switch (off) {
+ /* type, allocated_size, hdev are read-only */
+ case bpf_ctx_range_till(struct hid_bpf_ctx, type, hdev):
+ return access_type == BPF_READ;
+ }
+
+ /* everything else is read/write */
+ return true;
+}
+
+const struct bpf_verifier_ops hid_verifier_ops = {
+ .get_func_proto = hid_func_proto,
+ .is_valid_access = hid_is_valid_access
+};
+
+/* Must be called with bpf_hid_mutex held. */
+static void bpf_hid_run_array_detach(struct hid_device *hdev,
+ enum bpf_hid_attach_type type)
+{
+ struct bpf_prog_array *run_array;
+
+ run_array = rcu_replace_pointer(hdev->bpf.run_array[type], NULL,
+ lockdep_is_held(&bpf_hid_mutex));
+ bpf_prog_array_free(run_array);
+
+ if (hid_hooks.array_detached)
+ hid_hooks.array_detached(hdev, type);
+}
+
+static int link_index(struct hid_device *hdev, enum bpf_hid_attach_type type,
+ struct bpf_hid_link *link)
+{
+ struct bpf_hid_link *pos;
+ int i = 0;
+
+ list_for_each_entry(pos, &hdev->bpf.links[type], node) {
+ if (pos == link)
+ return i;
+ i++;
+ }
+ return -ENOENT;
+}
+
+static int link_count(struct hid_device *hdev, enum bpf_hid_attach_type type)
+{
+ struct list_head *pos;
+ int i = 0;
+
+ list_for_each(pos, &hdev->bpf.links[type])
+ i++;
+ return i;
+}
+
+static void fill_prog_array(struct hid_device *hdev, enum bpf_hid_attach_type type,
+ struct bpf_prog_array *prog_array)
+{
+ struct bpf_hid_link *pos;
+ unsigned int i = 0;
+
+ list_for_each_entry(pos, &hdev->bpf.links[type], node) {
+ prog_array->items[i].prog = pos->link.prog;
+ i++;
+ }
+}
+
+static void bpf_hid_link_release(struct bpf_link *link)
+{
+ struct bpf_hid_link *hid_link =
+ container_of(link, struct bpf_hid_link, link);
+ enum bpf_hid_attach_type type = hid_link->hid_type;
+ struct bpf_prog_array *old_array, *new_array;
+ struct hid_device *hdev;
+ int cnt, idx;
+
+ mutex_lock(&bpf_hid_mutex);
+
+ hdev = hid_link->hdev;
+ if (!hdev)
+ goto out_unlock;
+
+ /* Remember link position in case of safe delete */
+ idx = link_index(hdev, type, hid_link);
+ list_del(&hid_link->node);
+
+ cnt = link_count(hdev, type);
+ if (!cnt) {
+ bpf_hid_run_array_detach(hdev, type);
+ goto out_unlock;
+ }
+
+ old_array = rcu_dereference_protected(hdev->bpf.run_array[type],
+ lockdep_is_held(&bpf_hid_mutex));
+ new_array = bpf_prog_array_alloc(cnt, GFP_KERNEL);
+ if (!new_array) {
+ WARN_ON(bpf_prog_array_delete_safe_at(old_array, idx));
+ goto out_unlock;
+ }
+ fill_prog_array(hdev, type, new_array);
+ rcu_assign_pointer(hdev->bpf.run_array[type], new_array);
+ bpf_prog_array_free(old_array);
+
+out_unlock:
+ hid_link->hdev = NULL;
+ mutex_unlock(&bpf_hid_mutex);
+}
+
+static int bpf_hid_link_detach(struct bpf_link *link)
+{
+ bpf_hid_link_release(link);
+ return 0;
+}
+
+static void bpf_hid_link_dealloc(struct bpf_link *link)
+{
+ struct bpf_hid_link *hid_link =
+ container_of(link, struct bpf_hid_link, link);
+
+ kfree(hid_link);
+}
+
+static int bpf_hid_link_update_prog(struct bpf_link *link,
+ struct bpf_prog *new_prog,
+ struct bpf_prog *old_prog)
+{
+ struct bpf_hid_link *hid_link =
+ container_of(link, struct bpf_hid_link, link);
+ enum bpf_hid_attach_type type = hid_link->hid_type;
+ struct bpf_prog_array *run_array;
+ struct hid_device *hdev;
+ int idx, ret;
+
+ if (old_prog && old_prog != link->prog)
+ return -EPERM;
+ if (new_prog->type != link->prog->type)
+ return -EINVAL;
+
+ mutex_lock(&bpf_hid_mutex);
+
+ hdev = hid_link->hdev;
+ if (!hdev) {
+ /* hid dying */
+ ret = -ENOLINK;
+ goto out_unlock;
+ }
+
+ run_array = rcu_dereference_protected(hdev->bpf.run_array[type],
+ lockdep_is_held(&bpf_hid_mutex));
+ idx = link_index(hdev, type, hid_link);
+ ret = bpf_prog_array_update_at(run_array, idx, new_prog);
+ if (ret)
+ goto out_unlock;
+
+ old_prog = xchg(&link->prog, new_prog);
+ bpf_prog_put(old_prog);
+
+out_unlock:
+ mutex_unlock(&bpf_hid_mutex);
+ return ret;
+}
+
+static int bpf_hid_link_fill_info(const struct bpf_link *link,
+ struct bpf_link_info *info)
+{
+ const struct bpf_hid_link *hid_link =
+ container_of(link, struct bpf_hid_link, link);
+ int hidraw_ino = -1;
+ struct hid_device *hdev;
+ struct hidraw *hidraw;
+
+ mutex_lock(&bpf_hid_mutex);
+ hdev = hid_link->hdev;
+ if (hdev && hdev->hidraw) {
+ hidraw = hdev->hidraw;
+ hidraw_ino = hidraw->minor;
+ }
+ mutex_unlock(&bpf_hid_mutex);
+
+ info->hid.hidraw_ino = hidraw_ino;
+ info->hid.attach_type = hid_link->type;
+ return 0;
+}
+
+static void bpf_hid_link_show_fdinfo(const struct bpf_link *link,
+ struct seq_file *seq)
+{
+ struct bpf_link_info info = {};
+
+ bpf_hid_link_fill_info(link, &info);
+ seq_printf(seq,
+ "hidraw_ino:\t%u\n"
+ "attach_type:\t%u\n",
+ info.hid.hidraw_ino,
+ info.hid.attach_type);
+}
+
+static const struct bpf_link_ops bpf_hid_link_ops = {
+ .release = bpf_hid_link_release,
+ .dealloc = bpf_hid_link_dealloc,
+ .detach = bpf_hid_link_detach,
+ .update_prog = bpf_hid_link_update_prog,
+ .fill_link_info = bpf_hid_link_fill_info,
+ .show_fdinfo = bpf_hid_link_show_fdinfo,
+};
+
+/* Must be called with bpf_hid_mutex held. */
+static int __bpf_hid_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr,
+ struct hid_device *hdev,
+ enum bpf_hid_attach_type type)
+{
+ __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+ struct bpf_prog_array *run_array;
+ u32 prog_cnt = 0, flags = 0;
+
+ run_array = rcu_dereference_protected(hdev->bpf.run_array[type],
+ lockdep_is_held(&bpf_hid_mutex));
+ if (run_array)
+ prog_cnt = bpf_prog_array_length(run_array);
+
+ if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)))
+ return -EFAULT;
+ if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
+ return -EFAULT;
+ if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
+ return 0;
+
+ return bpf_prog_array_copy_to_user(run_array, prog_ids,
+ attr->query.prog_cnt);
+}
+
+int bpf_hid_prog_query(const union bpf_attr *attr,
+ union bpf_attr __user *uattr)
+{
+ enum bpf_hid_attach_type type;
+ struct hid_device *hdev;
+ int ret;
+
+ if (attr->query.query_flags || !hid_hooks.hdev_from_fd)
+ return -EINVAL;
+
+ type = to_bpf_hid_attach_type(attr->query.attach_type);
+ if (type < 0)
+ return -EINVAL;
+
+ hdev = hid_hooks.hdev_from_fd(attr->query.target_fd);
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+
+ mutex_lock(&bpf_hid_mutex);
+ ret = __bpf_hid_prog_query(attr, uattr, hdev, type);
+ mutex_unlock(&bpf_hid_mutex);
+
+ return ret;
+}
+
+static int bpf_hid_max_progs(enum bpf_hid_attach_type type)
+{
+ switch (type) {
+ case BPF_HID_ATTACH_DEVICE_EVENT:
+ return 64;
+ default:
+ return 0;
+ }
+}
+
+static int bpf_hid_link_attach(struct hid_device *hdev, struct bpf_link *link,
+ enum bpf_hid_attach_type type)
+{
+ struct bpf_hid_link *hid_link =
+ container_of(link, struct bpf_hid_link, link);
+ struct bpf_prog_array *run_array;
+ int cnt, err = 0;
+
+ mutex_lock(&bpf_hid_mutex);
+
+ cnt = link_count(hdev, type);
+ if (cnt >= bpf_hid_max_progs(type)) {
+ err = -E2BIG;
+ goto out_unlock;
+ }
+
+ if (hid_hooks.link_attach) {
+ err = hid_hooks.link_attach(hdev, type);
+ if (err)
+ goto out_unlock;
+ }
+
+ run_array = bpf_prog_array_alloc(cnt + 1, GFP_KERNEL);
+ if (!run_array) {
+ err = -ENOMEM;
+ goto out_unlock;
+ }
+
+ list_add_tail(&hid_link->node, &hdev->bpf.links[type]);
+
+ fill_prog_array(hdev, type, run_array);
+ run_array = rcu_replace_pointer(hdev->bpf.run_array[type], run_array,
+ lockdep_is_held(&bpf_hid_mutex));
+ bpf_prog_array_free(run_array);
+
+out_unlock:
+ mutex_unlock(&bpf_hid_mutex);
+ return err;
+}
+
+int bpf_hid_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
+{
+ enum bpf_hid_attach_type hid_type;
+ struct bpf_link_primer link_primer;
+ struct bpf_hid_link *hid_link;
+ enum bpf_attach_type type;
+ struct hid_device *hdev;
+ int err;
+
+ if (attr->link_create.flags || !hid_hooks.hdev_from_fd)
+ return -EINVAL;
+
+ type = attr->link_create.attach_type;
+ hid_type = to_bpf_hid_attach_type(type);
+ if (hid_type < 0)
+ return -EINVAL;
+
+ hdev = hid_hooks.hdev_from_fd(attr->link_create.target_fd);
+ if (IS_ERR(hdev))
+ return PTR_ERR(hdev);
+
+ hid_link = kzalloc(sizeof(*hid_link), GFP_USER);
+ if (!hid_link)
+ return -ENOMEM;
+
+ bpf_link_init(&hid_link->link, BPF_LINK_TYPE_HID,
+ &bpf_hid_link_ops, prog);
+ hid_link->hdev = hdev;
+ hid_link->type = type;
+ hid_link->hid_type = hid_type;
+
+ err = bpf_link_prime(&hid_link->link, &link_primer);
+ if (err) {
+ kfree(hid_link);
+ return err;
+ }
+
+ err = bpf_hid_link_attach(hdev, &hid_link->link, hid_type);
+ if (err) {
+ bpf_link_cleanup(&link_primer);
+ return err;
+ }
+
+ return bpf_link_settle(&link_primer);
+}
+
+const struct bpf_prog_ops hid_prog_ops = {
+};
+
+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 cc570891322b..a94e78ec3211 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:
case BPF_PROG_TYPE_EXT: /* extends any prog */
return true;
default:
@@ -3200,6 +3202,8 @@ 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:
+ return BPF_PROG_TYPE_HID;
default:
return BPF_PROG_TYPE_UNSPEC;
}
@@ -3343,6 +3347,8 @@ 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:
+ return bpf_hid_prog_query(attr, uattr);
default:
return -EINVAL;
}
@@ -4337,6 +4343,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;
}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index afe3d0d7f5f2..5978b92cacd3 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/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,
};
enum bpf_attach_type {
@@ -997,6 +998,7 @@ enum bpf_attach_type {
BPF_SK_REUSEPORT_SELECT,
BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
BPF_PERF_EVENT,
+ BPF_HID_DEVICE_EVENT,
__MAX_BPF_ATTACH_TYPE
};
@@ -1011,6 +1013,7 @@ enum bpf_link_type {
BPF_LINK_TYPE_NETNS = 5,
BPF_LINK_TYPE_XDP = 6,
BPF_LINK_TYPE_PERF_EVENT = 7,
+ BPF_LINK_TYPE_HID = 8,
MAX_BPF_LINK_TYPE,
};
@@ -5870,6 +5873,10 @@ struct bpf_link_info {
struct {
__u32 ifindex;
} xdp;
+ struct {
+ __s32 hidraw_ino;
+ __u32 attach_type;
+ } hid;
};
} __attribute__((aligned(8)));
--
2.35.1
When tracing the incoming events, if a bpf program is already loaded,
the next bpf program will see the potentially changed data.
Add a flag to BPF_LINK_CREATE that allows to chose the position of the
inserted program: at the beginning or at the end.
This way, we can have a tracing program that compare the raw event from
the device and the transformed stream from all the other bpf programs.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
new in v2
---
include/uapi/linux/bpf.h | 10 ++++++++++
kernel/bpf/hid.c | 11 +++++++----
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 417cf1c31579..23ebe5e96d69 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1123,6 +1123,16 @@ enum bpf_link_type {
*/
#define BPF_F_XDP_HAS_FRAGS (1U << 5)
+/* HID flag used in BPF_LINK_CREATE command
+ *
+ * NONE(default): The bpf program will be added at the tail of the list
+ * of existing bpf program for this type.
+ *
+ * BPF_F_INSERT_HEAD: The bpf program will be added at the beginning
+ * of the list of existing bpf program for this type..
+ */
+#define BPF_F_INSERT_HEAD (1U << 0)
+
/* When BPF ldimm64's insn[0].src_reg != 0 then this can have
* the following extensions:
*
diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c
index b3dc1cd37a3e..141eb4169079 100644
--- a/kernel/bpf/hid.c
+++ b/kernel/bpf/hid.c
@@ -416,7 +416,7 @@ static int bpf_hid_max_progs(enum bpf_hid_attach_type type)
}
static int bpf_hid_link_attach(struct hid_device *hdev, struct bpf_link *link,
- enum bpf_hid_attach_type type)
+ enum bpf_hid_attach_type type, u32 flags)
{
struct bpf_hid_link *hid_link =
container_of(link, struct bpf_hid_link, link);
@@ -443,7 +443,10 @@ static int bpf_hid_link_attach(struct hid_device *hdev, struct bpf_link *link,
goto out_unlock;
}
- list_add_tail(&hid_link->node, &hdev->bpf.links[type]);
+ if (flags & BPF_F_INSERT_HEAD)
+ list_add(&hid_link->node, &hdev->bpf.links[type]);
+ else
+ list_add_tail(&hid_link->node, &hdev->bpf.links[type]);
fill_prog_array(hdev, type, run_array);
run_array = rcu_replace_pointer(hdev->bpf.run_array[type], run_array,
@@ -467,7 +470,7 @@ int bpf_hid_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
struct hid_device *hdev;
int err;
- if (attr->link_create.flags || !hid_hooks.hdev_from_fd)
+ if ((attr->link_create.flags & ~BPF_F_INSERT_HEAD) || !hid_hooks.hdev_from_fd)
return -EINVAL;
type = attr->link_create.attach_type;
@@ -495,7 +498,7 @@ int bpf_hid_link_create(const union bpf_attr *attr, struct bpf_prog *prog)
return err;
}
- err = bpf_hid_link_attach(hdev, &hid_link->link, hid_type);
+ err = bpf_hid_link_attach(hdev, &hid_link->link, hid_type, attr->link_create.flags);
if (err) {
bpf_link_cleanup(&link_primer);
return err;
--
2.35.1
On Fri, Mar 04, 2022 at 06:28:30PM +0100, Benjamin Tissoires wrote:
> Everything should be available in the selftest part of the tree, but
> providing an example without uhid and hidraw will be more easy to
> follow for users.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
> ---
>
> changes in v2:
> - split the series by bpf/libbpf/hid/selftests and samples
> ---
> samples/bpf/.gitignore | 1 +
> samples/bpf/Makefile | 4 ++
> samples/bpf/hid_mouse_kern.c | 66 ++++++++++++++++++
> samples/bpf/hid_mouse_user.c | 129 +++++++++++++++++++++++++++++++++++
> 4 files changed, 200 insertions(+)
> create mode 100644 samples/bpf/hid_mouse_kern.c
> create mode 100644 samples/bpf/hid_mouse_user.c
>
> diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
> index 0e7bfdbff80a..65440bd618b2 100644
> --- a/samples/bpf/.gitignore
> +++ b/samples/bpf/.gitignore
> @@ -2,6 +2,7 @@
> cpustat
> fds_example
> hbm
> +hid_mouse
> ibumad
> lathist
> lwt_len_hist
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 38638845db9d..84ef458487df 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -60,6 +60,8 @@ tprogs-y += xdp_redirect_map
> tprogs-y += xdp_redirect
> tprogs-y += xdp_monitor
>
> +tprogs-y += hid_mouse
> +
> # Libbpf dependencies
> LIBBPF_SRC = $(TOOLS_PATH)/lib/bpf
> LIBBPF_OUTPUT = $(abspath $(BPF_SAMPLES_PATH))/libbpf
> @@ -124,6 +126,7 @@ xdp_redirect_cpu-objs := xdp_redirect_cpu_user.o $(XDP_SAMPLE)
> xdp_redirect_map-objs := xdp_redirect_map_user.o $(XDP_SAMPLE)
> xdp_redirect-objs := xdp_redirect_user.o $(XDP_SAMPLE)
> xdp_monitor-objs := xdp_monitor_user.o $(XDP_SAMPLE)
> +hid_mouse-objs := hid_mouse_user.o
>
> # Tell kbuild to always build the programs
> always-y := $(tprogs-y)
> @@ -181,6 +184,7 @@ always-y += ibumad_kern.o
> always-y += hbm_out_kern.o
> always-y += hbm_edt_kern.o
> always-y += xdpsock_kern.o
> +always-y += hid_mouse_kern.o
>
> ifeq ($(ARCH), arm)
> # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
> diff --git a/samples/bpf/hid_mouse_kern.c b/samples/bpf/hid_mouse_kern.c
> new file mode 100644
> index 000000000000..c24a12e06b40
> --- /dev/null
> +++ b/samples/bpf/hid_mouse_kern.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021 Benjamin Tissoires
It's 2022 now :(
Other than that, looks nice and simple, good work!
Reviewed-by: Greg Kroah-Hartman <[email protected]>
the program inverts the definition of X and Y at a given place in the
report descriptor of my mouse.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
---
samples/bpf/hid_mouse_kern.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/samples/bpf/hid_mouse_kern.c b/samples/bpf/hid_mouse_kern.c
index c24a12e06b40..958820caaf5d 100644
--- a/samples/bpf/hid_mouse_kern.c
+++ b/samples/bpf/hid_mouse_kern.c
@@ -62,5 +62,30 @@ int hid_x_event(struct hid_bpf_ctx *ctx)
return 0;
}
+SEC("hid/rdesc_fixup")
+int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)
+{
+ if (ctx->type != HID_BPF_RDESC_FIXUP)
+ return 0;
+
+ bpf_printk("rdesc: %02x %02x %02x",
+ ctx->data[0],
+ ctx->data[1],
+ ctx->data[2]);
+ bpf_printk(" %02x %02x %02x",
+ ctx->data[3],
+ ctx->data[4],
+ ctx->data[5]);
+ bpf_printk(" %02x %02x %02x ...",
+ ctx->data[6],
+ ctx->data[7],
+ ctx->data[8]);
+
+ ctx->data[39] = 0x31;
+ ctx->data[41] = 0x30;
+
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
u32 _version SEC("version") = LINUX_VERSION_CODE;
--
2.35.1
Now that BPF can be compatible with HID, add the capability into HID.
drivers/hid/hid-bpf.c takes care of the glue between bpf and HID, and
hid-core can then inject any incoming event from the device into a BPF
program to filter/analyze it.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
changes in v2:
- split the series by bpf/libbpf/hid/selftests and samples
- addressed review comments from v1
---
drivers/hid/Makefile | 1 +
drivers/hid/hid-bpf.c | 157 +++++++++++++++++++++++++++++++++++++++++
drivers/hid/hid-core.c | 21 +++++-
include/linux/hid.h | 11 +++
4 files changed, 187 insertions(+), 3 deletions(-)
create mode 100644 drivers/hid/hid-bpf.c
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 6d3e630e81af..08d2d7619937 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -4,6 +4,7 @@
#
hid-y := hid-core.o hid-input.o hid-quirks.o
hid-$(CONFIG_DEBUG_FS) += hid-debug.o
+hid-$(CONFIG_BPF) += hid-bpf.o
obj-$(CONFIG_HID) += hid.o
obj-$(CONFIG_UHID) += uhid.o
diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c
new file mode 100644
index 000000000000..8120e598de9f
--- /dev/null
+++ b/drivers/hid/hid-bpf.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * BPF in HID support for Linux
+ *
+ * Copyright (c) 2022 Benjamin Tissoires
+ */
+
+#include <linux/filter.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+
+#include <uapi/linux/bpf_hid.h>
+#include <linux/hid.h>
+
+static int __hid_bpf_match_sysfs(struct device *dev, const void *data)
+{
+ struct kernfs_node *kn = dev->kobj.sd;
+ struct kernfs_node *uevent_kn;
+
+ uevent_kn = kernfs_find_and_get_ns(kn, "uevent", NULL);
+
+ return uevent_kn == data;
+}
+
+static struct hid_device *hid_bpf_fd_to_hdev(int fd)
+{
+ struct device *dev;
+ struct hid_device *hdev;
+ struct fd f = fdget(fd);
+ struct inode *inode;
+ struct kernfs_node *node;
+
+ if (!f.file) {
+ hdev = ERR_PTR(-EBADF);
+ goto out;
+ }
+
+ inode = file_inode(f.file);
+ node = inode->i_private;
+
+ dev = bus_find_device(&hid_bus_type, NULL, node, __hid_bpf_match_sysfs);
+
+ if (dev)
+ hdev = to_hid_device(dev);
+ else
+ hdev = ERR_PTR(-EINVAL);
+
+ out:
+ fdput(f);
+ return hdev;
+}
+
+static int hid_bpf_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type)
+{
+ int err = 0;
+
+ switch (type) {
+ case BPF_HID_ATTACH_DEVICE_EVENT:
+ if (!hdev->bpf.ctx) {
+ hdev->bpf.ctx = bpf_hid_allocate_ctx(hdev, HID_BPF_MAX_BUFFER_SIZE);
+ if (IS_ERR(hdev->bpf.ctx)) {
+ err = PTR_ERR(hdev->bpf.ctx);
+ hdev->bpf.ctx = NULL;
+ }
+ }
+ break;
+ default:
+ /* do nothing */
+ }
+
+ return err;
+}
+
+static void hid_bpf_array_detached(struct hid_device *hdev, enum bpf_hid_attach_type type)
+{
+ switch (type) {
+ case BPF_HID_ATTACH_DEVICE_EVENT:
+ kfree(hdev->bpf.ctx);
+ hdev->bpf.ctx = NULL;
+ break;
+ default:
+ /* do nothing */
+ }
+}
+
+static int hid_bpf_run_progs(struct hid_device *hdev, enum bpf_hid_attach_type type,
+ struct hid_bpf_ctx *ctx, u8 *data, int size)
+{
+ enum hid_bpf_event event = HID_BPF_UNDEF;
+
+ if (type < 0 || !ctx)
+ return -EINVAL;
+
+ if (size > ctx->allocated_size)
+ return -E2BIG;
+
+ switch (type) {
+ case BPF_HID_ATTACH_DEVICE_EVENT:
+ event = HID_BPF_DEVICE_EVENT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (!hdev->bpf.run_array[type])
+ return 0;
+
+ memset(ctx->data, 0, ctx->allocated_size);
+ ctx->type = event;
+
+ if (size && data) {
+ memcpy(ctx->data, data, size);
+ ctx->size = size;
+ } else {
+ ctx->size = 0;
+ }
+
+ return BPF_PROG_RUN_ARRAY(hdev->bpf.run_array[type], ctx, bpf_prog_run);
+}
+
+u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size)
+{
+ int ret;
+
+ if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_DEVICE_EVENT))
+ return data;
+
+ ret = hid_bpf_run_progs(hdev, BPF_HID_ATTACH_DEVICE_EVENT,
+ hdev->bpf.ctx, data, *size);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (!hdev->bpf.ctx->size)
+ return ERR_PTR(-EINVAL);
+
+ *size = hdev->bpf.ctx->size;
+
+ return hdev->bpf.ctx->data;
+}
+
+int __init hid_bpf_module_init(void)
+{
+ struct bpf_hid_hooks hooks = {
+ .hdev_from_fd = hid_bpf_fd_to_hdev,
+ .link_attach = hid_bpf_link_attach,
+ .array_detached = hid_bpf_array_detached,
+ };
+
+ bpf_hid_set_hooks(&hooks);
+
+ return 0;
+}
+
+void __exit hid_bpf_module_exit(void)
+{
+ bpf_hid_set_hooks(NULL);
+}
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f1aed5bbd000..a80bffe6ce4a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1748,13 +1748,21 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
struct hid_driver *hdrv;
unsigned int a;
u32 rsize, csize = size;
- u8 *cdata = data;
+ u8 *cdata;
int ret = 0;
+ data = hid_bpf_raw_event(hid, data, &size);
+ if (IS_ERR(data)) {
+ ret = PTR_ERR(data);
+ goto out;
+ }
+
report = hid_get_report(report_enum, data);
if (!report)
goto out;
+ cdata = data;
+
if (report_enum->numbered) {
cdata++;
csize--;
@@ -2528,10 +2536,12 @@ int hid_add_device(struct hid_device *hdev)
hid_debug_register(hdev, dev_name(&hdev->dev));
ret = device_add(&hdev->dev);
- if (!ret)
+ if (!ret) {
hdev->status |= HID_STAT_ADDED;
- else
+ } else {
hid_debug_unregister(hdev);
+ bpf_hid_exit(hdev);
+ }
return ret;
}
@@ -2567,6 +2577,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_input_lock, 1);
mutex_init(&hdev->ll_open_lock);
+ bpf_hid_init(hdev);
return hdev;
}
@@ -2574,6 +2585,7 @@ EXPORT_SYMBOL_GPL(hid_allocate_device);
static void hid_remove_device(struct hid_device *hdev)
{
+ bpf_hid_exit(hdev);
if (hdev->status & HID_STAT_ADDED) {
device_del(&hdev->dev);
hid_debug_unregister(hdev);
@@ -2700,6 +2712,8 @@ static int __init hid_init(void)
hid_debug_init();
+ hid_bpf_module_init();
+
return 0;
err_bus:
bus_unregister(&hid_bus_type);
@@ -2709,6 +2723,7 @@ static int __init hid_init(void)
static void __exit hid_exit(void)
{
+ hid_bpf_module_exit();
hid_debug_exit();
hidraw_exit();
bus_unregister(&hid_bus_type);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 56f6f4ad45a7..8fd79011f461 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -27,6 +27,7 @@
#include <linux/mutex.h>
#include <linux/power_supply.h>
#include <uapi/linux/hid.h>
+#include <uapi/linux/bpf_hid.h>
/*
* We parse each description item into this structure. Short items data
@@ -1210,4 +1211,14 @@ do { \
#define hid_dbg_once(hid, fmt, ...) \
dev_dbg_once(&(hid)->dev, fmt, ##__VA_ARGS__)
+#ifdef CONFIG_BPF
+u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size);
+int hid_bpf_module_init(void);
+void hid_bpf_module_exit(void);
+#else
+static inline u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *rd, int *size) { return rd; }
+static inline int hid_bpf_module_init(void) { return 0; }
+static inline void hid_bpf_module_exit(void) {}
+#endif
+
#endif
--
2.35.1
On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires
<[email protected]> 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 <[email protected]>
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
> ---
>
> new in v2
> ---
> kernel/bpf/syscall.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index db402ebc5570..cc570891322b 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,17 @@ 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:
> + case BPF_PROG_TYPE_EXT: /* extends any prog */
> + return true;
> + default:
> + return false;
> + }
> +}
I am not sure whether we should do this. This is a behavior change, that may
break some user space. Also, BPF_PROG_TYPE_EXT is checked in
is_perfmon_prog_type(), and this change will make that case useless.
Thanks,
Song
[...]
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
<[email protected]> wrote:
>
[...]
> +#endif
> +
> +static inline bool bpf_hid_link_empty(struct bpf_hid *bpf,
> + enum bpf_hid_attach_type type)
> +{
> + return list_empty(&bpf->links[type]);
> +}
> +
> +struct bpf_hid_hooks {
> + struct hid_device *(*hdev_from_fd)(int fd);
> + int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> + void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
shall we call this array_detach()? detached sounds like a function that
checks the status of link/hook.
[...]
> --- /dev/null
> +++ b/kernel/bpf/hid.c
> @@ -0,0 +1,437 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * based on kernel/bpf/net-namespace.c
> + */
I guess we don't need this comment.
> +
> +#include <linux/bpf.h>
> +#include <linux/bpf-hid.h>
> +#include <linux/filter.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
[...]
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
<[email protected]> wrote:
>
> Now that BPF can be compatible with HID, add the capability into HID.
> drivers/hid/hid-bpf.c takes care of the glue between bpf and HID, and
> hid-core can then inject any incoming event from the device into a BPF
> program to filter/analyze it.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
[...]
> +
> +static int hid_bpf_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type)
> +{
> + int err = 0;
> +
> + switch (type) {
> + case BPF_HID_ATTACH_DEVICE_EVENT:
> + if (!hdev->bpf.ctx) {
> + hdev->bpf.ctx = bpf_hid_allocate_ctx(hdev, HID_BPF_MAX_BUFFER_SIZE);
> + if (IS_ERR(hdev->bpf.ctx)) {
> + err = PTR_ERR(hdev->bpf.ctx);
> + hdev->bpf.ctx = NULL;
> + }
> + }
> + break;
> + default:
> + /* do nothing */
Do we need to show warning and/or return EINVAL here?
> + }
> +
> + return err;
> +}
> +
> +static void hid_bpf_array_detached(struct hid_device *hdev, enum bpf_hid_attach_type type)
> +{
> + switch (type) {
> + case BPF_HID_ATTACH_DEVICE_EVENT:
> + kfree(hdev->bpf.ctx);
> + hdev->bpf.ctx = NULL;
> + break;
> + default:
> + /* do nothing */
ditto
[...]
On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires
<[email protected]> wrote:
>
> Hi,
>
> This is a followup of my v1 at [0].
>
> 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 tried to address as many comments as I could and here is the short log
> of changes:
>
> 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
>
The set looks good so far. I will review the rest later.
[...]
A quick note about how we organize these patches. Maybe we can
merge some of these patches like:
> bpf: introduce hid program type
> bpf/hid: add a new attach type to change the report descriptor
> bpf/hid: add new BPF type to trigger commands from userspace
I guess the three can merge into one.
> HID: hook up with bpf
> HID: allow to change the report descriptor from an eBPF program
> HID: bpf: compute only the required buffer size for the device
> HID: bpf: only call hid_bpf_raw_event() if a ctx is available
I haven't read through all of them, but I guess they can probably merge
as well.
> libbpf: add HID program type and API
> libbpf: add new attach type BPF_HID_RDESC_FIXUP
> libbpf: add new attach type BPF_HID_USER_EVENT
There 3 can merge, and maybe also the one below
> libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
> samples/bpf: add new hid_mouse example
> samples/bpf: add a report descriptor fixup
> samples/bpf: fix bpf_program__attach_hid() api change
Maybe it makes sense to merge these 3?
> bpf/hid: add hid_{get|set}_data helpers
> HID: bpf: implement hid_bpf_get|set_data
> bpf/hid: add bpf_hid_raw_request helper function
> HID: add implementation of bpf_hid_raw_request
We can have 1 or 2 patches for these helpers
> selftests/bpf: add tests for the HID-bpf initial implementation
> selftests/bpf: add report descriptor fixup tests
> selftests/bpf: add tests for hid_{get|set}_data helpers
> selftests/bpf: add test for user call of HID bpf programs
> selftests/bpf: hid: rely on uhid event to know if a test device is
> ready
> selftests/bpf: add tests for bpf_hid_hw_request
> selftests/bpf: Add a test for BPF_F_INSERT_HEAD
These selftests could also merge into 1 or 2 patches I guess.
I understand rearranging these patches may take quite some effort.
But I do feel that's a cleaner approach (from someone doesn't know
much about HID). If you really hate it that way, we can discuss...
Thanks,
Song
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
<[email protected]> wrote:
>
> HID-bpf program type are needing a new SEC.
> To bind a hid-bpf program, we can rely on bpf_program__attach_fd()
> so export a new function to the API.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
Acked-by: Song Liu <[email protected]>
>
> ---
>
> changes in v2:
> - split the series by bpf/libbpf/hid/selftests and samples
> ---
> tools/lib/bpf/libbpf.c | 7 +++++++
> tools/lib/bpf/libbpf.h | 2 ++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 10 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 81bf01d67671..356bbd3ad2c7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8680,6 +8680,7 @@ static const struct bpf_sec_def section_defs[] = {
> SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE),
> SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> + SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
> };
>
> #define MAX_TYPE_NAME_SIZE 32
> @@ -10659,6 +10660,12 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
> return bpf_program__attach_iter(prog, NULL);
> }
>
> +struct bpf_link *
> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd)
> +{
> + return bpf_program__attach_fd(prog, hid_fd, 0, "hid");
> +}
> +
> struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
> {
> if (!prog->sec_def || !prog->sec_def->attach_fn)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c8d8daad212e..f677ac0a9ede 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -529,6 +529,8 @@ struct bpf_iter_attach_opts {
> LIBBPF_API struct bpf_link *
> bpf_program__attach_iter(const struct bpf_program *prog,
> const struct bpf_iter_attach_opts *opts);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd);
>
> /*
> * Libbpf allows callers to adjust BPF programs before being loaded
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 47e70c9058d9..fdc6fa743953 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -424,6 +424,7 @@ LIBBPF_0.6.0 {
> LIBBPF_0.7.0 {
> global:
> bpf_btf_load;
> + bpf_program__attach_hid;
> bpf_program__expected_attach_type;
> bpf_program__log_buf;
> bpf_program__log_level;
> --
> 2.35.1
>
Hi Song,
Thanks a lot for the review.
I'll comment on the review in more details next week, but I have a
quick question here:
On Sat, Mar 5, 2022 at 2:14 AM Song Liu <[email protected]> wrote:
>
> On Fri, Mar 4, 2022 at 9:29 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > Hi,
> >
> > This is a followup of my v1 at [0].
> >
> > 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 tried to address as many comments as I could and here is the short log
> > of changes:
> >
> > 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
> >
>
> The set looks good so far. I will review the rest later.
>
> [...]
>
> A quick note about how we organize these patches. Maybe we can
> merge some of these patches like:
Just to be sure we are talking about the same thing: you mean squash
the patch together?
>
> > bpf: introduce hid program type
> > bpf/hid: add a new attach type to change the report descriptor
> > bpf/hid: add new BPF type to trigger commands from userspace
> I guess the three can merge into one.
>
> > HID: hook up with bpf
> > HID: allow to change the report descriptor from an eBPF program
> > HID: bpf: compute only the required buffer size for the device
> > HID: bpf: only call hid_bpf_raw_event() if a ctx is available
> I haven't read through all of them, but I guess they can probably merge
> as well.
There are certainly patches that we could squash together (3 and 4
from this list into the previous ones), but I'd like to keep some sort
of granularity here to not have a patch bomb that gets harder to come
back later.
>
> > libbpf: add HID program type and API
> > libbpf: add new attach type BPF_HID_RDESC_FIXUP
> > libbpf: add new attach type BPF_HID_USER_EVENT
> There 3 can merge, and maybe also the one below
> > libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
Yeah, the libbpf changes are small enough to not really justify
separate patches.
>
> > samples/bpf: add new hid_mouse example
> > samples/bpf: add a report descriptor fixup
> > samples/bpf: fix bpf_program__attach_hid() api change
> Maybe it makes sense to merge these 3?
Sure, why not.
>
> > bpf/hid: add hid_{get|set}_data helpers
> > HID: bpf: implement hid_bpf_get|set_data
> > bpf/hid: add bpf_hid_raw_request helper function
> > HID: add implementation of bpf_hid_raw_request
> We can have 1 or 2 patches for these helpers
OK, the patches should be self-contained enough.
>
> > selftests/bpf: add tests for the HID-bpf initial implementation
> > selftests/bpf: add report descriptor fixup tests
> > selftests/bpf: add tests for hid_{get|set}_data helpers
> > selftests/bpf: add test for user call of HID bpf programs
> > selftests/bpf: hid: rely on uhid event to know if a test device is
> > ready
> > selftests/bpf: add tests for bpf_hid_hw_request
> > selftests/bpf: Add a test for BPF_F_INSERT_HEAD
> These selftests could also merge into 1 or 2 patches I guess.
I'd still like to link them to the granularity of the bpf changes, so
I can refer a selftest change to a specific commit/functionality
added. But that's just my personal taste, and I can be convinced
otherwise. This should give us maybe 4 patches instead of 7.
>
> I understand rearranging these patches may take quite some effort.
> But I do feel that's a cleaner approach (from someone doesn't know
> much about HID). If you really hate it that way, we can discuss...
>
No worries. I don't mind iterating on the series. IIRC I already
rewrote it twice from scratch, and that's when the selftests I
introduced in the second rewrite were tremendously helpful :) And
honestly I don't think it'll be too much effort to reorder/squash the
patches given that the v2 is *very* granular.
Anyway, I prefer having the reviewers happy so we can have a solid
rock API from day 1 than keeping it obscure for everyone and having to
deal with design issues forever. So if it takes 10 or 20 revisions to
have everybody on the same page, that's fine with me (not that I want
to have that many revisions, just that I won't be afraid of the
bikeshedding we might have at some point).
Cheers,
Benjamin
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
<[email protected]> wrote:
>
> HID is a protocol that could benefit from using BPF too.
[...]
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +
> +struct bpf_prog;
> +struct bpf_prog_array;
> +struct hid_device;
> +
> +enum bpf_hid_attach_type {
> + BPF_HID_ATTACH_INVALID = -1,
> + BPF_HID_ATTACH_DEVICE_EVENT = 0,
> + MAX_BPF_HID_ATTACH_TYPE
Is it typical to have different BPF programs for different attach types?
Otherwise, (different types may have similar BPF programs), maybe
we can pass type as an argument to the program (shared among
different types)?
[...]
> +struct hid_device;
> +
> +enum hid_bpf_event {
> + HID_BPF_UNDEF = 0,
> + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */
> +};
> +
> +struct hid_bpf_ctx {
> + enum hid_bpf_event type; /* read-only */
> + __u16 allocated_size; /* the allocated size of data below (RO) */
There is a (6-byte?) hole here.
> + struct hid_device *hdev; /* read-only */
> +
> + __u16 size; /* used size in data (RW) */
> + __u8 data[]; /* data buffer (RW) */
> +};
Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
from vmlinuxh?
[...]
> +
> +static bool hid_is_valid_access(int off, int size,
> + enum bpf_access_type access_type,
> + const struct bpf_prog *prog,
> + struct bpf_insn_access_aux *info)
> +{
> + /* everything not in ctx is prohibited */
> + if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE)
> + return false;
Mabe add the following here to fail unaligned accesses
if (off % size != 0)
return false;
[...]
On Sat, Mar 5, 2022 at 12:12 AM Song Liu <[email protected]> wrote:
>
> On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Benjamin Tissoires <[email protected]>
> >
> > ---
> >
> > new in v2
> > ---
> > kernel/bpf/syscall.c | 14 +++++++++++++-
> > 1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index db402ebc5570..cc570891322b 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,17 @@ 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:
> > + case BPF_PROG_TYPE_EXT: /* extends any prog */
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
>
> I am not sure whether we should do this. This is a behavior change, that may
> break some user space. Also, BPF_PROG_TYPE_EXT is checked in
> is_perfmon_prog_type(), and this change will make that case useless.
Sure, I can drop it from v3 and make this function appear for HID only.
Regarding BPF_PROG_TYPE_EXT, it was already in both
is_net_admin_prog_type() and is_perfmon_prog_type(), so I duplicated
it here, but I agree, given that it's already in the first function
there, CPA_SYS_ADMIN is already checked.
Cheers,
Benjamin
>
> Thanks,
> Song
>
> [...]
>
On Sat, Mar 05, 2022 at 11:07:04AM +0100, Benjamin Tissoires wrote:
> On Sat, Mar 5, 2022 at 12:12 AM Song Liu <[email protected]> wrote:
> >
> > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires
> > <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Benjamin Tissoires <[email protected]>
> > >
> > > ---
> > >
> > > new in v2
> > > ---
> > > kernel/bpf/syscall.c | 14 +++++++++++++-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index db402ebc5570..cc570891322b 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,17 @@ 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:
> > > + case BPF_PROG_TYPE_EXT: /* extends any prog */
> > > + return true;
> > > + default:
> > > + return false;
> > > + }
> > > +}
> >
> > I am not sure whether we should do this. This is a behavior change, that may
> > break some user space. Also, BPF_PROG_TYPE_EXT is checked in
> > is_perfmon_prog_type(), and this change will make that case useless.
>
> Sure, I can drop it from v3 and make this function appear for HID only.
For BPF_PROG_TYPE_LIRC_MODE2, I don't think this change will break userspace.
This is called from ir-keytable(1) which is called from udev. It should have
all the necessary permissions.
In addition, the vast majority IR decoders are non-bpf. bpf ir decoders have
very few users at the moment.
I am working on completely new userspace tooling which will make extensive
use of bpf ir decoding with full lircd and IRP compatibility, but this is not
finished yet (see https://github.com/seanyoung/cir).
Thanks
Sean
On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires
<[email protected]> wrote:
> > >
> >
> > The set looks good so far. I will review the rest later.
> >
> > [...]
> >
> > A quick note about how we organize these patches. Maybe we can
> > merge some of these patches like:
>
> Just to be sure we are talking about the same thing: you mean squash
> the patch together?
Right, squash some patches together.
>
> >
> > > bpf: introduce hid program type
> > > bpf/hid: add a new attach type to change the report descriptor
> > > bpf/hid: add new BPF type to trigger commands from userspace
> > I guess the three can merge into one.
> >
> > > HID: hook up with bpf
> > > HID: allow to change the report descriptor from an eBPF program
> > > HID: bpf: compute only the required buffer size for the device
> > > HID: bpf: only call hid_bpf_raw_event() if a ctx is available
> > I haven't read through all of them, but I guess they can probably merge
> > as well.
>
> There are certainly patches that we could squash together (3 and 4
> from this list into the previous ones), but I'd like to keep some sort
> of granularity here to not have a patch bomb that gets harder to come
> back later.
Totally agreed with the granularity of patches. I am not a big fan of patch
bombs either. :)
I guess the problem I have with the current version is that I don't have a
big picture of the design while reading through relatively big patches. A
overview with the following information in the cover letter would be really
help here:
1. How different types of programs are triggered (IRQ, user input, etc.);
2. What are the operations and/or outcomes of these programs;
3. How would programs of different types (or attach types) interact
with each other (via bpf maps? chaining?)
4. What's the new uapi;
5. New helpers and other logistics
Sometimes, I find the changes to uapi are the key for me to understand the
patches, and I would like to see one or two patches with all the UAPI
changes (i.e. bpf_hid_attach_type). However, that may or may not apply to
this set due to granularity concerns.
Does this make sense?
Thanks,
Song
>
> >
> > > libbpf: add HID program type and API
> > > libbpf: add new attach type BPF_HID_RDESC_FIXUP
> > > libbpf: add new attach type BPF_HID_USER_EVENT
> > There 3 can merge, and maybe also the one below
> > > libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
>
> Yeah, the libbpf changes are small enough to not really justify
> separate patches.
>
> >
> > > samples/bpf: add new hid_mouse example
> > > samples/bpf: add a report descriptor fixup
> > > samples/bpf: fix bpf_program__attach_hid() api change
> > Maybe it makes sense to merge these 3?
>
> Sure, why not.
>
> >
> > > bpf/hid: add hid_{get|set}_data helpers
> > > HID: bpf: implement hid_bpf_get|set_data
> > > bpf/hid: add bpf_hid_raw_request helper function
> > > HID: add implementation of bpf_hid_raw_request
> > We can have 1 or 2 patches for these helpers
>
> OK, the patches should be self-contained enough.
>
> >
> > > selftests/bpf: add tests for the HID-bpf initial implementation
> > > selftests/bpf: add report descriptor fixup tests
> > > selftests/bpf: add tests for hid_{get|set}_data helpers
> > > selftests/bpf: add test for user call of HID bpf programs
> > > selftests/bpf: hid: rely on uhid event to know if a test device is
> > > ready
> > > selftests/bpf: add tests for bpf_hid_hw_request
> > > selftests/bpf: Add a test for BPF_F_INSERT_HEAD
> > These selftests could also merge into 1 or 2 patches I guess.
>
> I'd still like to link them to the granularity of the bpf changes, so
> I can refer a selftest change to a specific commit/functionality
> added. But that's just my personal taste, and I can be convinced
> otherwise. This should give us maybe 4 patches instead of 7.
>
> >
> > I understand rearranging these patches may take quite some effort.
> > But I do feel that's a cleaner approach (from someone doesn't know
> > much about HID). If you really hate it that way, we can discuss...
> >
>
> No worries. I don't mind iterating on the series. IIRC I already
> rewrote it twice from scratch, and that's when the selftests I
> introduced in the second rewrite were tremendously helpful :) And
> honestly I don't think it'll be too much effort to reorder/squash the
> patches given that the v2 is *very* granular.
>
> Anyway, I prefer having the reviewers happy so we can have a solid
> rock API from day 1 than keeping it obscure for everyone and having to
> deal with design issues forever. So if it takes 10 or 20 revisions to
> have everybody on the same page, that's fine with me (not that I want
> to have that many revisions, just that I won't be afraid of the
> bikeshedding we might have at some point).
>
> Cheers,
> Benjamin
>
On Sat, Mar 5, 2022 at 1:03 AM Song Liu <[email protected]> wrote:
>
> On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > HID is a protocol that could benefit from using BPF too.
>
> [...]
>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +
> > +struct bpf_prog;
> > +struct bpf_prog_array;
> > +struct hid_device;
> > +
> > +enum bpf_hid_attach_type {
> > + BPF_HID_ATTACH_INVALID = -1,
> > + BPF_HID_ATTACH_DEVICE_EVENT = 0,
> > + MAX_BPF_HID_ATTACH_TYPE
>
> Is it typical to have different BPF programs for different attach types?
> Otherwise, (different types may have similar BPF programs), maybe
> we can pass type as an argument to the program (shared among
> different types)?
Not quite sure I am entirely following you, but I consider the various
attach types to be quite different and thus you can not really reuse
the same BPF program with 2 different attach types.
In my view, we have 4 attach types:
- BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from
the given device (so this is net-like event stream)
- BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and
this is called to change the device capabilities. So you can not reuse
the other programs for this one
- BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace
process owning the program. There we can use functions that are
sleeping (we are not in IRQ context), so this is also fundamentally
different from the 3 others.
- BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into,
we get a bpf program run. This can be suspend/resume, or even specific
request to the device (change a feature on the device or get its
current state). Again, IMO fundamentally different from the others.
So I'm open to any suggestions, but if we can keep the userspace API
being defined with different SEC in libbpf, that would be the best.
>
> [...]
>
> > +struct hid_device;
> > +
> > +enum hid_bpf_event {
> > + HID_BPF_UNDEF = 0,
> > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */
> > +};
> > +
> > +struct hid_bpf_ctx {
> > + enum hid_bpf_event type; /* read-only */
> > + __u16 allocated_size; /* the allocated size of data below (RO) */
>
> There is a (6-byte?) hole here.
>
> > + struct hid_device *hdev; /* read-only */
> > +
> > + __u16 size; /* used size in data (RW) */
> > + __u8 data[]; /* data buffer (RW) */
> > +};
>
> Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
> from vmlinuxh?
I had a thought at this context today, and I think I am getting to the
limit of what I understand.
My first worry is that the way I wrote it there, with a variable data
field length is that this is not forward compatible. Unless BTF and
CORE are making magic, this will bite me in the long run IMO.
But then, you are talking about not using uapi, and I am starting to
wonder: am I doing the things correctly?
To solve my first issue (and the weird API I had to introduce in the
bpf_hid_get/set_data), I came up to the following:
instead of exporting the data directly in the context, I could create
a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a
RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve()
does.
This way, I can directly access the fields within the bpf program
without having to worry about the size.
But now, I am wondering whether the uapi I defined here is correct in
the way CORE works.
My goal is to have HID-BPF programs to be CORE compatible, and not
have to recompile them depending on the underlying kernel.
I can not understand right now if I need to add some other BTF helpers
in the same way the access to struct xdp_md and struct xdp_buff are
converted between one and other, or if defining a forward compatible
struct hid_bpf_ctx is enough.
As far as I understand, .convert_ctx_access allows to export a stable
uapi to the bpf prog users with the verifier doing the conversion
between the structs for me. But is this really required for all the
BPF programs if we want them to be CORE?
Also, I am starting to wonder if I should not hide fields in the
context to the users. The .data field could be a pointer and only
accessed through the helper I mentioned above. This would be forward
compatible, and also allows to use whatever available memory in the
kernel to be forwarded to the BPF program. This way I can skip the
memcpy part and work directly with the incoming dma data buffer from
the IRQ.
But is it best practice to do such a thing?
Cheers,
Benjamin
>
> [...]
>
> > +
> > +static bool hid_is_valid_access(int off, int size,
> > + enum bpf_access_type access_type,
> > + const struct bpf_prog *prog,
> > + struct bpf_insn_access_aux *info)
> > +{
> > + /* everything not in ctx is prohibited */
> > + if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE)
> > + return false;
>
> Mabe add the following here to fail unaligned accesses
>
> if (off % size != 0)
> return false;
> [...]
>
> On Mar 7, 2022, at 5:30 PM, Andrii Nakryiko <[email protected]> wrote:
>
> On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> <[email protected]> wrote:
>>
>> HID-bpf program type are needing a new SEC.
>> To bind a hid-bpf program, we can rely on bpf_program__attach_fd()
>> so export a new function to the API.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>>
>> ---
>>
>> changes in v2:
>> - split the series by bpf/libbpf/hid/selftests and samples
>> ---
>> tools/lib/bpf/libbpf.c | 7 +++++++
>> tools/lib/bpf/libbpf.h | 2 ++
>> tools/lib/bpf/libbpf.map | 1 +
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 81bf01d67671..356bbd3ad2c7 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -8680,6 +8680,7 @@ static const struct bpf_sec_def section_defs[] = {
>> SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
>> SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE),
>> SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
>> + SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
>
> no SEC_SLOPPY_PFX for any new program type, please
>
>
>> };
>>
>> #define MAX_TYPE_NAME_SIZE 32
>> @@ -10659,6 +10660,12 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
>> return bpf_program__attach_iter(prog, NULL);
>> }
>>
>> +struct bpf_link *
>> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd)
>> +{
>> + return bpf_program__attach_fd(prog, hid_fd, 0, "hid");
>> +}
>> +
>> struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
>> {
>> if (!prog->sec_def || !prog->sec_def->attach_fn)
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index c8d8daad212e..f677ac0a9ede 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -529,6 +529,8 @@ struct bpf_iter_attach_opts {
>> LIBBPF_API struct bpf_link *
>> bpf_program__attach_iter(const struct bpf_program *prog,
>> const struct bpf_iter_attach_opts *opts);
>> +LIBBPF_API struct bpf_link *
>> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd);
>>
>> /*
>> * Libbpf allows callers to adjust BPF programs before being loaded
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> index 47e70c9058d9..fdc6fa743953 100644
>> --- a/tools/lib/bpf/libbpf.map
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -424,6 +424,7 @@ LIBBPF_0.6.0 {
>> LIBBPF_0.7.0 {
>> global:
>> bpf_btf_load;
>> + bpf_program__attach_hid;
>
> should go into 0.8.0
Ah, I missed this one.
btw, bpf_xdp_attach and buddies should also go into 0.8.0, no?
>
>> bpf_program__expected_attach_type;
>> bpf_program__log_buf;
>> bpf_program__log_level;
>> --
>> 2.35.1
>>
On Sat, Mar 5, 2022 at 2:07 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Sat, Mar 5, 2022 at 12:12 AM Song Liu <[email protected]> wrote:
> >
> > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires
> > <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Benjamin Tissoires <[email protected]>
> > >
> > > ---
> > >
> > > new in v2
> > > ---
> > > kernel/bpf/syscall.c | 14 +++++++++++++-
> > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index db402ebc5570..cc570891322b 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,17 @@ 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:
> > > + case BPF_PROG_TYPE_EXT: /* extends any prog */
> > > + return true;
> > > + default:
> > > + return false;
> > > + }
> > > +}
> >
> > I am not sure whether we should do this. This is a behavior change, that may
> > break some user space. Also, BPF_PROG_TYPE_EXT is checked in
> > is_perfmon_prog_type(), and this change will make that case useless.
>
> Sure, I can drop it from v3 and make this function appear for HID only.
>
> Regarding BPF_PROG_TYPE_EXT, it was already in both
> is_net_admin_prog_type() and is_perfmon_prog_type(), so I duplicated
> it here, but I agree, given that it's already in the first function
> there, CPA_SYS_ADMIN is already checked.
I think with current code, a user with CAP_BPF, CAP_NET_ADMIN, and
CAP_PERFMON (but not CAP_SYS_ADMIN) can load programs of type
BPF_PROG_TYPE_EXT. But after the patch, the same user will not be
able to do it. Did I misread it? It is not a common case though.
On Mon, Mar 7, 2022 at 5:38 PM Song Liu <[email protected]> wrote:
>
>
>
> > On Mar 7, 2022, at 5:30 PM, Andrii Nakryiko <[email protected]> wrote:
> >
> > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> > <[email protected]> wrote:
> >>
> >> HID-bpf program type are needing a new SEC.
> >> To bind a hid-bpf program, we can rely on bpf_program__attach_fd()
> >> so export a new function to the API.
> >>
> >> Signed-off-by: Benjamin Tissoires <[email protected]>
> >>
> >> ---
> >>
> >> changes in v2:
> >> - split the series by bpf/libbpf/hid/selftests and samples
> >> ---
> >> tools/lib/bpf/libbpf.c | 7 +++++++
> >> tools/lib/bpf/libbpf.h | 2 ++
> >> tools/lib/bpf/libbpf.map | 1 +
> >> 3 files changed, 10 insertions(+)
> >>
> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> >> index 81bf01d67671..356bbd3ad2c7 100644
> >> --- a/tools/lib/bpf/libbpf.c
> >> +++ b/tools/lib/bpf/libbpf.c
> >> @@ -8680,6 +8680,7 @@ static const struct bpf_sec_def section_defs[] = {
> >> SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> >> SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE),
> >> SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> >> + SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
> >
> > no SEC_SLOPPY_PFX for any new program type, please
> >
> >
> >> };
> >>
> >> #define MAX_TYPE_NAME_SIZE 32
> >> @@ -10659,6 +10660,12 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
> >> return bpf_program__attach_iter(prog, NULL);
> >> }
> >>
> >> +struct bpf_link *
> >> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd)
> >> +{
> >> + return bpf_program__attach_fd(prog, hid_fd, 0, "hid");
> >> +}
> >> +
> >> struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
> >> {
> >> if (!prog->sec_def || !prog->sec_def->attach_fn)
> >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> >> index c8d8daad212e..f677ac0a9ede 100644
> >> --- a/tools/lib/bpf/libbpf.h
> >> +++ b/tools/lib/bpf/libbpf.h
> >> @@ -529,6 +529,8 @@ struct bpf_iter_attach_opts {
> >> LIBBPF_API struct bpf_link *
> >> bpf_program__attach_iter(const struct bpf_program *prog,
> >> const struct bpf_iter_attach_opts *opts);
> >> +LIBBPF_API struct bpf_link *
> >> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd);
> >>
> >> /*
> >> * Libbpf allows callers to adjust BPF programs before being loaded
> >> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> >> index 47e70c9058d9..fdc6fa743953 100644
> >> --- a/tools/lib/bpf/libbpf.map
> >> +++ b/tools/lib/bpf/libbpf.map
> >> @@ -424,6 +424,7 @@ LIBBPF_0.6.0 {
> >> LIBBPF_0.7.0 {
> >> global:
> >> bpf_btf_load;
> >> + bpf_program__attach_hid;
> >
> > should go into 0.8.0
>
> Ah, I missed this one.
>
> btw, bpf_xdp_attach and buddies should also go into 0.8.0, no?
not really, they were released in libbpf v0.7, it's just any new
incoming API that should go into 0.8.0
>
> >
> >> bpf_program__expected_attach_type;
> >> bpf_program__log_buf;
> >> bpf_program__log_level;
> >> --
> >> 2.35.1
> >>
>
On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Sat, Mar 5, 2022 at 1:03 AM Song Liu <[email protected]> wrote:
> >
> > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> > <[email protected]> wrote:
> > >
> > > HID is a protocol that could benefit from using BPF too.
> >
> > [...]
> >
> > > +#include <linux/list.h>
> > > +#include <linux/slab.h>
> > > +
> > > +struct bpf_prog;
> > > +struct bpf_prog_array;
> > > +struct hid_device;
> > > +
> > > +enum bpf_hid_attach_type {
> > > + BPF_HID_ATTACH_INVALID = -1,
> > > + BPF_HID_ATTACH_DEVICE_EVENT = 0,
> > > + MAX_BPF_HID_ATTACH_TYPE
> >
> > Is it typical to have different BPF programs for different attach types?
> > Otherwise, (different types may have similar BPF programs), maybe
> > we can pass type as an argument to the program (shared among
> > different types)?
>
> Not quite sure I am entirely following you, but I consider the various
> attach types to be quite different and thus you can not really reuse
> the same BPF program with 2 different attach types.
>
> In my view, we have 4 attach types:
> - BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from
> the given device (so this is net-like event stream)
> - BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and
> this is called to change the device capabilities. So you can not reuse
> the other programs for this one
> - BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace
> process owning the program. There we can use functions that are
> sleeping (we are not in IRQ context), so this is also fundamentally
> different from the 3 others.
> - BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into,
> we get a bpf program run. This can be suspend/resume, or even specific
> request to the device (change a feature on the device or get its
> current state). Again, IMO fundamentally different from the others.
>
> So I'm open to any suggestions, but if we can keep the userspace API
> being defined with different SEC in libbpf, that would be the best.
Thanks for this information. Different attach_types sound right for the use
case.
>
> >
> > [...]
> >
> > > +struct hid_device;
> > > +
> > > +enum hid_bpf_event {
> > > + HID_BPF_UNDEF = 0,
> > > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */
> > > +};
> > > +
> > > +struct hid_bpf_ctx {
> > > + enum hid_bpf_event type; /* read-only */
> > > + __u16 allocated_size; /* the allocated size of data below (RO) */
> >
> > There is a (6-byte?) hole here.
> >
> > > + struct hid_device *hdev; /* read-only */
> > > +
> > > + __u16 size; /* used size in data (RW) */
> > > + __u8 data[]; /* data buffer (RW) */
> > > +};
> >
> > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
> > from vmlinuxh?
>
> I had a thought at this context today, and I think I am getting to the
> limit of what I understand.
>
> My first worry is that the way I wrote it there, with a variable data
> field length is that this is not forward compatible. Unless BTF and
> CORE are making magic, this will bite me in the long run IMO.
>
> But then, you are talking about not using uapi, and I am starting to
> wonder: am I doing the things correctly?
>
> To solve my first issue (and the weird API I had to introduce in the
> bpf_hid_get/set_data), I came up to the following:
> instead of exporting the data directly in the context, I could create
> a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a
> RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve()
> does.
>
> This way, I can directly access the fields within the bpf program
> without having to worry about the size.
>
> But now, I am wondering whether the uapi I defined here is correct in
> the way CORE works.
>
> My goal is to have HID-BPF programs to be CORE compatible, and not
> have to recompile them depending on the underlying kernel.
>
> I can not understand right now if I need to add some other BTF helpers
> in the same way the access to struct xdp_md and struct xdp_buff are
> converted between one and other, or if defining a forward compatible
> struct hid_bpf_ctx is enough.
> As far as I understand, .convert_ctx_access allows to export a stable
> uapi to the bpf prog users with the verifier doing the conversion
> between the structs for me. But is this really required for all the
> BPF programs if we want them to be CORE?
>
> Also, I am starting to wonder if I should not hide fields in the
> context to the users. The .data field could be a pointer and only
> accessed through the helper I mentioned above. This would be forward
> compatible, and also allows to use whatever available memory in the
> kernel to be forwarded to the BPF program. This way I can skip the
> memcpy part and work directly with the incoming dma data buffer from
> the IRQ.
>
> But is it best practice to do such a thing?
I think .convert_ctx_access is the way to go if we want to access the data
buffer without memcpy. I am not sure how much work is needed to make
it compatible with CORE though.
To make sure I understand the case, do we want something like
bpf_prog(struct hid_bpf_ctx *ctx)
{
/* makes sure n < ctx->size */
x = ctx->data[n]; /* read data */
ctx->data[n] = <something>; /* write data */
ctx->size = <something <= n>; /* change data size */
}
We also need it to be CORE, so that we may modify hid_bpf_ctx by
inserting more members to it before data.
Is this accurate?
Song
On Sat, Mar 5, 2022 at 8:58 AM Sean Young <[email protected]> wrote:
>
> On Sat, Mar 05, 2022 at 11:07:04AM +0100, Benjamin Tissoires wrote:
> > On Sat, Mar 5, 2022 at 12:12 AM Song Liu <[email protected]> wrote:
> > >
> > > On Fri, Mar 4, 2022 at 9:30 AM Benjamin Tissoires
> > > <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Benjamin Tissoires <[email protected]>
> > > >
> > > > ---
> > > >
> > > > new in v2
> > > > ---
> > > > kernel/bpf/syscall.c | 14 +++++++++++++-
> > > > 1 file changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > index db402ebc5570..cc570891322b 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,17 @@ 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:
> > > > + case BPF_PROG_TYPE_EXT: /* extends any prog */
> > > > + return true;
> > > > + default:
> > > > + return false;
> > > > + }
> > > > +}
> > >
> > > I am not sure whether we should do this. This is a behavior change, that may
> > > break some user space. Also, BPF_PROG_TYPE_EXT is checked in
> > > is_perfmon_prog_type(), and this change will make that case useless.
> >
> > Sure, I can drop it from v3 and make this function appear for HID only.
>
> For BPF_PROG_TYPE_LIRC_MODE2, I don't think this change will break userspace.
> This is called from ir-keytable(1) which is called from udev. It should have
> all the necessary permissions.
>
> In addition, the vast majority IR decoders are non-bpf. bpf ir decoders have
> very few users at the moment.
>
> I am working on completely new userspace tooling which will make extensive
> use of bpf ir decoding with full lircd and IRP compatibility, but this is not
> finished yet (see https://github.com/seanyoung/cir).
Thanks for these information. I guess change for BPF_PROG_TYPE_LIRC_MODE2
is ok then. Would you mind ack or review this change (either current version or
a later version)?
Thanks,
Song
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
<[email protected]> wrote:
>
> HID-bpf program type are needing a new SEC.
> To bind a hid-bpf program, we can rely on bpf_program__attach_fd()
> so export a new function to the API.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
> ---
>
> changes in v2:
> - split the series by bpf/libbpf/hid/selftests and samples
> ---
> tools/lib/bpf/libbpf.c | 7 +++++++
> tools/lib/bpf/libbpf.h | 2 ++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 10 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 81bf01d67671..356bbd3ad2c7 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8680,6 +8680,7 @@ static const struct bpf_sec_def section_defs[] = {
> SEC_DEF("cgroup/setsockopt", CGROUP_SOCKOPT, BPF_CGROUP_SETSOCKOPT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> SEC_DEF("struct_ops+", STRUCT_OPS, 0, SEC_NONE),
> SEC_DEF("sk_lookup", SK_LOOKUP, BPF_SK_LOOKUP, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> + SEC_DEF("hid/device_event", HID, BPF_HID_DEVICE_EVENT, SEC_ATTACHABLE_OPT | SEC_SLOPPY_PFX),
no SEC_SLOPPY_PFX for any new program type, please
> };
>
> #define MAX_TYPE_NAME_SIZE 32
> @@ -10659,6 +10660,12 @@ static struct bpf_link *attach_iter(const struct bpf_program *prog, long cookie)
> return bpf_program__attach_iter(prog, NULL);
> }
>
> +struct bpf_link *
> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd)
> +{
> + return bpf_program__attach_fd(prog, hid_fd, 0, "hid");
> +}
> +
> struct bpf_link *bpf_program__attach(const struct bpf_program *prog)
> {
> if (!prog->sec_def || !prog->sec_def->attach_fn)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index c8d8daad212e..f677ac0a9ede 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -529,6 +529,8 @@ struct bpf_iter_attach_opts {
> LIBBPF_API struct bpf_link *
> bpf_program__attach_iter(const struct bpf_program *prog,
> const struct bpf_iter_attach_opts *opts);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_hid(const struct bpf_program *prog, int hid_fd);
>
> /*
> * Libbpf allows callers to adjust BPF programs before being loaded
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 47e70c9058d9..fdc6fa743953 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -424,6 +424,7 @@ LIBBPF_0.6.0 {
> LIBBPF_0.7.0 {
> global:
> bpf_btf_load;
> + bpf_program__attach_hid;
should go into 0.8.0
> bpf_program__expected_attach_type;
> bpf_program__log_buf;
> bpf_program__log_level;
> --
> 2.35.1
>
On Mon, Mar 7, 2022 at 7:12 PM Song Liu <[email protected]> wrote:
>
> On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires
> <[email protected]> wrote:
> > > >
> > >
> > > The set looks good so far. I will review the rest later.
> > >
> > > [...]
> > >
> > > A quick note about how we organize these patches. Maybe we can
> > > merge some of these patches like:
> >
> > Just to be sure we are talking about the same thing: you mean squash
> > the patch together?
>
> Right, squash some patches together.
>
> >
> > >
> > > > bpf: introduce hid program type
> > > > bpf/hid: add a new attach type to change the report descriptor
> > > > bpf/hid: add new BPF type to trigger commands from userspace
> > > I guess the three can merge into one.
> > >
> > > > HID: hook up with bpf
> > > > HID: allow to change the report descriptor from an eBPF program
> > > > HID: bpf: compute only the required buffer size for the device
> > > > HID: bpf: only call hid_bpf_raw_event() if a ctx is available
> > > I haven't read through all of them, but I guess they can probably merge
> > > as well.
> >
> > There are certainly patches that we could squash together (3 and 4
> > from this list into the previous ones), but I'd like to keep some sort
> > of granularity here to not have a patch bomb that gets harder to come
> > back later.
>
> Totally agreed with the granularity of patches. I am not a big fan of patch
> bombs either. :)
>
> I guess the problem I have with the current version is that I don't have a
> big picture of the design while reading through relatively big patches. A
> overview with the following information in the cover letter would be really
> help here:
> 1. How different types of programs are triggered (IRQ, user input, etc.);
> 2. What are the operations and/or outcomes of these programs;
> 3. How would programs of different types (or attach types) interact
> with each other (via bpf maps? chaining?)
> 4. What's the new uapi;
> 5. New helpers and other logistics
>
> Sometimes, I find the changes to uapi are the key for me to understand the
> patches, and I would like to see one or two patches with all the UAPI
> changes (i.e. bpf_hid_attach_type). However, that may or may not apply to
> this set due to granularity concerns.
>
> Does this make sense?
>
It definitely does. And as I read that, I realized that if I manage to
get such a clear depiction of what HID-BPF is, it would certainly be a
good idea to paste that in a file into the Documentation directory as
well :)
I think you have a slightly better picture now with the exchanges we
are having on the individual patches, but I'll try to come out with
that description in the cover letter for v3.
Cheers,
Benjamin
On Tue, Mar 8, 2022 at 1:57 AM Song Liu <[email protected]> wrote:
>
> On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > On Sat, Mar 5, 2022 at 1:03 AM Song Liu <[email protected]> wrote:
> > >
> > > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> > > <[email protected]> wrote:
> > > >
> > > > HID is a protocol that could benefit from using BPF too.
> > >
> > > [...]
> > >
> > > > +#include <linux/list.h>
> > > > +#include <linux/slab.h>
> > > > +
> > > > +struct bpf_prog;
> > > > +struct bpf_prog_array;
> > > > +struct hid_device;
> > > > +
> > > > +enum bpf_hid_attach_type {
> > > > + BPF_HID_ATTACH_INVALID = -1,
> > > > + BPF_HID_ATTACH_DEVICE_EVENT = 0,
> > > > + MAX_BPF_HID_ATTACH_TYPE
> > >
> > > Is it typical to have different BPF programs for different attach types?
> > > Otherwise, (different types may have similar BPF programs), maybe
> > > we can pass type as an argument to the program (shared among
> > > different types)?
> >
> > Not quite sure I am entirely following you, but I consider the various
> > attach types to be quite different and thus you can not really reuse
> > the same BPF program with 2 different attach types.
> >
> > In my view, we have 4 attach types:
> > - BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from
> > the given device (so this is net-like event stream)
> > - BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and
> > this is called to change the device capabilities. So you can not reuse
> > the other programs for this one
> > - BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace
> > process owning the program. There we can use functions that are
> > sleeping (we are not in IRQ context), so this is also fundamentally
> > different from the 3 others.
> > - BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into,
> > we get a bpf program run. This can be suspend/resume, or even specific
> > request to the device (change a feature on the device or get its
> > current state). Again, IMO fundamentally different from the others.
> >
> > So I'm open to any suggestions, but if we can keep the userspace API
> > being defined with different SEC in libbpf, that would be the best.
>
> Thanks for this information. Different attach_types sound right for the use
> case.
>
> >
> > >
> > > [...]
> > >
> > > > +struct hid_device;
> > > > +
> > > > +enum hid_bpf_event {
> > > > + HID_BPF_UNDEF = 0,
> > > > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */
> > > > +};
> > > > +
> > > > +struct hid_bpf_ctx {
> > > > + enum hid_bpf_event type; /* read-only */
> > > > + __u16 allocated_size; /* the allocated size of data below (RO) */
> > >
> > > There is a (6-byte?) hole here.
> > >
> > > > + struct hid_device *hdev; /* read-only */
> > > > +
> > > > + __u16 size; /* used size in data (RW) */
> > > > + __u8 data[]; /* data buffer (RW) */
> > > > +};
> > >
> > > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
> > > from vmlinuxh?
> >
> > I had a thought at this context today, and I think I am getting to the
> > limit of what I understand.
> >
> > My first worry is that the way I wrote it there, with a variable data
> > field length is that this is not forward compatible. Unless BTF and
> > CORE are making magic, this will bite me in the long run IMO.
> >
> > But then, you are talking about not using uapi, and I am starting to
> > wonder: am I doing the things correctly?
> >
> > To solve my first issue (and the weird API I had to introduce in the
> > bpf_hid_get/set_data), I came up to the following:
> > instead of exporting the data directly in the context, I could create
> > a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a
> > RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve()
> > does.
> >
> > This way, I can directly access the fields within the bpf program
> > without having to worry about the size.
> >
> > But now, I am wondering whether the uapi I defined here is correct in
> > the way CORE works.
> >
> > My goal is to have HID-BPF programs to be CORE compatible, and not
> > have to recompile them depending on the underlying kernel.
> >
> > I can not understand right now if I need to add some other BTF helpers
> > in the same way the access to struct xdp_md and struct xdp_buff are
> > converted between one and other, or if defining a forward compatible
> > struct hid_bpf_ctx is enough.
> > As far as I understand, .convert_ctx_access allows to export a stable
> > uapi to the bpf prog users with the verifier doing the conversion
> > between the structs for me. But is this really required for all the
> > BPF programs if we want them to be CORE?
> >
> > Also, I am starting to wonder if I should not hide fields in the
> > context to the users. The .data field could be a pointer and only
> > accessed through the helper I mentioned above. This would be forward
> > compatible, and also allows to use whatever available memory in the
> > kernel to be forwarded to the BPF program. This way I can skip the
> > memcpy part and work directly with the incoming dma data buffer from
> > the IRQ.
> >
> > But is it best practice to do such a thing?
>
> I think .convert_ctx_access is the way to go if we want to access the data
> buffer without memcpy. I am not sure how much work is needed to make
> it compatible with CORE though.
>
> To make sure I understand the case, do we want something like
>
> bpf_prog(struct hid_bpf_ctx *ctx)
> {
> /* makes sure n < ctx->size */
> x = ctx->data[n]; /* read data */
> ctx->data[n] = <something>; /* write data */
> ctx->size = <something <= n>; /* change data size */
> }
>
> We also need it to be CORE, so that we may modify hid_bpf_ctx by
> inserting more members to it before data.
>
> Is this accurate?
>
Yes, you pretty much summed it all (except maybe that we might want to
have allocated_size in addition to size so we can also grow the value
of .size within the allocated limit).
All in all, what I want for HID bpf programs is to be able to read and
write an array of bytes, and change its size within an allocated
kernel limit.
This will apply to every HID bpf attach type, to the exception of some
BPF_HID_ATTACH_DRIVER_EVENT when we are receiving a suspend/resume
notification. Though in the suspend/resume case we won't have the data
array available, so it won't matter much.
I want the HID bpf programs to be CORE, but if you tell me that it
would matter only if we need to reshuffle hid_bpf_ctx, I would be fine
simply put a comment "new fields must be added at the end" like some
other definitions of contexts are doing.
Besides that, I currently do not want to allow access to the content
of struct hid_device (or any other kernel struct) in HID BPF programs.
That might be of interest at some point for debugging, but with just
the array capability I should be able to achieve all of my use cases.
Cheers,
Benjamin
On Fri, Mar 04, 2022 at 06:28:25PM +0100, Benjamin Tissoires 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 <[email protected]>
> Signed-off-by: Benjamin Tissoires <[email protected]>
For BPF_PROG_TYPE_LIRC_MODE2, I don't think this change will break userspace.
This is called from ir-keytable(1) which is called from udev. It should have
all the necessary permissions.
In addition, the vast majority IR decoders are non-bpf. bpf ir decoders have
very few users at the moment.
Acked-by: Sean Young <[email protected]>
Sean
>
> ---
>
> new in v2
> ---
> kernel/bpf/syscall.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index db402ebc5570..cc570891322b 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,17 @@ 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:
> + case BPF_PROG_TYPE_EXT: /* extends any prog */
> + 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 +2262,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 Tue, Mar 8, 2022 at 10:20 AM Benjamin Tissoires
<[email protected]> wrote:
>
> On Tue, Mar 8, 2022 at 1:57 AM Song Liu <[email protected]> wrote:
> >
> > On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires
> > <[email protected]> wrote:
> > >
> > > On Sat, Mar 5, 2022 at 1:03 AM Song Liu <[email protected]> wrote:
> > > >
> > > > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
> > > > <[email protected]> wrote:
> > > > >
[...]
> > > > > +struct hid_bpf_ctx {
> > > > > + enum hid_bpf_event type; /* read-only */
> > > > > + __u16 allocated_size; /* the allocated size of data below (RO) */
> > > >
> > > > There is a (6-byte?) hole here.
> > > >
> > > > > + struct hid_device *hdev; /* read-only */
> > > > > +
> > > > > + __u16 size; /* used size in data (RW) */
> > > > > + __u8 data[]; /* data buffer (RW) */
> > > > > +};
> > > >
> > > > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
> > > > from vmlinuxh?
> > >
> > > I had a thought at this context today, and I think I am getting to the
> > > limit of what I understand.
> > >
> > > My first worry is that the way I wrote it there, with a variable data
> > > field length is that this is not forward compatible. Unless BTF and
> > > CORE are making magic, this will bite me in the long run IMO.
> > >
> > > But then, you are talking about not using uapi, and I am starting to
> > > wonder: am I doing the things correctly?
> > >
> > > To solve my first issue (and the weird API I had to introduce in the
> > > bpf_hid_get/set_data), I came up to the following:
> > > instead of exporting the data directly in the context, I could create
> > > a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a
> > > RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve()
> > > does.
> > >
> > > This way, I can directly access the fields within the bpf program
> > > without having to worry about the size.
> > >
> > > But now, I am wondering whether the uapi I defined here is correct in
> > > the way CORE works.
> > >
> > > My goal is to have HID-BPF programs to be CORE compatible, and not
> > > have to recompile them depending on the underlying kernel.
> > >
> > > I can not understand right now if I need to add some other BTF helpers
> > > in the same way the access to struct xdp_md and struct xdp_buff are
> > > converted between one and other, or if defining a forward compatible
> > > struct hid_bpf_ctx is enough.
> > > As far as I understand, .convert_ctx_access allows to export a stable
> > > uapi to the bpf prog users with the verifier doing the conversion
> > > between the structs for me. But is this really required for all the
> > > BPF programs if we want them to be CORE?
> > >
> > > Also, I am starting to wonder if I should not hide fields in the
> > > context to the users. The .data field could be a pointer and only
> > > accessed through the helper I mentioned above. This would be forward
> > > compatible, and also allows to use whatever available memory in the
> > > kernel to be forwarded to the BPF program. This way I can skip the
> > > memcpy part and work directly with the incoming dma data buffer from
> > > the IRQ.
> > >
> > > But is it best practice to do such a thing?
> >
> > I think .convert_ctx_access is the way to go if we want to access the data
> > buffer without memcpy. I am not sure how much work is needed to make
> > it compatible with CORE though.
I spent the week working on that, and I am really amazed at how smart
and simple the .convert_ctx_access is working.
With that in place, I can hide the internal fields and export
"virtual" public fields that are remapped on the fly by the kernel at
load time :)
> >
> > To make sure I understand the case, do we want something like
> >
> > bpf_prog(struct hid_bpf_ctx *ctx)
> > {
> > /* makes sure n < ctx->size */
> > x = ctx->data[n]; /* read data */
> > ctx->data[n] = <something>; /* write data */
> > ctx->size = <something <= n>; /* change data size */
> > }
> >
> > We also need it to be CORE, so that we may modify hid_bpf_ctx by
> > inserting more members to it before data.
> >
> > Is this accurate?
> >
I have been trying to implement that, based on the PTR_TO_PACKET implementation.
It works, but is kind of verbose while using it because we need to
teach the verifier about the size of the arrays.
For instance, I have the following:
int bpf_prog(struct hid_bpf_ctx *ctx)
{
/* we need to store a pointer on the stack to store its accessible size */
__u8 *data = ctx->data;
if (data + 3 > ctx->data_end)
return 0; /* EPERM, bounds check */
data[1] = data[0] + 3;
return 0;
}
This is OK, but it gets worse if I want to access a random offset:
__s64 offset = 0;
int bpf_prog(struct hid_bpf_ctx *ctx)
{
__u8 *data = ctx->data;
__u16 *x;
/* first assign the size of data */
if (data + 4 > ctx->data_end)
return 0; /* EPERM, bounds check */
/* teach the verifier the range of offset (needs to be a s64) */
if (offset >= 0 && offset < 2) {
x = (__u16 *)&data[offset];
/* now store the size of x in its register */
if (x + 2 > ctx->data_end)
return 0;
/* finally we can read/write the data */
*x += 1;
}
return 0;
}
OTOH, I managed to define a simpler helper that allows me to return a
pointer to the internal data:
BPF_CALL_3(bpf_hid_get_data, struct hid_bpf_ctx_kern*, ctx, u64,
offset, u64, size)
{
if (!size)
return (unsigned long)0;
if (offset + size > ctx->data_end - ctx->data)
return (unsigned long)0;
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,
};
Which makes the previous bpf code into:
__u64 offset = 0;
int bpf_prog(struct hid_bpf_ctx *ctx)
{
__u16 *x = bpf_hid_get_data(ctx, offset, 2);
if (!x)
return 0; /* EPERM, bounds check */
*x += 1;
return 0;
}
The advantage of both of those solutions is that they are removing the
need for bpf_hid_{get|set}_bytes().
The second solution is much simpler in terms of usage, but it doesn't
feel "right" to have to reimplement the wheel when we should have
direct array accesses.
OTOH, the first solution with the packet implementation is bulky and
requiring users to do that for every single access of the data is IMO
way too much...
Any thoughts?
Cheers,
Benjamin
Hi Benjamin,
On 04/03/2022 19:28, Benjamin Tissoires wrote:
> Hi,
>
> This is a followup of my v1 at [0].
>
> 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 tried to address as many comments as I could and here is the short log
> of changes:
>
> 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
I posted a couple of comments to the series, but other than that for the
whole series you can use:
Reviewed-by: Tero Kristo <[email protected]>
Tested-by: Tero Kristo <[email protected]>
I did test this with my USI-BPF program + userspace code, they work with
few minor updates compared to previous version.
-Tero
>
>
>
> [0] https://lore.kernel.org/linux-input/[email protected]/T/#t
>
>
> Benjamin Tissoires (28):
> bpf: add new is_sys_admin_prog_type() helper
> bpf: introduce hid program type
> HID: hook up with bpf
> libbpf: add HID program type and API
> selftests/bpf: add tests for the HID-bpf initial implementation
> samples/bpf: add new hid_mouse example
> bpf/hid: add a new attach type to change the report descriptor
> HID: allow to change the report descriptor from an eBPF program
> libbpf: add new attach type BPF_HID_RDESC_FIXUP
> selftests/bpf: add report descriptor fixup tests
> samples/bpf: add a report descriptor fixup
> bpf/hid: add hid_{get|set}_data helpers
> HID: bpf: implement hid_bpf_get|set_data
> selftests/bpf: add tests for hid_{get|set}_data helpers
> bpf/hid: add new BPF type to trigger commands from userspace
> libbpf: add new attach type BPF_HID_USER_EVENT
> selftests/bpf: add test for user call of HID bpf programs
> selftests/bpf: hid: rely on uhid event to know if a test device is
> ready
> bpf/hid: add bpf_hid_raw_request helper function
> HID: add implementation of bpf_hid_raw_request
> selftests/bpf: add tests for bpf_hid_hw_request
> bpf/verifier: prevent non GPL programs to be loaded against HID
> HID: bpf: compute only the required buffer size for the device
> HID: bpf: only call hid_bpf_raw_event() if a ctx is available
> bpf/hid: Add a flag to add the program at the beginning of the list
> libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
> selftests/bpf: Add a test for BPF_F_INSERT_HEAD
> samples/bpf: fix bpf_program__attach_hid() api change
>
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-bpf.c | 361 +++++++++
> drivers/hid/hid-core.c | 34 +-
> include/linux/bpf-hid.h | 129 +++
> include/linux/bpf_types.h | 4 +
> include/linux/hid.h | 25 +
> include/uapi/linux/bpf.h | 59 ++
> include/uapi/linux/bpf_hid.h | 50 ++
> kernel/bpf/Makefile | 3 +
> kernel/bpf/hid.c | 652 +++++++++++++++
> kernel/bpf/syscall.c | 26 +-
> kernel/bpf/verifier.c | 7 +
> samples/bpf/.gitignore | 1 +
> samples/bpf/Makefile | 4 +
> samples/bpf/hid_mouse_kern.c | 91 +++
> samples/bpf/hid_mouse_user.c | 129 +++
> tools/include/uapi/linux/bpf.h | 59 ++
> tools/lib/bpf/libbpf.c | 22 +-
> tools/lib/bpf/libbpf.h | 2 +
> tools/lib/bpf/libbpf.map | 1 +
> tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++
> tools/testing/selftests/bpf/progs/hid.c | 216 +++++
> 22 files changed, 2649 insertions(+), 15 deletions(-)
> 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 Benjamin,
On 04/03/2022 19:28, Benjamin Tissoires wrote:
> Now that BPF can be compatible with HID, add the capability into HID.
> drivers/hid/hid-bpf.c takes care of the glue between bpf and HID, and
> hid-core can then inject any incoming event from the device into a BPF
> program to filter/analyze it.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
>
> ---
>
> changes in v2:
> - split the series by bpf/libbpf/hid/selftests and samples
> - addressed review comments from v1
> ---
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-bpf.c | 157 +++++++++++++++++++++++++++++++++++++++++
> drivers/hid/hid-core.c | 21 +++++-
> include/linux/hid.h | 11 +++
> 4 files changed, 187 insertions(+), 3 deletions(-)
> create mode 100644 drivers/hid/hid-bpf.c
>
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 6d3e630e81af..08d2d7619937 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -4,6 +4,7 @@
> #
> hid-y := hid-core.o hid-input.o hid-quirks.o
> hid-$(CONFIG_DEBUG_FS) += hid-debug.o
> +hid-$(CONFIG_BPF) += hid-bpf.o
>
> obj-$(CONFIG_HID) += hid.o
> obj-$(CONFIG_UHID) += uhid.o
> diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c
> new file mode 100644
> index 000000000000..8120e598de9f
> --- /dev/null
> +++ b/drivers/hid/hid-bpf.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * BPF in HID support for Linux
> + *
> + * Copyright (c) 2022 Benjamin Tissoires
> + */
> +
> +#include <linux/filter.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include <uapi/linux/bpf_hid.h>
> +#include <linux/hid.h>
> +
> +static int __hid_bpf_match_sysfs(struct device *dev, const void *data)
> +{
> + struct kernfs_node *kn = dev->kobj.sd;
> + struct kernfs_node *uevent_kn;
> +
> + uevent_kn = kernfs_find_and_get_ns(kn, "uevent", NULL);
> +
> + return uevent_kn == data;
> +}
> +
> +static struct hid_device *hid_bpf_fd_to_hdev(int fd)
> +{
> + struct device *dev;
> + struct hid_device *hdev;
> + struct fd f = fdget(fd);
> + struct inode *inode;
> + struct kernfs_node *node;
> +
> + if (!f.file) {
> + hdev = ERR_PTR(-EBADF);
> + goto out;
> + }
> +
> + inode = file_inode(f.file);
> + node = inode->i_private;
> +
> + dev = bus_find_device(&hid_bus_type, NULL, node, __hid_bpf_match_sysfs);
> +
> + if (dev)
> + hdev = to_hid_device(dev);
> + else
> + hdev = ERR_PTR(-EINVAL);
> +
> + out:
> + fdput(f);
> + return hdev;
> +}
> +
> +static int hid_bpf_link_attach(struct hid_device *hdev, enum bpf_hid_attach_type type)
> +{
> + int err = 0;
> +
> + switch (type) {
> + case BPF_HID_ATTACH_DEVICE_EVENT:
> + if (!hdev->bpf.ctx) {
> + hdev->bpf.ctx = bpf_hid_allocate_ctx(hdev, HID_BPF_MAX_BUFFER_SIZE);
> + if (IS_ERR(hdev->bpf.ctx)) {
> + err = PTR_ERR(hdev->bpf.ctx);
> + hdev->bpf.ctx = NULL;
> + }
> + }
> + break;
> + default:
> + /* do nothing */
These cause following error:
CC drivers/hid/hid-bpf.o
drivers/hid/hid-bpf.c: In function ‘hid_bpf_link_attach’:
drivers/hid/hid-bpf.c:88:2: error: label at end of compound statement
88 | default:
| ^~~~~~~
drivers/hid/hid-bpf.c: In function ‘hid_bpf_link_attached’:
drivers/hid/hid-bpf.c:101:2: error: label at end of compound statement
101 | default:
| ^~~~~~~
drivers/hid/hid-bpf.c: In function ‘hid_bpf_array_detached’:
drivers/hid/hid-bpf.c:116:2: error: label at end of compound statement
116 | default:
| ^~~~~~~
make[2]: *** [scripts/Makefile.build:288: drivers/hid/hid-bpf.o] Error 1
make[1]: *** [scripts/Makefile.build:550: drivers/hid] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1831: drivers] Error 2
To fix that, you need to add a break statement at end:
default:
/* do nothing */
break;
Same for couple of other occurrences in the file.
-Tero