lkml.org 
[lkml]   [2022]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH bpf-next v2 02/28] bpf: introduce hid program type
    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>
    >

    \
     
     \ /
      Last update: 2022-03-07 18:58    [W:2.743 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site