On Thu, 2025-02-20 at 17:05 -0800, Amery Hung wrote:
> On Thu, Feb 20, 2025 at 3:34 PM Amery Hung <ameryhung@xxxxxxxxx> wrote:
> >
> > On Thu, Feb 20, 2025 at 3:10 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > >
> > > On Thu, 2025-02-20 at 13:25 -0800, Amery Hung wrote:
> > >
> > > [...]
> > >
> > > Given that prologue and epilogue generation is already tested,
> > > it appears that it would be sufficient to add only two tests:
> > > 'test_kfunc_pro_epilogue' / 'syscall_pro_epilogue'.
> > > Not sure if testing prologue and epilogue generation separately adds
> > > much value in this context, wdyt?
> > >
> >
> > Agree. I will only keep the syscall_pro_epilogue test.
> >
> > > [...]
> > >
> > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > > index 6c296ff551e0..ddebab05934f 100644
> > > > --- a/kernel/bpf/btf.c
> > > > +++ b/kernel/bpf/btf.c
> > > > @@ -606,6 +606,7 @@ s32 bpf_find_btf_id(const char *name, u32 kind, struct btf **btf_p)
> > > > spin_unlock_bh(&btf_idr_lock);
> > > > return ret;
> > > > }
> > > > +EXPORT_SYMBOL_GPL(bpf_find_btf_id);
> > >
> > > I think this is not necessary, see below.
> > >
> > > [...]
> > >
> > > > @@ -1410,6 +1493,13 @@ static void st_ops_unreg(void *kdata, struct bpf_link *link)
> > > >
> > > > static int st_ops_init(struct btf *btf)
> > > > {
> > > > + struct btf *kfunc_btf;
> > > > +
> > > > + bpf_cgroup_from_id_id = bpf_find_btf_id("bpf_cgroup_from_id", BTF_KIND_FUNC, &kfunc_btf);
> > > > + bpf_cgroup_release_id = bpf_find_btf_id("bpf_cgroup_release", BTF_KIND_FUNC, &kfunc_btf);
> > >
> > > Maybe use BTF_ID_LIST for this?
> > > E.g. BTF_ID_LIST(bpf_testmod_dtor_ids) in this file, or
> > > BTF_ID_LIST(special_kfunc_list) in verifier.c?
> > >
> > > (Just in case, sorry if you know this already,
> > > BTF_ID_LIST declares are set of symbols with special suffix/prefix,
> > > at build time tools/bpf/resolve_btfids looks for such symbols and patches
> > > their values to correspond to BTF ids of specified functions and structures).
> > >
> >
> > Ah yes. It is an artifact when I was testing a patch for generating
> > kfunc in module btf. But since there is no use case, I removed that
> > part. I will change it to BTF_ID_LIST. Thanks for catching this.
> >
>
> Actually when I use BTF_ID_LIST with a kernel kfunc, I got the warning
> below. Since it was not able to resolve the btf id, the test program
> failed to load as the generated byte code will contain invalid kfunc
> id.
>
> BTF [M] bpf_testmod.ko
> WARN: resolve_btfids: unresolved symbol bpf_cgroup_release
> WARN: resolve_btfids: unresolved symbol bpf_cgroup_from_id
> MOD bpf_testmod.ko
>
> I am not familiar with how resolve_btfids work, specifically when
> building a kernel module. Do you have any suggestions?
Looks like there is no way.
resolve_btfids is called for module as follows:
resolve_btfids -b <path-to>/vmlinux bpf_testmod.ko
Where -b specifies base BTF.
However, the bpf_testmod.ko has .BTF.base section, where distilled BTF
is stored. In such case tools/bpf/resolve_btfids/main.c:elf_collect()
overrides base passed from command line, and uses .BTF.base instead.
However, `bpf_cgroup_release` is not referenced in bpf_testmod.c,
thus it does not get into distilled BTF. Hence, its id remains unresolved.
And it cannot be referenced in bpf_testmod.c, because it is not an
exported symbol.
So, for this particular case there are only two options: resolve id
dynamically, as you did, or use a kfunc internal to this module
instead (which should simplify the test, imo).
The broader question if want a capability to use BTF_ID_LIST referring
to kernel functions from modules remains open.
[...]
