Message-ID: <b5r55f2uan7qm5h34nfu2qmoap2gm3ox3dtp2kjpaxebjrzxvp@zqx23ecrnj4q>
Date: Wed, 8 May 2024 13:53:30 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
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 May 07 2024, Alexei Starovoitov wrote:
> 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.

I've posted my HID WIP on [1]. It probably won't compile as my local
original branch was having a merge of HID and bpf trees.

> 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.

yeah, this is a concern.

> 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?

right now I need 4 hooks in HID, the first 2 are already upstream:
- whenever I need to retrieve the report descriptor (this happens in a
  sleepable context, but non sleepable is fine)
- whenever I receive an event from a device (non sleepable context, this
  is coming from a hard IRQ context)
- whenever someone tries to write to the device through
  hid_hw_raw_request (original context is sleepable, and for being able
  to communicate with the device we need sleepable context in bpf)
- same but from hid_hw_output_report

Again, the first 2 are working just fine.

Implementing the latter twos requires sleepable context because we
might:

1. a request is made from user-space
2. we jump into hid-bpf
3. the bpf program "converts" the request from report ID 1 to 2 (because
we export a slightly different API)
4. the bpf program directly emits hid_bpf_raw_request (sleepable
operation)
5. the bpf program returns the correct value
6. hid-core doesn't attempt to communicate with the device as bpf
already did.

In the series, I also realized that I need sleepable and non sleepable
contexts for this kind of situation, because I want tracing and
firewalling available (non sleepable context), while still allowing to
communicate with the device. But when you communicate with the device
from bpf, the sleepable bpf program is not invoked or this allows
infinite loops.

> 
> > 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.

In the end I have 4. However, I might have programs that overwrite twice
the same callback (see the 2 SEC("fmod_ret/hid_bpf_device_event") in
[2]).

> 
> > 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?

yes

> If indirect call is fine from performance pov,
> then tailcall or struct_ops+device_argument might fit.

performance is not a requirement. It's better if we have low latency but
we are not talking the same requirements than network.

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

I'll need to have a deeper look at it, yeah.

> 
> 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).

Actually I'd like to remove bpf_tail_call entirely, because it requires
to pre-load a BPF program at boot, and in some situations (RHEL) this
creates issues. I haven't been able to debug what was happening, I
couldn't reproduce it myself, but removing that bit would be nice :)

Cheers,
Benjamin

[1] https://lore.kernel.org/bpf/20240508-hid_bpf_async_fun-v1-0-558375a25657@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/tree/drivers/hid/bpf/progs/XPPen__ArtistPro16Gen2.bpf.c?h=for-next

Powered by blists - more mailing lists