linux-kernel - Re: [PATCH HID v3 03/16] HID: bpf: implement HID-BPF through bpf_struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 14 Jun 2024 11:25:25 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Shuah Khan <shuah@...nel.org>, Jiri Kosina <jikos@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Alexei Starovoitov <ast@...nel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	"open list:HID CORE LAYER" <linux-input@...r.kernel.org>, "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH HID v3 03/16] HID: bpf: implement HID-BPF through
 bpf_struct_ops

On Jun 10 2024, Alexei Starovoitov wrote:
> On Sat, Jun 8, 2024 at 2:01 AM Benjamin Tissoires <bentiss@...nel.org> 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@...nel.org>

And I'm going to apply the series now

Cheers,
Benjamin