Re: [PATCH bpf-next v3 00/17] Introduce eBPF support for HID devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Benjamin,

I tested this iteration of the set, and I faced couple of problems with it.

1) There were some conflicts as I could not figure out the correct kernel commit on which to apply the series on. I applied this on top of last weeks bpf-next (see below) with some local merge fixes.

commit 2af7e566a8616c278e1d7287ce86cd3900bed943 (bpf-next/master, bpf-next/for-next)
Author: Saeed Mahameed <saeedm@xxxxxxxxxx>
Date:   Tue Mar 22 10:22:24 2022 -0700

    net/mlx5e: Fix build warning, detected write beyond size of field

2) hid_is_valid_access() causes some trouble and it rejects pretty much every BPF program which tries to use ctx->retval. This appears to be because prog->expected_attach_type is not populated, I had to apply below local tweak to overcome this problem:

diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c
index 30a62e8e0f0a..bf64411e6e9b 100644
--- a/kernel/bpf/hid.c
+++ b/kernel/bpf/hid.c
@@ -180,8 +180,7 @@ static bool hid_is_valid_access(int off, int size,
        case offsetof(struct hid_bpf_ctx, retval):
                if (size != size_default)
                        return false;
-               return (prog->expected_attach_type == BPF_HID_USER_EVENT ||
-                       prog->expected_attach_type == BPF_HID_DRIVER_EVENT);
+               return true;
        default:
                if (size != size_default)
                        return false;

Proper fix would probably be to actually populate the expected_attach_type, but I could not figure out quickly where this should be done, or whether it is actually done on some other base commit.

With those, for the whole series:

Tested-by: Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx>

On 18/03/2022 18:15, Benjamin Tissoires wrote:
Hi,

This is a followup of my v1 at [0] and v2 at [1].

The short summary of the previous cover letter and discussions is that
HID could benefit from BPF for the following use cases:

- simple fixup of report descriptor:
   benefits are faster development time and testing, with the produced
   bpf program being shipped in the kernel directly (the shipping part
   is *not* addressed here).

- Universal Stylus Interface:
   allows a user-space program to define its own kernel interface

- Surface Dial:
   somehow similar to the previous one except that userspace can decide
   to change the shape of the exported device

- firewall:
   still partly missing there, there is not yet interception of hidraw
   calls, but it's coming in a followup series, I promise

- tracing:
   well, tracing.


I think I addressed the comments from the previous version, but there are
a few things I'd like to note here:

- I did not take the various rev-by and tested-by (thanks a lot for those)
   because the uapi changed significantly in v3, so I am not very confident
   in taking those rev-by blindly

- I mentioned in my discussion with Song that I'll put a summary of the uapi
   in the cover letter, but I ended up adding a (long) file in the Documentation
   directory. So please maybe start by reading 17/17 to have an overview of
   what I want to achieve

- I added in the libbpf and bpf the new type BPF_HID_DRIVER_EVENT, even though
   I don't have a user of it right now in the kernel. I wanted to have them in
   the docs, but we might not want to have them ready here.
   In terms of code, it just means that we can attach such programs types
   but that they will never get triggered.

Anyway, I have been mulling on this for the past 2 weeks, and I think that
maybe sharing this now is better than me just starring at the code over and
over.


Short summary of changes:

v3:
===

- squashed back together most of the libbpf and bpf changes into bigger
   commits that give a better overview of the whole interactions

- reworked the user API to not expose .data as a directly accessible field
   from the context, but instead forces everyone to use hid_bpf_get_data (or
   get/set_bits)

- added BPF_HID_DRIVER_EVENT (see note above)

- addressed the various nitpicks from v2

- added a big Documentation file (and so adding now the doc maintainers to the
   long list of recipients)

v2:
===

- split the series by subsystem (bpf, HID, libbpf, selftests and
   samples)

- Added an extra patch at the beginning to not require CAP_NET_ADMIN for
   BPF_PROG_TYPE_LIRC_MODE2 (please shout if this is wrong)

- made the bpf context attached to HID program of dynamic size:
   * the first 1 kB will be able to be addressed directly
   * the rest can be retrieved through bpf_hid_{set|get}_data
     (note that I am definitivey not happy with that API, because there
     is part of it in bits and other in bytes. ouch)

- added an extra patch to prevent non GPL HID bpf programs to be loaded
   of type BPF_PROG_TYPE_HID
   * same here, not really happy but I don't know where to put that check
     in verifier.c

- added a new flag BPF_F_INSERT_HEAD for BPF_LINK_CREATE syscall when in
   used with HID program types.
   * this flag is used for tracing, to be able to load a program before
     any others that might already have been inserted and that might
     change the data stream.

Cheers,
Benjamin



[0] https://lore.kernel.org/linux-input/20220224110828.2168231-1-benjamin.tissoires@xxxxxxxxxx/T/#t
[1] https://lore.kernel.org/linux-input/20220304172852.274126-1-benjamin.tissoires@xxxxxxxxxx/T/#t


Benjamin Tissoires (17):
   bpf: add new is_sys_admin_prog_type() helper
   bpf: introduce hid program type
   bpf/verifier: prevent non GPL programs to be loaded against HID
   libbpf: add HID program type and API
   HID: hook up with bpf
   HID: allow to change the report descriptor from an eBPF program
   selftests/bpf: add tests for the HID-bpf initial implementation
   selftests/bpf: add report descriptor fixup tests
   selftests/bpf: Add a test for BPF_F_INSERT_HEAD
   selftests/bpf: add test for user call of HID bpf programs
   samples/bpf: add new hid_mouse example
   bpf/hid: add more HID helpers
   HID: bpf: implement hid_bpf_get|set_bits
   HID: add implementation of bpf_hid_raw_request
   selftests/bpf: add tests for hid_{get|set}_bits helpers
   selftests/bpf: add tests for bpf_hid_hw_request
   Documentation: add HID-BPF docs

  Documentation/hid/hid-bpf.rst                | 444 +++++++++++
  Documentation/hid/index.rst                  |   1 +
  drivers/hid/Makefile                         |   1 +
  drivers/hid/hid-bpf.c                        | 328 ++++++++
  drivers/hid/hid-core.c                       |  34 +-
  include/linux/bpf-hid.h                      | 127 +++
  include/linux/bpf_types.h                    |   4 +
  include/linux/hid.h                          |  36 +-
  include/uapi/linux/bpf.h                     |  67 ++
  include/uapi/linux/bpf_hid.h                 |  71 ++
  include/uapi/linux/hid.h                     |  10 +
  kernel/bpf/Makefile                          |   3 +
  kernel/bpf/btf.c                             |   1 +
  kernel/bpf/hid.c                             | 728 +++++++++++++++++
  kernel/bpf/syscall.c                         |  27 +-
  kernel/bpf/verifier.c                        |   7 +
  samples/bpf/.gitignore                       |   1 +
  samples/bpf/Makefile                         |   4 +
  samples/bpf/hid_mouse_kern.c                 | 117 +++
  samples/bpf/hid_mouse_user.c                 | 129 +++
  tools/include/uapi/linux/bpf.h               |  67 ++
  tools/lib/bpf/libbpf.c                       |  23 +-
  tools/lib/bpf/libbpf.h                       |   2 +
  tools/lib/bpf/libbpf.map                     |   1 +
  tools/testing/selftests/bpf/config           |   3 +
  tools/testing/selftests/bpf/prog_tests/hid.c | 788 +++++++++++++++++++
  tools/testing/selftests/bpf/progs/hid.c      | 205 +++++
  27 files changed, 3204 insertions(+), 25 deletions(-)
  create mode 100644 Documentation/hid/hid-bpf.rst
  create mode 100644 drivers/hid/hid-bpf.c
  create mode 100644 include/linux/bpf-hid.h
  create mode 100644 include/uapi/linux/bpf_hid.h
  create mode 100644 kernel/bpf/hid.c
  create mode 100644 samples/bpf/hid_mouse_kern.c
  create mode 100644 samples/bpf/hid_mouse_user.c
  create mode 100644 tools/testing/selftests/bpf/prog_tests/hid.c
  create mode 100644 tools/testing/selftests/bpf/progs/hid.c




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux