lkml.org 
[lkml]   [2022]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC bpf-next v4 5/7] HID: initial BPF new way implementation
    On Thu, Apr 21, 2022 at 04:07:38PM +0200, Benjamin Tissoires wrote:
    > Declare an entry point that can use fmod_ret BPF programs, and
    > also an API to access and change the incoming data.
    >
    > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
    >
    > ---
    >
    > new-ish in v4:
    > - far from complete, but gives an overview of what we can do now.
    > ---
    > drivers/hid/hid-core.c | 115 ++++++++++++++++++++++++++++++++++++++++
    > include/linux/hid_bpf.h | 29 ++++++++++
    > 2 files changed, 144 insertions(+)
    > create mode 100644 include/linux/hid_bpf.h
    >
    > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
    > index db925794fbe6..ff4e1b47d2fb 100644
    > --- a/drivers/hid/hid-core.c
    > +++ b/drivers/hid/hid-core.c
    > @@ -32,6 +32,9 @@
    > #include <linux/hiddev.h>
    > #include <linux/hid-debug.h>
    > #include <linux/hidraw.h>
    > +#include <linux/btf.h>
    > +#include <linux/btf_ids.h>
    > +#include <linux/hid_bpf.h>
    >
    > #include "hid-ids.h"
    >
    > @@ -2008,6 +2011,99 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
    > }
    > EXPORT_SYMBOL_GPL(hid_report_raw_event);
    >
    > +struct hid_bpf_ctx_kern {
    > + struct hid_device *hdev;
    > + struct hid_bpf_ctx ctx;
    > + u8 *data;
    > + size_t size;
    > +};
    > +
    > +__weak int hid_bpf_device_event(struct hid_bpf_ctx *ctx, s64 type)
    > +{
    > + return 0;
    > +}
    > +ALLOW_ERROR_INJECTION(hid_bpf_device_event, NS_ERRNO);
    > +
    > +noinline __u8 *
    > +hid_bpf_kfunc_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t __sz)
    > +{
    > + struct hid_bpf_ctx_kern *ctx_kern;
    > +
    > + if (!ctx)
    > + return NULL;
    > +
    > + ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
    > +
    > + return ctx_kern->data;
    > +}

    It probably needs to check that "rdonly_buf_sz" or "__sz" constant passed
    by the program is less than what was allocated in hid_bpf_ctx_kern->data.
    Right?

    > +
    > +noinline void
    > +hid_bpf_kfunc_data_release(__u8 *data)
    > +{
    > +}

    Not clear what release() is for.
    hid_bpf_kfunc_get_data() will return PTR_TO_MEM.
    There is no need to release it.

    > +
    > +noinline int
    > +hid_bpf_kfunc_hw_request(struct hid_bpf_ctx *ctx)
    > +{
    > + if (!ctx)
    > + return -EINVAL;
    > +
    > + pr_err("%s test ctx->bus: %04x %s:%d", __func__, ctx->bus, __FILE__, __LINE__);
    > +
    > + return 0;
    > +}
    > +
    > +/*
    > + * The following set contains all functions we agree BPF programs
    > + * can use.
    > + */
    > +BTF_SET_START(hid_bpf_kfunc_ids)
    > +BTF_ID(func, hid_bpf_kfunc_get_data)
    > +BTF_ID(func, hid_bpf_kfunc_data_release)
    > +BTF_ID(func, hid_bpf_kfunc_hw_request)
    > +BTF_SET_END(hid_bpf_kfunc_ids)
    > +
    > +/*
    > + * The following set contains all functions to provide a kernel
    > + * resource to the BPF program.
    > + * We need to add a counterpart release function.
    > + */
    > +BTF_SET_START(hid_bpf_kfunc_acquire_ids)
    > +BTF_ID(func, hid_bpf_kfunc_get_data)
    > +BTF_SET_END(hid_bpf_kfunc_acquire_ids)
    > +
    > +/*
    > + * The following set is the release counterpart of the previous
    > + * function set.
    > + */
    > +BTF_SET_START(hid_bpf_kfunc_release_ids)
    > +BTF_ID(func, hid_bpf_kfunc_data_release)
    > +BTF_SET_END(hid_bpf_kfunc_release_ids)
    > +
    > +/*
    > + * The following set tells which functions are sleepable.
    > + */
    > +BTF_SET_START(hid_bpf_kfunc_sleepable_ids)
    > +BTF_ID(func, hid_bpf_kfunc_hw_request)
    > +BTF_SET_END(hid_bpf_kfunc_sleepable_ids)
    > +
    > +static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
    > + .owner = THIS_MODULE,
    > + .check_set = &hid_bpf_kfunc_ids,
    > + .acquire_set = &hid_bpf_kfunc_acquire_ids,
    > + .release_set = &hid_bpf_kfunc_release_ids,
    > + .ret_null_set = &hid_bpf_kfunc_acquire_ids, /* duplicated to force BPF programs to
    > + * check the validity of the returned pointer
    > + * in acquire function
    > + */
    > + .sleepable_set = &hid_bpf_kfunc_sleepable_ids,
    > +};
    > +
    > +static int __init hid_bpf_init(void)
    > +{
    > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &hid_bpf_kfunc_set);
    > +}
    > +
    > /**
    > * hid_input_report - report data from lower layer (usb, bt...)
    > *
    > @@ -2025,6 +2121,17 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, u32 size, int i
    > struct hid_driver *hdrv;
    > struct hid_report *report;
    > int ret = 0;
    > + struct hid_bpf_ctx_kern ctx_kern = {
    > + .hdev = hid,
    > + .ctx = {
    > + .bus = hid->bus,
    > + .group = hid->group,
    > + .vendor = hid->vendor,
    > + .product = hid->product,
    > + },
    > + .data = data,
    > + .size = size,
    > + };

    I'm not sure why hid_bpf_ctx_kern is needed.
    Just to scope all args into one struct?

    > +/*
    > + * The following is the HID BPF API.
    > + *
    > + * It should be treated as UAPI, so extra care is required
    > + * when making change to this file.
    > + */
    > +
    > +struct hid_bpf_ctx {
    > + __u16 bus; /* BUS ID */
    > + __u16 group; /* Report group */
    > + __u32 vendor; /* Vendor ID */
    > + __u32 product; /* Product ID */
    > + __u32 version; /* HID version */
    > +};

    Your choice of course,
    but not clear why kernel has to populate it with explicit:
    + .bus = hid->bus,
    + .group = hid->group,
    + .vendor = hid->vendor,
    + .product = hid->product,
    and waste cpu cycles on that while bpf program can read all these fields
    on demand when necessary.

    \
     
     \ /
      Last update: 2022-04-26 06:20    [W:4.112 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site