| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2b398ad6-31be-8997-4115-851d79f2d0d2@fb.com>
Date: Fri, 23 Apr 2021 13:24:37 -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 2/6] libbpf: rename static variables during
linking
On 4/23/21 11:53 AM, Andrii Nakryiko wrote:
> Prepend <obj_name>.. prefix to each static variable in BTF info during static
> linking. This makes them uniquely named for the sake of BPF skeleton use,
> allowing to read/write static BPF variables from user-space. This uniqueness
> guarantee depends on each linked file name uniqueness, of course. Double dots
> separator was chosen both to be different (but similar) to the separator that
> Clang is currently using for static variables defined inside functions as well
> as to generate a natural (in libbpf parlance, at least) obj__var naming pattern
> in BPF skeleton. Static linker also checks for static variable to already
> contain ".." separator and skips the rename to allow multi-pass linking and not
> keep making variable name ever increasing, if derived object name is changing on
> each pass (as is the case for selftests).
>
> This patch also adds opts to bpf_linker__add_file() API, which currently
> allows to override object name for a given file and could be extended with other
> per-file options in the future. This is not a breaking change because
> bpf_linker__add_file() isn't yet released officially.
>
> This patch also includes fixes to few selftests that are already using static
> variables. They have to go in in the same patch to not break selftest build.
"in in" => "in"
> Keep in mind, this static variable rename only happens during static linking.
> For any existing user of BPF skeleton using static variables nothing changes,
> because those use cases are using variable names generated by Clang. Only new
> users utilizing static linker might need to adjust BPF skeleton use, which
> currently will be always new use cases. So ther is no risk of breakage.
>
> static_linked selftests is modified to also validate conflicting static variable
> names are handled correctly both during static linking and in BPF skeleton.
>
> Signed-off-by: Andrii Nakryiko <andrii@...nel.org>
> ---
> tools/bpf/bpftool/gen.c | 2 +-
> tools/lib/bpf/libbpf.h | 12 +-
> tools/lib/bpf/linker.c | 121 +++++++++++++++++-
> .../selftests/bpf/prog_tests/skeleton.c | 8 +-
> .../selftests/bpf/prog_tests/static_linked.c | 8 +-
> .../selftests/bpf/progs/bpf_iter_test_kern4.c | 4 +-
> .../selftests/bpf/progs/test_check_mtu.c | 4 +-
> .../selftests/bpf/progs/test_cls_redirect.c | 4 +-
> .../bpf/progs/test_snprintf_single.c | 2 +-
> .../selftests/bpf/progs/test_sockmap_listen.c | 4 +-
> .../selftests/bpf/progs/test_static_linked1.c | 6 +-
> .../selftests/bpf/progs/test_static_linked2.c | 4 +-
> 12 files changed, 151 insertions(+), 28 deletions(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 440a2fcb6441..06fee4a2910a 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -638,7 +638,7 @@ static int do_object(int argc, char **argv)
> while (argc) {
> file = GET_ARG();
>
> - err = bpf_linker__add_file(linker, file);
> + err = bpf_linker__add_file(linker, file, NULL);
> if (err) {
> p_err("failed to link '%s': %s (%d)", file, strerror(err), err);
> goto out;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index bec4e6a6e31d..67505030c8d1 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -768,10 +768,20 @@ struct bpf_linker_opts {
> };
> #define bpf_linker_opts__last_field sz
>
> +struct bpf_linker_file_opts {
> + /* size of this struct, for forward/backward compatiblity */
> + size_t sz;
> + /* object name override, similar to the one in bpf_object_open_opts */
> + const char *object_name;
> +};
> +#define bpf_linker_file_opts__last_field sz
> +
> struct bpf_linker;
>
> LIBBPF_API struct bpf_linker *bpf_linker__new(const char *filename, struct bpf_linker_opts *opts);
> -LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filename);
> +LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker,
> + const char *filename,
> + const struct bpf_linker_file_opts *opts);
> LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
> LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
>
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 9de084b1c699..adc3aa9ce040 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -4,6 +4,8 @@
> *
> * Copyright (c) 2021 Facebook
> */
> +#define _GNU_SOURCE
> +#include <ctype.h>
> #include <stdbool.h>
> #include <stddef.h>
> #include <stdio.h>
> @@ -47,6 +49,16 @@ struct src_sec {
> int sec_type_id;
> };
>
> +#define MAX_OBJ_NAME_LEN 64
> +
> +/* According to C standard, only first 63 characters of C identifiers are
> + * guaranteed to be significant. So for transformed static variables of the most
> + * verbose form ('<obj_name>..<func_name>.<var_name>') we need to reserve extra
> + * 64 (function name and dot) + 63 (variable name) + 2 (for .. separator)
> + * characters.
> + */
> +#define MAX_VAR_NAME_LEN (MAX_OBJ_NAME_LEN + 2 + 63 + 1 + 63)
> +
> struct src_obj {
> const char *filename;
> int fd;
> @@ -67,6 +79,10 @@ struct src_obj {
> int *sym_map;
> /* mapping from the src BTF type IDs to dst ones */
> int *btf_type_map;
> +
> + /* BPF object name used for static variable prefixing */
> + char obj_name[MAX_OBJ_NAME_LEN];
> + size_t obj_name_len;
> };
>
> /* single .BTF.ext data section */
> @@ -158,7 +174,9 @@ struct bpf_linker {
>
> static int init_output_elf(struct bpf_linker *linker, const char *file);
>
> -static int linker_load_obj_file(struct bpf_linker *linker, const char *filename, struct src_obj *obj);
> +static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> + const struct bpf_linker_file_opts *opts,
> + struct src_obj *obj);
> static int linker_sanity_check_elf(struct src_obj *obj);
> static int linker_sanity_check_elf_symtab(struct src_obj *obj, struct src_sec *sec);
> static int linker_sanity_check_elf_relos(struct src_obj *obj, struct src_sec *sec);
> @@ -435,15 +453,19 @@ static int init_output_elf(struct bpf_linker *linker, const char *file)
> return 0;
> }
>
> -int bpf_linker__add_file(struct bpf_linker *linker, const char *filename)
> +int bpf_linker__add_file(struct bpf_linker *linker, const char *filename,
> + const struct bpf_linker_file_opts *opts)
> {
> struct src_obj obj = {};
> int err = 0;
>
> + if (!OPTS_VALID(opts, bpf_linker_file_opts))
> + return -EINVAL;
> +
> if (!linker->elf)
> return -EINVAL;
>
> - err = err ?: linker_load_obj_file(linker, filename, &obj);
> + err = err ?: linker_load_obj_file(linker, filename, opts, &obj);
> err = err ?: linker_append_sec_data(linker, &obj);
> err = err ?: linker_append_elf_syms(linker, &obj);
> err = err ?: linker_append_elf_relos(linker, &obj);
> @@ -529,7 +551,49 @@ static struct src_sec *add_src_sec(struct src_obj *obj, const char *sec_name)
> return sec;
> }
>
> -static int linker_load_obj_file(struct bpf_linker *linker, const char *filename, struct src_obj *obj)
> +static void sanitize_obj_name(char *name)
> +{
> + int i;
> +
> + for (i = 0; name[i]; i++) {
> + if (name[i] == '_')
> + continue;
> + if (i == 0 && isalpha(name[i]))
> + continue;
> + if (i > 0 && isalnum(name[i]))
> + continue;
> +
> + name[i] = '_';
> + }
> +}
> +
> +static bool str_has_suffix(const char *str, const char *suffix)
> +{
> + size_t n1 = strlen(str), n2 = strlen(suffix);
> +
> + if (n1 < n2)
> + return false;
> +
> + return strcmp(str + n1 - n2, suffix) == 0;
> +}
> +
> +static void get_obj_name(char *name, const char *file)
> +{
> + /* Using basename() GNU version which doesn't modify arg. */
> + strncpy(name, basename(file), MAX_OBJ_NAME_LEN - 1);
> + name[MAX_OBJ_NAME_LEN - 1] = '\0';
> +
> + if (str_has_suffix(name, ".bpf.o"))
> + name[strlen(name) - sizeof(".bpf.o") + 1] = '\0';
> + else if (str_has_suffix(name, ".o"))
> + name[strlen(name) - sizeof(".o") + 1] = '\0';
> +
> + sanitize_obj_name(name);
> +}
> +
> +static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
> + const struct bpf_linker_file_opts *opts,
> + struct src_obj *obj)
> {
> #if __BYTE_ORDER == __LITTLE_ENDIAN
> const int host_endianness = ELFDATA2LSB;
> @@ -549,6 +613,14 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>
> obj->filename = filename;
>
> + if (OPTS_GET(opts, object_name, NULL)) {
> + strncpy(obj->obj_name, opts->object_name, MAX_OBJ_NAME_LEN);
> + obj->obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
Looks we don't have examples/selftests which actually use this option.
The only place to use bpf_linker__add_file() is bpftool which did not
have option to overwrite the obj file name.
The code looks fine to me though.
> + } else {
> + get_obj_name(obj->obj_name, filename);
> + }
> + obj->obj_name_len = strlen(obj->obj_name);
> +
> obj->fd = open(filename, O_RDONLY);
> if (obj->fd < 0) {
> err = -errno;
> @@ -2264,6 +2336,47 @@ static int linker_append_btf(struct bpf_linker *linker, struct src_obj *obj)
> obj->btf_type_map[i] = glob_sym->btf_id;
> continue;
> }
> + } else if (btf_is_var(t) && btf_var(t)->linkage == BTF_VAR_STATIC) {
> + /* Static variables are renamed to include
> + * "<obj_name>.." prefix (note double dots), similarly
> + * to how static variables inside functions are named
> + * "<func_name>.<var_name>" by compiler. This allows to
> + * have unique identifiers for static variables across
> + * all linked object files (assuming unique filenames,
> + * of course), which BPF skeleton relies on.
> + *
> + * So worst case static variable inside the function
> + * will have the form "<obj_name>..<func_name>.<var_name"
<var_name => <var_name>
> + * and will get sanitized by BPF skeleton generation
> + * logic to a field with <obj_name>__<func_name>_<var_name>
> + * name. Typical static variable will have a
> + * <obj_name>__<var_name> name, implying arguably nice
> + * per-file scoping.
> + *
> + * If static var name already contains '..', though,
> + * don't rename it, because it was already renamed by
> + * previous linker passes.
> + */
> + name = btf__str_by_offset(obj->btf, t->name_off);
> + if (!strstr(name, "..")) {
> + char new_name[MAX_VAR_NAME_LEN];
> +
> + memcpy(new_name, obj->obj_name, obj->obj_name_len);
> + new_name[obj->obj_name_len] = '.';
> + new_name[obj->obj_name_len + 1] = '.';
> + new_name[obj->obj_name_len + 2] = '\0';
> + /* -3 is for '..' separator and terminating '\0' */
> + strncat(new_name, name, MAX_VAR_NAME_LEN - obj->obj_name_len - 3);
> +
> + id = btf__add_str(obj->btf, new_name);
> + if (id < 0)
> + return id;
> +
> + /* btf__add_str() might invalidate t, so re-fetch */
> + t = btf__type_by_id(obj->btf, i);
> +
> + ((struct btf_type *)t)->name_off = id;
> + }
> }
>
> id = btf__add_type(linker->btf, obj->btf, t);
> diff --git a/tools/testing/selftests/bpf/prog_tests/skeleton.c b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> index fe87b77af459..bbade99fa544 100644
> --- a/tools/testing/selftests/bpf/prog_tests/skeleton.c
> +++ b/tools/testing/selftests/bpf/prog_tests/skeleton.c
> @@ -82,10 +82,10 @@ void test_skeleton(void)
> CHECK(data->out2 != 2, "res2", "got %lld != exp %d\n", data->out2, 2);
> CHECK(bss->out3 != 3, "res3", "got %d != exp %d\n", (int)bss->out3, 3);
> CHECK(bss->out4 != 4, "res4", "got %lld != exp %d\n", bss->out4, 4);
> - CHECK(bss->handler_out5.a != 5, "res5", "got %d != exp %d\n",
> - bss->handler_out5.a, 5);
> - CHECK(bss->handler_out5.b != 6, "res6", "got %lld != exp %d\n",
> - bss->handler_out5.b, 6);
> + CHECK(bss->test_skeleton__handler_out5.a != 5, "res5", "got %d != exp %d\n",
> + bss->test_skeleton__handler_out5.a, 5);
> + CHECK(bss->test_skeleton__handler_out5.b != 6, "res6", "got %lld != exp %d\n",
> + bss->test_skeleton__handler_out5.b, 6);
> CHECK(bss->out6 != 14, "res7", "got %d != exp %d\n", bss->out6, 14);
>
> CHECK(bss->bpf_syscall != kcfg->CONFIG_BPF_SYSCALL, "ext1",
> diff --git a/tools/testing/selftests/bpf/prog_tests/static_linked.c b/tools/testing/selftests/bpf/prog_tests/static_linked.c
> index 46556976dccc..f16736eab900 100644
> --- a/tools/testing/selftests/bpf/prog_tests/static_linked.c
> +++ b/tools/testing/selftests/bpf/prog_tests/static_linked.c
> @@ -14,12 +14,12 @@ void test_static_linked(void)
> return;
>
> skel->rodata->rovar1 = 1;
> - skel->bss->static_var1 = 2;
> - skel->bss->static_var11 = 3;
> + skel->bss->test_static_linked1__static_var = 2;
> + skel->bss->test_static_linked1__static_var1 = 3;
>
> skel->rodata->rovar2 = 4;
> - skel->bss->static_var2 = 5;
> - skel->bss->static_var22 = 6;
> + skel->bss->test_static_linked2__static_var = 5;
> + skel->bss->test_static_linked2__static_var2 = 6;
>
> err = test_static_linked__load(skel);
> if (!ASSERT_OK(err, "skel_load"))
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> index ee49493dc125..43bf8ec8ae79 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_test_kern4.c
> @@ -9,8 +9,8 @@ __u32 map1_id = 0, map2_id = 0;
> __u32 map1_accessed = 0, map2_accessed = 0;
> __u64 map1_seqnum = 0, map2_seqnum1 = 0, map2_seqnum2 = 0;
>
> -static volatile const __u32 print_len;
> -static volatile const __u32 ret1;
> +volatile const __u32 print_len = 0;
> +volatile const __u32 ret1 = 0;
I am little bit puzzled why bpf_iter_test_kern4.c is impacted. I think
this is not in a static link test, right? The same for a few tests below.
>
> SEC("iter/bpf_map")
> int dump_bpf_map(struct bpf_iter__bpf_map *ctx)
> diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c
> index c4a9bae96e75..71184af57749 100644
> --- a/tools/testing/selftests/bpf/progs/test_check_mtu.c
> +++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c
> @@ -11,8 +11,8 @@
> char _license[] SEC("license") = "GPL";
>
> /* Userspace will update with MTU it can see on device */
> -static volatile const int GLOBAL_USER_MTU;
> -static volatile const __u32 GLOBAL_USER_IFINDEX;
> +volatile const int GLOBAL_USER_MTU;
> +volatile const __u32 GLOBAL_USER_IFINDEX;
>
> /* BPF-prog will update these with MTU values it can see */
> __u32 global_bpf_mtu_xdp = 0;
> diff --git a/tools/testing/selftests/bpf/progs/test_cls_redirect.c b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> index 3c1e042962e6..e2a5acc4785c 100644
> --- a/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> +++ b/tools/testing/selftests/bpf/progs/test_cls_redirect.c
> @@ -39,8 +39,8 @@ char _license[] SEC("license") = "Dual BSD/GPL";
> /**
> * Destination port and IP used for UDP encapsulation.
> */
> -static volatile const __be16 ENCAPSULATION_PORT;
> -static volatile const __be32 ENCAPSULATION_IP;
> +volatile const __be16 ENCAPSULATION_PORT;
> +volatile const __be32 ENCAPSULATION_IP;
>
> typedef struct {
> uint64_t processed_packets_total;
> diff --git a/tools/testing/selftests/bpf/progs/test_snprintf_single.c b/tools/testing/selftests/bpf/progs/test_snprintf_single.c
> index 402adaf344f9..6b63ba86b409 100644
> --- a/tools/testing/selftests/bpf/progs/test_snprintf_single.c
> +++ b/tools/testing/selftests/bpf/progs/test_snprintf_single.c
> @@ -5,7 +5,7 @@
> #include <bpf/bpf_helpers.h>
>
> /* The format string is filled from the userspace such that loading fails */
> -static const char fmt[10];
> +const char fmt[10] = {};
>
> SEC("raw_tp/sys_enter")
> int handler(const void *ctx)
> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> index a39eba9f5201..a1cc58b10c7c 100644
> --- a/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_listen.c
> @@ -28,8 +28,8 @@ struct {
> __type(value, unsigned int);
> } verdict_map SEC(".maps");
>
> -static volatile bool test_sockmap; /* toggled by user-space */
> -static volatile bool test_ingress; /* toggled by user-space */
> +bool test_sockmap = false; /* toggled by user-space */
> +bool test_ingress = false; /* toggled by user-space */
>
> SEC("sk_skb/stream_parser")
> int prog_stream_parser(struct __sk_buff *skb)
[...]
Powered by blists - more mailing lists