diff mbox series

[bpf-next,v2,2/2] selftests/bpf: Test gen_pro/epilogue that generate kfuncs

Message ID 20250220212532.783859-2-ameryhung@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next,v2,1/2] bpf: Search and add kfuncs in struct_ops prologue and epilogue | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 11 this patch: 11
netdev/build_tools success Errors and warnings before: 26 (+1) this patch: 26 (+1)
netdev/cc_maintainers warning 15 maintainers not CCed: sdf@fomichev.me linux-stm32@st-md-mailman.stormreply.com jolsa@kernel.org shuah@kernel.org john.fastabend@gmail.com mcoquelin.stm32@gmail.com linux-arm-kernel@lists.infradead.org alexandre.torgue@foss.st.com kpsingh@kernel.org yonghong.song@linux.dev song@kernel.org linux-kselftest@vger.kernel.org haoluo@google.com mykolal@fb.com martin.lau@linux.dev
netdev/build_clang success Errors and warnings before: 5029 this patch: 5029
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1380 this patch: 1380
netdev/checkpatch warning CHECK: Lines should not end with a '(' WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 84 exceeds 80 columns WARNING: line length of 85 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-11 success Logs for aarch64-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-12 success Logs for aarch64-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-50 success Logs for x86_64-llvm-18 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-17 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-51 success Logs for x86_64-llvm-18 / veristat-meta
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-17 / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-21 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-18 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-44 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-20 success Logs for s390x-gcc / veristat-meta
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for s390x-gcc / veristat-kernel
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / GCC BPF
bpf/vmtest-bpf-next-VM_Test-43 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-49 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-45 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-gcc / veristat-meta / x86_64-gcc veristat_meta
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / GCC BPF / GCC BPF
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-gcc / veristat-kernel / x86_64-gcc veristat_kernel
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-47 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-48 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-46 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18

Commit Message

Amery Hung Feb. 20, 2025, 9:25 p.m. UTC 
Test gen_prologue and gen_epilogue that generate kfuncs that have not
been seen in the main program.

The main bpf program and return value checks are identical to
pro_epilogue.c introduced in commit 47e69431b57a ("selftests/bpf: Test
gen_prologue and gen_epilogue"). However, now when bpf_testmod_st_ops
detects a program name with prefix "test_kfunc_", it generates slightly
different prologue and epilogue: They still add 1000 to args->a in
prologue, add 10000 to args->a and set r0 to 2 * args->a in epilogue,
but involve kfuncs.

At high level, the alternative version of prologue and epilogue look
like this:

  cgrp = bpf_cgroup_from_id(0);
  if (cgrp)
          bpf_cgroup_release(cgrp);
  else
          /* Perform what original bpf_testmod_st_ops prologue or
           * epilogue does
           */

Since 0 is never a valid cgroup id, the original prologue or epilogue
logic will be performed. As a result, the __retval check should expect
the exact same return value.

Signed-off-by: Amery Hung <ameryhung@gmail.com>
---
 include/linux/filter.h                        |  10 +
 kernel/bpf/btf.c                              |   1 +
 .../selftests/bpf/prog_tests/pro_epilogue.c   |   2 +
 .../bpf/progs/pro_epilogue_with_kfunc.c       | 182 ++++++++++++++++++
 .../selftests/bpf/test_kmods/bpf_testmod.c    |  90 +++++++++
 5 files changed, 285 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/pro_epilogue_with_kfunc.c

Comments

Eduard Zingerman Feb. 20, 2025, 11:10 p.m. UTC | #1 
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?

[...]

> 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).

> +	if (!bpf_cgroup_from_id_id || !bpf_cgroup_release_id)
> +		return -EINVAL;
> +
>  	return 0;
>  }
>
Amery Hung Feb. 20, 2025, 11:34 p.m. UTC | #2 
On Thu, Feb 20, 2025 at 3:10 PM Eduard Zingerman <eddyz87@gmail.com> 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.

Thanks,
Amery

> > +     if (!bpf_cgroup_from_id_id || !bpf_cgroup_release_id)
> > +             return -EINVAL;
> > +
> >       return 0;
> >  }
> >
>
>
Amery Hung Feb. 21, 2025, 1:05 a.m. UTC | #3 
On Thu, Feb 20, 2025 at 3:34 PM Amery Hung <ameryhung@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 3:10 PM Eduard Zingerman <eddyz87@gmail.com> 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?

Thanks,
Amery

