Re: [PATCH bpf-next v1 1/6] HID: initial BPF implementation

On Sat, Feb 26, 2022 at 8:27 AM Song Liu song@kernel.org wrote:

On Thu, Feb 24, 2022 at 3:09 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:

...

HID is a protocol that could benefit from using BPF too.

This patch implements a net-like use of BPF capability for HID. Any incoming report coming from the device gets injected into a series of BPF programs that can modify it or even discard it by setting the size in the context to 0.

The kernel/bpf implementation is based on net-namespace.c, with only the bpf_link part kept, there is no real points in keeping the bpf_prog_{attach|detach} API.

The implementation is split into 2 parts:

• the kernel/bpf part which isn't aware of the HID usage, but takes care of handling the BPF links
• the drivers/hid/hid-bpf.c part which knows about HID

Note that HID can be compiled in as a module, and so the functions that kernel/bpf/hid.c needs to call in hid.ko are exported in struct hid_hooks.

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

drivers/hid/Makefile | 1 + drivers/hid/hid-bpf.c | 176 ++++++++ drivers/hid/hid-core.c | 21 +- include/linux/bpf-hid.h | 87 ++++ include/linux/bpf_types.h | 4 + include/linux/hid.h | 16 + include/uapi/linux/bpf.h | 7 + include/uapi/linux/bpf_hid.h | 39 ++ kernel/bpf/Makefile | 3 + kernel/bpf/hid.c | 437 +++++++++++++++++++ kernel/bpf/syscall.c | 8 + samples/bpf/.gitignore | 1 + samples/bpf/Makefile | 4 + samples/bpf/hid_mouse_kern.c | 66 +++ samples/bpf/hid_mouse_user.c | 129 ++++++ tools/include/uapi/linux/bpf.h | 7 + tools/lib/bpf/libbpf.c | 7 + tools/lib/bpf/libbpf.h | 2 + tools/lib/bpf/libbpf.map | 1 + tools/testing/selftests/bpf/prog_tests/hid.c | 318 ++++++++++++++ tools/testing/selftests/bpf/progs/hid.c | 20 + 21 files changed, 1351 insertions(+), 3 deletions(-) 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

Please split kernel changes, libbpf changes, selftests, and sample code into separate patches.

OK, done locally.

...

diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index 6d3e630e81af..08d2d7619937 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -4,6 +4,7 @@ # hid-y := hid-core.o hid-input.o hid-quirks.o hid-$(CONFIG_DEBUG_FS) += hid-debug.o +hid-$(CONFIG_BPF) += hid-bpf.o

obj-$(CONFIG_HID) += hid.o obj-$(CONFIG_UHID) += uhid.o diff --git a/drivers/hid/hid-bpf.c b/drivers/hid/hid-bpf.c new file mode 100644 index 000000000000..6c8445820944 --- /dev/null +++ b/drivers/hid/hid-bpf.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • BPF in HID support for Linux
• • Copyright (c) 2021 Benjamin Tissoires

Maybe 2022?

heh, maybe :)

[...]

...

+static int hid_bpf_run_progs(struct hid_device *hdev, enum bpf_hid_attach_type type,

•                        struct hid_bpf_ctx *ctx, u8 *data, int size)
  

+{

•   enum hid_bpf_event event = HID_BPF_UNDEF;
  

•   if (type < 0 || !ctx)
  

•           return -EINVAL;
  

•   switch (type) {
  

•   case BPF_HID_ATTACH_DEVICE_EVENT:
  

•           event = HID_BPF_DEVICE_EVENT;
  

•           if (size > sizeof(ctx->u.device.data))
  

•                   return -E2BIG;
  

•           break;
  

•   default:
  

•           return -EINVAL;
  

•   }
  

•   if (!hdev->bpf.run_array[type])
  

•           return 0;
  

•   memset(ctx, 0, sizeof(*ctx));
  

•   ctx->hdev = hdev;
  

•   ctx->type = event;
  

•   if (size && data) {
  

•           switch (event) {
  

•           case HID_BPF_DEVICE_EVENT:
  

•                   memcpy(ctx->u.device.data, data, size);
  

•                   ctx->u.device.size = size;
  

•                   break;
  

•           default:
  

•                   /* do nothing */
  

•           }
  

•   }
  

•   BPF_PROG_RUN_ARRAY(hdev->bpf.run_array[type], ctx, bpf_prog_run);
  

I guess we need "return BPF_PROG_RUN_ARRAY(...)"?

ack

...

•   return 0;
  

+}

+u8 *hid_bpf_raw_event(struct hid_device *hdev, u8 *data, int *size) +{

•   int ret;
  

•   if (bpf_hid_link_empty(&hdev->bpf, BPF_HID_ATTACH_DEVICE_EVENT))
  

•           return data;
  

•   ret = hid_bpf_run_progs(hdev, BPF_HID_ATTACH_DEVICE_EVENT,
  

•                           hdev->bpf.ctx, data, *size);
  

•   if (ret)
  

•           return data;
  

shall we return ERR_PTR(ret)?

I initially wanted to have a bpf_program returning something other than 0 being a signal to silently ignore the report. But the API will be more consistent if we simply return an error in the same way we do for bpf.ctx->u.device.size just below.

IOW, will change in v2

...

•   if (!hdev->bpf.ctx->u.device.size)
  

•           return ERR_PTR(-EINVAL);
  

•   *size = hdev->bpf.ctx->u.device.size;
  

•   return hdev->bpf.ctx->u.device.data;
  

+}

