On Mon, Mar 21, 2022 at 10:52 PM Song Liu song@kernel.org wrote:
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.
Ack
[...]
+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.
[...]
[...]
+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.
I'd rather not use data_in. data_in is forwarded as it is in the ctx of the program, so adding a bulky API where the first byte is the target_fd doesn't make a lot of sense IMO.
However, I just managed to achieve what I initially wanted to do without luck: just use the struct bpf_prog as the sole argument. I thought iterating over all hid devices would be painful, but it turns out that is exactly what hid_bpf_fd_to_hdev() was doing, so there is no penalty in doing so.
Anyway, I'll drop ctx_in in the next version.
Cheers, Benjamin