All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Yonghong Song <yhs@fb.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 21:28:40 -0700	[thread overview]
Message-ID: <CAEf4BzZjW8OJQjMJ0qCTftnuStUXxKpf4eVrigCLDf4+VcxJSQ@mail.gmail.com> (raw)
In-Reply-To: <8c6d3655-f9c7-d0b0-b10f-00f679c44c1e@fb.com>

On Thu, Apr 22, 2021 at 7:36 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> 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.

Yes, and also PROTECTED + HIDDEN/DEFAULT -> PROTECTED. But we don't
special handle PROTECTED right now. So I think it makes sense to error
out if anyone tries to use PROTECTED, which is why I'll restrict to
HIDDEN and DEFAULT for now.

>
> >>>
> >>
> >> Sure, we can restrict this to STV_DEFAULT and STV_HIDDEN for now. >>
> [...]

  reply	other threads:[~2021-04-23  4:28 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
2021-04-23  4:28           ` Andrii Nakryiko [this message]
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=CAEf4BzZjW8OJQjMJ0qCTftnuStUXxKpf4eVrigCLDf4+VcxJSQ@mail.gmail.com \
    --to=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 \
    --cc=yhs@fb.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.