> Thanks,
> Amery
>
> > > +     if (!bpf_cgroup_from_id_id || !bpf_cgroup_release_id)
> > > +             return -EINVAL;
> > > +
> > >       return 0;
> > >  }
> > >
> >
> >
Eduard Zingerman Feb. 21, 2025, 3:02 a.m. UTC | #4 
On Thu, 2025-02-20 at 17:05 -0800, Amery Hung wrote:
> On Thu, Feb 20, 2025 at 3:34 PM Amery Hung <ameryhung@gmail.com> wrote:
> > 
> > On Thu, Feb 20, 2025 at 3:10 PM Eduard Zingerman <eddyz87@gmail.com> 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.

[...]
Amery Hung Feb. 21, 2025, 5:06 a.m. UTC | #5 
On Thu, Feb 20, 2025 at 7:02 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 3:10 PM Eduard Zingerman <eddyz87@gmail.com> 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.
>
> [...]
>
Eduard Zingerman Feb. 21, 2025, 6:05 a.m. UTC | #6 
On Thu, 2025-02-20 at 21:06 -0800, Amery Hung wrote:
> On Thu, Feb 20, 2025 at 7:02 PM Eduard Zingerman <eddyz87@gmail.com> wrote:

[...]

> > 
> > 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.

Fair enough.
Sorry for the false trail with BTF_ID_LIST,
this resolve_btfids corner case was a surprise for me.
diff mbox series

Patch

diff --git a/include/linux/filter.h b/include/linux/filter.h
index a3ea46281595..3ed6eb9e7c73 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -469,6 +469,16 @@  static inline bool insn_is_cast_user(const struct bpf_insn *insn)
 		.off   = 0,					\
 		.imm   = BPF_CALL_IMM(FUNC) })
 
+/* Kfunc call */
+
+#define BPF_CALL_KFUNC(OFF, IMM)				\
+	((struct bpf_insn) {					\
+		.code  = BPF_JMP | BPF_CALL,			\
+		.dst_reg = 0,					\
+		.src_reg = BPF_PSEUDO_KFUNC_CALL,		\
+		.off   = OFF,					\
+		.imm   = IMM })
+
 /* Raw code statement block */
 
 #define BPF_RAW_INSN(CODE, DST, SRC, OFF, IMM)			\
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);
 
 const struct btf_type *btf_type_skip_modifiers(const struct btf *btf,
 					       u32 id, u32 *res_id)
diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
index 509883e6823a..5d3c00a08a88 100644
--- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -6,6 +6,7 @@ 
 #include "epilogue_tailcall.skel.h"
 #include "pro_epilogue_goto_start.skel.h"
 #include "epilogue_exit.skel.h"
+#include "pro_epilogue_with_kfunc.skel.h"
 
 struct st_ops_args {
 	__u64 a;
@@ -55,6 +56,7 @@  void test_pro_epilogue(void)
 	RUN_TESTS(pro_epilogue);
 	RUN_TESTS(pro_epilogue_goto_start);
 	RUN_TESTS(epilogue_exit);
+	RUN_TESTS(pro_epilogue_with_kfunc);
 	if (test__start_subtest("tailcall"))
 		test_tailcall();
 }
