Re: [PATCH v3 bpf-next 3/3] selftests/bpf: add custom SEC() handling selftest

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Alan Maguire <alan.maguire@oracle.com>,
	Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v3 bpf-next 3/3] selftests/bpf: add custom SEC() handling selftest
Date: Mon, 14 Feb 2022 21:22:23 -0800	[thread overview]
Message-ID: <CAEf4BzZz_sXFi4dk8k_Eg14VJ4MO+v3V5z76h6i0rDX92dM47A@mail.gmail.com> (raw)
In-Reply-To: <CAADnVQ+Rcim71k8z2ifsEbJRRFwPHCxhOmzv+VCL3Qk1kMtKsA@mail.gmail.com>

On Mon, Feb 14, 2022 at 4:29 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Feb 14, 2022 at 3:14 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On Mon, 14 Feb 2022, Alexei Starovoitov wrote:
> >
> > > Alan,
> > > can you demo your "okprobe" feature based on this api?
> > > Any rough patches would do.
> >
> > sure; see below.  Requires Andrii's v3 patches to be applied first,
> > and demonstrates okprobe handling for a kprobe function that exists
> > and one that doesn't - the important thing is skeleton attach
> > can succeed even when a function is missing (as it would be if
> > the associated module wasn't loaded).
> >
> > > The "o" handling will be done in which callback?
> > >
> >
> > We set program type at init and do custom attach using the function
> > name (specified in the program section after the "okprobe" prefix).
> > However we make sure to catch -ENOENT attach failures and return 0
> > with a NULL link so skeleton attach can proceed.
> >
> > From 9bbd615b71f8f59ff743608bc86d7a2a346da2a8 Mon Sep 17 00:00:00 2001
> > From: Alan Maguire <alan.maguire@oracle.com>
> > Date: Mon, 14 Feb 2022 22:57:56 +0000
> > Subject: [PATCH bpf-next] selftests/bpf: demonstrate further use of custom
> >  SEC() handling
> >
> > Register and use SEC() handling for "okprobe/" kprobe programs
> > (Optional kprobe) which should be attached as kprobes but
> > critically should not stop skeleton loading if attach fails
> > due to non-existence of the to-be-probed function.  This mode
> > of SEC() handling is useful for tracing module functions
> > where the module might not be loaded.
> >
> > Note - this patch is based on the v3 of Andrii's section
> > handling patches [1] and these need to be applied for it to
> > apply cleanly.
> >
> > [1] https://lore.kernel.org/bpf/20220211211450.2224877-1-andrii@kernel.org/
> >
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
>
> Thanks. The patch certainly helps to understand the api usage.
>
> >  static int custom_init_prog(struct bpf_program *prog, long cookie)
> >  {
> >         if (cookie == COOKIE_ABC1)
> >                 bpf_program__set_autoload(prog, false);
> > +       else if (cookie == COOKIE_OKPROBE)
> > +               bpf_program__set_type(prog, BPF_PROG_TYPE_KPROBE);
>
> I think bpf_program__set_type() would have worked
> from the prepare_load_fn callback as well.
>
> What would be a recommended way of setting it?

Currently, bpf_program__set_type() could be done only from init
callback, as by the time preload_fn is called, libbpf already captured
prog_type. This can be adjusted, but I think it's cleaner to do it in
init callback, so that user code, if it chooses to iterate programs
with bpf_object__for_each_program() and such, would get correct
information before bpf_object__load().

I also just checked bpf_program__set_type(), it seems like it's void
function, but it should certainly fail, if called after
bpf_object__load() phase. Not sure if it's an ABI-breaking change to
change it to int, but would be good to adjust this. Same for
bpf_program__set_expected_attach_type().