[...]

...

diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h new file mode 100644 index 000000000000..243ac45a253f --- /dev/null +++ b/include/uapi/linux/bpf_hid.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */

+/*

• • HID BPF public headers
• • Copyright (c) 2021 Benjamin Tissoires
• */

+#ifndef _UAPI__LINUX_BPF_HID_H__ +#define _UAPI__LINUX_BPF_HID_H__

+#include <linux/types.h>

+#define HID_BPF_MAX_BUFFER_SIZE 16384 /* 16kb */

+struct hid_device;

+enum hid_bpf_event {

•   HID_BPF_UNDEF = 0,
  

•   HID_BPF_DEVICE_EVENT,
  

+};

+/* type is HID_BPF_DEVICE_EVENT */ +struct hid_bpf_ctx_device_event {

•   __u8 data[HID_BPF_MAX_BUFFER_SIZE];
  

16kB sounds pretty big to me, do we usually need that much?

That's painful but it seems so: see commit 6a0eaf5123e0 ("HID: Increase HID maximum report size to 16KB").

I wanted to have a static definition of the buffer, but maybe I could terminate the struct with `_u8 data[]` and dynamically (re-)alloc the buffer depending on the context.

If the verifier doesn't reject that (why would it? given that it should rely on hid_is_valid_access()), I'll implement this in v2.

...

•   unsigned long size;
  

+};

+struct hid_bpf_ctx {

•   enum hid_bpf_event type;
  

•   struct hid_device *hdev;
  

•   union {
  

•           struct hid_bpf_ctx_device_event device;
  

•   } u;
  

+};

+#endif /* _UAPI__LINUX_BPF_HID_H__ */

[...]

...

diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c new file mode 100644 index 000000000000..d3cb952bfc26 --- /dev/null +++ b/kernel/bpf/hid.c

[...]

...

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 9c7a72b65eee..230ca6964a7e 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3,6 +3,7 @@ */ #include <linux/bpf.h> #include <linux/bpf-cgroup.h> +#include <linux/bpf-hid.h> #include <linux/bpf_trace.h> #include <linux/bpf_lirc.h> #include <linux/bpf_verifier.h> @@ -2174,6 +2175,7 @@ static bool is_net_admin_prog_type(enum bpf_prog_type prog_type) case BPF_PROG_TYPE_CGROUP_SYSCTL: case BPF_PROG_TYPE_SOCK_OPS: case BPF_PROG_TYPE_EXT: /* extends any prog */

•   case BPF_PROG_TYPE_HID:
  

Is this net_admin type?

Not really :) I initially copied over from the LIRC2 code, which is something quite similar in terms of abuse of BPF.

Maybe I should add an extra patch before introducing is_sys_admin_prog_type() and move over LIRC2 there before adding HID.

...

            return true;
    case BPF_PROG_TYPE_CGROUP_SKB:
            /* always unpriv */

@@ -3188,6 +3190,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) return BPF_PROG_TYPE_SK_LOOKUP; case BPF_XDP: return BPF_PROG_TYPE_XDP;

•   case BPF_HID_DEVICE_EVENT:
  

•           return BPF_PROG_TYPE_HID;
    default:
            return BPF_PROG_TYPE_UNSPEC;
    }
  

@@ -3331,6 +3335,8 @@ static int bpf_prog_query(const union bpf_attr *attr, case BPF_SK_MSG_VERDICT: case BPF_SK_SKB_VERDICT: return sock_map_bpf_prog_query(attr, uattr);

•   case BPF_HID_DEVICE_EVENT:
  

•           return bpf_hid_prog_query(attr, uattr);
    default:
            return -EINVAL;
    }
  

@@ -4325,6 +4331,8 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr) ret = bpf_perf_link_attach(attr, prog); break; #endif

•   case BPF_PROG_TYPE_HID:
  

•           return bpf_hid_link_create(attr, prog);
    default:
            ret = -EINVAL;
    }
  

[...]

...

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index afe3d0d7f5f2..5978b92cacd3 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -952,6 +952,7 @@ enum bpf_prog_type { BPF_PROG_TYPE_LSM, BPF_PROG_TYPE_SK_LOOKUP, BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */

•   BPF_PROG_TYPE_HID,
  

};

enum bpf_attach_type { @@ -997,6 +998,7 @@ enum bpf_attach_type { BPF_SK_REUSEPORT_SELECT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, BPF_PERF_EVENT,

•   BPF_HID_DEVICE_EVENT,
    __MAX_BPF_ATTACH_TYPE
  

};

@@ -1011,6 +1013,7 @@ enum bpf_link_type { BPF_LINK_TYPE_NETNS = 5, BPF_LINK_TYPE_XDP = 6, BPF_LINK_TYPE_PERF_EVENT = 7,

•   BPF_LINK_TYPE_HID = 8,
  
    MAX_BPF_LINK_TYPE,
  

}; @@ -5870,6 +5873,10 @@ struct bpf_link_info { struct { __u32 ifindex; } xdp;

•           struct  {
  

•                   __s32 hidraw_ino;
  

•                   __u32 attach_type;
  

•           } hid;
    };
  

} __attribute__((aligned(8)));

And thanks for the initial review :)

Cheers, Benjamin