On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@xxxxxx> wrote:
>
>
>
> On 4/16/21 1:23 PM, Andrii Nakryiko wrote:
> > Define __hidden helper macro in bpf_helpers.h, which is a short-hand for
> > __attribute__((visibility("hidden"))). Add libbpf support to mark BPF
> > subprograms marked with __hidden as static in BTF information to enforce BPF
> > verifier's static function validation algorithm, which takes more information
> > (caller's context) into account during a subprogram validation.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > tools/lib/bpf/bpf_helpers.h | 8 ++++++
> > tools/lib/bpf/btf.c | 5 ----
> > tools/lib/bpf/libbpf.c | 45 ++++++++++++++++++++++++++++++++-
> > tools/lib/bpf/libbpf_internal.h | 6 +++++
> > 4 files changed, 58 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 75c7581b304c..9720dc0b4605 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -47,6 +47,14 @@
> > #define __weak __attribute__((weak))
> > #endif
> >
> > +/*
> > + * Use __hidden attribute to mark a non-static BPF subprogram effectively
> > + * static for BPF verifier's verification algorithm purposes, allowing more
> > + * extensive and permissive BPF verification process, taking into account
> > + * subprogram's caller context.
> > + */
> > +#define __hidden __attribute__((visibility("hidden")))
>
> To prevent potential external __hidden macro definition conflict, how
> about
>
> #ifdef __hidden
> #undef __hidden
> #define __hidden __attribute__((visibility("hidden")))
> #endif
>
We do force #undef only with __always_inline because of the bad
definition in linux/stddef.h And we check #ifndef for __weak, because
__weak is defined in kernel headers. This is not really the case for
__hidden, the only definition is in
tools/lib/traceevent/event-parse-local.h, which I don't think we
should worry about in BPF context. So I wanted to keep it simple and
fix only if that really causes some real conflicts.
And keep in mind that in BPF code bpf_helpers.h is usually included as
one of the first few headers anyways.
> > +
> > /* When utilizing vmlinux.h with BPF CO-RE, user BPF programs can't include
> > * any system-level headers (such as stddef.h, linux/version.h, etc), and
> > * commonly-used macros like NULL and KERNEL_VERSION aren't available through
[...]
> > @@ -698,6 +700,15 @@ bpf_object__add_programs(struct bpf_object *obj, Elf_Data *sec_data,
> > if (err)
> > return err;
> >
> > + /* if function is a global/weak symbol, but has hidden
> > + * visibility (or any non-default one), mark its BTF FUNC as
> > + * static to enable more permissive BPF verification mode with
> > + * more outside context available to BPF verifier
> > + */
> > + if (GELF_ST_BIND(sym.st_info) != STB_LOCAL
> > + && GELF_ST_VISIBILITY(sym.st_other) != STV_DEFAULT)
>
> Maybe we should check GELF_ST_VISIBILITY(sym.st_other) == STV_HIDDEN
> instead?
It felt like only STV_DEFAULT should be "exported", semantically
speaking. Everything else would be treated as if it was static, except
that C rules require that function has to be global. Do you think
there is some danger to do it this way?
Currently static linker doesn't do anything special for STV_INTERNAL
and STV_PROTECTED, so we could just disable those. Do you prefer that?
I just felt that there is no risk of regression if we do this for
non-STV_DEFAULT generically.
>
> > + prog->mark_btf_static = true;
> > +
> > nr_progs++;
> > obj->nr_programs = nr_progs;
> >
[...]
