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