After HID-BPF struct_ops was merged into 6.11-rc0, there are a few mishaps: - the bpf_wq API changed and needs to be updated here - libbpf now auto-attach all the struct_ops it sees in the bpf object, leading to attempting at attaching them multiple times
Fix the selftests but also prevent the same struct_ops to be attached more than once as this enters various locks, confusions, and kernel oopses.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- Benjamin Tissoires (4): selftests/hid: fix bpf_wq new API selftests/hid: disable struct_ops auto-attach HID: bpf: prevent the same struct_ops to be attached more than once selftests/hid: add test for attaching multiple time the same struct_ops
drivers/hid/bpf/hid_bpf_struct_ops.c | 5 +++++ tools/testing/selftests/hid/hid_bpf.c | 26 ++++++++++++++++++++++ tools/testing/selftests/hid/progs/hid.c | 2 +- .../testing/selftests/hid/progs/hid_bpf_helpers.h | 2 +- 4 files changed, 33 insertions(+), 2 deletions(-) --- base-commit: 6e504d2c61244a01226c5100c835e44fb9b85ca8 change-id: 20240723-fix-6-11-bpf-cfa63dcda5bc
Best regards,
Since commit f56f4d541eab ("bpf: helpers: fix bpf_wq_set_callback_impl signature"), the API for bpf_wq changed a bit.
We need to update the selftests/hid code to reflect that or the bpf program will not load.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- tools/testing/selftests/hid/progs/hid.c | 2 +- tools/testing/selftests/hid/progs/hid_bpf_helpers.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c index ee9bbbcf751b..5ecc845ef792 100644 --- a/tools/testing/selftests/hid/progs/hid.c +++ b/tools/testing/selftests/hid/progs/hid.c @@ -455,7 +455,7 @@ struct { __type(value, struct elem); } hmap SEC(".maps");
-static int wq_cb_sleepable(void *map, int *key, struct bpf_wq *work) +static int wq_cb_sleepable(void *map, int *key, void *work) { __u8 buf[9] = {2, 3, 4, 5, 6, 7, 8, 9, 10}; struct hid_bpf_ctx *hid_ctx; diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h index cfe37f491906..e5db897586bb 100644 --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h @@ -114,7 +114,7 @@ extern int hid_bpf_try_input_report(struct hid_bpf_ctx *ctx, extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym; extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym; extern int bpf_wq_set_callback_impl(struct bpf_wq *wq, - int (callback_fn)(void *map, int *key, struct bpf_wq *wq), + int (callback_fn)(void *map, int *key, void *wq), unsigned int flags__k, void *aux__ign) __ksym; #define bpf_wq_set_callback(timer, cb, flags) \ bpf_wq_set_callback_impl(timer, cb, flags, NULL)
Since commit 08ac454e258e ("libbpf: Auto-attach struct_ops BPF maps in BPF skeleton"), libbpf automatically calls bpf_map__attach_struct_ops() on every struct_ops it sees in the bpf object. The problem is that our test bpf object has many of them but only one should be manually loaded at a time, or we end up locking the syscall.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- tools/testing/selftests/hid/hid_bpf.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c index dc0408a831d0..9c935fd0dffc 100644 --- a/tools/testing/selftests/hid/hid_bpf.c +++ b/tools/testing/selftests/hid/hid_bpf.c @@ -532,6 +532,7 @@ static void load_programs(const struct test_program programs[], FIXTURE_DATA(hid_bpf) * self, const FIXTURE_VARIANT(hid_bpf) * variant) { + struct bpf_map *iter_map; int err = -EINVAL;
ASSERT_LE(progs_count, ARRAY_SIZE(self->hid_links)) @@ -564,6 +565,13 @@ static void load_programs(const struct test_program programs[], *ops_hid_id = self->hid_id; }
+ /* we disable the auto-attach feature of all maps because we + * only want the tested one to be manually attached in the next + * call to bpf_map__attach_struct_ops() + */ + bpf_object__for_each_map(iter_map, *self->skel->skeleton->obj) + bpf_map__set_autoattach(iter_map, false); + err = hid__load(self->skel); ASSERT_OK(err) TH_LOG("hid_skel_load failed: %d", err);
If the struct_ops is already attached, we should bail out or we will end up in various locks and pointer issues while unregistering.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- drivers/hid/bpf/hid_bpf_struct_ops.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c index f59cce6e437f..cd696c59ba0f 100644 --- a/drivers/hid/bpf/hid_bpf_struct_ops.c +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c @@ -183,6 +183,10 @@ static int hid_bpf_reg(void *kdata, struct bpf_link *link) struct hid_device *hdev; int count, err = 0;
+ /* prevent multiple attach of the same struct_ops */ + if (ops->hdev) + return -EINVAL; + hdev = hid_get_device(ops->hid_id); if (IS_ERR(hdev)) return PTR_ERR(hdev); @@ -248,6 +252,7 @@ static void hid_bpf_unreg(void *kdata, struct bpf_link *link)
list_del_rcu(&ops->list); synchronize_srcu(&hdev->bpf.srcu); + ops->hdev = NULL;
reconnect = hdev->bpf.rdesc_ops == ops; if (reconnect)
Turns out that we would en up in a bad state if we attempt to attach twice the same HID-BPF struct_ops, so have a test for it.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- tools/testing/selftests/hid/hid_bpf.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c index 9c935fd0dffc..75b7b4ef6cfa 100644 --- a/tools/testing/selftests/hid/hid_bpf.c +++ b/tools/testing/selftests/hid/hid_bpf.c @@ -694,6 +694,24 @@ TEST_F(hid_bpf, subprog_raw_event) ASSERT_EQ(buf[2], 52); }
+/* + * Attach hid_first_event to the given uhid device, + * attempt at re-attaching it, we should not lock and + * return an invalid struct bpf_link + */ +TEST_F(hid_bpf, multiple_attach) +{ + const struct test_program progs[] = { + { .name = "hid_first_event" }, + }; + struct bpf_link *link; + + LOAD_PROGRAMS(progs); + + link = bpf_map__attach_struct_ops(self->skel->maps.first_event); + ASSERT_NULL(link) TH_LOG("unexpected return value when re-attaching the struct_ops"); +} + /* * Ensures that we can attach/detach programs */
On Tue, 23 Jul 2024 18:21:50 +0200, Benjamin Tissoires wrote:
After HID-BPF struct_ops was merged into 6.11-rc0, there are a few mishaps:
- the bpf_wq API changed and needs to be updated here
- libbpf now auto-attach all the struct_ops it sees in the bpf object, leading to attempting at attaching them multiple times
Fix the selftests but also prevent the same struct_ops to be attached more than once as this enters various locks, confusions, and kernel oopses.
[...]
Applied to hid/hid.git (for-6.11/upstream-fixes), thanks!
[1/4] selftests/hid: fix bpf_wq new API https://git.kernel.org/hid/hid/c/ff9fbcafbaf1 [2/4] selftests/hid: disable struct_ops auto-attach https://git.kernel.org/hid/hid/c/f64c1a459339 [3/4] HID: bpf: prevent the same struct_ops to be attached more than once https://git.kernel.org/hid/hid/c/acd34cfc48b3 [4/4] selftests/hid: add test for attaching multiple time the same struct_ops https://git.kernel.org/hid/hid/c/facdbdfe0e62
Cheers,
On Tue, 23 Jul 2024, Benjamin Tissoires wrote:
After HID-BPF struct_ops was merged into 6.11-rc0, there are a few mishaps:
- the bpf_wq API changed and needs to be updated here
- libbpf now auto-attach all the struct_ops it sees in the bpf object, leading to attempting at attaching them multiple times
Fix the selftests but also prevent the same struct_ops to be attached more than once as this enters various locks, confusions, and kernel oopses.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Benjamin Tissoires (4): selftests/hid: fix bpf_wq new API selftests/hid: disable struct_ops auto-attach HID: bpf: prevent the same struct_ops to be attached more than once selftests/hid: add test for attaching multiple time the same struct_ops
Benjamin,
for the series
Acked-by: Jiri Kosina jkosina@suse.com
Let's get this fixed ASAP. Thanks,
linux-kselftest-mirror@lists.linaro.org
-
Benjamin Tissoires -
Jiri Kosina