by Benjamin Tissoires

[permalink] [raw]

Subject: Re: [PATCH hid v11 02/14] HID: initial BPF implementation

On Tue, Oct 25, 2022 at 11:52 PM Alexei Starovoitov
<[email protected]> wrote:
>
> On Tue, Oct 25, 2022 at 11:34:46AM +0200, Benjamin Tissoires wrote:
> > include/linux/hid.h | 5 +
> > include/linux/hid_bpf.h | 102 +++
> > include/uapi/linux/hid_bpf.h | 25 +
> > tools/include/uapi/linux/hid.h | 62 ++
> > tools/include/uapi/linux/hid_bpf.h | 25 +
>
> ...
>
> > +++ b/include/linux/hid_bpf.h
> > @@ -0,0 +1,102 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +
> > +#ifndef __HID_BPF_H
> > +#define __HID_BPF_H
> > +
> > +#include <linux/spinlock.h>
> > +#include <uapi/linux/hid.h>
> > +#include <uapi/linux/hid_bpf.h>
> > +
> > +struct hid_device;
> > +
> > +/*
> > + * The following is the HID BPF API.
> > + *
> > + * It should be treated as UAPI, so extra care is required
> > + * when making change to this file.
> > + */
>
> I thought at the maintainer summit we discussed that it shouldn't be
> treated as uapi. There is no need to draw this line right now.
> If the whole concept turns out to be useful and api is stable
> then promote it.

I'd still like to keep the kfunc API explicitly marked as "danger
zone". I want to let contributors know that changing this part has an
impact on the existing available bpf programs that are out of the tree
(the HID firewall for instance might be out of the tree).

I'll reword it to not mark it as uapi though.

>
> > +++ b/include/uapi/linux/hid_bpf.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +
> > +#ifndef _UAPI_HID_BPF_H
> > +#define _UAPI_HID_BPF_H
> > +
> > +#include <linux/const.h>
> > +#include <linux/hid.h>
> > +
> > +/**
> > + * enum hid_bpf_attach_flags - flags used when attaching a HIF-BPF program
> > + *
> > + * @HID_BPF_FLAG_NONE: no specific flag is used, the kernel choses where to
> > + * insert the program
> > + * @HID_BPF_FLAG_INSERT_HEAD: insert the given program before any other program
> > + * currently attached to the device. This doesn't
> > + * guarantee that this program will always be first
> > + * @HID_BPF_FLAG_MAX: sentinel value, not to be used by the callers
> > + */
> > +enum hid_bpf_attach_flags {
> > + HID_BPF_FLAG_NONE = 0,
> > + HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> > + HID_BPF_FLAG_MAX,
> > +};
> > +
> > +#endif /* _UAPI_HID_BPF_H */
>
> Not sure what is the purpose of this uapi file.
> Since it's enum the progs can get it from vmlinux.h.

Good point. It can easily go into the non uapi hid_bpf.h

>
> > diff --git a/tools/include/uapi/linux/hid.h b/tools/include/uapi/linux/hid.h
> > new file mode 100644
> > index 000000000000..3e63bea3b3e2
> > --- /dev/null
> > +++ b/tools/include/uapi/linux/hid.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> > +/*
> > + * Copyright (c) 1999 Andreas Gal
> > + * Copyright (c) 2000-2001 Vojtech Pavlik
> > + * Copyright (c) 2006-2007 Jiri Kosina
> > + */
> > +#ifndef _UAPI__HID_H
> > +#define _UAPI__HID_H
>
> This is a copy of include/uapi/linux/hid.h ?

Yes it is

> Probably should be a separate commit to make it obvious.
>

I'll need to assess why I needed that. I think it was related to the
selftests, but now that they are in selftests/hid, I can probably have
a special include in the Makefile to not have to duplicate the file at
all.

Thanks for the review :)

Cheers,
Benjamin

2022-10-28 03:43:20

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH hid v11 04/14] HID: bpf jmp table: simplify the logic of cleaning up programs

Hi Benjamin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on hid/master]

url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/Introduce-eBPF-support-for-HID-devices/20221025-173852
base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git master
patch link: https://lore.kernel.org/r/20221025093458.457089-5-benjamin.tissoires%40redhat.com
patch subject: [PATCH hid v11 04/14] HID: bpf jmp table: simplify the logic of cleaning up programs
config: arm64-randconfig-r015-20221027
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/e16299dd3103acbce798add51a7595b8c59ecb9e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Benjamin-Tissoires/Introduce-eBPF-support-for-HID-devices/20221025-173852
git checkout e16299dd3103acbce798add51a7595b8c59ecb9e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/hid/bpf/ fs/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/hid/bpf/hid_bpf_jmp_table.c:288:9: warning: variable 'cnt' set but not used [-Wunused-but-set-variable]
int i, cnt, index = -1, map_fd = -1, err = -EINVAL;
^
1 warning generated.

vim +/cnt +288 drivers/hid/bpf/hid_bpf_jmp_table.c

281
282 /*
283 * Insert the given BPF program represented by its fd in the jmp table.
284 * Returns the index in the jump table or a negative error.
285 */
286 static int hid_bpf_insert_prog(int prog_fd, struct bpf_prog *prog)
287 {
> 288 int i, cnt, index = -1, map_fd = -1, err = -EINVAL;
289
290 /* retrieve a fd of our prog_array map in BPF */
291 map_fd = skel_map_get_fd_by_id(jmp_table.map->id);
292
293 if (map_fd < 0) {
294 err = -EINVAL;
295 goto out;
296 }
297
298 cnt = 0;
299 /* find the first available index in the jmp_table
300 * and count how many time this program has been inserted
301 */
302 for (i = 0; i < HID_BPF_MAX_PROGS; i++) {
303 if (!jmp_table.progs[i] && index < 0) {
304 /* mark the index as used */
305 jmp_table.progs[i] = prog;
306 index = i;
307 __set_bit(i, jmp_table.enabled);
308 cnt++;
309 } else {
310 if (jmp_table.progs[i] == prog)
311 cnt++;
312 }
313 }
314 if (index < 0) {
315 err = -ENOMEM;
316 goto out;
317 }
318
319 /* insert the program in the jump table */
320 err = skel_map_update_elem(map_fd, &index, &prog_fd, 0);
321 if (err)
322 goto out;
323
324 /*
325 * The program has been safely inserted, decrement the reference count
326 * so it doesn't interfere with the number of actual user handles.
327 * This is safe to do because:
328 * - we overrite the put_ptr in the prog fd map
329 * - we also have a cleanup function that monitors when a program gets
330 * released and we manually do the cleanup in the prog fd map
331 */
332 bpf_prog_sub(prog, 1);
333
334 /* return the index */
335 err = index;
336
337 out:
338 if (err < 0)
339 __hid_bpf_do_release_prog(map_fd, index);
340 if (map_fd >= 0)
341 close_fd(map_fd);
342 return err;
343 }
344

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (3.99 kB)
config (171.02 kB)
Download all attachments

2022-10-31 15:57:18

Hi Benjamin,

Tested this on top of 6.1-rc1 against my HID-USI work, appears to be
good. Also tried out the HID selftests, and they pass, so for the whole
series:

Tested-by: Tero Kristo <[email protected]>

