Date:   Thu, 22 Apr 2021 17:50:19 -0700
From:   Yonghong Song <yhs@...com>
To:     Andrii Nakryiko <andrii@...nel.org>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, <ast@...com>, <daniel@...earbox.net>
CC:     <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 15/17] selftests/bpf: add function linking
 selftest



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@...nel.org>

Ack with a small nit below.

Acked-by: Yonghong Song <yhs@...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);
> +
[...]

Powered by blists - more mailing lists