From: Yonghong Song <yhs@fb.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 11/17] libbpf: add linker extern resolution support for functions and global variables
Date: Thu, 22 Apr 2021 19:36:15 -0700 [thread overview]
Message-ID: <8c6d3655-f9c7-d0b0-b10f-00f679c44c1e@fb.com> (raw)
In-Reply-To: <cffdf4e2-220d-cc2e-67e9-b83e848bdddf@fb.com>
On 4/22/21 4:57 PM, Yonghong Song wrote:
>
>
> On 4/22/21 3:12 PM, Andrii Nakryiko wrote:
>> On Thu, Apr 22, 2021 at 2:27 PM Yonghong Song <yhs@fb.com> wrote:
>>>
>>>
>>>
>>> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
>>>> Add BPF static linker logic to resolve extern variables and
>>>> functions across
>>>> multiple linked together BPF object files.
>>>>
>>>> For that, linker maintains a separate list of struct glob_sym
>>>> structures,
>>>> which keeps track of few pieces of metadata (is it extern or
>>>> resolved global,
>>>> is it a weak symbol, which ELF section it belongs to, etc) and ties
>>>> together
>>>> BTF type info and ELF symbol information and keeps them in sync.
>>>>
>>>> With adding support for extern variables/funcs, it's now possible
>>>> for some
>>>> sections to contain both extern and non-extern definitions. This
>>>> means that
>>>> some sections may start out as ephemeral (if only externs are
>>>> present and thus
>>>> there is not corresponding ELF section), but will be "upgraded" to
>>>> actual ELF
>>>> section as symbols are resolved or new non-extern definitions are
>>>> appended.
>>>>
>>>> Additional care is taken to not duplicate extern entries in sections
>>>> like
>>>> .kconfig and .ksyms.
>>>>
>>>> Given libbpf requires BTF type to always be present for .kconfig/.ksym
>>>> externs, linker extends this requirement to all the externs, even
>>>> those that
>>>> are supposed to be resolved during static linking and which won't be
>>>> visible
>>>> to libbpf. With BTF information always present, static linker will
>>>> check not
>>>> just ELF symbol matches, but entire BTF type signature match as
>>>> well. That
>>>> logic is stricter that BPF CO-RE checks. It probably should be
>>>> re-used by
>>>> .ksym resolution logic in libbpf as well, but that's left for follow up
>>>> patches.
>>>>
>>>> To make it unnecessary to rewrite ELF symbols and minimize BTF type
>>>> rewriting/removal, ELF symbols that correspond to externs initially
>>>> will be
>>>> updated in place once they are resolved. Similarly for BTF type
>>>> info, VAR/FUNC
>>>> and var_secinfo's (sec_vars in struct bpf_linker) are staying
>>>> stable, but
>>>> types they point to might get replaced when extern is resolved. This
>>>> might
>>>> leave some left-over types (even though we try to minimize this for
>>>> common
>>>> cases of having extern funcs with not argument names vs concrete
>>>> function with
>>>> names properly specified). That can be addresses later with a
>>>> generic BTF
>>>> garbage collection. That's left for a follow up as well.
>>>>
>>>> Given BTF type appending phase is separate from ELF symbol
>>>> appending/resolution, special struct glob_sym->underlying_btf_id
>>>> variable is
>>>> used to communicate resolution and rewrite decisions. 0 means
>>>> underlying_btf_id needs to be appended (it's not yet in final
>>>> linker->btf), <0
>>>> values are used for temporary storage of source BTF type ID (not yet
>>>> rewritten), so -glob_sym->underlying_btf_id is BTF type id in
>>>> obj-btf. But by
>>>> the end of linker_append_btf() phase, that underlying_btf_id will be
>>>> remapped
>>>> and will always be > 0. This is the uglies part of the whole
>>>> process, but
>>>> keeps the other parts much simpler due to stability of sec_var and
>>>> VAR/FUNC
>>>> types, as well as ELF symbol, so please keep that in mind while
>>>> reviewing.
>>>
>>> This is indeed complicated. I has some comments below. Please check
>>> whether my understanding is correct or not.
>>>
>>>>
>>>> BTF-defined maps require some extra custom logic and is addressed
>>>> separate in
>>>> the next patch, so that to keep this one smaller and easier to review.
>>>>
>>>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>>>> ---
>>>> tools/lib/bpf/linker.c | 844
>>>> ++++++++++++++++++++++++++++++++++++++---
>>>> 1 file changed, 785 insertions(+), 59 deletions(-)
>>>>
[...]
>>>
>>>> + src_sec = &obj->secs[sym->st_shndx];
>>>> + if (src_sec->skipped)
>>>> + return 0;
>>>> + dst_sec = &linker->secs[src_sec->dst_id];
>>>> +
>>>> + /* allow only one STT_SECTION symbol per section */
>>>> + if (sym_type == STT_SECTION && dst_sec->sec_sym_idx) {
>>>> + obj->sym_map[src_sym_idx] = dst_sec->sec_sym_idx;
>>>> + return 0;
>>>> + }
>>>> + }
>>>> +
>>>> + if (sym_bind == STB_LOCAL)
>>>> + goto add_sym;
>>>> +
>>>> + /* find matching BTF info */
>>>> + err = find_glob_sym_btf(obj, sym, sym_name, &btf_sec_id,
>>>> &btf_id);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + if (sym_is_extern && btf_sec_id) {
>>>> + const char *sec_name = NULL;
>>>> + const struct btf_type *t;
>>>> +
>>>> + t = btf__type_by_id(obj->btf, btf_sec_id);
>>>> + sec_name = btf__str_by_offset(obj->btf, t->name_off);
>>>> +
>>>> + /* Clang puts unannotated extern vars into
>>>> + * '.extern' BTF DATASEC. Treat them the same
>>>> + * as unannotated extern funcs (which are
>>>> + * currently not put into any DATASECs).
>>>> + * Those don't have associated src_sec/dst_sec.
>>>> + */
>>>> + if (strcmp(sec_name, BTF_EXTERN_SEC) != 0) {
>>>> + src_sec = find_src_sec_by_name(obj, sec_name);
>>>> + if (!src_sec) {
>>>> + pr_warn("failed to find matching ELF
>>>> sec '%s'\n", sec_name);
>>>> + return -ENOENT;
>>>> + }
>>>> + dst_sec = &linker->secs[src_sec->dst_id];
>>>> + }
>>>> + }
>>>> +
>>>> + glob_sym = find_glob_sym(linker, sym_name);
>>>> + if (glob_sym) {
>>>> + /* Preventively resolve to existing symbol. This is
>>>> + * needed for further relocation symbol remapping in
>>>> + * the next step of linking.
>>>> + */
>>>> + obj->sym_map[src_sym_idx] = glob_sym->sym_idx;
>>>> +
>>>> + /* If both symbols are non-externs, at least one of
>>>> + * them has to be STB_WEAK, otherwise they are in
>>>> + * a conflict with each other.
>>>> + */
>>>> + if (!sym_is_extern && !glob_sym->is_extern
>>>> + && !glob_sym->is_weak && sym_bind != STB_WEAK) {
>>>> + pr_warn("conflicting non-weak symbol #%d (%s)
>>>> definition in '%s'\n",
>>>> + src_sym_idx, sym_name, obj->filename);
>>>> + return -EINVAL;
>>>> }
>>>>
>>>> + if (!glob_syms_match(sym_name, linker, glob_sym, obj,
>>>> sym, src_sym_idx, btf_id))
>>>> + return -EINVAL;
>>>> +
>>>> + dst_sym = get_sym_by_idx(linker, glob_sym->sym_idx);
>>>> +
>>>> + /* If new symbol is strong, then force dst_sym to be
>>>> strong as
>>>> + * well; this way a mix of weak and non-weak extern
>>>> + * definitions will end up being strong.
>>>> + */
>>>> + if (sym_bind == STB_GLOBAL) {
>>>> + /* We still need to preserve type (NOTYPE or
>>>> + * OBJECT/FUNC, depending on whether the
>>>> symbol is
>>>> + * extern or not)
>>>> + */
>>>> + sym_update_bind(dst_sym, STB_GLOBAL);
>>>> + glob_sym->is_weak = false;
>>>> + }
>>>> +
>>>> + /* Non-default visibility is "contaminating", with
>>>> stricter
>>>> + * visibility overwriting more permissive ones, even
>>>> if more
>>>> + * permissive visibility comes from just an extern
>>>> definition
>>>> + */
>>>> + if (sym_vis > ELF64_ST_VISIBILITY(dst_sym->st_other))
>>>> + sym_update_visibility(dst_sym, sym_vis);
>>>
>>> For visibility, maybe we can just handle DEFAULT and HIDDEN, and others
>>> are not supported? DEFAULT + DEFAULT/HIDDEN => DEFAULT, HIDDEN + HIDDEN
>>> => HIDDEN?
Looking at your selftest. Your current approach, DEFAULT + DEFAULT ->
DEFAULT, HIDDEN + HIDDEN/DEFAULT -> HIDDEN should work fine. This is
also align with ELF principal to accommodate the least permissive
visibility.
>>>
>>
>> Sure, we can restrict this to STV_DEFAULT and STV_HIDDEN for now. >>
[...]
next prev parent reply other threads:[~2021-04-23 2:36 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-16 20:23 [PATCH v2 bpf-next 00/17] BPF static linker: support externs Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 01/17] bpftool: support dumping BTF VAR's "extern" linkage Andrii Nakryiko
2021-04-22 3:02 ` Yonghong Song
2021-04-16 20:23 ` [PATCH v2 bpf-next 02/17] bpftool: dump more info about DATASEC members Andrii Nakryiko
2021-04-22 3:09 ` Yonghong Song
2021-04-16 20:23 ` [PATCH v2 bpf-next 03/17] libbpf: suppress compiler warning when using SEC() macro with externs Andrii Nakryiko
2021-04-22 3:47 ` Yonghong Song
2021-04-22 3:59 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 04/17] libbpf: mark BPF subprogs with hidden visibility as static for BPF verifier Andrii Nakryiko
2021-04-22 5:43 ` Yonghong Song
2021-04-22 18:09 ` Andrii Nakryiko
2021-04-22 23:00 ` Yonghong Song
2021-04-22 23:28 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 05/17] libbpf: allow gaps in BPF program sections to support overriden weak functions Andrii Nakryiko
2021-04-22 6:25 ` Yonghong Song
2021-04-22 18:10 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 06/17] libbpf: refactor BTF map definition parsing Andrii Nakryiko
2021-04-22 15:33 ` Yonghong Song
2021-04-22 18:40 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 07/17] libbpf: factor out symtab and relos sanity checks Andrii Nakryiko
2021-04-22 16:06 ` Yonghong Song
2021-04-22 18:16 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 08/17] libbpf: make few internal helpers available outside of libbpf.c Andrii Nakryiko
2021-04-22 16:19 ` Yonghong Song
2021-04-22 18:18 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 09/17] libbpf: extend sanity checking ELF symbols with externs validation Andrii Nakryiko
2021-04-22 16:35 ` Yonghong Song
2021-04-22 18:20 ` Andrii Nakryiko
2021-04-22 23:13 ` Yonghong Song
2021-04-16 20:23 ` [PATCH v2 bpf-next 10/17] libbpf: tighten BTF type ID rewriting with error checking Andrii Nakryiko
2021-04-22 16:50 ` Yonghong Song
2021-04-22 18:24 ` Andrii Nakryiko
2021-04-23 2:54 ` Alexei Starovoitov
2021-04-23 4:25 ` Andrii Nakryiko
2021-04-23 5:11 ` Alexei Starovoitov
2021-04-23 16:09 ` Andrii Nakryiko
2021-04-23 16:18 ` Alexei Starovoitov
2021-04-23 16:30 ` Andrii Nakryiko
2021-04-23 16:34 ` Alexei Starovoitov
2021-04-23 17:02 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 11/17] libbpf: add linker extern resolution support for functions and global variables Andrii Nakryiko
2021-04-22 21:27 ` Yonghong Song
2021-04-22 22:12 ` Andrii Nakryiko
2021-04-22 23:57 ` Yonghong Song
2021-04-23 2:36 ` Yonghong Song [this message]
2021-04-23 4:28 ` Andrii Nakryiko
2021-04-23 4:27 ` Andrii Nakryiko
2021-04-16 20:23 ` [PATCH v2 bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section Andrii Nakryiko
2021-04-22 22:56 ` Yonghong Song
2021-04-22 23:32 ` Andrii Nakryiko
2021-04-16 20:24 ` [PATCH v2 bpf-next 13/17] selftests/bpf: use -O0 instead of -Og in selftests builds Andrii Nakryiko
2021-04-23 0:05 ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 14/17] selftests/bpf: omit skeleton generation for multi-linked BPF object files Andrii Nakryiko
2021-04-23 0:13 ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 15/17] selftests/bpf: add function linking selftest Andrii Nakryiko
2021-04-23 0:50 ` Yonghong Song
2021-04-23 2:34 ` Alexei Starovoitov
2021-04-23 4:29 ` Andrii Nakryiko
2021-04-23 17:18 ` Andrii Nakryiko
2021-04-23 17:35 ` Yonghong Song
2021-04-23 17:55 ` Andrii Nakryiko
2021-04-23 17:58 ` Yonghong Song
2021-04-23 17:59 ` Andrii Nakryiko
2021-04-16 20:24 ` [PATCH v2 bpf-next 16/17] selftests/bpf: add global variables " Andrii Nakryiko
2021-04-23 1:01 ` Yonghong Song
2021-04-16 20:24 ` [PATCH v2 bpf-next 17/17] sleftests/bpf: add map " Andrii Nakryiko
2021-04-23 1:20 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8c6d3655-f9c7-d0b0-b10f-00f679c44c1e@fb.com \
--to=yhs@fb.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).