linux-kernel - Re: [PATCH] bpf: verifier: allow arrays of progs to be used in sleepable context

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: Tue, 7 May 2024 19:03:10 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Benjamin Tissoires <bentiss@...nel.org>
Cc: Daniel Borkmann <daniel@...earbox.net>, Alexei Starovoitov <ast@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, Andrii Nakryiko <andrii@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, KP Singh <kpsingh@...nel.org>, 
	Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>, 
	bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] bpf: verifier: allow arrays of progs to be used in
 sleepable context

On Tue, May 7, 2024 at 6:32 AM Benjamin Tissoires <bentiss@...nel.org> wrote:
>
> Yes, exactly that. See [0] for my current WIP. I've just sent it, not
> for reviews, but so you see what I meant here.

The patches helped to understand, for sure, and on surface
they kind of make sense, but without seeing what is that
hid specific kfunc that will use it
it's hard to make a call.
The (u64)(long) casting concerns and prog lifetime are
difficult to get right. The verifier won't help and it all
will fall on code reviews.
So I'd rather not go this route.
Let's explore first what exactly the goal here.
We've talked about sleepable tail_calls, this async callbacks
from hid kfuncs, and struct-ops.
Maybe none of them fit well and we need something else.
Could you please explain (maybe once again) what is the end goal?

> Last time I checked, I thought struct_ops were only for defining one set
> of operations. And you could overwrite them exactly once.
> But after reading more carefully how it was used in tcp_cong.c, it seems
> we can have multiple programs which define the same struct_ops, and then
> it's the kernel which will choose which one needs to be run.

struct-ops is pretty much a mechanism for kernel to define
a set of callbacks and bpf prog to provide implementation for
these callbacks. The kernel choses when to call them.
tcp-bpf is one such user. sched_ext is another and more advanced.
Currently struct-ops bpf prog loading/attaching mechanism
only specifies the struct-ops. There is no device-id argument,
but that can be extended and kernel can keep per-device a set
of bpf progs.
struct-ops is a bit of overkill if you have only one callback.
It's typically for a set of callbacks.

> Last, I'm not entirely sure how I can specify which struct_ops needs to be
> attached to which device, but it's worth a shot. I've already realized
> that I would probably have to drop the current way of HID-BPF is running,
> so now it's just technical bits to assemble :)

You need to call different bpf progs per device, right?
If indirect call is fine from performance pov,
then tailcall or struct_ops+device_argument might fit.

If you want max perf with direct calls then
we'd need to generalize xdp dispatcher.

So far it sounds that tailcalls might be the best actually,
since prog lifetime is handled by prog array map.
Maybe instead of bpf_tail_call helper we should add a kfunc that
will operate on prog array differently?
(if current bpf_tail_call semantics don't fit).