On Tue, Oct 18, 2022 at 11:52 AM <sdf@xxxxxxxxxx> wrote:
>
> On 10/17, Andrii Nakryiko wrote:
> > Teach libbpf to not add BPF_F_MMAPABLE flag unnecessarily for ARRAY maps
> > that are backing data sections, if such data sections don't expose any
> > variables to user-space. Exposed variables are those that have
> > STB_GLOBAL or STB_WEAK ELF binding and correspond to BTF VAR's
> > BTF_VAR_GLOBAL_ALLOCATED linkage.
>
> > The overall idea is that if some data section doesn't have any variable
> > that
> > is exposed through BPF skeleton, then there is no reason to make such
> > BPF array mmapable. Making BPF array mmapable is not a free no-op
> > action, because BPF verifier doesn't allow users to put special objects
> > (such as BPF spin locks, RB tree nodes, linked list nodes, kptrs, etc;
> > anything that has a sensitive internal state that should not be modified
> > arbitrarily from user space) into mmapable arrays, as there is no way to
> > prevent user space from corrupting such sensitive state through direct
> > memory access through memory-mapped region.
>
> > By making sure that libbpf doesn't add BPF_F_MMAPABLE flag to BPF array
> > maps corresponding to data sections that only have static variables
> > (which are not supposed to be visible to user space according to libbpf
> > and BPF skeleton rules), users now can have spinlocks, kptrs, etc in
> > either default .bss/.data sections or custom .data.* sections (assuming
> > there are no global variables in such sections).
>
> > The only possible hiccup with this approach is the need to use global
> > variables during BPF static linking, even if it's not intended to be
> > shared with user space through BPF skeleton. To allow such scenarios,
> > extend libbpf's STV_HIDDEN ELF visibility attribute handling to
> > variables. Libbpf is already treating global hidden BPF subprograms as
> > static subprograms and adjusts BTF accordingly to make BPF verifier
> > verify such subprograms as static subprograms with preserving entire BPF
> > verifier state between subprog calls. This patch teaches libbpf to treat
> > global hidden variables as static ones and adjust BTF information
> > accordingly as well. This allows to share variables between multiple
> > object files during static linking, but still keep them internal to BPF
> > program and not get them exposed through BPF skeleton.
>
> > Note, that if the user has some advanced scenario where they absolutely
> > need BPF_F_MMAPABLE flag on .data/.bss/.rodata BPF array map despite
> > only having static variables, they still can achieve this by forcing it
> > through explicit bpf_map__set_map_flags() API.
>
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> Acked-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
>
> Left a nit for spelling and the same 'log err vs size' question.
>
>
> > ---
> > tools/lib/bpf/libbpf.c | 95 ++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 77 insertions(+), 18 deletions(-)
>
[...]
> > - /* extern-backing datasecs (.ksyms, .kconfig) have their size and
> > - * variable offsets set at the previous step, so we skip any fixups
> > - * for such sections
> > + /* Extern-backing datasecs (.ksyms, .kconfig) have their size and
> > + * variable offsets set at the previous step. Further, not every
> > + * extern BTF VAR has corresponding ELF symbol preserved, so we skip
>
> [..]
>
> > + * all fixups altogether for such sections and go straight to storting
> > + * VARs within their DATASEC.
>
> nit: s/storting/sorting/
eagle eye :) will fix!
>
> > */
> > - if (t->size)
> > + if (strcmp(sec_name, KCONFIG_SEC) == 0 || strcmp(sec_name, KSYMS_SEC)
> > == 0)
> > goto sort_vars;
>
> > - err = find_elf_sec_sz(obj, sec_name, &size);
> > - if (err || !size) {
> > - pr_debug("sec '%s': invalid size %u bytes\n", sec_name, size);
> > - return -ENOENT;
> > - }
> > + /* Clang leaves DATASEC size and VAR offsets as zeroes, so we need to
> > + * fix this up. But BPF static linker already fixes this up and fills
> > + * all the sizes and offsets during static linking. So this step has
> > + * to be optional. But the STV_HIDDEN handling is non-optional for any
> > + * non-extern DATASEC, so the variable fixup loop below handles both
> > + * functions at the same time, paying the cost of BTF VAR <-> ELF
> > + * symbol matching just once.
> > + */
> > + if (t->size == 0) {
> > + err = find_elf_sec_sz(obj, sec_name, &size);
> > + if (err || !size) {
> > + pr_debug("sec '%s': invalid size %u bytes\n", sec_name, size);
>
> nit: same suggestion here - let's log err instead?
sure
>
> > + return -ENOENT;
> > + }
>
> > - t->size = size;
> > + t->size = size;
> > + fixup_offsets = true;
> > + }
>
> > for (i = 0, vsi = btf_var_secinfos(t); i < vars; i++, vsi++) {
> > const struct btf_type *t_var;
[...]
