diff mbox series

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

Message ID 20220304172852.274126-13-benjamin.tissoires@redhat.com
State New
Headers show
Series Introduce eBPF support for HID devices | expand

Commit Message

Benjamin Tissoires March 4, 2022, 5:28 p.m. UTC
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(+)

Comments

Greg KH March 4, 2022, 6:37 p.m. UTC | #1
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>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Greg KH March 4, 2022, 6:41 p.m. UTC | #2
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.

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?

thanks,

greg k-h
Benjamin Tissoires March 5, 2022, 10:33 a.m. UTC | #3
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.

And as I mentioned in the commit notes, I knew the API proposed in
this patch was not correct, simply because while working on the
selftests it was completely wrong :)

In terms of API usage, does it feel wrong for you to have only a
subset of the array available "for free" and enforce the rest to be
used through these helpers?
That's the point I am not sure but I feel like 1024 (or slightly more)
would be enough for most use cases, and when we are dealing with
bigger data sets the helpers can be justified.

Thanks for the suggestion.

Cheers,
Benjamin

>
> thanks,
>
> greg k-h
>
Greg KH March 5, 2022, 10:47 a.m. UTC | #4
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.
> 
> And as I mentioned in the commit notes, I knew the API proposed in
> this patch was not correct, simply because while working on the
> selftests it was completely wrong :)
> 
> In terms of API usage, does it feel wrong for you to have only a
> subset of the array available "for free" and enforce the rest to be
> used through these helpers?

It does feel "odd" but:

> That's the point I am not sure but I feel like 1024 (or slightly more)
> would be enough for most use cases, and when we are dealing with
> bigger data sets the helpers can be justified.

How often are hid reports that big?  If providing access to the first
1024 or so bytes for free would handle the huge majority of fixup cases,
and only if you have crazy devices with large reports would you have to
use the functions, it might be fine.

The problem is, only time will tell, and then, it's too late to change
the api :)

So I agree with this for now, stick with what you have (if you split
this into 4 functions), and let's see what happens.

thanks,

greg k-h
Song Liu March 11, 2022, 12:40 a.m. UTC | #5
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
Benjamin Tissoires March 11, 2022, 3:08 p.m. UTC | #6
On Fri, Mar 11, 2022 at 1:41 AM Song Liu <song@kernel.org> wrote:
>
> 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?
>

Yes, we really need per-bit access, and yes only one BPF program is
working on a ctx/buffer at a time.

The per-bit access is a HID requirement and a much more convenient way
of accessing data in the buffer. Well, there is another advantage too
that I'll add later.

Basically, in the HID world, HW makers are trying to 'compact' the
reports their device is sending to a minimum value.

For instance, when you have a 3 buttons + wheel mouse you may need:
3 bits of information for the 3 buttons
4 bits for the wheel
16 bits for X
16 bits for Y.

This usually translates almost verbatim in the report (we can add one
bit of padding between buttons and wheel), which means that accessing
the wheel data requires the user to access the offset 4 (bits) of size
4 bits in the report.

Some HW vendors are not even bothering aligning the data, so this can
be messy from time to time with just plain byte access.
All in all, the HID report descriptor gives you that information, and
internally, the HID stack stores the offset in bits and the sizes in
bits to access them without too much trouble.

The second advantage I have with these 2 accessors is that it allows
me to not know statically the offset and size values. Because the
helper in the kernel checks them for me, I can use registers values
that are unknown to the verifier and can basically have:

```
__u64 offsetX = 0;
__u64 offsetY = 0;
__u32 sizeX = 0;
__u32 sizeY = 0;

SEC("hid/device_event")
int invert_xy(struct hid_bpf_ctx *ctx)
{
  __u16 x, y;

  if (sizeX == 16) {
    x = bpf_hid_get_bits(ctx, offsetX, sizeX);
    bpf_hid_set_bits(ctx, offsetX, -x);
  }
  if (sizeY == 16) {
    y = bpf_hid_get_bits(ctx, offsetY, sizeY);
    bpf_hid_set_bits(ctx, offsetY, -y);
  }
  return 0;
}
```

Then, I have my userspace program parse the report descriptor, set the
correct values for size{X|Y} and offset{X|Y} and I can have this
program compiled once and redistributed many times.

Cheers,
Benjamin
diff mbox series

Patch

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.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5284,6 +5314,8 @@  union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
+	FN(hid_get_data),		\
+	FN(hid_set_data),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/bpf/hid.c b/kernel/bpf/hid.c
index 37500313e270..640e55ba66ec 100644
--- a/kernel/bpf/hid.c
+++ b/kernel/bpf/hid.c
@@ -37,10 +37,63 @@  void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks)
 }
 EXPORT_SYMBOL_GPL(bpf_hid_set_hooks);
 
+BPF_CALL_5(bpf_hid_get_data, void*, ctx, u64, offset, u32, n, void*, data, u64, size)
+{
+	struct hid_bpf_ctx *bpf_ctx = ctx;
+
+	if (!hid_hooks.hid_get_data)
+		return -EOPNOTSUPP;
+
+	return hid_hooks.hid_get_data(bpf_ctx->hdev,
+				      bpf_ctx->data, bpf_ctx->allocated_size,
+				      offset, n,
+				      data, size);
+}
+
+static const struct bpf_func_proto bpf_hid_get_data_proto = {
+	.func      = bpf_hid_get_data,
+	.gpl_only  = true,
+	.ret_type  = RET_INTEGER,
+	.arg1_type = ARG_PTR_TO_CTX,
+	.arg2_type = ARG_ANYTHING,
+	.arg3_type = ARG_ANYTHING,
+	.arg4_type = ARG_PTR_TO_MEM,
+	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
+BPF_CALL_5(bpf_hid_set_data, void*, ctx, u64, offset, u32, n, void*, data, u64, size)
+{
+	struct hid_bpf_ctx *bpf_ctx = ctx;
+
+	if (!hid_hooks.hid_set_data)
+		return -EOPNOTSUPP;
+
+	hid_hooks.hid_set_data(bpf_ctx->hdev,
+			       bpf_ctx->data, bpf_ctx->allocated_size,
+			       offset, n,
+			       data, size);
+	return 0;
+}
+
+static const struct bpf_func_proto bpf_hid_set_data_proto = {
+	.func      = bpf_hid_set_data,
+	.gpl_only  = true,
+	.ret_type  = RET_INTEGER,
+	.arg1_type = ARG_PTR_TO_CTX,
+	.arg2_type = ARG_ANYTHING,
+	.arg3_type = ARG_ANYTHING,
+	.arg4_type = ARG_PTR_TO_MEM,
+	.arg5_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
 static const struct bpf_func_proto *
 hid_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
 	switch (func_id) {
+	case BPF_FUNC_hid_get_data:
+		return &bpf_hid_get_data_proto;
+	case BPF_FUNC_hid_set_data:
+		return &bpf_hid_set_data_proto;
 	default:
 		return bpf_base_func_proto(func_id);
 	}
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a7a8d9cfcf24..4845a20e6f96 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/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.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5284,6 +5314,8 @@  union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
+	FN(hid_get_data),		\
+	FN(hid_set_data),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper