Re: [PATCH RFC bpf-next 0/4] bpf: Add support to attach return prog in kprobe multi

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, Feb 13, 2024 at 10:09:09PM +0100, Jiri Olsa wrote:
> On Tue, Feb 13, 2024 at 10:20:46AM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 13, 2024 at 4:09 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 08:06:06PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > But the way you implement it with extra flag and extra fd parameter
> > > > > > makes it harder to have a nice high-level support in libbpf (and
> > > > > > presumably other BPF loader libraries) for this.
> > > > > >
> > > > > > When I was thinking about doing something like this, I was considering
> > > > > > adding a new program type, actually. That way it's possible to define
> > > > > > this "let's skip return probe" protocol without backwards
> > > > > > compatibility concerns. It's easier to use it declaratively in libbpf.
> > > > >
> > > > > ok, that seems cleaner.. but we need to use current kprobe programs,
> > > > > so not sure at the moment how would that fit in.. did you mean new
> > > > > link type?
> > > >
> > > > It's kind of a less important detail, actually. New program type would
> > > > allow us to have an entirely different context type, but I think we
> > > > can make do with the existing kprobe program type. We can have a
> > > > separate attach_type and link type, just like multi-kprobe and
> > > > multi-uprobe are still kprobe programs.
> > >
> > > ok, having new attach type on top of kprobe_multi link makes sense
> > >
> > > >
> > > > >
> > > > > > You just declare SEC("kprobe.wrap/...") (or whatever the name,
> > > > > > something to designate that it's both entry and exit probe) as one
> > > > > > program and in the code there would be some way to determine whether
> > > > > > we are in entry mode or exit mode (helper or field in the custom
> > > > > > context type, the latter being faster and more usable, but it's
> > > > > > probably not critical).
> > > > >
> > > > > hum, so the single program would be for both entry and exit probe,
> > > > > I'll need to check how bad it'd be for us, but it'd probably mean
> > > > > just one extra tail call, so it's likely ok
> > > >
> > > > I guess, I don't know what you are doing there :) I'd recommend
> > > > looking at utilizing BPF global subprogs instead of tail calls, if
> > > > your kernel allows for that, as that's a saner way to scale BPF
> > > > verification.
> > >
> > > ok, we should probably do that.. given this enhancement will be
> > > available on latest kernel anyway, we could use global subprogs
> > > as well
> > >
> > > the related bpftrace might be bit more challenging.. will have to
> > > generate program calling entry or return program now, but seems
> > > doable of course
> > 
> > So you want users to still have separate kprobe and kretprobe in
> > bpftrace, but combine them into this kwrapper transparently? It does
> 
> no I meant I'd need to generate the wrapper program for the new
> interface.. which is extra compared to current bpftrace changes
> 
> > seem doable, but hopefully we'll be able to write kwrapper programs in
> > bpftrace directly as well.
> 
> yes, it should be fine
> 
> SNIP
> 
> > > >
> > > > Yes, I realize special-casing zero might be a bit inconvenient, but I
> > > > think simplicity trumps a potential for zero to be a valid value (and
> > > > there are always ways to work around zero as a meaningful value).
> > > >
> > > > Now, in more complicated cases 8 bytes of temporary session state
> > > > isn't enough, just like BPF cookie being 8 byte (read-only) value
> > > > might not be enough. But the solution is the same as with the BPF
> > > > cookie. You just use those 8 bytes as a key into ARRAY/HASHMAP/whatnot
> > > > storage. It's simple and fast enough for pretty much any case.
> > >
> > > I was recently asked for a way to have function arguments available
> > > in the return kprobe as it is in fexit programs (which was not an
> > > option to use, because we don't have fast multi attach for it)
> > >
> > > using the hash map to store arguments and storing its key in the
> > > session data might be solution for this
> > 
> > if you are ok using hashmap keyed by tid, you can do it today without
> > any kernel changes. With session cookie you'll be able to utilize
> > faster ARRAY map (by building a simple ID allocator to get a free slot
> > in ARRAY map).
> 
> ok
> 
> SNIP
> 
> > > > I bet there is something similar in the kretprobe case, where we can
> > > > carve out 8 bytes and pass it to both entry and exit parts of kwrapper
> > > > program.
> > >
> > > for kprobes.. both kprobe and kprobe_multi/fprobe use rethook to invoke
> > > return probes, so I guess we could use it and store that shared data
> > > in there
> > >
> > > btw Masami is in process of removing rethook from kprobe_multi/fprobe,
> > > as part of migrating fprobe on top of ftrace [0]
> > >
> > > but instead the rethook I think there'll be some sort of shadow stack/data
> > > area accessible from both entry and return probes, that we could use
> > 
> > ok, cool. We also need to be careful to not share session cookie
> > between unrelated programs. E.g., if two independent kwrapper programs
> > are attached to the same function, they should each have their own
> > cookie. Otherwise it's not clear how to build anything reliable on top
> > of that, tbh. This might be a problem, though, right?
> 
> IIRC it's tracer specific data, the shadow stack data should be unique
> for tracer and its called function, but I'll double check on that

Masami,
we recently discussed the possibility to store data between entry/return probe,
IIUC your current patchset [0] allows that, but it seems to be shared across all
the tracers for the given function (__ftrace_return_to_handler).. is the plan to
make the shadow stack per tracer and function? /cc Steven

thanks,
jirka


[0] https://lore.kernel.org/bpf/170723204881.502590.11906735097521170661.stgit@devnote2/