Re: [PATCH bpf-next v2 12/28] bpf/hid: add hid_{get|set}_data helpers

[Song Liu]

On Sat, Mar 5, 2022 at 2:47 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > >
> > > > ---
> > > >
> > > > 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