linux-kernel - Re: [PATCH HID v2 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]

Message-ID: <CAADnVQJo71xGQKLTW6Z1xsTLjtikN8bfemPUF4zj2c2Uvi5JMw@mail.gmail.com>
Date: Fri, 7 Jun 2024 09:51:34 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Benjamin Tissoires <bentiss@...nel.org>
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 v2 03/16] HID: bpf: implement HID-BPF through bpf_struct_ops

On Fri, Jun 7, 2024 at 8:28 AM Benjamin Tissoires <bentiss@...nel.org> wrote:
> +struct hid_bpf_ops {
> +       /* hid_id needs to stay first so we can easily change it
> +        * from userspace.
> +        */
> +       int                     hid_id;
> +       u32                     flags;
> +
> +       /* private: internal use only */
> +       struct list_head        list;
> +
> +       /* public: rest is public */

Didn't notice it before, but the above comments are misleading.
The whole struct is private to the kernel and bpf prog, while
registering, can only touch a handful.
I'd drop "internal use" and "is public". It's not an uapi.

> +
> +/* fast path fields are put first to fill one cache line */

Also misleading. The whole struct fits one cache line.

> +
> +       /**
> +        * @hid_device_event: called whenever an event is coming in from the device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this event
> +        *
> +        * Context: Interrupt context.
> +        */
> +       int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type);
> +
> +/* control/slow paths put last */
> +
> +       /**
> +        * @hid_rdesc_fixup: called when the probe function parses the report descriptor
> +        * of the HID device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this device
> +        */
> +       int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
> +
> +       /* private: internal use only */
> +       struct hid_device *hdev;
> +} ____cacheline_aligned_in_smp;

Such alignment is an overkill.
I don't think you can measure the difference.

> +
>  struct hid_bpf_prog_list {
>         u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV];
>         u8 prog_cnt;
> @@ -129,6 +188,10 @@ struct hid_bpf {
>         bool destroyed;                 /* prevents the assignment of any progs */
>
>         spinlock_t progs_lock;          /* protects RCU update of progs */
> +
> +       struct hid_bpf_ops *rdesc_ops;
> +       struct list_head prog_list;
> +       struct mutex prog_list_lock;    /* protects RCU update of prog_list */

mutex protects rcu update... sounds very odd.
Just say that mutex protects prog_list update, because "RCU update"
has a different meaning. RCU logic itself is what protects Update part of rcU.

The rest looks good.