Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220304172852.274126-1-benjamin.tissoires@redhat.com> <20220304172852.274126-3-benjamin.tissoires@redhat.com>
In-Reply-To: <20220304172852.274126-3-benjamin.tissoires@redhat.com>
From:   Song Liu <song@kernel.org>
Date:   Fri, 4 Mar 2022 16:02:49 -0800
Message-ID: <CAPhsuW5CYF9isR4ffRdm3xA_n_FBoL+AGFkzNn4dn2LgRaQQkg@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 02/28] bpf: introduce hid program type
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        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>,
        open list <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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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;
[...]