On Sat, 12 Feb 2022, Andrii Nakryiko wrote:
> On Fri, Feb 11, 2022 at 4:18 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Fri, Feb 11, 2022 at 03:31:56PM -0800, Andrii Nakryiko wrote:
> > > On Fri, Feb 11, 2022 at 3:13 PM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Feb 11, 2022 at 01:14:50PM -0800, Andrii Nakryiko wrote:
> > > > > Add a selftest validating various aspects of libbpf's handling of custom
> > > > > SEC() handlers. It also demonstrates how libraries can ensure very early
> > > > > callbacks registration and unregistration using
> > > > > __attribute__((constructor))/__attribute__((destructor)) functions.
> > > > >
> > > > > Reviewed-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> > > > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > > > > ---
> > > > > .../bpf/prog_tests/custom_sec_handlers.c | 176 ++++++++++++++++++
> > > > > .../bpf/progs/test_custom_sec_handlers.c | 63 +++++++
> > > > > 2 files changed, 239 insertions(+)
> > > > > create mode 100644 tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > > > > create mode 100644 tools/testing/selftests/bpf/progs/test_custom_sec_handlers.c
> > > > >
> > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c b/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > > > > new file mode 100644
> > > > > index 000000000000..28264528280d
> > > > > --- /dev/null
> > > > > +++ b/tools/testing/selftests/bpf/prog_tests/custom_sec_handlers.c
> > > > > @@ -0,0 +1,176 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/* Copyright (c) 2022 Facebook */
> > > > > +
> > > > > +#include <test_progs.h>
> > > > > +#include "test_custom_sec_handlers.skel.h"
> > > > > +
> > > > > +#define COOKIE_ABC1 1
> > > > > +#define COOKIE_ABC2 2
> > > > > +#define COOKIE_CUSTOM 3
> > > > > +#define COOKIE_FALLBACK 4
> > > > > +#define COOKIE_KPROBE 5
> > > > > +
> > > > > +static int custom_init_prog(struct bpf_program *prog, long cookie)
> > > > > +{
> > > > > + if (cookie == COOKIE_ABC1)
> > > > > + bpf_program__set_autoload(prog, false);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > >
> > > > What is the value of init_fn callback?
> > > > afaict it was and still unused in libbpf.
> > > > The above example doesn't make a compelling case, since set_autoload
> > > > can be done out of preload callback.
> > > > Maybe drop init_fn for now and extend libbpf_prog_handler_opts
> > > > when there is actual need for it?
> > >
> > > Great question, but no, you can't set_autoload() in the preload
> > > handler, because once preload is called, loading of the program is
> > > inevitable.
> >
> > Ahh!, but we can add 'if (prog->load)' in bpf_object_load_prog_instance()
> > after preload_fn() was called.
>
> Yes we can and solve this *one specific* scenario. But there is a
> bunch of preparatory stuff that's happening for bpf_program before we
> get to actually loading finalized instructions. All the relocations,
> marking whether we need vmlinux BTF, etc. All that is skipped if
> !prog->load.
>
> I don't want to go and analyze every single possible scenario (and
> probably still miss a bunch of subtle ones) to understand if it's
> always equivalent. Libbpf's contract is that
> bpf_program__set_autoload() is called before bpf_object__load(). You
> are asking me to redesign this contract to move it much deeper into
> bpf_object__load() (and potentially break a bunch of subtle things)
> just to avoid init_fn callback. Hard sell :)
>
> Basically, init_fn is allowed to do everything that normal user code
> is allowed to do between bpf_object__open() and bpf_object__load().
> preload_fn() doesn't have this luxury, but gets access to
> bpf_prog_load opts that normal user code doesn't have access, but it's
> not free to do all the stuff that user is free to do before
> bpf_object__load(). They are not interchangeable.
>
> > Surely the libbpf would waste some time preping the prog with relos,
> > but that's not a big deal.
> > Another option is to move preload_fn earlier.
> > Especially since currently it's only setting attach types.
>
> It should be able to affect logging and all the attach parameter. I
> didn't want to design new OPTS struct just for this callback, so I'm
> trying to reuse bpf_prog_load_opts as a contract. That means I can't
> easily change prog_type (but that's trivial to handle in init_fn) and
> insns (but I can hardly see how that can be done safely at all), but
> otherwise those opts give the full power of low-level bpf_prog_load.
>
> I keep a possibility open to change preload_fn contract to actually
> execute bpf_prog_load() on its own and return prog fd, but I'm
> hesitant because all the libbpf log handling and retries, and other
> niceties will be lost, making trivial things like adding extra
> BPF_F_SLEEPABLE flag not trivial at all. But here's the thing, we can
> later add "advanced" load callback that will be mutually exclusive
> with preload_fn and would be able to handle more advanced cases. But
> that can be done as an extra extension without changing anything about
> current interface.
>
> >
> > Calling the callback 'preload' when it cannot affect the load is odd too.
>
> It's what happening before loading, I never had intention to prevent
> load... Would "prepare_load_fn" be a better name?
>
> >
> > > We might need to adjust the obj->loaded flag so that set_autoload()
> > > returns an error when called from the preload() callback, but that's a
> > > bit orthogonal. I suspect there will be few more adjustments like this
> > > as we get actual users of this API in the wild.
> > >
> > > It's not used by libbpf because we do all the initialization outside
> > > of the callback, as it is the same for all programs and serves as
> > > "default" behavior that custom handlers can override.
> > >
> > > Also, keep in mind that this is the very beginning of v0.8 dev cycle,
> > > we'll have time to adjust interfaces and callback contracts in the
> > > next 2-3 months, if necessary. USDT library open-sourcing will almost
> > > 100% happen during this time frame (though I think USDT library is a
> > > pretty simple use case, so isn't a great "stress test"). But it also
> > > seems like perf might need to use fallback handler for their crazy
> > > SEC() conventions, that will be a good test as well.
> >
> > It would be much easier to take your word if there was an actual example
> > (like libusdt) that demonstrates the use of callbacks.
> > "We will have time to fix things before release" isn't very comforting
> > in the case of big api extension like this one.
>
> Hmm. For libusdt it would be literally:
>
> libbpf_register_prog_handler("usdt", BPF_PROG_TYPE_KPROBE, 0, NULL);
>
> Done.
>
> There is no way (at least currently) to support auto-attach through
> skeleton__attach() or bpf_program__attach(), because single USDT
> attachment is actually multiple program attachments (due to inlining).
> So until libbpf provides APIs to construct "composite" bpf_link from a
> single link, there won't be auto-attach. We might add it later, but I
> don't want to design the entire world in one patch set :)
>
> USDT is too simple a use case, perhaps. I'm trying to also take into
> consideration perf's custom SEC("lock_page=__lock_page page->flags")
> use case, hypothetical SEC("perf_event/cpu_cycles:1000") case, and
> just thinking from the "first principles" what some advanced library
> might what to be able to do with this. Alan's uprobe attach by
> function name would be implementable through these APIs outside of
> libbpf as well (except then we won't be able to add func_name into
> bpf_uprobe_opts, which would be a pity).
>
> I can postpone this whole patch set until later as well, don't care
> all that much. I hate callback APIs anyways :)
>
> We can do USDT library without all this and the user experience won't
> change all that much, actually.
>
Here's one case I had to implement which would be made easier with
this support:
- optional attach using an "o" prefix ("okprobe", "okretprobe"),
where attach failure is not fatal. Used for cases where multiple
possible candidate functions in modules are traced, but we
have no guarantees which module is loaded, and hence which
function is available. Could be implemented by overriding
attach and returning 0 with a NULL link for cases where attach
fails with ENOENT, and overriding preload to set program type
to KPROBE. Having optional support like this in pluggable
section handling makes skeleton interactions simpler where we
mix and match required kprobes and optional ones.
Having custom section handling is nice because the above was
required in a bunch of different tools, and with custom section
handling, skeleton interactions are a lot cleaner.
I would expect tracers would find this functionality useful
too, for example supporting per-pid conventions for uprobe
tracing like "uprobe1234/prog:func".
Alan
