Re: [PATCH bpf-next v2 14/28] selftests/bpf: add tests for hid_{get|set}_data helpers

On Tue, Mar 15, 2022 at 5:51 PM Tero Kristo tero.kristo@linux.intel.com wrote:

Hi Benjamin,

On 04/03/2022 19:28, Benjamin Tissoires wrote:

...

Simple test added here, with one use of each helper.

Signed-off-by: Benjamin Tissoires benjamin.tissoires@redhat.com

---

changes in v2:

• split the patch with libbpf left outside.

---

tools/testing/selftests/bpf/prog_tests/hid.c | 65 ++++++++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 45 ++++++++++++++ 2 files changed, 110 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/hid.c b/tools/testing/selftests/bpf/prog_tests/hid.c index 91543b8078ca..74426523dd6f 100644 --- a/tools/testing/selftests/bpf/prog_tests/hid.c +++ b/tools/testing/selftests/bpf/prog_tests/hid.c @@ -297,6 +297,68 @@ static int test_hid_raw_event(struct hid *hid_skel, int uhid_fd, int sysfs_fd) return ret; }

+/*

• • Attach hid_set_get_data to the given uhid device,
• • retrieve and open the matching hidraw node,
• • inject one event in the uhid device,
• • check that the program makes correct use of bpf_hid_{set|get}_data.
• */

+static int test_hid_set_get_data(struct hid *hid_skel, int uhid_fd, int sysfs_fd) +{

• int err, hidraw_ino, hidraw_fd = -1;
  

• char hidraw_path[64] = {0};
  

• u8 buf[10] = {0};
  

• int ret = -1;
  

• /* attach hid_set_get_data program */
  

• hid_skel->links.hid_set_get_data =
  

•         bpf_program__attach_hid(hid_skel->progs.hid_set_get_data, sysfs_fd);
  

• if (!ASSERT_OK_PTR(hid_skel->links.hid_set_get_data,
  

•                    "attach_hid(hid_set_get_data)"))
  

•         return PTR_ERR(hid_skel->links.hid_set_get_data);
  

• hidraw_ino = get_hidraw(hid_skel->links.hid_set_get_data);
  

• if (!ASSERT_GE(hidraw_ino, 0, "get_hidraw"))
  

•         goto cleanup;
  

• /* open hidraw node to check the other side of the pipe */
  

• sprintf(hidraw_path, "/dev/hidraw%d", hidraw_ino);
  

• hidraw_fd = open(hidraw_path, O_RDWR | O_NONBLOCK);
  

• if (!ASSERT_GE(hidraw_fd, 0, "open_hidraw"))
  

•         goto cleanup;
  

• /* inject one event */
  

• buf[0] = 1;
  

• buf[1] = 42;
  

• send_event(uhid_fd, buf, 6);
  

• /* read the data from hidraw */
  

• memset(buf, 0, sizeof(buf));
  

• err = read(hidraw_fd, buf, sizeof(buf));
  

• if (!ASSERT_EQ(err, 6, "read_hidraw"))
  

•         goto cleanup;
  

• if (!ASSERT_EQ(buf[2], (42 >> 2), "hid_set_get_data"))
  

•         goto cleanup;
  

• if (!ASSERT_EQ(buf[3], 1, "hid_set_get_data"))
  

•         goto cleanup;
  

• if (!ASSERT_EQ(buf[4], 42, "hid_set_get_data"))
  

•         goto cleanup;
  

• ret = 0;
  

+cleanup:

• if (hidraw_fd >= 0)
  

•         close(hidraw_fd);
  

• hid__detach(hid_skel);
  

• return ret;
  

+}

• /*
  • Attach hid_rdesc_fixup to the given uhid device,
  • retrieve and open the matching hidraw node,

@@ -395,6 +457,9 @@ void serial_test_hid_bpf(void) err = test_hid_raw_event(hid_skel, uhid_fd, sysfs_fd); ASSERT_OK(err, "hid");

• err = test_hid_set_get_data(hid_skel, uhid_fd, sysfs_fd);
  

• ASSERT_OK(err, "hid_set_get_data");
  

• err = test_rdesc_fixup(hid_skel, uhid_fd, sysfs_fd);
  ASSERT_OK(err, "hid_rdesc_fixup");
  
  

diff --git a/tools/testing/selftests/bpf/progs/hid.c b/tools/testing/selftests/bpf/progs/hid.c index 2270448d0d3f..de6668471940 100644 --- a/tools/testing/selftests/bpf/progs/hid.c +++ b/tools/testing/selftests/bpf/progs/hid.c @@ -66,3 +66,48 @@ int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)

  return 0;

}

+SEC("hid/device_event") +int hid_set_get_data(struct hid_bpf_ctx *ctx) +{

• int ret;
  

• __u8 *buf;
  

• buf = bpf_ringbuf_reserve(&ringbuf, 8, 0);
  

Ordering of patches is probably wrong, it seems the ringbuf is defined in patch #21 but used here.

Also, this usage of ringbuf leads into running out of available memory in the buffer if used for long time, it is not evident from the test case written here but I spent a couple of hours debugging my own BPF program that used ringbuf in similar way as what is done here. Basically the producer idx is increased with the bpf_ringbuf_reserve / discard, but the consumer index is not if you don't have a consumer in place.

Oh, that's good to know.

FWIW, on v3, we won't have to use a ringbuf at all. This is a new API break (sorry for that), but I think this will be better in the long run. I have chosen to use the hid_bpf_get_data() from [0]. The net advantage is that we get a pointer to the internal buffer that can be of any size, and we can even re-inject that same pointer into bpf_hid_raw_request() without having to use another buffer.

I'm currently working on the documentation part and small fixes now that the big rewrite with that implementation is now done.

Cheers, Benjamin

I ended up using a global statically allocated buffer for the purpose for now.

-Tero

[0] https://lore.kernel.org/linux-input/CAO-hwJJqP5iivQZOu0LTYa1D5OuM_aVi=LH27Ud...