[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <65869842-b1a0-5e95-9ca2-42aaf86644a8@fb.com>
Date: Thu, 22 Apr 2021 16:00:20 -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 04/17] libbpf: mark BPF subprogs with hidden
visibility as static for BPF verifier
On 4/22/21 11:09 AM, Andrii Nakryiko wrote:
> On Wed, Apr 21, 2021 at 10:43 PM Yonghong Song <yhs@...com> 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@...nel.org>
>>> ---
>>> 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.
That is fine. Conflict of __hidden is a low risk and we can deal with it
later if needed.
>
>
>>> +
>>> /* 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?
Yes, let us just deal with STV_DEFAULT and STV_HIDDEN. We already
specialized STV_HIDDEN, so we should not treat STV_INTERNAL/PROTECTED
as what they mean in ELF standard, so let us disable them for now.
>
> 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;
>>>
>
> [...]
>
Powered by blists - more mailing lists