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

[Greg KH]

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.

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