Re: [PATCH bpf-next v2 02/28] bpf: introduce hid program type

On Mon, Mar 7, 2022 at 10:39 AM Benjamin Tissoires
[off-list ref] wrote:

On Sat, Mar 5, 2022 at 1:03 AM Song Liu [off-list ref] wrote:

quoted

On Fri, Mar 4, 2022 at 9:31 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

HID is a protocol that could benefit from using BPF too.

[...]

quoted

+#include <linux/list.h>
+#include <linux/slab.h>
+
+struct bpf_prog;
+struct bpf_prog_array;
+struct hid_device;
+
+enum bpf_hid_attach_type {
+       BPF_HID_ATTACH_INVALID = -1,
+       BPF_HID_ATTACH_DEVICE_EVENT = 0,
+       MAX_BPF_HID_ATTACH_TYPE

Is it typical to have different BPF programs for different attach types?
Otherwise, (different types may have similar BPF programs), maybe
we can pass type as an argument to the program (shared among
different types)?

Not quite sure I am entirely following you, but I consider the various
attach types to be quite different and thus you can not really reuse
the same BPF program with 2 different attach types.

In my view, we have 4 attach types:
- BPF_HID_ATTACH_DEVICE_EVENT: called whenever we receive an IRQ from
the given device (so this is net-like event stream)
- BPF_HID_ATTACH_RDESC_FIXUP: there can be only one of this type, and
this is called to change the device capabilities. So you can not reuse
the other programs for this one
- BPF_HID_ATTACH_USER_EVENT: called explicitly by the userspace
process owning the program. There we can use functions that are
sleeping (we are not in IRQ context), so this is also fundamentally
different from the 3 others.
- BPF_HID_ATTACH_DRIVER_EVENT: whenever the driver gets called into,
we get a bpf program run. This can be suspend/resume, or even specific
request to the device (change a feature on the device or get its
current state). Again, IMO fundamentally different from the others.

So I'm open to any suggestions, but if we can keep the userspace API
being defined with different SEC in libbpf, that would be the best.

Thanks for this information. Different attach_types sound right for the use
case.

quoted

[...]

quoted

+struct hid_device;
+
+enum hid_bpf_event {
+       HID_BPF_UNDEF = 0,
+       HID_BPF_DEVICE_EVENT,           /* when attach type is BPF_HID_DEVICE_EVENT */
+};
+
+struct hid_bpf_ctx {
+       enum hid_bpf_event type;        /* read-only */
+       __u16 allocated_size;           /* the allocated size of data below (RO) */

There is a (6-byte?) hole here.

quoted

+       struct hid_device *hdev;        /* read-only */
+
+       __u16 size;                     /* used size in data (RW) */
+       __u8 data[];                    /* data buffer (RW) */
+};

Do we really need hit_bpf_ctx in uapi? Maybe we can just use it
from vmlinuxh?

I had a thought at this context today, and I think I am getting to the
limit of what I understand.

My first worry is that the way I wrote it there, with a variable data
field length is that this is not forward compatible. Unless BTF and
CORE are making magic, this will bite me in the long run IMO.

But then, you are talking about not using uapi, and I am starting to
wonder: am I doing the things correctly?

To solve my first issue (and the weird API I had to introduce in the
bpf_hid_get/set_data), I came up to the following:
instead of exporting the data directly in the context, I could create
a helper bpf_hid_get_data_buf(ctx, const uint size) that returns a
RET_PTR_TO_ALLOC_MEM_OR_NULL in the same way bpf_ringbuf_reserve()
does.

This way, I can directly access the fields within the bpf program
without having to worry about the size.

But now, I am wondering whether the uapi I defined here is correct in
the way CORE works.

My goal is to have HID-BPF programs to be CORE compatible, and not
have to recompile them depending on the underlying kernel.

I can not understand right now if I need to add some other BTF helpers
in the same way the access to struct xdp_md and struct xdp_buff are
converted between one and other, or if defining a forward compatible
struct hid_bpf_ctx is enough.
As far as I understand, .convert_ctx_access allows to export a stable
uapi to the bpf prog users with the verifier doing the conversion
between the structs for me. But is this really required for all the
BPF programs if we want them to be CORE?

Also, I am starting to wonder if I should not hide fields in the
context to the users. The .data field could be a pointer and only
accessed through the helper I mentioned above. This would be forward
compatible, and also allows to use whatever available memory in the
kernel to be forwarded to the BPF program. This way I can skip the
memcpy part and work directly with the incoming dma data buffer from
the IRQ.

But is it best practice to do such a thing?

I think .convert_ctx_access is the way to go if we want to access the data
buffer without memcpy. I am not sure how much work is needed to make
it compatible with CORE though.

To make sure I understand the case, do we want something like

bpf_prog(struct hid_bpf_ctx *ctx)
{
    /* makes sure n < ctx->size */
    x = ctx->data[n]; /* read data */
    ctx->data[n] = <something>; /* write data */
    ctx->size = <something <= n>; /* change data size */
}

We also need it to be CORE, so that we may modify hid_bpf_ctx by
inserting more members to it before data.

Is this accurate?

Song