| 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: <065e8768-b066-185f-48f9-7ca8f15a2547@fb.com>
Date: Fri, 23 Apr 2021 14:56:02 -0700
From: Yonghong Song <yhs@...com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
CC: Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>,
Networking <netdev@...r.kernel.org>,
Alexei Starovoitov <ast@...com>,
Daniel Borkmann <daniel@...earbox.net>,
Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during
linking
On 4/23/21 2:38 PM, Andrii Nakryiko wrote:
> On Fri, Apr 23, 2021 at 1:24 PM Yonghong Song <yhs@...com> wrote:
>>
>>
>>
>> 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"
>
> heh, I knew this would be confusing :) it's "go in" a verb and "in the
> same patch" as where they go into. But I'll re-phrase in the next
> version.
>
>>
>>> 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(-)
>>>
>
> [...]
>
>>> +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.
>
> Right, I was a bit lazy in adding this to bpftool, but I'm sure we'll
> want to support overriding obj file name, at least to resolve object
> file name conflicts. One other problem is syntax, I haven't thought
> through what's the best way to do this with bpftool. Something like
> this would do it:
>
> bpftool gen object dest.o input1.o=custom_obj_name
> path/to/file2.o=another_custom_obj_name
>
> But this is too bike-shedding a topic which I want to avoid for now.
Okay. The additional option thing can be done later.
>
>>
>>> + } 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>
>
> good catch, will fix
>
>>> + * 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;
>>> + }
>>> }
>>>
>
> [...]
>
>>> 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.
>
> All the selftests are passed through a static linker, so it will
> append obj_name to each static variable. So I just minimized use of
> static variables to avoid too much code churn. If this variable was
> static, it would have to be accessed as
> skel->rodata->bpf_iter_test_kern4__print_len, for example.
Okay this should be fine. selftests/bpf specific. I just feel that
some people may get confused if they write/see a single program in
selftest and they have to use obj_varname format and thinking this
is a new standard, but actually it is due to static linking buried
in Makefile. Maybe add a note in selftests/README.rst so we
can point to people if there is confusion.
>
>>
>>>
>>> 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";
>>>
>
> [...]
>
Powered by blists - more mailing lists