On Mon, Mar 21, 2022 at 9:07 AM Benjamin Tissoires benjamin.tissoires@redhat.com wrote:
Hi Song,
many thanks for the quick response.
On Fri, Mar 18, 2022 at 9:48 PM Song Liu song@kernel.org wrote:
[...]
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
We need to mirror these changes to tools/include/uapi/linux/bpf.h.
OK. I did that in patch 4/17 but I can bring in the changes there too.
Let's keep changes to the two files in the same patch. This will make sure they are back ported together.
[...]
+enum hid_bpf_event {
HID_BPF_UNDEF = 0,HID_BPF_DEVICE_EVENT, /* when attach type is BPF_HID_DEVICE_EVENT */HID_BPF_RDESC_FIXUP, /* ................... BPF_HID_RDESC_FIXUP */HID_BPF_USER_EVENT, /* ................... BPF_HID_USER_EVENT */Why don't we have a DRIVER_EVENT type here?
For driver event, I want to have a little bit more of information which tells which event we have:
- HID_BPF_DRIVER_PROBE
- HID_BPF_DRIVER_SUSPEND
- HID_BPF_DRIVER_RAW_REQUEST
- HID_BPF_DRIVER_RAW_REQUEST_ANSWER
- etc...
However, I am not entirely sure on the implementation of all of those, so I left them aside for now.
I'll work on that for v4.
This set is already pretty big. I guess we can add them in a follow-up set.
[...]
+BPF_CALL_3(bpf_hid_get_data, struct hid_bpf_ctx_kern*, ctx, u64, offset, u64, size) +{
if (!size)return 0UL;if (offset + size > ctx->allocated_size)return 0UL;return (unsigned long)(ctx->data + offset);+}
+static const struct bpf_func_proto bpf_hid_get_data_proto = {
.func = bpf_hid_get_data,.gpl_only = true,.ret_type = RET_PTR_TO_ALLOC_MEM_OR_NULL,.arg1_type = ARG_PTR_TO_CTX,.arg2_type = ARG_ANYTHING,.arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO,I think we should use ARG_CONST_SIZE or ARG_CONST_SIZE_OR_ZERO?
I initially tried this with ARG_CONST_SIZE_OR_ZERO but it doesn't work for 2 reasons:
- we need to pair the argument ARG_CONST_SIZE_* with a pointer to a
memory just before, which doesn't really make sense here
- ARG_CONST_SIZE_* isn't handled in the same way
ARG_CONST_ALLOC_SIZE_OR_ZERO is. The latter tells the verifier that the given size is the available size of the returned PTR_TO_ALLOC_MEM_OR_NULL, which is exactly what we want.
I misread the logic initially. It makes sense now.
+};
+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;default:return bpf_base_func_proto(func_id);}+}
[...]
+static int hid_bpf_prog_test_run(struct bpf_prog *prog,
const union bpf_attr *attr,union bpf_attr __user *uattr)+{
struct hid_device *hdev = NULL;struct bpf_prog_array *progs;bool valid_prog = false;int i;int target_fd, ret;void __user *data_out = u64_to_user_ptr(attr->test.data_out);void __user *data_in = u64_to_user_ptr(attr->test.data_in);u32 user_size_in = attr->test.data_size_in;u32 user_size_out = attr->test.data_size_out;u32 allocated_size = max(user_size_in, user_size_out);struct hid_bpf_ctx_kern ctx = {.type = HID_BPF_USER_EVENT,.allocated_size = allocated_size,};if (!hid_hooks.hdev_from_fd)return -EOPNOTSUPP;if (attr->test.ctx_size_in != sizeof(int))return -EINVAL;ctx_size_in is always 4 bytes?
Yes. Basically what I had in mind is that the "ctx" for user_prog_test_run is the file descriptor to the sysfs that represent the HID device. This seemed to me to be the easiest to handle for users.
I'm open to suggestions though.
How about we use data_in? ctx for test_run usually means the program ctx, which is struct hid_bpf_ctx here.
Thanks, Song