[PATCH HID 0/4] HID: selftest fixes after merge into 6.11-rc0 tree

[PATCH HID 0/4] HID: selftest fixes after merge into 6.11-rc0 tree

[Benjamin Tissoires]

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,
-- 
Benjamin Tissoires <bentiss@kernel.org>

[PATCH HID 1/4] selftests/hid: fix bpf_wq new API

[Benjamin Tissoires]

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)

-- 
2.44.0

[PATCH HID 2/4] selftests/hid: disable struct_ops auto-attach

[Benjamin Tissoires]

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);
 

-- 
2.44.0

[PATCH HID 3/4] HID: bpf: prevent the same struct_ops to be attached more than once

[Benjamin Tissoires]

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)

-- 
2.44.0

[PATCH HID 4/4] selftests/hid: add test for attaching multiple time the same struct_ops

[Benjamin Tissoires]

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
  */

-- 
2.44.0

Re: [PATCH HID 0/4] HID: selftest fixes after merge into 6.11-rc0 tree

[Benjamin Tissoires]

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,
-- 
Benjamin Tissoires <bentiss@kernel.org>

Re: [PATCH HID 0/4] HID: selftest fixes after merge into 6.11-rc0 tree

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs