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

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Tero Kristo <tero.kristo@linux.intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	Jiri Kosina <jikos@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@kernel.org>, Shuah Khan <shuah@kernel.org>,
	Dave Marchevsky <davemarchevsky@fb.com>,
	Joe Stringer <joe@cilium.io>,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	netdev@vger.kernel.org, bpf@vger.kernel.org,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 14/28] selftests/bpf: add tests for hid_{get|set}_data helpers
Date: Tue, 15 Mar 2022 18:02:57 +0100	[thread overview]
Message-ID: <CAO-hwJK=S5jpqbScWrkhAWGUaZKtuKiNvspidWfs7xS4tAHjQA@mail.gmail.com> (raw)
In-Reply-To: <e1acd50f-fe67-14a8-846f-66b52e77abc0@linux.intel.com>

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=LH27Udc_VYkbFsrww@mail.gmail.com/