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

[Benjamin Tissoires]

On Jun 10 2024, Alexei Starovoitov wrote:
> On Sat, Jun 8, 2024 at 2:01 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
> >
> > +
> > +static int hid_bpf_ops_init_member(const struct btf_type *t,
> > +                                const struct btf_member *member,
> > +                                void *kdata, const void *udata)
> > +{
> > +       u32 moff = __btf_member_bit_offset(t, member) / 8;
> > +       u32 flags;
> > +
> > +       switch (moff) {
> > +       case offsetof(struct hid_bpf_ops, hid_id):
> > +               /* For hid_id and flags fields, this function has to copy it
> > +                * and return 1 to indicate that the data has been handled by
> > +                * the struct_ops type, or the verifier will reject the map if
> > +                * the value of those fields is not zero.
> > +                */
> > +               ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
> > +               return 1;
> > +       case offsetof(struct hid_bpf_ops, flags):
> > +               flags = ((struct hid_bpf_ops *)udata)->flags;
> > +               if (flags & ~BPF_F_BEFORE)
> > +                       return -EINVAL;
> > +               ((struct hid_bpf_ops *)kdata)->flags = flags;
> 
> minor nit: I'd cast kdata/udate in the beginning of
> the function to make the lines shorter and less verbose.
> Similar to how bpf_tcp_ca_init_member() does it.

The change looks like:

---
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
index 9192c66cde20..dde547e734ed 100644
--- a/drivers/hid/bpf/hid_bpf_struct_ops.c
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -97,8 +97,14 @@ static int hid_bpf_ops_init_member(const struct btf_type *t,
                                 const struct btf_member *member,
                                 void *kdata, const void *udata)
 {
-       u32 moff = __btf_member_bit_offset(t, member) / 8;
-       u32 flags;
+       const struct hid_bpf_ops *uhid_bpf_ops;
+       struct hid_bpf_ops *khid_bpf_ops;
+       u32 moff;
+
+       uhid_bpf_ops = (const struct hid_bpf_ops *)udata;
+       khid_bpf_ops = (struct hid_bpf_ops *)kdata;
+
+       moff = __btf_member_bit_offset(t, member) / 8;
 
        switch (moff) {
        case offsetof(struct hid_bpf_ops, hid_id):
@@ -107,13 +113,12 @@ static int hid_bpf_ops_init_member(const struct btf_type *t,
                 * the struct_ops type, or the verifier will reject the map if
                 * the value of those fields is not zero.
                 */
-               ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
+               khid_bpf_ops->hid_id = uhid_bpf_ops->hid_id;
                return 1;
        case offsetof(struct hid_bpf_ops, flags):
-               flags = ((struct hid_bpf_ops *)udata)->flags;
-               if (flags & ~BPF_F_BEFORE)
+               if (uhid_bpf_ops->flags & ~BPF_F_BEFORE)
                        return -EINVAL;
-               ((struct hid_bpf_ops *)kdata)->flags = flags;
+               khid_bpf_ops->flags = uhid_bpf_ops->flags;
                return 1;
        }
        return 0;
---

> 
> Acked-by: Alexei Starovoitov <ast@xxxxxxxxxx>

And I'm going to apply the series now

Cheers,
Benjamin