Re: [PATCH bpf-next v2 02/28] bpf: introduce hid program type

From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: 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>,
	Tero Kristo <tero.kristo@linux.intel.com>,
	lkml <linux-kernel@vger.kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	linux-kselftest@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 02/28] bpf: introduce hid program type
Date: Mon, 7 Mar 2022 18:57:53 +0100	[thread overview]
Message-ID: <CAO-hwJ+oF+WtsQziHBJ6ZESG+C+zw71Hje+SYa8Zn-V4nPZDMQ@mail.gmail.com> (raw)
In-Reply-To: <YiJYlfIywoG5yIMd@kroah.com>

On Fri, Mar 4, 2022 at 7:21 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Mar 04, 2022 at 06:28:26PM +0100, Benjamin Tissoires 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 can be 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 here is only focusing on the bpf changes. The HID
> > changes that hooks onto this are coming in a separate patch.
> >
> > Given that HID can be compiled in as a module, and 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>
> >
> > ---
> >
> > changes in v2:
> > - split the series by bpf/libbpf/hid/selftests and samples
> > - unsigned long -> __u16 in uapi/linux/bpf_hid.h
> > - change the bpf_ctx to be of variable size, with a min of 1024 bytes
> > - make this 1 kB available directly from bpf program, the rest will
> >   need a helper
> > - add some more doc comments in uapi
> > ---
> >  include/linux/bpf-hid.h        | 108 ++++++++
> >  include/linux/bpf_types.h      |   4 +
> >  include/linux/hid.h            |   5 +
> >  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 +
> >  tools/include/uapi/linux/bpf.h |   7 +
> >  9 files changed, 618 insertions(+)
> >  create mode 100644 include/linux/bpf-hid.h
> >  create mode 100644 include/uapi/linux/bpf_hid.h
> >  create mode 100644 kernel/bpf/hid.c
> >
> > diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
> > new file mode 100644
> > index 000000000000..3cda78051b5f
> > --- /dev/null
> > +++ b/include/linux/bpf-hid.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _BPF_HID_H
> > +#define _BPF_HID_H
> > +
> > +#include <linux/mutex.h>
> > +#include <uapi/linux/bpf.h>
> > +#include <uapi/linux/bpf_hid.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +
> > +struct bpf_prog;
> > +struct bpf_prog_array;
> > +struct hid_device;
> > +
> > +enum bpf_hid_attach_type {
> > +     BPF_HID_ATTACH_INVALID = -1,
> > +     BPF_HID_ATTACH_DEVICE_EVENT = 0,
> > +     MAX_BPF_HID_ATTACH_TYPE
> > +};
> > +
> > +struct bpf_hid {
> > +     struct hid_bpf_ctx *ctx;
> > +
> > +     /* Array of programs to run compiled from links */
> > +     struct bpf_prog_array __rcu *run_array[MAX_BPF_HID_ATTACH_TYPE];
> > +     struct list_head links[MAX_BPF_HID_ATTACH_TYPE];
> > +};
> > +
> > +static inline enum bpf_hid_attach_type
> > +to_bpf_hid_attach_type(enum bpf_attach_type attach_type)
> > +{
> > +     switch (attach_type) {
> > +     case BPF_HID_DEVICE_EVENT:
> > +             return BPF_HID_ATTACH_DEVICE_EVENT;
> > +     default:
> > +             return BPF_HID_ATTACH_INVALID;
> > +     }
> > +}
> > +
> > +static inline struct hid_bpf_ctx *bpf_hid_allocate_ctx(struct hid_device *hdev,
> > +                                                    size_t data_size)
> > +{
> > +     struct hid_bpf_ctx *ctx;
> > +
> > +     /* ensure data_size is between min and max */
> > +     data_size = clamp_val(data_size,
> > +                           HID_BPF_MIN_BUFFER_SIZE,
> > +                           HID_BPF_MAX_BUFFER_SIZE);
>
> Do you want to return an error if the data size is not within the range?

That was not something I was counting on.
Though I am thinking of not necessarily clamping this value in the end
because I might have found a way to not do the initial memcpy when
running a prog, and so not having to limit the size of the data.

> Otherwise people will just start to use crazy values and you will always
> be limiting them?

The users of this helper are really limited to drivers/hid/hid_pbf.c
and kernel/bpf/hid.c. And they are known in advance and there must be
only one user per attach type.
The only thing where the data might explode is when in used with
SEC(hid/device_event), because we statically allocate one bpf_ctx
based on the device report descriptor.
But if the required size is bigger than HID_BPF_MAX_BUFFER_SIZE, the
device will not probe or at least already logs something in the dmesg
that we are using a too big buffer.

>
> > +
> > +     ctx = kzalloc(sizeof(*ctx) + data_size, GFP_KERNEL);
> > +     if (!ctx)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     ctx->hdev = hdev;
> > +     ctx->allocated_size = data_size;
> > +
> > +     return ctx;
> > +}
>
> And why is this an inline function?  Why not put it in a .c file?

The problem I have here is that the hid module can be loaded as an
external module. So I can not directly use that helper from hid.ko
from kernel/bpf/hid.c (I need it there once for the
SEC(hid/user_event) bprogram attach type).

