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-13-benjamin.tissoires@redhat.com> <YiJdRQxYzfncfTR5@kroah.com>
 <CAO-hwJJ3Yi+JLr40J8nXccjF8PrjiQw1w0Bskz8QHXdNVh1n+A@mail.gmail.com> <YiM/tTYeuAcnz/Xh@kroah.com>
In-Reply-To: <YiM/tTYeuAcnz/Xh@kroah.com>
From:   Song Liu <song@kernel.org>
Date:   Thu, 10 Mar 2022 16:40:43 -0800
Message-ID: <CAPhsuW4Yhpr6jeY8QCCvUcg_1REGWRRy7m5GXZw5Ehtt3eyAHQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 12/28] bpf/hid: add hid_{get|set}_data helpers
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Mar 5, 2022 at 2:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Mar 05, 2022 at 11:33:07AM +0100, Benjamin Tissoires wrote:
> > On Fri, Mar 4, 2022 at 7:41 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Mar 04, 2022 at 06:28:36PM +0100, Benjamin Tissoires wrote:
> > > > When we process an incoming HID report, it is common to have to account
> > > > for fields that are not aligned in the report. HID is using 2 helpers
> > > > hid_field_extract() and implement() to pick up any data at any offset
> > > > within the report.
> > > >
> > > > Export those 2 helpers in BPF programs so users can also rely on them.
> > > > The second net worth advantage of those helpers is that now we can
> > > > fetch data anywhere in the report without knowing at compile time the
> > > > location of it. The boundary checks are done in hid-bpf.c, to prevent
> > > > a memory leak.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > ---
> > > >
> > > > changes in v2:
> > > > - split the patch with libbpf and HID left outside.
> > > > ---
> > > >  include/linux/bpf-hid.h        |  4 +++
> > > >  include/uapi/linux/bpf.h       | 32 ++++++++++++++++++++
> > > >  kernel/bpf/hid.c               | 53 ++++++++++++++++++++++++++++++++++
> > > >  tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++++++
> > > >  4 files changed, 121 insertions(+)
> > > >
> > > > diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
> > > > index 0c5000b28b20..69bb28523ceb 100644
> > > > --- a/include/linux/bpf-hid.h
> > > > +++ b/include/linux/bpf-hid.h
> > > > @@ -93,6 +93,10 @@ struct bpf_hid_hooks {
> > > >       int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > > >       void (*link_attached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > > >       void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > > > +     int (*hid_get_data)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> > > > +                         u64 offset, u32 n, u8 *data, u64 data_size);
> > > > +     int (*hid_set_data)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> > > > +                         u64 offset, u32 n, u8 *data, u64 data_size);
> > > >  };
> > > >
> > > >  #ifdef CONFIG_BPF
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index a7a8d9cfcf24..4845a20e6f96 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -5090,6 +5090,36 @@ union bpf_attr {
> > > >   *   Return
> > > >   *           0 on success, or a negative error in case of failure. On error
> > > >   *           *dst* buffer is zeroed out.
> > > > + *
> > > > + * int bpf_hid_get_data(void *ctx, u64 offset, u32 n, u8 *data, u64 size)
> > > > + *   Description
> > > > + *           Get the data of size n (in bits) at the given offset (bits) in the
> > > > + *           ctx->event.data field and store it into data.
> > > > + *
> > > > + *           if n is less or equal than 32, we can address with bit precision,
> > > > + *           the value in the buffer. However, data must be a pointer to a u32
> > > > + *           and size must be 4.
> > > > + *
> > > > + *           if n is greater than 32, offset and n must be a multiple of 8
> > > > + *           and the result is working with a memcpy internally.
> > > > + *   Return
> > > > + *           The length of data copied into data. On error, a negative value
> > > > + *           is returned.
> > > > + *
> > > > + * int bpf_hid_set_data(void *ctx, u64 offset, u32 n, u8 *data, u64 size)
> > > > + *   Description
> > > > + *           Set the data of size n (in bits) at the given offset (bits) in the
> > > > + *           ctx->event.data field.
> > > > + *
> > > > + *           if n is less or equal than 32, we can address with bit precision,
> > > > + *           the value in the buffer. However, data must be a pointer to a u32
> > > > + *           and size must be 4.
> > > > + *
> > > > + *           if n is greater than 32, offset and n must be a multiple of 8
> > > > + *           and the result is working with a memcpy internally.
> > > > + *   Return
> > > > + *           The length of data copied into ctx->event.data. On error, a negative
> > > > + *           value is returned.
> > >
> >
> > Quick answer on this one (before going deeper with the other remarks next week):
> >
> > > Wait, nevermind my reviewed-by previously, see my comment about how this
> > > might be split into 4:
> > >         bpf_hid_set_bytes()
> > >         bpf_hid_get_bytes()
> > >         bpf_hid_set_bits()
> > >         bpf_hid_get_bits()
> > >
> > > Should be easier to understand and maintain over time, right?
> >
> > Yes, definitively. I thought about adding a `bytes` suffix to the
> > function name for n > 32, but not the `bits` one, meaning the API was
> > still bunkers in my head.

Do we really need per-bit access? I was under the impression that only
one BPF program is working on a ctx/buffer at a time, so we can just do
read-modify-write at byte level, no?

Thanks,
Song