On Tue, Feb 8, 2022 at 10:22 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, Feb 09, 2022 at 11:13:15AM IST, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > The main change is a move of the single line
> > #include "iterators.lskel.h"
> > from iterators/iterators.c to bpf_preload_kern.c.
> > Which means that generated light skeleton can be used from user space or
> > user mode driver like iterators.c or from the kernel module or the kernel itself.
> > The direct use of light skeleton from the kernel module simplifies the code,
> > since UMD is no longer necessary. The libbpf.a required user space and UMD. The
> > CO-RE in the kernel and generated "loader bpf program" used by the light
> > skeleton are capable to perform complex loading operations traditionally
> > provided by libbpf. In addition UMD approach was launching UMD process
> > every time bpffs has to be mounted. With light skeleton in the kernel
> > the bpf_preload kernel module loads bpf iterators once and pins them
> > multiple times into different bpffs mounts.
> >
> > Acked-by: Yonghong Song <yhs@xxxxxx>
> > Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > ---
> > kernel/bpf/inode.c | 39 ++----
> > kernel/bpf/preload/Kconfig | 7 +-
> > kernel/bpf/preload/Makefile | 14 +--
> > kernel/bpf/preload/bpf_preload.h | 8 +-
> > kernel/bpf/preload/bpf_preload_kern.c | 119 ++++++++----------
> > kernel/bpf/preload/bpf_preload_umd_blob.S | 7 --
> > .../preload/iterators/bpf_preload_common.h | 13 --
> > kernel/bpf/preload/iterators/iterators.c | 108 ----------------
> > kernel/bpf/syscall.c | 2 +
> > 9 files changed, 70 insertions(+), 247 deletions(-)
> > delete mode 100644 kernel/bpf/preload/bpf_preload_umd_blob.S
> > delete mode 100644 kernel/bpf/preload/iterators/bpf_preload_common.h
> > delete mode 100644 kernel/bpf/preload/iterators/iterators.c
> >
> > [...]
> >
> > -static int __init load_umd(void)
> > +static int __init load(void)
> > {
> > int err;
> >
> > - err = umd_load_blob(&umd_ops.info, &bpf_preload_umd_start,
> > - &bpf_preload_umd_end - &bpf_preload_umd_start);
> > + err = load_skel();
> > if (err)
> > return err;
> > - bpf_preload_ops = &umd_ops;
> > + bpf_preload_ops = &ops;
> > return err;
> > }
> >
> > -static void __exit fini_umd(void)
> > +static void __exit fini(void)
> > {
> > - struct pid *tgid;
> > -
> > bpf_preload_ops = NULL;
> > -
> > - /* kill UMD in case it's still there due to earlier error */
> > - tgid = umd_ops.info.tgid;
> > - if (tgid) {
> > - kill_pid(tgid, SIGKILL, 1);
> > -
> > - wait_event(tgid->wait_pidfd, thread_group_exited(tgid));
> > - umd_cleanup_helper(&umd_ops.info);
> > - }
> > - umd_unload_blob(&umd_ops.info);
> > + free_links_and_skel();
> > }
> > -late_initcall(load_umd);
> > -module_exit(fini_umd);
> > +late_initcall(load);
>
> Since load_skel invokes the loader which resolve kfuncs, and if the module is
> builtin, btf_vmlinux's kfunc_set_tab is not fully populated yet (other
> register_btf_id_kfunc_set calls may also happen in late_initcall), so I think it
> may be a problem.
>
> When I worked on it we didn't have this case where BPF syscall can be invoked at
> this point, but obviously that is true now. So I think ordering it later than
> late_initcall (or moving register_btf_id_kfunc_set invoking initcalls before) is
> required. WDYT?
Good point. Probably best to convert register_btf_kfunc_id_set invocations
to core_initcall.
I can follow up with a trivial patch later or you can beat me to it.
