Re: [PATCH HID 03/13] HID: bpf: implement HID-BPF through bpf_struct_ops

[Alexei Starovoitov]

On Wed, May 29, 2024 at 12:38 AM Benjamin Tissoires <bentiss@kernelorg> wrote:
>
> > Take a look at include/linux/bpf_mprog.h
> > I suspect it might be useful here too.
>
> From the quick look of it this is interesting, but for plain struct
> bpf_prog, right?
>
> So I would have to adapt this include for struct_ops?

yeah. It needs work to be adapted to work with struct-ops.
I mentioned it mainly from uapi perspective.
BPF_F_BEFORE, BPF_F_AFTER, BPF_F_REPLACE.
Would be good to keep things consistent.

> > > @@ -112,6 +113,63 @@ struct hid_ops {
> > >
> > >  extern struct hid_ops *hid_ops;
> > >
> > > +/**
> > > + * struct hid_bpf_ops - A BPF struct_ops of callbacks allowing to attach HID-BPF
> > > + *                     programs to a HID device
> > > + * @hid_id: the HID uniq ID to attach to. This is writeable before ``load()``, and
> > > + *         cannot be changed after
> > > + * @flags: &enum hid_bpf_attach_flags to assign flags before ``load()``.
> > > + *        Writeable only before ``load()``
> > > + */
> > > +struct hid_bpf_ops {
> > > +       /* hid_id needs to stay first so we can easily change it
> > > +        * from userspace.
> > > +        */
> >
> > hmm.
> > patch 5 just does:
> > +       skel->struct_ops.mouse_invert->hid_id = hid_id;
> >
> > I don't see a reason why it needs to be first.
>
> See the selftests (and my external loader, udev-hid-bpf):
>
>                 map = bpf_object__find_map_by_name(*self->skel->skeleton->obj,
>                                                           programs[i].name + 4);
>                 ASSERT_OK_PTR(map) TH_LOG("can not find struct_ops by name '%s'",
>                                           programs[i].name + 4);
>
>                 /* hid_id is the first field of struct hid_bpf_ops */
>                 ops_hid_id = bpf_map__initial_value(map, NULL);
>                 ASSERT_OK_PTR(ops_hid_id) TH_LOG("unable to retrieve struct_ops data");
>
>                 *ops_hid_id = self->hid_id;
>
> Basically, I want to have to deal with .bpf.o and not the skeletons for
> the generic case. I want to be able to drop a file in /lib/firmware/hid
> and have udev-hid-bpf load it without having to recompile udev-hid-bpf.

and with additional restriction of only one struct-ops prog per elf file,
right?
Otherwise multiple hid_bpf_ops will be in the same elf section and
the same map.

> I couldn't seem to be able to access the struct_ops BTF information from
> libbpf, so I decided to have hid_id and flags first and consider this to
> be ABI while I figure out how to get to the real offsets :)

I see. I guess it's ok for now.