On Thu, Feb 20, 2025 at 7:02 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
I see. Thanks for the explanation!
>
> 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).
>
Currently, generating module kfunc in pro/epilogue is not supported
due to the complication in resolving insn->off of the kfunc call. I
have one patch that test how it can work. But after some discussion
with Martin, we think we can revisit it when there is a struct_ops
module that wants to do so. I will keep the current dynamic btf id
resolution in the next respin over using module kfunc unless people
think we should support it now.
> The broader question if want a capability to use BTF_ID_LIST referring
> to kernel functions from modules remains open.
>
> [...]
>