diff --git a/tools/testing/selftests/bpf/progs/pro_epilogue_with_kfunc.c b/tools/testing/selftests/bpf/progs/pro_epilogue_with_kfunc.c
new file mode 100644
index 000000000000..c8fc31c4c3c4
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/pro_epilogue_with_kfunc.c
@@ -0,0 +1,182 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../test_kmods/bpf_testmod.h"
+#include "../test_kmods/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+void __kfunc_btf_root(void)
+{
+	bpf_kfunc_st_ops_inc10(NULL);
+}
+
+static __noinline __used int subprog(struct st_ops_args *args)
+{
+	args->a += 1;
+	return args->a;
+}
+
+__success
+/* prologue */
+__xlated("0: r8 = r1")
+__xlated("1: r1 = 0")
+__xlated("2: call kernel-function")
+__xlated("3: if r0 != 0x0 goto pc+5")
+__xlated("4: r6 = *(u64 *)(r8 +0)")
+__xlated("5: r7 = *(u64 *)(r6 +0)")
+__xlated("6: r7 += 1000")
+__xlated("7: *(u64 *)(r6 +0) = r7")
+__xlated("8: goto pc+2")
+__xlated("9: r1 = r0")
+__xlated("10: call kernel-function")
+__xlated("11: r1 = r8")
+/* main prog */
+__xlated("12: r1 = *(u64 *)(r1 +0)")
+__xlated("13: r6 = r1")
+__xlated("14: call kernel-function")
+__xlated("15: r1 = r6")
+__xlated("16: call pc+1")
+__xlated("17: exit")
+SEC("struct_ops/test_prologue")
+__naked int test_kfunc_prologue(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: r1 = *(u64 *)(r1 +0)")
+__xlated("2: r6 = r1")
+__xlated("3: call kernel-function")
+__xlated("4: r1 = r6")
+__xlated("5: call pc+")
+/* epilogue */
+__xlated("6: r1 = 0")
+__xlated("7: call kernel-function")
+__xlated("8: if r0 != 0x0 goto pc+6")
+__xlated("9: r1 = *(u64 *)(r10 -8)")
+__xlated("10: r1 = *(u64 *)(r1 +0)")
+__xlated("11: r6 = *(u64 *)(r1 +0)")
+__xlated("12: r6 += 10000")
+__xlated("13: *(u64 *)(r1 +0) = r6")
+__xlated("14: goto pc+2")
+__xlated("15: r1 = r0")
+__xlated("16: call kernel-function")
+__xlated("17: r0 = r6")
+__xlated("18: r0 *= 2")
+__xlated("19: exit")
+SEC("struct_ops/test_epilogue")
+__naked int test_kfunc_epilogue(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+__success
+/* prologue */
+__xlated("0: r8 = r1")
+__xlated("1: r1 = 0")
+__xlated("2: call kernel-function")
+__xlated("3: if r0 != 0x0 goto pc+5")
+__xlated("4: r6 = *(u64 *)(r8 +0)")
+__xlated("5: r7 = *(u64 *)(r6 +0)")
+__xlated("6: r7 += 1000")
+__xlated("7: *(u64 *)(r6 +0) = r7")
+__xlated("8: goto pc+2")
+__xlated("9: r1 = r0")
+__xlated("10: call kernel-function")
+__xlated("11: r1 = r8")
+/* save __u64 *ctx to stack */
+__xlated("12: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("13: r1 = *(u64 *)(r1 +0)")
+__xlated("14: r6 = r1")
+__xlated("15: call kernel-function")
+__xlated("16: r1 = r6")
+__xlated("17: call pc+")
+/* epilogue */
+__xlated("18: r1 = 0")
+__xlated("19: call kernel-function")
+__xlated("20: if r0 != 0x0 goto pc+6")
+__xlated("21: r1 = *(u64 *)(r10 -8)")
+__xlated("22: r1 = *(u64 *)(r1 +0)")
+__xlated("23: r6 = *(u64 *)(r1 +0)")
+__xlated("24: r6 += 10000")
+__xlated("25: *(u64 *)(r1 +0) = r6")
+__xlated("26: goto pc+2")
+__xlated("27: r1 = r0")
+__xlated("28: call kernel-function")
+__xlated("29: r0 = r6")
+__xlated("30: r0 *= 2")
+__xlated("31: exit")
+SEC("struct_ops/test_pro_epilogue")
+__naked int test_kfunc_pro_epilogue(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r6 = r1;"
+	"call %[bpf_kfunc_st_ops_inc10];"
+	"r1 = r6;"
+	"call subprog;"
+	"exit;"
+	:
+	: __imm(bpf_kfunc_st_ops_inc10)
+	: __clobber_all);
+}
+
+SEC("syscall")
+__retval(1011) /* PROLOGUE_A [1000] + KFUNC_INC10 + SUBPROG_A [1] */
+int syscall_prologue(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_prologue(&args);
+}
+
+SEC("syscall")
+__retval(20022) /* (KFUNC_INC10 + SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_epilogue(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(22022) /* (PROLOGUE_A [1000] + KFUNC_INC10 + SUBPROG_A [1] + EPILOGUE_A [10000]) * 2 */
+int syscall_pro_epilogue(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_pro_epilogue(&args);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops pro_epilogue_with_kfunc = {
+	.test_prologue = (void *)test_kfunc_prologue,
+	.test_epilogue = (void *)test_kfunc_epilogue,
+	.test_pro_epilogue = (void *)test_kfunc_pro_epilogue,
+};
diff --git a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
index 89dc502de9d4..ecf2ef073fee 100644
--- a/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/test_kmods/bpf_testmod.c
@@ -1308,6 +1308,83 @@  static int bpf_test_mod_st_ops__test_pro_epilogue(struct st_ops_args *args)
 	return 0;
 }
 
+static int bpf_cgroup_from_id_id;
+static int bpf_cgroup_release_id;
+
+static int st_ops_gen_prologue_with_kfunc(struct bpf_insn *insn_buf, bool direct_write,
+					  const struct bpf_prog *prog)
+{
+	struct bpf_insn *insn = insn_buf;
+
+	/* r8 = r1; // r8 will be "u64 *ctx".
+	 * r1 = 0;
+	 * r0 = bpf_cgroup_from_id(r1);
+	 * if r0 != 0 goto pc+5;
+	 * r6 = r8[0]; // r6 will be "struct st_ops *args".
+	 * r7 = r6->a;
+	 * r7 += 1000;
+	 * r6->a = r7;
+	 * goto pc+2;
+	 * r1 = r0;
+	 * bpf_cgroup_release(r1);
+	 * r1 = r8;
+	 */
+	*insn++ = BPF_MOV64_REG(BPF_REG_8, BPF_REG_1);
+	*insn++ = BPF_MOV64_IMM(BPF_REG_1, 0);
+	*insn++ = BPF_CALL_KFUNC(0, bpf_cgroup_from_id_id);
+	*insn++ = BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 5);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_8, 0);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_6, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_ALU64_IMM(BPF_ADD, BPF_REG_7, 1000);
+	*insn++ = BPF_STX_MEM(BPF_DW, BPF_REG_6, BPF_REG_7, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_JMP_IMM(BPF_JA, 0, 0, 2);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_0);
+	*insn++ = BPF_CALL_KFUNC(0, bpf_cgroup_release_id),
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_8);
+	*insn++ = prog->insnsi[0];
+
+	return insn - insn_buf;
+}
+
+static int st_ops_gen_epilogue_with_kfunc(struct bpf_insn *insn_buf, const struct bpf_prog *prog,
+					  s16 ctx_stack_off)
+{
+	struct bpf_insn *insn = insn_buf;
+
+	/* r1 = 0;
+	 * r0 = bpf_cgroup_from_id(r1);
+	 * if r0 != 0 goto pc+6;
+	 * r1 = stack[ctx_stack_off]; // r1 will be "u64 *ctx"
+	 * r1 = r1[0]; // r1 will be "struct st_ops *args"
+	 * r6 = r1->a;
+	 * r6 += 10000;
+	 * r1->a = r6;
+	 * goto pc+2
+	 * r1 = r0;
+	 * bpf_cgroup_release(r1);
+	 * r0 = r6;
+	 * r0 *= 2;
+	 * BPF_EXIT;
+	 */
+	*insn++ = BPF_MOV64_IMM(BPF_REG_1, 0);
+	*insn++ = BPF_CALL_KFUNC(0, bpf_cgroup_from_id_id);
+	*insn++ = BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 6);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_FP, ctx_stack_off);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, 0);
+	*insn++ = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_1, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_ALU64_IMM(BPF_ADD, BPF_REG_6, 10000);
+	*insn++ = BPF_STX_MEM(BPF_DW, BPF_REG_1, BPF_REG_6, offsetof(struct st_ops_args, a));
+	*insn++ = BPF_JMP_IMM(BPF_JA, 0, 0, 2);
+	*insn++ = BPF_MOV64_REG(BPF_REG_1, BPF_REG_0);
+	*insn++ = BPF_CALL_KFUNC(0, bpf_cgroup_release_id),
+	*insn++ = BPF_MOV64_REG(BPF_REG_0, BPF_REG_6);
+	*insn++ = BPF_ALU64_IMM(BPF_MUL, BPF_REG_0, 2);
+	*insn++ = BPF_EXIT_INSN();
+
+	return insn - insn_buf;
+}
+
+#define KFUNC_PRO_EPI_PREFIX "test_kfunc_"
 static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
 			       const struct bpf_prog *prog)
 {
@@ -1317,6 +1394,9 @@  static int st_ops_gen_prologue(struct bpf_insn *insn_buf, bool direct_write,
 	    strcmp(prog->aux->attach_func_name, "test_pro_epilogue"))
 		return 0;
 
+	if (!strncmp(prog->aux->name, KFUNC_PRO_EPI_PREFIX, strlen(KFUNC_PRO_EPI_PREFIX)))
+		return st_ops_gen_prologue_with_kfunc(insn_buf, direct_write, prog);
+
 	/* r6 = r1[0]; // r6 will be "struct st_ops *args". r1 is "u64 *ctx".
 	 * r7 = r6->a;
 	 * r7 += 1000;
@@ -1340,6 +1420,9 @@  static int st_ops_gen_epilogue(struct bpf_insn *insn_buf, const struct bpf_prog
 	    strcmp(prog->aux->attach_func_name, "test_pro_epilogue"))
 		return 0;
 
+	if (!strncmp(prog->aux->name, KFUNC_PRO_EPI_PREFIX, strlen(KFUNC_PRO_EPI_PREFIX)))
+		return st_ops_gen_epilogue_with_kfunc(insn_buf, prog, ctx_stack_off);
+
 	/* r1 = stack[ctx_stack_off]; // r1 will be "u64 *ctx"
 	 * r1 = r1[0]; // r1 will be "struct st_ops *args"
 	 * r6 = r1->a;
@@ -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);
+	if (!bpf_cgroup_from_id_id || !bpf_cgroup_release_id)
+		return -EINVAL;
+
 	return 0;
 }