Re: [PATCH bpf-next v3 12/17] bpf/hid: add more HID helpers

From: Song Liu
Date: Fri Mar 18 2022 - 17:19:32 EST

Next message: kernel test robot: "[bvanassche:ufs-for-next 306/307] drivers/scsi/ufs/ufshcd.c:8314:14: warning: variable 'name' set but not used"
Previous message: Namhyung Kim: "Re: [PATCH 0/2] locking: Add new lock contention tracepoints (v3)"
In reply to: Benjamin Tissoires: "[PATCH bpf-next v3 12/17] bpf/hid: add more HID helpers"
Next in thread: Benjamin Tissoires: "Re: [PATCH bpf-next v3 12/17] bpf/hid: add more HID helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Mar 18, 2022 at 9:18 AM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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.
>
> The third exported helper allows to communicate with the HID device.
> We give a data buffer, and call either HID_GET_REPORT or HID_SET_REPORT
> on the device.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
>
> ---
>
> changes in v3:
> - renamed hid_{get|set}_data into hid_{get|set}_bits
> - squashed with bpf/hid: add bpf_hid_raw_request helper function
>
> changes in v2:
> - split the patch with libbpf and HID left outside.
> ---
> include/linux/bpf-hid.h | 6 +++
> include/uapi/linux/bpf.h | 36 +++++++++++++++++
> kernel/bpf/hid.c | 73 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 36 +++++++++++++++++
> 4 files changed, 151 insertions(+)
>
> diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
> index 7f596554fe8c..82b7466b5008 100644
> --- a/include/linux/bpf-hid.h
> +++ b/include/linux/bpf-hid.h
> @@ -102,6 +102,12 @@ struct bpf_hid_hooks {
> int (*pre_link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> void (*post_link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> void (*array_detach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> + int (*hid_get_bits)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> + u64 offset, u32 n, u32 *data);
> + int (*hid_set_bits)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> + u64 offset, u32 n, u32 data);
> + int (*hid_raw_request)(struct hid_device *hdev, u8 *buf, size_t size,
> + u8 rtype, u8 reqtype);
> };
>
> #ifdef CONFIG_BPF
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0e8438e93768..41ab1d068369 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5155,6 +5155,39 @@ union bpf_attr {
> * by a call to bpf_hid_discard;
> * Return
> * The pointer to the data. On error, a null value is returned.
> + *
> + * int bpf_hid_get_bits(void *ctx, u64 offset, u32 n, u32 *data)
> + * 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.
> + *
> + * n must be less or equal than 32, and we can address with bit
> + * precision the value in the buffer. data must be a pointer
> + * to a u32.
> + * Return
> + * The length of data copied into data. On error, a negative value
> + * is returned.
> + *
> + * int bpf_hid_set_bits(void *ctx, u64 offset, u32 n, u32 data)
> + * Description
> + * Set the data of size n (in bits) at the given offset (bits) in the
> + * ctx->event.data field.
> + *
> + * n must be less or equal than 32, and we can address with bit
> + * precision the value in the buffer.
> + * Return
> + * The length of data copied into ctx->event.data. On error, a negative
> + * value is returned.
> + *
Please use annotations like *offset*.

> + * int bpf_hid_raw_request(void *ctx, void *buf, u64 size, u8 report_type, u8 request_type)
> + * Description
> + * communicate with the HID device
> + *
> + * report_type is one of HID_INPUT_REPORT, HID_OUTPUT_REPORT, HID_FEATURE_REPORT
> + * request_type is one of HID_REQ_SET_REPORT or HID_REQ_GET_REPORT
> + * Return
> + * 0 on success.
> + * negative value on error.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -5352,6 +5385,9 @@ union bpf_attr {
> FN(skb_set_tstamp), \
> FN(ima_file_hash), \
> FN(hid_get_data), \
> + FN(hid_get_bits), \
> + FN(hid_set_bits), \
> + FN(hid_raw_request), \
> /* */
>
> /* 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 2dfeaaa8a83f..30a62e8e0f0a 100644
> --- a/kernel/bpf/hid.c
> +++ b/kernel/bpf/hid.c
> @@ -66,12 +66,85 @@ static const struct bpf_func_proto bpf_hid_get_data_proto = {
> .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO,
> };
>
> +BPF_CALL_4(bpf_hid_get_bits, struct hid_bpf_ctx_kern*, ctx, u64, offset, u32, n, u32*, data)
> +{
> + if (!hid_hooks.hid_get_bits)
> + return -EOPNOTSUPP;

Shall we also check offset and n are valid here?

> +
> + return hid_hooks.hid_get_bits(ctx->hdev,
> + ctx->data,
> + ctx->allocated_size,
> + offset, n,
> + data);
> +}
> +
> +static const struct bpf_func_proto bpf_hid_get_bits_proto = {
> + .func = bpf_hid_get_bits,
> + .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_INT,
> +};
> +
> +BPF_CALL_4(bpf_hid_set_bits, struct hid_bpf_ctx_kern*, ctx, u64, offset, u32, n, u32, data)
> +{
> + if (!hid_hooks.hid_set_bits)
> + return -EOPNOTSUPP;
> +
> + hid_hooks.hid_set_bits(ctx->hdev,
> + ctx->data,
> + ctx->allocated_size,
> + offset, n,
> + data);
> + return 0;
> +}
> +
> +static const struct bpf_func_proto bpf_hid_set_bits_proto = {
> + .func = bpf_hid_set_bits,
> + .gpl_only = true,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_ANYTHING,
> + .arg3_type = ARG_ANYTHING,
> + .arg4_type = ARG_ANYTHING,
> +};
> +
> +BPF_CALL_5(bpf_hid_raw_request, struct hid_bpf_ctx_kern*, ctx, void*, buf, u64, size,
> + u8, report_type, u8, request_type)
> +{
> + if (!hid_hooks.hid_raw_request)
> + return -EOPNOTSUPP;
> +
> + return hid_hooks.hid_raw_request(ctx->hdev, buf, size, report_type, request_type);
> +}
> +
> +static const struct bpf_func_proto bpf_hid_raw_request_proto = {
> + .func = bpf_hid_raw_request,
> + .gpl_only = true, /* hid_raw_request is EXPORT_SYMBOL_GPL */
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_CTX,
> + .arg2_type = ARG_PTR_TO_MEM,
> + .arg3_type = ARG_CONST_SIZE_OR_ZERO,
> + .arg4_type = ARG_ANYTHING,
> + .arg5_type = ARG_ANYTHING,
> +};
> +
> 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_get_bits:
> + return &bpf_hid_get_bits_proto;
> + case BPF_FUNC_hid_set_bits:
> + return &bpf_hid_set_bits_proto;
> + case BPF_FUNC_hid_raw_request:
> + if (prog->expected_attach_type != BPF_HID_DEVICE_EVENT)
> + return &bpf_hid_raw_request_proto;
> + return NULL;
> 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 0e8438e93768..41ab1d068369 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -5155,6 +5155,39 @@ union bpf_attr {
> * by a call to bpf_hid_discard;
> * Return
> * The pointer to the data. On error, a null value is returned.
> + *
> + * int bpf_hid_get_bits(void *ctx, u64 offset, u32 n, u32 *data)
> + * 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.
> + *
> + * n must be less or equal than 32, and we can address with bit
> + * precision the value in the buffer. data must be a pointer
> + * to a u32.
> + * Return
> + * The length of data copied into data. On error, a negative value
> + * is returned.
> + *
> + * int bpf_hid_set_bits(void *ctx, u64 offset, u32 n, u32 data)
> + * Description
> + * Set the data of size n (in bits) at the given offset (bits) in the
> + * ctx->event.data field.
> + *
> + * n must be less or equal than 32, and we can address with bit
> + * precision the value in the buffer.
> + * Return
> + * The length of data copied into ctx->event.data. On error, a negative
> + * value is returned.
> + *
> + * int bpf_hid_raw_request(void *ctx, void *buf, u64 size, u8 report_type, u8 request_type)
> + * Description
> + * communicate with the HID device
> + *
> + * report_type is one of HID_INPUT_REPORT, HID_OUTPUT_REPORT, HID_FEATURE_REPORT
> + * request_type is one of HID_REQ_SET_REPORT or HID_REQ_GET_REPORT
> + * Return
> + * 0 on success.
> + * negative value on error.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -5352,6 +5385,9 @@ union bpf_attr {
> FN(skb_set_tstamp), \
> FN(ima_file_hash), \
> FN(hid_get_data), \
> + FN(hid_get_bits), \
> + FN(hid_set_bits), \
> + FN(hid_raw_request), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> --
> 2.35.1
>

Next message: kernel test robot: "[bvanassche:ufs-for-next 306/307] drivers/scsi/ufs/ufshcd.c:8314:14: warning: variable 'name' set but not used"
Previous message: Namhyung Kim: "Re: [PATCH 0/2] locking: Add new lock contention tracepoints (v3)"
In reply to: Benjamin Tissoires: "[PATCH bpf-next v3 12/17] bpf/hid: add more HID helpers"
Next in thread: Benjamin Tissoires: "Re: [PATCH bpf-next v3 12/17] bpf/hid: add more HID helpers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]