On 25/10/2022 12:34, Benjamin Tissoires wrote:
> Hi,
>
> and here comes the v11 of the HID-BPF series.
>
> Again, for a full explanation of HID-BPF, please refer to the last patch
> in this series (14/14).
>
> Now that the bpf-core changes are all merged in v6.1, it is
> time to have that series entirely relying on HID, so it can get
> merged into the HID tree.
>
> Compared to v10, the most notable change is in the selftests and the
> samples: they are now namespaced into hid, not bpf.
>
> This means that HID-BPF has no more conflicts with the bpf tree.
>
> One other interesting change is in patch 4/14. I managed to reduce
> the scope of the embedded bpf programs, by tricking the refcount.
> I am planning on submitting an RFC for embedding those changes in
> bpf core (a map that doesn't increment refcount and a cleanup mechanism)
> but this can come as a later improvement.
> This new kind of maps and mechanisms might also be useful for other
> subsystems.
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (14):
> HID: Kconfig: split HID support and hid-core compilation
> HID: initial BPF implementation
> selftests: add tests for the HID-bpf initial implementation
> HID: bpf jmp table: simplify the logic of cleaning up programs
> HID: bpf: allocate data memory for device_event BPF programs
> selftests/hid: add test to change the report size
> HID: bpf: introduce hid_hw_request()
> selftests/hid: add tests for bpf_hid_hw_request
> HID: bpf: allow to change the report descriptor
> selftests/hid: add report descriptor fixup tests
> selftests/hid: Add a test for BPF_F_INSERT_HEAD
> samples/hid: add new hid BPF example
> samples/hid: add Surface Dial example
> Documentation: add HID-BPF docs
>
> Documentation/hid/hid-bpf.rst | 513 +++++++++++
> Documentation/hid/index.rst | 1 +
> MAINTAINERS | 3 +
> drivers/Makefile | 2 +-
> drivers/hid/Kconfig | 20 +-
> drivers/hid/Makefile | 2 +
> drivers/hid/bpf/Kconfig | 17 +
> drivers/hid/bpf/Makefile | 11 +
> drivers/hid/bpf/entrypoints/Makefile | 93 ++
> drivers/hid/bpf/entrypoints/README | 4 +
> drivers/hid/bpf/entrypoints/entrypoints.bpf.c | 34 +
> .../hid/bpf/entrypoints/entrypoints.lskel.h | 368 ++++++++
> drivers/hid/bpf/hid_bpf_dispatch.c | 526 +++++++++++
> drivers/hid/bpf/hid_bpf_dispatch.h | 28 +
> drivers/hid/bpf/hid_bpf_jmp_table.c | 572 ++++++++++++
> drivers/hid/hid-core.c | 32 +-
> include/linux/hid.h | 5 +
> include/linux/hid_bpf.h | 148 +++
> include/uapi/linux/hid_bpf.h | 25 +
> samples/hid/.gitignore | 8 +
> samples/hid/Makefile | 246 +++++
> samples/hid/Makefile.target | 75 ++
> samples/hid/hid_mouse.bpf.c | 134 +++
> samples/hid/hid_mouse.c | 160 ++++
> samples/hid/hid_surface_dial.bpf.c | 161 ++++
> samples/hid/hid_surface_dial.c | 231 +++++
> tools/include/uapi/linux/hid.h | 62 ++
> tools/include/uapi/linux/hid_bpf.h | 25 +
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/hid/.gitignore | 4 +
> tools/testing/selftests/hid/Makefile | 242 +++++
> tools/testing/selftests/hid/config | 20 +
> tools/testing/selftests/hid/hid_bpf.c | 846 ++++++++++++++++++
> tools/testing/selftests/hid/progs/hid.c | 206 +++++
> 34 files changed, 4815 insertions(+), 10 deletions(-)
> create mode 100644 Documentation/hid/hid-bpf.rst
> create mode 100644 drivers/hid/bpf/Kconfig
> create mode 100644 drivers/hid/bpf/Makefile
> create mode 100644 drivers/hid/bpf/entrypoints/Makefile
> create mode 100644 drivers/hid/bpf/entrypoints/README
> create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.bpf.c
> create mode 100644 drivers/hid/bpf/entrypoints/entrypoints.lskel.h
> create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.c
> create mode 100644 drivers/hid/bpf/hid_bpf_dispatch.h
> create mode 100644 drivers/hid/bpf/hid_bpf_jmp_table.c
> create mode 100644 include/linux/hid_bpf.h
> create mode 100644 include/uapi/linux/hid_bpf.h
> create mode 100644 samples/hid/.gitignore
> create mode 100644 samples/hid/Makefile
> create mode 100644 samples/hid/Makefile.target
> create mode 100644 samples/hid/hid_mouse.bpf.c
> create mode 100644 samples/hid/hid_mouse.c
> create mode 100644 samples/hid/hid_surface_dial.bpf.c
> create mode 100644 samples/hid/hid_surface_dial.c
> create mode 100644 tools/include/uapi/linux/hid.h
> create mode 100644 tools/include/uapi/linux/hid_bpf.h
> create mode 100644 tools/testing/selftests/hid/.gitignore
> create mode 100644 tools/testing/selftests/hid/Makefile
> create mode 100644 tools/testing/selftests/hid/config
> create mode 100644 tools/testing/selftests/hid/hid_bpf.c
> create mode 100644 tools/testing/selftests/hid/progs/hid.c
>