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

[Tero Kristo <tero.kristo@linux.intel.com>]

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.

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

-Tero


> +	if (!buf)
> +		return -12; /* -ENOMEM */
> +
> +	/* first try read/write with n > 32 */
> +	ret = bpf_hid_get_data(ctx, 0, 64, buf, 8);
> +	if (ret < 0)
> +		goto discard;
> +
> +	/* reinject it */
> +	ret = bpf_hid_set_data(ctx, 24, 64, buf, 8);
> +	if (ret < 0)
> +		goto discard;
> +
> +	/* extract data at bit offset 10 of size 4 (half a byte) */
> +	ret = bpf_hid_get_data(ctx, 10, 4, buf, 8);  /* expected to fail */
> +	if (ret > 0) {
> +		ret = -1;
> +		goto discard;
> +	}
> +
> +	ret = bpf_hid_get_data(ctx, 10, 4, buf, 4);
> +	if (ret < 0)
> +		goto discard;
> +
> +	/* reinject it */
> +	ret = bpf_hid_set_data(ctx, 16, 4, buf, 4);
> +	if (ret < 0)
> +		goto discard;
> +
> +	ret = 0;
> +
> + discard:
> +
> +	bpf_ringbuf_discard(buf, 0);
> +
> +	return ret;
> +}