So the solution would be to have the code in the c part of
kernel/bpf/hid.c and export the function as GPL, but I wanted to have
the minimum of knowledge of HID-BPF internals in that file. So I ended
up using an inline so I can reuse it independently in kernel/bpf/hid.c
and drivers/hid/hid-bpf.c.

>
> > +
> > +union bpf_attr;
> > +struct bpf_prog;
> > +
> > +#if IS_ENABLED(CONFIG_HID)
> > +int bpf_hid_prog_query(const union bpf_attr *attr,
> > +                    union bpf_attr __user *uattr);
> > +int bpf_hid_link_create(const union bpf_attr *attr,
> > +                     struct bpf_prog *prog);
> > +#else
> > +static inline int bpf_hid_prog_query(const union bpf_attr *attr,
> > +                                  union bpf_attr __user *uattr)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int bpf_hid_link_create(const union bpf_attr *attr,
> > +                                   struct bpf_prog *prog)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +#endif
> > +
> > +static inline bool bpf_hid_link_empty(struct bpf_hid *bpf,
> > +                                   enum bpf_hid_attach_type type)
> > +{
> > +     return list_empty(&bpf->links[type]);
> > +}
> > +
> > +struct bpf_hid_hooks {
> > +     struct hid_device *(*hdev_from_fd)(int fd);
> > +     int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > +     void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > +};
> > +
> > +#ifdef CONFIG_BPF
> > +int bpf_hid_init(struct hid_device *hdev);
> > +void bpf_hid_exit(struct hid_device *hdev);
> > +void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks);
> > +#else
> > +static inline int bpf_hid_init(struct hid_device *hdev)
> > +{
> > +     return 0;
> > +}
> > +
> > +static inline void bpf_hid_exit(struct hid_device *hdev) {}
> > +static inline void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks) {}
> > +#endif
> > +
> > +#endif /* _BPF_HID_H */
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index 48a91c51c015..1509862aacc4 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -76,6 +76,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_EXT, bpf_extension,
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
> >              void *, void *)
> >  #endif /* CONFIG_BPF_LSM */
> > +#if IS_ENABLED(CONFIG_HID)
> > +BPF_PROG_TYPE(BPF_PROG_TYPE_HID, hid,
> > +           __u32, u32)
>
> Why the mix of __u32 and u32 here?

This is actually valid. I tracked it down to kernel/bpf/btf.c with:

static union {
          struct bpf_ctx_convert {
  #define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
          prog_ctx_type _id##_prog; \
          kern_ctx_type _id##_kern;
  #include <linux/bpf_types.h>
  #undef BPF_PROG_TYPE
          } *__t;
          /* 't' is written once under lock. Read many times. */
          const struct btf_type *t;
} bpf_ctx_convert;

So prog_ctx_type represents a user API, while kern_ctx_type
represents the kernel counterpart.

That being said, this is plain wrong, because I am not using u32 as
bpf context, but a properly defined struct :o)

So I probably need to amend this to be either "void *, void *)" or
something better (I'll ask Song in my reply to him).

>
> > +#endif
> >  #endif
> >  BPF_PROG_TYPE(BPF_PROG_TYPE_SYSCALL, bpf_syscall,
> >             void *, void *)
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 7487b0586fe6..56f6f4ad45a7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -15,6 +15,7 @@
> >
> >
> >  #include <linux/bitops.h>
> > +#include <linux/bpf-hid.h>
> >  #include <linux/types.h>
> >  #include <linux/slab.h>
> >  #include <linux/list.h>
> > @@ -639,6 +640,10 @@ struct hid_device {                                                      /* device report descriptor */
> >       struct list_head debug_list;
> >       spinlock_t  debug_list_lock;
> >       wait_queue_head_t debug_wait;
> > +
> > +#ifdef CONFIG_BPF
> > +     struct bpf_hid bpf;
> > +#endif
> >  };
> >
> >  #define to_hid_device(pdev) \
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index afe3d0d7f5f2..5978b92cacd3 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/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;
>
> "ino"?  We have lots of letters to spell words out :)

no comments... :)

>
> > +                     __u32 attach_type;
> > +             } hid;
> >       };
> >  } __attribute__((aligned(8)));
> >
> > diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h
> > new file mode 100644
> > index 000000000000..975ca5bd526f
> > --- /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
>
> Did you forget the copyright line on the other .h file above?

oops

>
> > + */
> > +
> > +#ifndef _UAPI__LINUX_BPF_HID_H__
> > +#define _UAPI__LINUX_BPF_HID_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * The first 1024 bytes are available directly in the bpf programs.
> > + * To access the rest of the data (if allocated_size is bigger
> > + * than 1024, you need to use bpf_hid_ helpers.
> > + */
> > +#define HID_BPF_MIN_BUFFER_SIZE              1024
> > +#define HID_BPF_MAX_BUFFER_SIZE              16384           /* in sync with HID_MAX_BUFFER_SIZE */
>
> Can't you just use HID_MAX_BUFFER_SIZE?

Call me dumb, but curiously I could not get my code to compile there.
If I include linux/hid.h, things are getting messy and either the
tools or the kernel itself was not compiling properly (couldn't really
remember what was failing exactly, sorry).

>
> Anyway, all minor stuff, looks good!

Thanks. Not sure I'll keep the bpf_ctx the same after further
thoughts, but I appreciate the review :)

Cheers,
Benjamin

>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>