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

Song Liu
2022-02-26T07:20:27Z

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.

>
> 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?

[...]

> +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(...)"?

> +
> + 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)?

> +
> + 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?

> + 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?

> 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)));
>