| Message ID | 20210416202404.3443623-16-andrii@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | BPF static linker: support externs | expand |
On 4/16/21 1:24 PM, Andrii Nakryiko wrote: > Add selftest validating various aspects of statically linking functions: > - no conflicts and correct resolution for name-conflicting static funcs; > - correct resolution of extern functions; > - correct handling of weak functions, both resolution itself and libbpf's > handling of unused weak function that "lost" (it leaves gaps in code with > no ELF symbols); > - correct handling of hidden visibility to turn global function into > "static" for the purpose of BPF verification. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Ack with a small nit below. Acked-by: Yonghong Song <yhs@fb.com> > --- > tools/testing/selftests/bpf/Makefile | 3 +- > .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ > .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ > .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ > 4 files changed, 190 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c > create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c > create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 666b462c1218..427ccfec1a6a 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -308,9 +308,10 @@ endef > > SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c > > -LINKED_SKELS := test_static_linked.skel.h > +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h > > test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o > +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o > > LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) > > diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > new file mode 100644 > index 000000000000..03bf8ef131ce > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > @@ -0,0 +1,42 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2021 Facebook */ > + > +#include <test_progs.h> > +#include <sys/syscall.h> > +#include "linked_funcs.skel.h" > + > +void test_linked_funcs(void) > +{ > + int err; > + struct linked_funcs *skel; > + > + skel = linked_funcs__open(); > + if (!ASSERT_OK_PTR(skel, "skel_open")) > + return; > + > + skel->rodata->my_tid = syscall(SYS_gettid); > + skel->rodata->syscall_id = SYS_getpgid; > + > + err = linked_funcs__load(skel); > + if (!ASSERT_OK(err, "skel_load")) > + goto cleanup; > + > + err = linked_funcs__attach(skel); > + if (!ASSERT_OK(err, "skel_attach")) > + goto cleanup; > + > + /* trigger */ > + syscall(SYS_getpgid); > + > + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); > + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); > + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); > + > + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); > + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); > + /* output_weak2 should never be updated */ > + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); > + > +cleanup: > + linked_funcs__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c > new file mode 100644 > index 000000000000..cc621d4e4d82 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c > @@ -0,0 +1,73 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2021 Facebook */ > + > +#include "vmlinux.h" > +#include <bpf/bpf_helpers.h> > +#include <bpf/bpf_tracing.h> > + > +/* weak and shared between two files */ > +const volatile int my_tid __weak = 0; > +const volatile long syscall_id __weak = 0; Since the new compiler (llvm13) is recommended for this patch set. We can simplify the above two definition with int my_tid __weak; long syscall_id __weak; The same for the other file. But I am also okay with the current form to *satisfy* llvm10 some people may still use. > + > +int output_val1 = 0; > +int output_ctx1 = 0; > +int output_weak1 = 0; > + > +/* same "subprog" name in all files, but it's ok because they all are static */ > +static __noinline int subprog(int x) > +{ > + /* but different formula */ > + return x * 1; > +} > + > +/* Global functions can't be void */ > +int set_output_val1(int x) > +{ > + output_val1 = x + subprog(x); > + return x; > +} > + > +/* This function can't be verified as global, as it assumes raw_tp/sys_enter > + * context and accesses syscall id (second argument). So we mark it as > + * __hidden, so that libbpf will mark it as static in the final object file, > + * right before verifying it in the kernel. > + * > + * But we don't mark it as __hidden here, rather at extern site. __hidden is > + * "contaminating" visibility, so it will get propagated from either extern or > + * actual definition (including from the losing __weak definition). > + */ > +void set_output_ctx1(__u64 *ctx) > +{ > + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ > +} > + > +/* this weak instance should win because it's the first one */ > +__weak int set_output_weak(int x) > +{ > + output_weak1 = x; > + return x; > +} > + > +extern int set_output_val2(int x); > + > +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ > +__hidden extern void set_output_ctx2(__u64 *ctx); > + [...]
On Thu, Apr 22, 2021 at 5:51 PM Yonghong Song <yhs@fb.com> wrote: > > + > > +/* weak and shared between two files */ > > +const volatile int my_tid __weak = 0; > > +const volatile long syscall_id __weak = 0; > > Since the new compiler (llvm13) is recommended for this patch set. > We can simplify the above two definition with > int my_tid __weak; > long syscall_id __weak; > The same for the other file. > > But I am also okay with the current form > to *satisfy* llvm10 some people may still use. The test won't work with anything, but the latest llvm trunk, so " = 0" is useless. Let's remove it. Especially from the tests that rely on the latest llvm. No one can backport the latest llvm BPF backend to llvm10 front-end.
On Thu, Apr 22, 2021 at 7:35 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Apr 22, 2021 at 5:51 PM Yonghong Song <yhs@fb.com> wrote: > > > + > > > +/* weak and shared between two files */ > > > +const volatile int my_tid __weak = 0; > > > +const volatile long syscall_id __weak = 0; > > > > Since the new compiler (llvm13) is recommended for this patch set. > > We can simplify the above two definition with > > int my_tid __weak; > > long syscall_id __weak; > > The same for the other file. > > > > But I am also okay with the current form > > to *satisfy* llvm10 some people may still use. > > The test won't work with anything, but the latest llvm trunk, > so " = 0" is useless. > Let's remove it. > Especially from the tests that rely on the latest llvm. > No one can backport the latest llvm BPF backend to llvm10 front-end. Sure, I'll drop = 0, just a habit by now.
On Thu, Apr 22, 2021 at 5:50 PM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/16/21 1:24 PM, Andrii Nakryiko wrote: > > Add selftest validating various aspects of statically linking functions: > > - no conflicts and correct resolution for name-conflicting static funcs; > > - correct resolution of extern functions; > > - correct handling of weak functions, both resolution itself and libbpf's > > handling of unused weak function that "lost" (it leaves gaps in code with > > no ELF symbols); > > - correct handling of hidden visibility to turn global function into > > "static" for the purpose of BPF verification. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > Ack with a small nit below. > > Acked-by: Yonghong Song <yhs@fb.com> > > > --- > > tools/testing/selftests/bpf/Makefile | 3 +- > > .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ > > .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ > > .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ > > 4 files changed, 190 insertions(+), 1 deletion(-) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c > > create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c > > create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c > > > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > > index 666b462c1218..427ccfec1a6a 100644 > > --- a/tools/testing/selftests/bpf/Makefile > > +++ b/tools/testing/selftests/bpf/Makefile > > @@ -308,9 +308,10 @@ endef > > > > SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c > > > > -LINKED_SKELS := test_static_linked.skel.h > > +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h > > > > test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o > > +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o > > > > LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > > new file mode 100644 > > index 000000000000..03bf8ef131ce > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > > @@ -0,0 +1,42 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2021 Facebook */ > > + > > +#include <test_progs.h> > > +#include <sys/syscall.h> > > +#include "linked_funcs.skel.h" > > + > > +void test_linked_funcs(void) > > +{ > > + int err; > > + struct linked_funcs *skel; > > + > > + skel = linked_funcs__open(); > > + if (!ASSERT_OK_PTR(skel, "skel_open")) > > + return; > > + > > + skel->rodata->my_tid = syscall(SYS_gettid); > > + skel->rodata->syscall_id = SYS_getpgid; > > + > > + err = linked_funcs__load(skel); > > + if (!ASSERT_OK(err, "skel_load")) > > + goto cleanup; > > + > > + err = linked_funcs__attach(skel); > > + if (!ASSERT_OK(err, "skel_attach")) > > + goto cleanup; > > + > > + /* trigger */ > > + syscall(SYS_getpgid); > > + > > + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); > > + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); > > + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); > > + > > + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); > > + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); > > + /* output_weak2 should never be updated */ > > + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); > > + > > +cleanup: > > + linked_funcs__destroy(skel); > > +} > > diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c > > new file mode 100644 > > index 000000000000..cc621d4e4d82 > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c > > @@ -0,0 +1,73 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2021 Facebook */ > > + > > +#include "vmlinux.h" > > +#include <bpf/bpf_helpers.h> > > +#include <bpf/bpf_tracing.h> > > + > > +/* weak and shared between two files */ > > +const volatile int my_tid __weak = 0; > > +const volatile long syscall_id __weak = 0; > > Since the new compiler (llvm13) is recommended for this patch set. > We can simplify the above two definition with > int my_tid __weak; > long syscall_id __weak; > The same for the other file. This is not about old vs new compilers. I wanted to use .rodata variables, but I'll switch to .bss, no problem. > > But I am also okay with the current form > to *satisfy* llvm10 some people may still use. > > > + > > +int output_val1 = 0; > > +int output_ctx1 = 0; > > +int output_weak1 = 0; > > + > > +/* same "subprog" name in all files, but it's ok because they all are static */ > > +static __noinline int subprog(int x) > > +{ > > + /* but different formula */ > > + return x * 1; > > +} > > + > > +/* Global functions can't be void */ > > +int set_output_val1(int x) > > +{ > > + output_val1 = x + subprog(x); > > + return x; > > +} > > + > > +/* This function can't be verified as global, as it assumes raw_tp/sys_enter > > + * context and accesses syscall id (second argument). So we mark it as > > + * __hidden, so that libbpf will mark it as static in the final object file, > > + * right before verifying it in the kernel. > > + * > > + * But we don't mark it as __hidden here, rather at extern site. __hidden is > > + * "contaminating" visibility, so it will get propagated from either extern or > > + * actual definition (including from the losing __weak definition). > > + */ > > +void set_output_ctx1(__u64 *ctx) > > +{ > > + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ > > +} > > + > > +/* this weak instance should win because it's the first one */ > > +__weak int set_output_weak(int x) > > +{ > > + output_weak1 = x; > > + return x; > > +} > > + > > +extern int set_output_val2(int x); > > + > > +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ > > +__hidden extern void set_output_ctx2(__u64 *ctx); > > + > [...]
On 4/23/21 10:18 AM, Andrii Nakryiko wrote: > On Thu, Apr 22, 2021 at 5:50 PM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 4/16/21 1:24 PM, Andrii Nakryiko wrote: >>> Add selftest validating various aspects of statically linking functions: >>> - no conflicts and correct resolution for name-conflicting static funcs; >>> - correct resolution of extern functions; >>> - correct handling of weak functions, both resolution itself and libbpf's >>> handling of unused weak function that "lost" (it leaves gaps in code with >>> no ELF symbols); >>> - correct handling of hidden visibility to turn global function into >>> "static" for the purpose of BPF verification. >>> >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >> >> Ack with a small nit below. >> >> Acked-by: Yonghong Song <yhs@fb.com> >> >>> --- >>> tools/testing/selftests/bpf/Makefile | 3 +- >>> .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ >>> .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ >>> .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ >>> 4 files changed, 190 insertions(+), 1 deletion(-) >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c >>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c >>> >>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >>> index 666b462c1218..427ccfec1a6a 100644 >>> --- a/tools/testing/selftests/bpf/Makefile >>> +++ b/tools/testing/selftests/bpf/Makefile >>> @@ -308,9 +308,10 @@ endef >>> >>> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c >>> >>> -LINKED_SKELS := test_static_linked.skel.h >>> +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h >>> >>> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o >>> +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o >>> >>> LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>> new file mode 100644 >>> index 000000000000..03bf8ef131ce >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>> @@ -0,0 +1,42 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* Copyright (c) 2021 Facebook */ >>> + >>> +#include <test_progs.h> >>> +#include <sys/syscall.h> >>> +#include "linked_funcs.skel.h" >>> + >>> +void test_linked_funcs(void) >>> +{ >>> + int err; >>> + struct linked_funcs *skel; >>> + >>> + skel = linked_funcs__open(); >>> + if (!ASSERT_OK_PTR(skel, "skel_open")) >>> + return; >>> + >>> + skel->rodata->my_tid = syscall(SYS_gettid); >>> + skel->rodata->syscall_id = SYS_getpgid; >>> + >>> + err = linked_funcs__load(skel); >>> + if (!ASSERT_OK(err, "skel_load")) >>> + goto cleanup; >>> + >>> + err = linked_funcs__attach(skel); >>> + if (!ASSERT_OK(err, "skel_attach")) >>> + goto cleanup; >>> + >>> + /* trigger */ >>> + syscall(SYS_getpgid); >>> + >>> + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); >>> + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); >>> + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); >>> + >>> + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); >>> + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); >>> + /* output_weak2 should never be updated */ >>> + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); >>> + >>> +cleanup: >>> + linked_funcs__destroy(skel); >>> +} >>> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c >>> new file mode 100644 >>> index 000000000000..cc621d4e4d82 >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c >>> @@ -0,0 +1,73 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* Copyright (c) 2021 Facebook */ >>> + >>> +#include "vmlinux.h" >>> +#include <bpf/bpf_helpers.h> >>> +#include <bpf/bpf_tracing.h> >>> + >>> +/* weak and shared between two files */ >>> +const volatile int my_tid __weak = 0; >>> +const volatile long syscall_id __weak = 0; >> >> Since the new compiler (llvm13) is recommended for this patch set. >> We can simplify the above two definition with >> int my_tid __weak; >> long syscall_id __weak; >> The same for the other file. > > This is not about old vs new compilers. I wanted to use .rodata > variables, but I'll switch to .bss, no problem. I see. You can actually hone one "const volatile ing my_tid __weak = 0" and another "long syscall_id __weak". This way, you will be able to test both .rodata and .bss section. > >> >> But I am also okay with the current form >> to *satisfy* llvm10 some people may still use. >> >>> + >>> +int output_val1 = 0; >>> +int output_ctx1 = 0; >>> +int output_weak1 = 0; >>> + >>> +/* same "subprog" name in all files, but it's ok because they all are static */ >>> +static __noinline int subprog(int x) >>> +{ >>> + /* but different formula */ >>> + return x * 1; >>> +} >>> + >>> +/* Global functions can't be void */ >>> +int set_output_val1(int x) >>> +{ >>> + output_val1 = x + subprog(x); >>> + return x; >>> +} >>> + >>> +/* This function can't be verified as global, as it assumes raw_tp/sys_enter >>> + * context and accesses syscall id (second argument). So we mark it as >>> + * __hidden, so that libbpf will mark it as static in the final object file, >>> + * right before verifying it in the kernel. >>> + * >>> + * But we don't mark it as __hidden here, rather at extern site. __hidden is >>> + * "contaminating" visibility, so it will get propagated from either extern or >>> + * actual definition (including from the losing __weak definition). >>> + */ >>> +void set_output_ctx1(__u64 *ctx) >>> +{ >>> + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ >>> +} >>> + >>> +/* this weak instance should win because it's the first one */ >>> +__weak int set_output_weak(int x) >>> +{ >>> + output_weak1 = x; >>> + return x; >>> +} >>> + >>> +extern int set_output_val2(int x); >>> + >>> +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ >>> +__hidden extern void set_output_ctx2(__u64 *ctx); >>> + >> [...]
On Fri, Apr 23, 2021 at 10:35 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/23/21 10:18 AM, Andrii Nakryiko wrote: > > On Thu, Apr 22, 2021 at 5:50 PM Yonghong Song <yhs@fb.com> wrote: > >> > >> > >> > >> On 4/16/21 1:24 PM, Andrii Nakryiko wrote: > >>> Add selftest validating various aspects of statically linking functions: > >>> - no conflicts and correct resolution for name-conflicting static funcs; > >>> - correct resolution of extern functions; > >>> - correct handling of weak functions, both resolution itself and libbpf's > >>> handling of unused weak function that "lost" (it leaves gaps in code with > >>> no ELF symbols); > >>> - correct handling of hidden visibility to turn global function into > >>> "static" for the purpose of BPF verification. > >>> > >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > >> > >> Ack with a small nit below. > >> > >> Acked-by: Yonghong Song <yhs@fb.com> > >> > >>> --- > >>> tools/testing/selftests/bpf/Makefile | 3 +- > >>> .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ > >>> .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ > >>> .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ > >>> 4 files changed, 190 insertions(+), 1 deletion(-) > >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c > >>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c > >>> > >>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > >>> index 666b462c1218..427ccfec1a6a 100644 > >>> --- a/tools/testing/selftests/bpf/Makefile > >>> +++ b/tools/testing/selftests/bpf/Makefile > >>> @@ -308,9 +308,10 @@ endef > >>> > >>> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c > >>> > >>> -LINKED_SKELS := test_static_linked.skel.h > >>> +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h > >>> > >>> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o > >>> +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o > >>> > >>> LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) > >>> > >>> diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>> new file mode 100644 > >>> index 000000000000..03bf8ef131ce > >>> --- /dev/null > >>> +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>> @@ -0,0 +1,42 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* Copyright (c) 2021 Facebook */ > >>> + > >>> +#include <test_progs.h> > >>> +#include <sys/syscall.h> > >>> +#include "linked_funcs.skel.h" > >>> + > >>> +void test_linked_funcs(void) > >>> +{ > >>> + int err; > >>> + struct linked_funcs *skel; > >>> + > >>> + skel = linked_funcs__open(); > >>> + if (!ASSERT_OK_PTR(skel, "skel_open")) > >>> + return; > >>> + > >>> + skel->rodata->my_tid = syscall(SYS_gettid); > >>> + skel->rodata->syscall_id = SYS_getpgid; > >>> + > >>> + err = linked_funcs__load(skel); > >>> + if (!ASSERT_OK(err, "skel_load")) > >>> + goto cleanup; > >>> + > >>> + err = linked_funcs__attach(skel); > >>> + if (!ASSERT_OK(err, "skel_attach")) > >>> + goto cleanup; > >>> + > >>> + /* trigger */ > >>> + syscall(SYS_getpgid); > >>> + > >>> + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); > >>> + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); > >>> + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); > >>> + > >>> + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); > >>> + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); > >>> + /* output_weak2 should never be updated */ > >>> + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); > >>> + > >>> +cleanup: > >>> + linked_funcs__destroy(skel); > >>> +} > >>> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c > >>> new file mode 100644 > >>> index 000000000000..cc621d4e4d82 > >>> --- /dev/null > >>> +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c > >>> @@ -0,0 +1,73 @@ > >>> +// SPDX-License-Identifier: GPL-2.0 > >>> +/* Copyright (c) 2021 Facebook */ > >>> + > >>> +#include "vmlinux.h" > >>> +#include <bpf/bpf_helpers.h> > >>> +#include <bpf/bpf_tracing.h> > >>> + > >>> +/* weak and shared between two files */ > >>> +const volatile int my_tid __weak = 0; > >>> +const volatile long syscall_id __weak = 0; > >> > >> Since the new compiler (llvm13) is recommended for this patch set. > >> We can simplify the above two definition with > >> int my_tid __weak; > >> long syscall_id __weak; > >> The same for the other file. > > > > This is not about old vs new compilers. I wanted to use .rodata > > variables, but I'll switch to .bss, no problem. > > I see. You can actually hone one "const volatile ing my_tid __weak = 0" > and another "long syscall_id __weak". This way, you will be able to > test both .rodata and .bss section. I wonder if you meant to have one my_tid __weak in .bss and another my_tid __weak in .rodata. Or just my_tid in .bss and syscall_id in .rodata? If the former (mixing ELF sections across definitions of the same symbol), then it's disallowed right now. libbpf will error out on mismatched sections. I tested this with normal compilation, it does work and the final section is the section of the winner. But I think that's quite confusing, actually, so I'm going to leave it disallowed for now. E.g., if one file expects a read-write variable and another expects that same variable to be read-only, and the winner ends up being read-only one, then the file expecting read-write will essentially have incorrect code (and will be rejected by BPF verifier, if anything attempts to write). So I think it's better to reject it at the linking time. But I'll do one (my_tid) as .bss, and another (syscall_id) as .rodata. > > > > >> > >> But I am also okay with the current form > >> to *satisfy* llvm10 some people may still use. > >> > >>> + > >>> +int output_val1 = 0; > >>> +int output_ctx1 = 0; > >>> +int output_weak1 = 0; > >>> + > >>> +/* same "subprog" name in all files, but it's ok because they all are static */ > >>> +static __noinline int subprog(int x) > >>> +{ > >>> + /* but different formula */ > >>> + return x * 1; > >>> +} > >>> + > >>> +/* Global functions can't be void */ > >>> +int set_output_val1(int x) > >>> +{ > >>> + output_val1 = x + subprog(x); > >>> + return x; > >>> +} > >>> + > >>> +/* This function can't be verified as global, as it assumes raw_tp/sys_enter > >>> + * context and accesses syscall id (second argument). So we mark it as > >>> + * __hidden, so that libbpf will mark it as static in the final object file, > >>> + * right before verifying it in the kernel. > >>> + * > >>> + * But we don't mark it as __hidden here, rather at extern site. __hidden is > >>> + * "contaminating" visibility, so it will get propagated from either extern or > >>> + * actual definition (including from the losing __weak definition). > >>> + */ > >>> +void set_output_ctx1(__u64 *ctx) > >>> +{ > >>> + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ > >>> +} > >>> + > >>> +/* this weak instance should win because it's the first one */ > >>> +__weak int set_output_weak(int x) > >>> +{ > >>> + output_weak1 = x; > >>> + return x; > >>> +} > >>> + > >>> +extern int set_output_val2(int x); > >>> + > >>> +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ > >>> +__hidden extern void set_output_ctx2(__u64 *ctx); > >>> + > >> [...]
On 4/23/21 10:55 AM, Andrii Nakryiko wrote: > On Fri, Apr 23, 2021 at 10:35 AM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 4/23/21 10:18 AM, Andrii Nakryiko wrote: >>> On Thu, Apr 22, 2021 at 5:50 PM Yonghong Song <yhs@fb.com> wrote: >>>> >>>> >>>> >>>> On 4/16/21 1:24 PM, Andrii Nakryiko wrote: >>>>> Add selftest validating various aspects of statically linking functions: >>>>> - no conflicts and correct resolution for name-conflicting static funcs; >>>>> - correct resolution of extern functions; >>>>> - correct handling of weak functions, both resolution itself and libbpf's >>>>> handling of unused weak function that "lost" (it leaves gaps in code with >>>>> no ELF symbols); >>>>> - correct handling of hidden visibility to turn global function into >>>>> "static" for the purpose of BPF verification. >>>>> >>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >>>> >>>> Ack with a small nit below. >>>> >>>> Acked-by: Yonghong Song <yhs@fb.com> >>>> >>>>> --- >>>>> tools/testing/selftests/bpf/Makefile | 3 +- >>>>> .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ >>>>> .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ >>>>> .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ >>>>> 4 files changed, 190 insertions(+), 1 deletion(-) >>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>>>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c >>>>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c >>>>> >>>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >>>>> index 666b462c1218..427ccfec1a6a 100644 >>>>> --- a/tools/testing/selftests/bpf/Makefile >>>>> +++ b/tools/testing/selftests/bpf/Makefile >>>>> @@ -308,9 +308,10 @@ endef >>>>> >>>>> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c >>>>> >>>>> -LINKED_SKELS := test_static_linked.skel.h >>>>> +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h >>>>> >>>>> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o >>>>> +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o >>>>> >>>>> LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) >>>>> >>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>>>> new file mode 100644 >>>>> index 000000000000..03bf8ef131ce >>>>> --- /dev/null >>>>> +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c >>>>> @@ -0,0 +1,42 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* Copyright (c) 2021 Facebook */ >>>>> + >>>>> +#include <test_progs.h> >>>>> +#include <sys/syscall.h> >>>>> +#include "linked_funcs.skel.h" >>>>> + >>>>> +void test_linked_funcs(void) >>>>> +{ >>>>> + int err; >>>>> + struct linked_funcs *skel; >>>>> + >>>>> + skel = linked_funcs__open(); >>>>> + if (!ASSERT_OK_PTR(skel, "skel_open")) >>>>> + return; >>>>> + >>>>> + skel->rodata->my_tid = syscall(SYS_gettid); >>>>> + skel->rodata->syscall_id = SYS_getpgid; >>>>> + >>>>> + err = linked_funcs__load(skel); >>>>> + if (!ASSERT_OK(err, "skel_load")) >>>>> + goto cleanup; >>>>> + >>>>> + err = linked_funcs__attach(skel); >>>>> + if (!ASSERT_OK(err, "skel_attach")) >>>>> + goto cleanup; >>>>> + >>>>> + /* trigger */ >>>>> + syscall(SYS_getpgid); >>>>> + >>>>> + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); >>>>> + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); >>>>> + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); >>>>> + >>>>> + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); >>>>> + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); >>>>> + /* output_weak2 should never be updated */ >>>>> + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); >>>>> + >>>>> +cleanup: >>>>> + linked_funcs__destroy(skel); >>>>> +} >>>>> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c >>>>> new file mode 100644 >>>>> index 000000000000..cc621d4e4d82 >>>>> --- /dev/null >>>>> +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c >>>>> @@ -0,0 +1,73 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* Copyright (c) 2021 Facebook */ >>>>> + >>>>> +#include "vmlinux.h" >>>>> +#include <bpf/bpf_helpers.h> >>>>> +#include <bpf/bpf_tracing.h> >>>>> + >>>>> +/* weak and shared between two files */ >>>>> +const volatile int my_tid __weak = 0; >>>>> +const volatile long syscall_id __weak = 0; >>>> >>>> Since the new compiler (llvm13) is recommended for this patch set. >>>> We can simplify the above two definition with >>>> int my_tid __weak; >>>> long syscall_id __weak; >>>> The same for the other file. >>> >>> This is not about old vs new compilers. I wanted to use .rodata >>> variables, but I'll switch to .bss, no problem. >> >> I see. You can actually hone one "const volatile ing my_tid __weak = 0" >> and another "long syscall_id __weak". This way, you will be able to >> test both .rodata and .bss section. > > I wonder if you meant to have one my_tid __weak in .bss and another > my_tid __weak in .rodata. Or just my_tid in .bss and syscall_id in > .rodata? > > If the former (mixing ELF sections across definitions of the same > symbol), then it's disallowed right now. libbpf will error out on > mismatched sections. I tested this with normal compilation, it does > work and the final section is the section of the winner. > > But I think that's quite confusing, actually, so I'm going to leave it > disallowed for now. E.g., if one file expects a read-write variable > and another expects that same variable to be read-only, and the winner > ends up being read-only one, then the file expecting read-write will > essentially have incorrect code (and will be rejected by BPF verifier, > if anything attempts to write). So I think it's better to reject it at > the linking time. > > But I'll do one (my_tid) as .bss, and another (syscall_id) as .rodata. I mean this one. Permitting the same variable in both .bss and .rodata sections is never a good practice. > >> >>> >>>> >>>> But I am also okay with the current form >>>> to *satisfy* llvm10 some people may still use. >>>> >>>>> + >>>>> +int output_val1 = 0; >>>>> +int output_ctx1 = 0; >>>>> +int output_weak1 = 0; >>>>> + >>>>> +/* same "subprog" name in all files, but it's ok because they all are static */ >>>>> +static __noinline int subprog(int x) >>>>> +{ >>>>> + /* but different formula */ >>>>> + return x * 1; >>>>> +} >>>>> + >>>>> +/* Global functions can't be void */ >>>>> +int set_output_val1(int x) >>>>> +{ >>>>> + output_val1 = x + subprog(x); >>>>> + return x; >>>>> +} >>>>> + >>>>> +/* This function can't be verified as global, as it assumes raw_tp/sys_enter >>>>> + * context and accesses syscall id (second argument). So we mark it as >>>>> + * __hidden, so that libbpf will mark it as static in the final object file, >>>>> + * right before verifying it in the kernel. >>>>> + * >>>>> + * But we don't mark it as __hidden here, rather at extern site. __hidden is >>>>> + * "contaminating" visibility, so it will get propagated from either extern or >>>>> + * actual definition (including from the losing __weak definition). >>>>> + */ >>>>> +void set_output_ctx1(__u64 *ctx) >>>>> +{ >>>>> + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ >>>>> +} >>>>> + >>>>> +/* this weak instance should win because it's the first one */ >>>>> +__weak int set_output_weak(int x) >>>>> +{ >>>>> + output_weak1 = x; >>>>> + return x; >>>>> +} >>>>> + >>>>> +extern int set_output_val2(int x); >>>>> + >>>>> +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ >>>>> +__hidden extern void set_output_ctx2(__u64 *ctx); >>>>> + >>>> [...]
On Fri, Apr 23, 2021 at 10:58 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 4/23/21 10:55 AM, Andrii Nakryiko wrote: > > On Fri, Apr 23, 2021 at 10:35 AM Yonghong Song <yhs@fb.com> wrote: > >> > >> > >> > >> On 4/23/21 10:18 AM, Andrii Nakryiko wrote: > >>> On Thu, Apr 22, 2021 at 5:50 PM Yonghong Song <yhs@fb.com> wrote: > >>>> > >>>> > >>>> > >>>> On 4/16/21 1:24 PM, Andrii Nakryiko wrote: > >>>>> Add selftest validating various aspects of statically linking functions: > >>>>> - no conflicts and correct resolution for name-conflicting static funcs; > >>>>> - correct resolution of extern functions; > >>>>> - correct handling of weak functions, both resolution itself and libbpf's > >>>>> handling of unused weak function that "lost" (it leaves gaps in code with > >>>>> no ELF symbols); > >>>>> - correct handling of hidden visibility to turn global function into > >>>>> "static" for the purpose of BPF verification. > >>>>> > >>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > >>>> > >>>> Ack with a small nit below. > >>>> > >>>> Acked-by: Yonghong Song <yhs@fb.com> > >>>> > >>>>> --- > >>>>> tools/testing/selftests/bpf/Makefile | 3 +- > >>>>> .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ > >>>>> .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ > >>>>> .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ > >>>>> 4 files changed, 190 insertions(+), 1 deletion(-) > >>>>> create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>>>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c > >>>>> create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c > >>>>> > >>>>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > >>>>> index 666b462c1218..427ccfec1a6a 100644 > >>>>> --- a/tools/testing/selftests/bpf/Makefile > >>>>> +++ b/tools/testing/selftests/bpf/Makefile > >>>>> @@ -308,9 +308,10 @@ endef > >>>>> > >>>>> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c > >>>>> > >>>>> -LINKED_SKELS := test_static_linked.skel.h > >>>>> +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h > >>>>> > >>>>> test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o > >>>>> +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o > >>>>> > >>>>> LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) > >>>>> > >>>>> diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>>>> new file mode 100644 > >>>>> index 000000000000..03bf8ef131ce > >>>>> --- /dev/null > >>>>> +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c > >>>>> @@ -0,0 +1,42 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0 > >>>>> +/* Copyright (c) 2021 Facebook */ > >>>>> + > >>>>> +#include <test_progs.h> > >>>>> +#include <sys/syscall.h> > >>>>> +#include "linked_funcs.skel.h" > >>>>> + > >>>>> +void test_linked_funcs(void) > >>>>> +{ > >>>>> + int err; > >>>>> + struct linked_funcs *skel; > >>>>> + > >>>>> + skel = linked_funcs__open(); > >>>>> + if (!ASSERT_OK_PTR(skel, "skel_open")) > >>>>> + return; > >>>>> + > >>>>> + skel->rodata->my_tid = syscall(SYS_gettid); > >>>>> + skel->rodata->syscall_id = SYS_getpgid; > >>>>> + > >>>>> + err = linked_funcs__load(skel); > >>>>> + if (!ASSERT_OK(err, "skel_load")) > >>>>> + goto cleanup; > >>>>> + > >>>>> + err = linked_funcs__attach(skel); > >>>>> + if (!ASSERT_OK(err, "skel_attach")) > >>>>> + goto cleanup; > >>>>> + > >>>>> + /* trigger */ > >>>>> + syscall(SYS_getpgid); > >>>>> + > >>>>> + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); > >>>>> + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); > >>>>> + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); > >>>>> + > >>>>> + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); > >>>>> + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); > >>>>> + /* output_weak2 should never be updated */ > >>>>> + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); > >>>>> + > >>>>> +cleanup: > >>>>> + linked_funcs__destroy(skel); > >>>>> +} > >>>>> diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c > >>>>> new file mode 100644 > >>>>> index 000000000000..cc621d4e4d82 > >>>>> --- /dev/null > >>>>> +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c > >>>>> @@ -0,0 +1,73 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0 > >>>>> +/* Copyright (c) 2021 Facebook */ > >>>>> + > >>>>> +#include "vmlinux.h" > >>>>> +#include <bpf/bpf_helpers.h> > >>>>> +#include <bpf/bpf_tracing.h> > >>>>> + > >>>>> +/* weak and shared between two files */ > >>>>> +const volatile int my_tid __weak = 0; > >>>>> +const volatile long syscall_id __weak = 0; > >>>> > >>>> Since the new compiler (llvm13) is recommended for this patch set. > >>>> We can simplify the above two definition with > >>>> int my_tid __weak; > >>>> long syscall_id __weak; > >>>> The same for the other file. > >>> > >>> This is not about old vs new compilers. I wanted to use .rodata > >>> variables, but I'll switch to .bss, no problem. > >> > >> I see. You can actually hone one "const volatile ing my_tid __weak = 0" > >> and another "long syscall_id __weak". This way, you will be able to > >> test both .rodata and .bss section. > > > > I wonder if you meant to have one my_tid __weak in .bss and another > > my_tid __weak in .rodata. Or just my_tid in .bss and syscall_id in > > .rodata? > > > > If the former (mixing ELF sections across definitions of the same > > symbol), then it's disallowed right now. libbpf will error out on > > mismatched sections. I tested this with normal compilation, it does > > work and the final section is the section of the winner. > > > > But I think that's quite confusing, actually, so I'm going to leave it > > disallowed for now. E.g., if one file expects a read-write variable > > and another expects that same variable to be read-only, and the winner > > ends up being read-only one, then the file expecting read-write will > > essentially have incorrect code (and will be rejected by BPF verifier, > > if anything attempts to write). So I think it's better to reject it at > > the linking time. > > > > But I'll do one (my_tid) as .bss, and another (syscall_id) as .rodata. > > I mean this one. Permitting the same variable in both .bss and .rodata > sections is never a good practice. Ok, cool, that's what we do right now. I wonder why it is allowed by user-space linkers, it seems dangerous. > > > > >> > >>> > >>>> > >>>> But I am also okay with the current form > >>>> to *satisfy* llvm10 some people may still use. > >>>> > >>>>> + > >>>>> +int output_val1 = 0; > >>>>> +int output_ctx1 = 0; > >>>>> +int output_weak1 = 0; > >>>>> + > >>>>> +/* same "subprog" name in all files, but it's ok because they all are static */ > >>>>> +static __noinline int subprog(int x) > >>>>> +{ > >>>>> + /* but different formula */ > >>>>> + return x * 1; > >>>>> +} > >>>>> + > >>>>> +/* Global functions can't be void */ > >>>>> +int set_output_val1(int x) > >>>>> +{ > >>>>> + output_val1 = x + subprog(x); > >>>>> + return x; > >>>>> +} > >>>>> + > >>>>> +/* This function can't be verified as global, as it assumes raw_tp/sys_enter > >>>>> + * context and accesses syscall id (second argument). So we mark it as > >>>>> + * __hidden, so that libbpf will mark it as static in the final object file, > >>>>> + * right before verifying it in the kernel. > >>>>> + * > >>>>> + * But we don't mark it as __hidden here, rather at extern site. __hidden is > >>>>> + * "contaminating" visibility, so it will get propagated from either extern or > >>>>> + * actual definition (including from the losing __weak definition). > >>>>> + */ > >>>>> +void set_output_ctx1(__u64 *ctx) > >>>>> +{ > >>>>> + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ > >>>>> +} > >>>>> + > >>>>> +/* this weak instance should win because it's the first one */ > >>>>> +__weak int set_output_weak(int x) > >>>>> +{ > >>>>> + output_weak1 = x; > >>>>> + return x; > >>>>> +} > >>>>> + > >>>>> +extern int set_output_val2(int x); > >>>>> + > >>>>> +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ > >>>>> +__hidden extern void set_output_ctx2(__u64 *ctx); > >>>>> + > >>>> [...]
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 666b462c1218..427ccfec1a6a 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -308,9 +308,10 @@ endef SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c -LINKED_SKELS := test_static_linked.skel.h +LINKED_SKELS := test_static_linked.skel.h linked_funcs.skel.h test_static_linked.skel.h-deps := test_static_linked1.o test_static_linked2.o +linked_funcs.skel.h-deps := linked_funcs1.o linked_funcs2.o LINKED_BPF_SRCS := $(patsubst %.o,%.c,$(foreach skel,$(LINKED_SKELS),$($(skel)-deps))) diff --git a/tools/testing/selftests/bpf/prog_tests/linked_funcs.c b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c new file mode 100644 index 000000000000..03bf8ef131ce --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/linked_funcs.c @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Facebook */ + +#include <test_progs.h> +#include <sys/syscall.h> +#include "linked_funcs.skel.h" + +void test_linked_funcs(void) +{ + int err; + struct linked_funcs *skel; + + skel = linked_funcs__open(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + skel->rodata->my_tid = syscall(SYS_gettid); + skel->rodata->syscall_id = SYS_getpgid; + + err = linked_funcs__load(skel); + if (!ASSERT_OK(err, "skel_load")) + goto cleanup; + + err = linked_funcs__attach(skel); + if (!ASSERT_OK(err, "skel_attach")) + goto cleanup; + + /* trigger */ + syscall(SYS_getpgid); + + ASSERT_EQ(skel->bss->output_val1, 2000 + 2000, "output_val1"); + ASSERT_EQ(skel->bss->output_ctx1, SYS_getpgid, "output_ctx1"); + ASSERT_EQ(skel->bss->output_weak1, 42, "output_weak1"); + + ASSERT_EQ(skel->bss->output_val2, 2 * 1000 + 2 * (2 * 1000), "output_val2"); + ASSERT_EQ(skel->bss->output_ctx2, SYS_getpgid, "output_ctx2"); + /* output_weak2 should never be updated */ + ASSERT_EQ(skel->bss->output_weak2, 0, "output_weak2"); + +cleanup: + linked_funcs__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/linked_funcs1.c b/tools/testing/selftests/bpf/progs/linked_funcs1.c new file mode 100644 index 000000000000..cc621d4e4d82 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/linked_funcs1.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Facebook */ + +#include "vmlinux.h" +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +/* weak and shared between two files */ +const volatile int my_tid __weak = 0; +const volatile long syscall_id __weak = 0; + +int output_val1 = 0; +int output_ctx1 = 0; +int output_weak1 = 0; + +/* same "subprog" name in all files, but it's ok because they all are static */ +static __noinline int subprog(int x) +{ + /* but different formula */ + return x * 1; +} + +/* Global functions can't be void */ +int set_output_val1(int x) +{ + output_val1 = x + subprog(x); + return x; +} + +/* This function can't be verified as global, as it assumes raw_tp/sys_enter + * context and accesses syscall id (second argument). So we mark it as + * __hidden, so that libbpf will mark it as static in the final object file, + * right before verifying it in the kernel. + * + * But we don't mark it as __hidden here, rather at extern site. __hidden is + * "contaminating" visibility, so it will get propagated from either extern or + * actual definition (including from the losing __weak definition). + */ +void set_output_ctx1(__u64 *ctx) +{ + output_ctx1 = ctx[1]; /* long id, same as in BPF_PROG below */ +} + +/* this weak instance should win because it's the first one */ +__weak int set_output_weak(int x) +{ + output_weak1 = x; + return x; +} + +extern int set_output_val2(int x); + +/* here we'll force set_output_ctx2() to be __hidden in the final obj file */ +__hidden extern void set_output_ctx2(__u64 *ctx); + +SEC("raw_tp/sys_enter") +int BPF_PROG(handler1, struct pt_regs *regs, long id) +{ + if (my_tid != (u32)bpf_get_current_pid_tgid() || id != syscall_id) + return 0; + + set_output_val2(1000); + set_output_ctx2(ctx); /* ctx definition is hidden in BPF_PROG macro */ + + /* keep input value the same across both files to avoid dependency on + * handler call order; differentiate by output_weak1 vs output_weak2. + */ + set_output_weak(42); + + return 0; +} + +char LICENSE[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/linked_funcs2.c b/tools/testing/selftests/bpf/progs/linked_funcs2.c new file mode 100644 index 000000000000..a5a935a4f748 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/linked_funcs2.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2021 Facebook */ + +#include "vmlinux.h" +#include <bpf/bpf_helpers.h> +#include <bpf/bpf_tracing.h> + +/* weak and shared between both files */ +const volatile int my_tid __weak = 0; +const volatile long syscall_id __weak = 0; + +int output_val2 = 0; +int output_ctx2 = 0; +int output_weak2 = 0; /* should stay zero */ + +/* same "subprog" name in all files, but it's ok because they all are static */ +static __noinline int subprog(int x) +{ + /* but different formula */ + return x * 2; +} + +/* Global functions can't be void */ +int set_output_val2(int x) +{ + output_val2 = 2 * x + 2 * subprog(x); + return 2 * x; +} + +/* This function can't be verified as global, as it assumes raw_tp/sys_enter + * context and accesses syscall id (second argument). So we mark it as + * __hidden, so that libbpf will mark it as static in the final object file, + * right before verifying it in the kernel. + * + * But we don't mark it as __hidden here, rather at extern site. __hidden is + * "contaminating" visibility, so it will get propagated from either extern or + * actual definition (including from the losing __weak definition). + */ +void set_output_ctx2(__u64 *ctx) +{ + output_ctx2 = ctx[1]; /* long id, same as in BPF_PROG below */ +} + +/* this weak instance should lose, because it will be processed second */ +__weak int set_output_weak(int x) +{ + output_weak2 = x; + return 2 * x; +} + +extern int set_output_val1(int x); + +/* here we'll force set_output_ctx1() to be __hidden in the final obj file */ +__hidden extern void set_output_ctx1(__u64 *ctx); + +SEC("raw_tp/sys_enter") +int BPF_PROG(handler2, struct pt_regs *regs, long id) +{ + if (my_tid != (u32)bpf_get_current_pid_tgid() || id != syscall_id) + return 0; + + set_output_val1(2000); + set_output_ctx1(ctx); /* ctx definition is hidden in BPF_PROG macro */ + + /* keep input value the same across both files to avoid dependency on + * handler call order; differentiate by output_weak1 vs output_weak2. + */ + set_output_weak(42); + + return 0; +} + +char LICENSE[] SEC("license") = "GPL";
Add selftest validating various aspects of statically linking functions: - no conflicts and correct resolution for name-conflicting static funcs; - correct resolution of extern functions; - correct handling of weak functions, both resolution itself and libbpf's handling of unused weak function that "lost" (it leaves gaps in code with no ELF symbols); - correct handling of hidden visibility to turn global function into "static" for the purpose of BPF verification. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/testing/selftests/bpf/Makefile | 3 +- .../selftests/bpf/prog_tests/linked_funcs.c | 42 +++++++++++ .../selftests/bpf/progs/linked_funcs1.c | 73 +++++++++++++++++++ .../selftests/bpf/progs/linked_funcs2.c | 73 +++++++++++++++++++ 4 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/linked_funcs.c create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs1.c create mode 100644 tools/testing/selftests/bpf/progs/linked_funcs2.c