| Message ID | 20220304172852.274126-3-benjamin.tissoires@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce eBPF support for HID devices | expand |
On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > HID is a protocol that could benefit from using BPF too. [...] > +#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 Is it typical to have different BPF programs for different attach types? Otherwise, (different types may have similar BPF programs), maybe we can pass type as an argument to the program (shared among different types)? [...] > +struct hid_device; > + > +enum hid_bpf_event { > + HID_BPF_UNDEF = 0, > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ > +}; > + > +struct hid_bpf_ctx { > + enum hid_bpf_event type; /* read-only */ > + __u16 allocated_size; /* the allocated size of data below (RO) */ There is a (6-byte?) hole here. > + struct hid_device *hdev; /* read-only */ > + > + __u16 size; /* used size in data (RW) */ > + __u8 data[]; /* data buffer (RW) */ > +}; Do we really need hit_bpf_ctx in uapi? Maybe we can just use it from vmlinuxh? [...] > + > +static bool hid_is_valid_access(int off, int size, > + enum bpf_access_type access_type, > + const struct bpf_prog *prog, > + struct bpf_insn_access_aux *info) > +{ > + /* everything not in ctx is prohibited */ > + if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE) > + return false; Mabe add the following here to fail unaligned accesses if (off % size != 0) return false; [...]
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> >
On Sat, Mar 5, 2022 at 1:03 AM Song Liu <song@kernel.org> wrote: > > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > HID is a protocol that could benefit from using BPF too. > > [...] > > > +#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 > > Is it typical to have different BPF programs for different attach types? > Otherwise, (different types may have similar BPF programs), maybe > we can pass type as an argument to the program (shared among > different types)? Not quite sure I am entirely following you, but I consider the various attach types to be quite different and thus you can not really reuse the same BPF program with 2 different attach types. In my view, we have 4 attach types: - BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from the given device (so this is net-like event stream) - BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and this is called to change the device capabilities. So you can not reuse the other programs for this one - BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace process owning the program. There we can use functions that are sleeping (we are not in IRQ context), so this is also fundamentally different from the 3 others. - BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into, we get a bpf program run. This can be suspend/resume, or even specific request to the device (change a feature on the device or get its current state). Again, IMO fundamentally different from the others. So I'm open to any suggestions, but if we can keep the userspace API being defined with different SEC in libbpf, that would be the best. > > [...] > > > +struct hid_device; > > + > > +enum hid_bpf_event { > > + HID_BPF_UNDEF = 0, > > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ > > +}; > > + > > +struct hid_bpf_ctx { > > + enum hid_bpf_event type; /* read-only */ > > + __u16 allocated_size; /* the allocated size of data below (RO) */ > > There is a (6-byte?) hole here. > > > + struct hid_device *hdev; /* read-only */ > > + > > + __u16 size; /* used size in data (RW) */ > > + __u8 data[]; /* data buffer (RW) */ > > +}; > > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it > from vmlinuxh? I had a thought at this context today, and I think I am getting to the limit of what I understand. My first worry is that the way I wrote it there, with a variable data field length is that this is not forward compatible. Unless BTF and CORE are making magic, this will bite me in the long run IMO. But then, you are talking about not using uapi, and I am starting to wonder: am I doing the things correctly? To solve my first issue (and the weird API I had to introduce in the bpf_hid_get/set_data), I came up to the following: instead of exporting the data directly in the context, I could create a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve() does. This way, I can directly access the fields within the bpf program without having to worry about the size. But now, I am wondering whether the uapi I defined here is correct in the way CORE works. My goal is to have HID-BPF programs to be CORE compatible, and not have to recompile them depending on the underlying kernel. I can not understand right now if I need to add some other BTF helpers in the same way the access to struct xdp_md and struct xdp_buff are converted between one and other, or if defining a forward compatible struct hid_bpf_ctx is enough. As far as I understand, .convert_ctx_access allows to export a stable uapi to the bpf prog users with the verifier doing the conversion between the structs for me. But is this really required for all the BPF programs if we want them to be CORE? Also, I am starting to wonder if I should not hide fields in the context to the users. The .data field could be a pointer and only accessed through the helper I mentioned above. This would be forward compatible, and also allows to use whatever available memory in the kernel to be forwarded to the BPF program. This way I can skip the memcpy part and work directly with the incoming dma data buffer from the IRQ. But is it best practice to do such a thing? Cheers, Benjamin > > [...] > > > + > > +static bool hid_is_valid_access(int off, int size, > > + enum bpf_access_type access_type, > > + const struct bpf_prog *prog, > > + struct bpf_insn_access_aux *info) > > +{ > > + /* everything not in ctx is prohibited */ > > + if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE) > > + return false; > > Mabe add the following here to fail unaligned accesses > > if (off % size != 0) > return false; > [...] >
On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Sat, Mar 5, 2022 at 1:03 AM Song Liu <song@kernel.org> wrote: > > > > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > HID is a protocol that could benefit from using BPF too. > > > > [...] > > > > > +#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 > > > > Is it typical to have different BPF programs for different attach types? > > Otherwise, (different types may have similar BPF programs), maybe > > we can pass type as an argument to the program (shared among > > different types)? > > Not quite sure I am entirely following you, but I consider the various > attach types to be quite different and thus you can not really reuse > the same BPF program with 2 different attach types. > > In my view, we have 4 attach types: > - BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from > the given device (so this is net-like event stream) > - BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and > this is called to change the device capabilities. So you can not reuse > the other programs for this one > - BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace > process owning the program. There we can use functions that are > sleeping (we are not in IRQ context), so this is also fundamentally > different from the 3 others. > - BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into, > we get a bpf program run. This can be suspend/resume, or even specific > request to the device (change a feature on the device or get its > current state). Again, IMO fundamentally different from the others. > > So I'm open to any suggestions, but if we can keep the userspace API > being defined with different SEC in libbpf, that would be the best. Thanks for this information. Different attach_types sound right for the use case. > > > > > [...] > > > > > +struct hid_device; > > > + > > > +enum hid_bpf_event { > > > + HID_BPF_UNDEF = 0, > > > + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ > > > +}; > > > + > > > +struct hid_bpf_ctx { > > > + enum hid_bpf_event type; /* read-only */ > > > + __u16 allocated_size; /* the allocated size of data below (RO) */ > > > > There is a (6-byte?) hole here. > > > > > + struct hid_device *hdev; /* read-only */ > > > + > > > + __u16 size; /* used size in data (RW) */ > > > + __u8 data[]; /* data buffer (RW) */ > > > +}; > > > > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it > > from vmlinuxh? > > I had a thought at this context today, and I think I am getting to the > limit of what I understand. > > My first worry is that the way I wrote it there, with a variable data > field length is that this is not forward compatible. Unless BTF and > CORE are making magic, this will bite me in the long run IMO. > > But then, you are talking about not using uapi, and I am starting to > wonder: am I doing the things correctly? > > To solve my first issue (and the weird API I had to introduce in the > bpf_hid_get/set_data), I came up to the following: > instead of exporting the data directly in the context, I could create > a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a > RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve() > does. > > This way, I can directly access the fields within the bpf program > without having to worry about the size. > > But now, I am wondering whether the uapi I defined here is correct in > the way CORE works. > > My goal is to have HID-BPF programs to be CORE compatible, and not > have to recompile them depending on the underlying kernel. > > I can not understand right now if I need to add some other BTF helpers > in the same way the access to struct xdp_md and struct xdp_buff are > converted between one and other, or if defining a forward compatible > struct hid_bpf_ctx is enough. > As far as I understand, .convert_ctx_access allows to export a stable > uapi to the bpf prog users with the verifier doing the conversion > between the structs for me. But is this really required for all the > BPF programs if we want them to be CORE? > > Also, I am starting to wonder if I should not hide fields in the > context to the users. The .data field could be a pointer and only > accessed through the helper I mentioned above. This would be forward > compatible, and also allows to use whatever available memory in the > kernel to be forwarded to the BPF program. This way I can skip the > memcpy part and work directly with the incoming dma data buffer from > the IRQ. > > But is it best practice to do such a thing? I think .convert_ctx_access is the way to go if we want to access the data buffer without memcpy. I am not sure how much work is needed to make it compatible with CORE though. To make sure I understand the case, do we want something like bpf_prog(struct hid_bpf_ctx *ctx) { /* makes sure n < ctx->size */ x = ctx->data[n]; /* read data */ ctx->data[n] = <something>; /* write data */ ctx->size = <something <= n>; /* change data size */ } We also need it to be CORE, so that we may modify hid_bpf_ctx by inserting more members to it before data. Is this accurate? Song
On Tue, Mar 8, 2022 at 10:20 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Mar 8, 2022 at 1:57 AM Song Liu <song@kernel.org> wrote: > > > > On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires > > <benjamin.tissoires@redhat.com> wrote: > > > > > > On Sat, Mar 5, 2022 at 1:03 AM Song Liu <song@kernel.org> wrote: > > > > > > > > On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires > > > > <benjamin.tissoires@redhat.com> wrote: > > > > > [...] > > > > > +struct hid_bpf_ctx { > > > > > + enum hid_bpf_event type; /* read-only */ > > > > > + __u16 allocated_size; /* the allocated size of data below (RO) */ > > > > > > > > There is a (6-byte?) hole here. > > > > > > > > > + struct hid_device *hdev; /* read-only */ > > > > > + > > > > > + __u16 size; /* used size in data (RW) */ > > > > > + __u8 data[]; /* data buffer (RW) */ > > > > > +}; > > > > > > > > Do we really need hit_bpf_ctx in uapi? Maybe we can just use it > > > > from vmlinuxh? > > > > > > I had a thought at this context today, and I think I am getting to the > > > limit of what I understand. > > > > > > My first worry is that the way I wrote it there, with a variable data > > > field length is that this is not forward compatible. Unless BTF and > > > CORE are making magic, this will bite me in the long run IMO. > > > > > > But then, you are talking about not using uapi, and I am starting to > > > wonder: am I doing the things correctly? > > > > > > To solve my first issue (and the weird API I had to introduce in the > > > bpf_hid_get/set_data), I came up to the following: > > > instead of exporting the data directly in the context, I could create > > > a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a > > > RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve() > > > does. > > > > > > This way, I can directly access the fields within the bpf program > > > without having to worry about the size. > > > > > > But now, I am wondering whether the uapi I defined here is correct in > > > the way CORE works. > > > > > > My goal is to have HID-BPF programs to be CORE compatible, and not > > > have to recompile them depending on the underlying kernel. > > > > > > I can not understand right now if I need to add some other BTF helpers > > > in the same way the access to struct xdp_md and struct xdp_buff are > > > converted between one and other, or if defining a forward compatible > > > struct hid_bpf_ctx is enough. > > > As far as I understand, .convert_ctx_access allows to export a stable > > > uapi to the bpf prog users with the verifier doing the conversion > > > between the structs for me. But is this really required for all the > > > BPF programs if we want them to be CORE? > > > > > > Also, I am starting to wonder if I should not hide fields in the > > > context to the users. The .data field could be a pointer and only > > > accessed through the helper I mentioned above. This would be forward > > > compatible, and also allows to use whatever available memory in the > > > kernel to be forwarded to the BPF program. This way I can skip the > > > memcpy part and work directly with the incoming dma data buffer from > > > the IRQ. > > > > > > But is it best practice to do such a thing? > > > > I think .convert_ctx_access is the way to go if we want to access the data > > buffer without memcpy. I am not sure how much work is needed to make > > it compatible with CORE though. I spent the week working on that, and I am really amazed at how smart and simple the .convert_ctx_access is working. With that in place, I can hide the internal fields and export "virtual" public fields that are remapped on the fly by the kernel at load time :) > > > > To make sure I understand the case, do we want something like > > > > bpf_prog(struct hid_bpf_ctx *ctx) > > { > > /* makes sure n < ctx->size */ > > x = ctx->data[n]; /* read data */ > > ctx->data[n] = <something>; /* write data */ > > ctx->size = <something <= n>; /* change data size */ > > } > > > > We also need it to be CORE, so that we may modify hid_bpf_ctx by > > inserting more members to it before data. > > > > Is this accurate? > > I have been trying to implement that, based on the PTR_TO_PACKET implementation. It works, but is kind of verbose while using it because we need to teach the verifier about the size of the arrays. For instance, I have the following: int bpf_prog(struct hid_bpf_ctx *ctx) { /* we need to store a pointer on the stack to store its accessible size */ __u8 *data = ctx->data; if (data + 3 > ctx->data_end) return 0; /* EPERM, bounds check */ data[1] = data[0] + 3; return 0; } This is OK, but it gets worse if I want to access a random offset: __s64 offset = 0; int bpf_prog(struct hid_bpf_ctx *ctx) { __u8 *data = ctx->data; __u16 *x; /* first assign the size of data */ if (data + 4 > ctx->data_end) return 0; /* EPERM, bounds check */ /* teach the verifier the range of offset (needs to be a s64) */ if (offset >= 0 && offset < 2) { x = (__u16 *)&data[offset]; /* now store the size of x in its register */ if (x + 2 > ctx->data_end) return 0; /* finally we can read/write the data */ *x += 1; } return 0; } OTOH, I managed to define a simpler helper that allows me to return a pointer to the internal data: BPF_CALL_3(bpf_hid_get_data, struct hid_bpf_ctx_kern*, ctx, u64, offset, u64, size) { if (!size) return (unsigned long)0; if (offset + size > ctx->data_end - ctx->data) return (unsigned long)0; return (unsigned long)(ctx->data + offset); } static const struct bpf_func_proto bpf_hid_get_data_proto = { .func = bpf_hid_get_data, .gpl_only = true, .ret_type = RET_PTR_TO_ALLOC_MEM_OR_NULL, .arg1_type = ARG_PTR_TO_CTX, .arg2_type = ARG_ANYTHING, .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO, }; Which makes the previous bpf code into: __u64 offset = 0; int bpf_prog(struct hid_bpf_ctx *ctx) { __u16 *x = bpf_hid_get_data(ctx, offset, 2); if (!x) return 0; /* EPERM, bounds check */ *x += 1; return 0; } The advantage of both of those solutions is that they are removing the need for bpf_hid_{get|set}_bytes(). The second solution is much simpler in terms of usage, but it doesn't feel "right" to have to reimplement the wheel when we should have direct array accesses. OTOH, the first solution with the packet implementation is bulky and requiring users to do that for every single access of the data is IMO way too much... Any thoughts? Cheers, Benjamin
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); + + ctx = kzalloc(sizeof(*ctx) + data_size, GFP_KERNEL); + if (!ctx) + return ERR_PTR(-ENOMEM); + + ctx->hdev = hdev; + ctx->allocated_size = data_size; + + return ctx; +} + +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) +#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; + __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 + */ + +#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 */ + +struct hid_device; + +enum hid_bpf_event { + HID_BPF_UNDEF = 0, + HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */ +}; + +struct hid_bpf_ctx { + enum hid_bpf_event type; /* read-only */ + __u16 allocated_size; /* the allocated size of data below (RO) */ + struct hid_device *hdev; /* read-only */ + + __u16 size; /* used size in data (RW) */ + __u8 data[]; /* data buffer (RW) */ +}; + +#endif /* _UAPI__LINUX_BPF_HID_H__ */ + diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile index c1a9be6a4b9f..8d5619d3d7e5 100644 --- a/kernel/bpf/Makefile +++ b/kernel/bpf/Makefile @@ -35,6 +35,9 @@ ifeq ($(CONFIG_BPF_JIT),y) obj-$(CONFIG_BPF_SYSCALL) += bpf_struct_ops.o obj-${CONFIG_BPF_LSM} += bpf_lsm.o endif +ifneq ($(CONFIG_HID),) +obj-$(CONFIG_BPF_SYSCALL) += hid.o +endif obj-$(CONFIG_BPF_PRELOAD) += preload/ obj-$(CONFIG_BPF_SYSCALL) += relo_core.o diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c new file mode 100644 index 000000000000..db7f75a0a812 --- /dev/null +++ b/kernel/bpf/hid.c @@ -0,0 +1,437 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * based on kernel/bpf/net-namespace.c + */ + +#include <linux/bpf.h> +#include <linux/bpf-hid.h> +#include <linux/filter.h> +#include <linux/hid.h> +#include <linux/hidraw.h> + +/* + * Functions to manage BPF programs attached to hid + */ + +struct bpf_hid_link { + struct bpf_link link; + enum bpf_attach_type type; + enum bpf_hid_attach_type hid_type; + + /* Must be accessed with bpf_hid_mutex held. */ + struct hid_device *hdev; + struct list_head node; /* node in list of links attached to hid */ +}; + +/* Protects updates to bpf_hid */ +DEFINE_MUTEX(bpf_hid_mutex); + +static struct bpf_hid_hooks hid_hooks = {0}; + +void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks) +{ + if (hooks) + hid_hooks = *hooks; + else + memset(&hid_hooks, 0, sizeof(hid_hooks)); +} +EXPORT_SYMBOL_GPL(bpf_hid_set_hooks); + +static const struct bpf_func_proto * +hid_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) +{ + switch (func_id) { + default: + return bpf_base_func_proto(func_id); + } +} + +static bool hid_is_valid_access(int off, int size, + enum bpf_access_type access_type, + const struct bpf_prog *prog, + struct bpf_insn_access_aux *info) +{ + /* everything not in ctx is prohibited */ + if (off < 0 || off + size > sizeof(struct hid_bpf_ctx) + HID_BPF_MIN_BUFFER_SIZE) + return false; + + switch (off) { + /* type, allocated_size, hdev are read-only */ + case bpf_ctx_range_till(struct hid_bpf_ctx, type, hdev): + return access_type == BPF_READ; + } + + /* everything else is read/write */ + return true; +} + +const struct bpf_verifier_ops hid_verifier_ops = { + .get_func_proto = hid_func_proto, + .is_valid_access = hid_is_valid_access +}; + +/* Must be called with bpf_hid_mutex held. */ +static void bpf_hid_run_array_detach(struct hid_device *hdev, + enum bpf_hid_attach_type type) +{ + struct bpf_prog_array *run_array; + + run_array = rcu_replace_pointer(hdev->bpf.run_array[type], NULL, + lockdep_is_held(&bpf_hid_mutex)); + bpf_prog_array_free(run_array); + + if (hid_hooks.array_detached) + hid_hooks.array_detached(hdev, type); +} + +static int link_index(struct hid_device *hdev, enum bpf_hid_attach_type type, + struct bpf_hid_link *link) +{ + struct bpf_hid_link *pos; + int i = 0; + + list_for_each_entry(pos, &hdev->bpf.links[type], node) { + if (pos == link) + return i; + i++; + } + return -ENOENT; +} + +static int link_count(struct hid_device *hdev, enum bpf_hid_attach_type type) +{ + struct list_head *pos; + int i = 0; + + list_for_each(pos, &hdev->bpf.links[type]) + i++; + return i; +} + +static void fill_prog_array(struct hid_device *hdev, enum bpf_hid_attach_type type, + struct bpf_prog_array *prog_array) +{ + struct bpf_hid_link *pos; + unsigned int i = 0; + + list_for_each_entry(pos, &hdev->bpf.links[type], node) { + prog_array->items[i].prog = pos->link.prog; + i++; + } +} + +static void bpf_hid_link_release(struct bpf_link *link) +{ + struct bpf_hid_link *hid_link = + container_of(link, struct bpf_hid_link, link); + enum bpf_hid_attach_type type = hid_link->hid_type; + struct bpf_prog_array *old_array, *new_array; + struct hid_device *hdev; + int cnt, idx; + + mutex_lock(&bpf_hid_mutex); + + hdev = hid_link->hdev; + if (!hdev) + goto out_unlock; + + /* Remember link position in case of safe delete */ + idx = link_index(hdev, type, hid_link); + list_del(&hid_link->node); + + cnt = link_count(hdev, type); + if (!cnt) { + bpf_hid_run_array_detach(hdev, type); + goto out_unlock; + } + + old_array = rcu_dereference_protected(hdev->bpf.run_array[type], + lockdep_is_held(&bpf_hid_mutex)); + new_array = bpf_prog_array_alloc(cnt, GFP_KERNEL); + if (!new_array) { + WARN_ON(bpf_prog_array_delete_safe_at(old_array, idx)); + goto out_unlock; + } + fill_prog_array(hdev, type, new_array); + rcu_assign_pointer(hdev->bpf.run_array[type], new_array); + bpf_prog_array_free(old_array); + +out_unlock: + hid_link->hdev = NULL; + mutex_unlock(&bpf_hid_mutex); +} + +static int bpf_hid_link_detach(struct bpf_link *link) +{ + bpf_hid_link_release(link); + return 0; +} + +static void bpf_hid_link_dealloc(struct bpf_link *link) +{ + struct bpf_hid_link *hid_link = + container_of(link, struct bpf_hid_link, link); + + kfree(hid_link); +} + +static int bpf_hid_link_update_prog(struct bpf_link *link, + struct bpf_prog *new_prog, + struct bpf_prog *old_prog) +{ + struct bpf_hid_link *hid_link = + container_of(link, struct bpf_hid_link, link); + enum bpf_hid_attach_type type = hid_link->hid_type; + struct bpf_prog_array *run_array; + struct hid_device *hdev; + int idx, ret; + + if (old_prog && old_prog != link->prog) + return -EPERM; + if (new_prog->type != link->prog->type) + return -EINVAL; + + mutex_lock(&bpf_hid_mutex); + + hdev = hid_link->hdev; + if (!hdev) { + /* hid dying */ + ret = -ENOLINK; + goto out_unlock; + } + + run_array = rcu_dereference_protected(hdev->bpf.run_array[type], + lockdep_is_held(&bpf_hid_mutex)); + idx = link_index(hdev, type, hid_link); + ret = bpf_prog_array_update_at(run_array, idx, new_prog); + if (ret) + goto out_unlock; + + old_prog = xchg(&link->prog, new_prog); + bpf_prog_put(old_prog); + +out_unlock: + mutex_unlock(&bpf_hid_mutex); + return ret; +} + +static int bpf_hid_link_fill_info(const struct bpf_link *link, + struct bpf_link_info *info) +{ + const struct bpf_hid_link *hid_link = + container_of(link, struct bpf_hid_link, link); + int hidraw_ino = -1; + struct hid_device *hdev; + struct hidraw *hidraw; + + mutex_lock(&bpf_hid_mutex); + hdev = hid_link->hdev; + if (hdev && hdev->hidraw) { + hidraw = hdev->hidraw; + hidraw_ino = hidraw->minor; + } + mutex_unlock(&bpf_hid_mutex); + + info->hid.hidraw_ino = hidraw_ino; + info->hid.attach_type = hid_link->type; + return 0; +} + +static void bpf_hid_link_show_fdinfo(const struct bpf_link *link, + struct seq_file *seq) +{ + struct bpf_link_info info = {}; + + bpf_hid_link_fill_info(link, &info); + seq_printf(seq, + "hidraw_ino:\t%u\n" + "attach_type:\t%u\n", + info.hid.hidraw_ino, + info.hid.attach_type); +} + +static const struct bpf_link_ops bpf_hid_link_ops = { + .release = bpf_hid_link_release, + .dealloc = bpf_hid_link_dealloc, + .detach = bpf_hid_link_detach, + .update_prog = bpf_hid_link_update_prog, + .fill_link_info = bpf_hid_link_fill_info, + .show_fdinfo = bpf_hid_link_show_fdinfo, +}; + +/* Must be called with bpf_hid_mutex held. */ +static int __bpf_hid_prog_query(const union bpf_attr *attr, + union bpf_attr __user *uattr, + struct hid_device *hdev, + enum bpf_hid_attach_type type) +{ + __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids); + struct bpf_prog_array *run_array; + u32 prog_cnt = 0, flags = 0; + + run_array = rcu_dereference_protected(hdev->bpf.run_array[type], + lockdep_is_held(&bpf_hid_mutex)); + if (run_array) + prog_cnt = bpf_prog_array_length(run_array); + + if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags))) + return -EFAULT; + if (copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt))) + return -EFAULT; + if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) + return 0; + + return bpf_prog_array_copy_to_user(run_array, prog_ids, + attr->query.prog_cnt); +} + +int bpf_hid_prog_query(const union bpf_attr *attr, + union bpf_attr __user *uattr) +{ + enum bpf_hid_attach_type type; + struct hid_device *hdev; + int ret; + + if (attr->query.query_flags || !hid_hooks.hdev_from_fd) + return -EINVAL; + + type = to_bpf_hid_attach_type(attr->query.attach_type); + if (type < 0) + return -EINVAL; + + hdev = hid_hooks.hdev_from_fd(attr->query.target_fd); + if (IS_ERR(hdev)) + return PTR_ERR(hdev); + + mutex_lock(&bpf_hid_mutex); + ret = __bpf_hid_prog_query(attr, uattr, hdev, type); + mutex_unlock(&bpf_hid_mutex); + + return ret; +} + +static int bpf_hid_max_progs(enum bpf_hid_attach_type type) +{ + switch (type) { + case BPF_HID_ATTACH_DEVICE_EVENT: + return 64; + default: + return 0; + } +} + +static int bpf_hid_link_attach(struct hid_device *hdev, struct bpf_link *link, + enum bpf_hid_attach_type type) +{ + struct bpf_hid_link *hid_link = + container_of(link, struct bpf_hid_link, link); + struct bpf_prog_array *run_array; + int cnt, err = 0; + + mutex_lock(&bpf_hid_mutex); + + cnt = link_count(hdev, type); + if (cnt >= bpf_hid_max_progs(type)) { + err = -E2BIG; + goto out_unlock; + } + + if (hid_hooks.link_attach) { + err = hid_hooks.link_attach(hdev, type); + if (err) + goto out_unlock; + } + + run_array = bpf_prog_array_alloc(cnt + 1, GFP_KERNEL); + if (!run_array) { + err = -ENOMEM; + goto out_unlock; + } + + list_add_tail(&hid_link->node, &hdev->bpf.links[type]); + + fill_prog_array(hdev, type, run_array); + run_array = rcu_replace_pointer(hdev->bpf.run_array[type], run_array, + lockdep_is_held(&bpf_hid_mutex)); + bpf_prog_array_free(run_array); + +out_unlock: + mutex_unlock(&bpf_hid_mutex); + return err; +} + +int bpf_hid_link_create(const union bpf_attr *attr, struct bpf_prog *prog) +{ + enum bpf_hid_attach_type hid_type; + struct bpf_link_primer link_primer; + struct bpf_hid_link *hid_link; + enum bpf_attach_type type; + struct hid_device *hdev; + int err; + + if (attr->link_create.flags || !hid_hooks.hdev_from_fd) + return -EINVAL; + + type = attr->link_create.attach_type; + hid_type = to_bpf_hid_attach_type(type); + if (hid_type < 0) + return -EINVAL; + + hdev = hid_hooks.hdev_from_fd(attr->link_create.target_fd); + if (IS_ERR(hdev)) + return PTR_ERR(hdev); + + hid_link = kzalloc(sizeof(*hid_link), GFP_USER); + if (!hid_link) + return -ENOMEM; + + bpf_link_init(&hid_link->link, BPF_LINK_TYPE_HID, + &bpf_hid_link_ops, prog); + hid_link->hdev = hdev; + hid_link->type = type; + hid_link->hid_type = hid_type; + + err = bpf_link_prime(&hid_link->link, &link_primer); + if (err) { + kfree(hid_link); + return err; + } + + err = bpf_hid_link_attach(hdev, &hid_link->link, hid_type); + if (err) { + bpf_link_cleanup(&link_primer); + return err; + } + + return bpf_link_settle(&link_primer); +} + +const struct bpf_prog_ops hid_prog_ops = { +}; + +int bpf_hid_init(struct hid_device *hdev) +{ + int type; + + for (type = 0; type < MAX_BPF_HID_ATTACH_TYPE; type++) + INIT_LIST_HEAD(&hdev->bpf.links[type]); + + return 0; +} +EXPORT_SYMBOL_GPL(bpf_hid_init); + +void bpf_hid_exit(struct hid_device *hdev) +{ + enum bpf_hid_attach_type type; + struct bpf_hid_link *hid_link; + + mutex_lock(&bpf_hid_mutex); + for (type = 0; type < MAX_BPF_HID_ATTACH_TYPE; type++) { + bpf_hid_run_array_detach(hdev, type); + list_for_each_entry(hid_link, &hdev->bpf.links[type], node) { + hid_link->hdev = NULL; /* auto-detach link */ + } + } + mutex_unlock(&bpf_hid_mutex); +} +EXPORT_SYMBOL_GPL(bpf_hid_exit); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index cc570891322b..a94e78ec3211 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> @@ -2205,6 +2206,7 @@ static bool is_sys_admin_prog_type(enum bpf_prog_type prog_type) { switch (prog_type) { case BPF_PROG_TYPE_LIRC_MODE2: + case BPF_PROG_TYPE_HID: case BPF_PROG_TYPE_EXT: /* extends any prog */ return true; default: @@ -3200,6 +3202,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; } @@ -3343,6 +3347,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; } @@ -4337,6 +4343,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)));
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