Re: unstable bpf helpers proposal. Was: [PATCH bpf-next v2 1/2] bpf: add bpf_ct_lookup_{tcp,udp}() helpers

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Apr 03, 2020 at 04:56:01PM -0700, Matt Cover wrote:
> > I think doing BTF annotation for EXPORT_SYMBOL_BPF(bpf_icmp_send); is trivial.
> 
> I've been looking into this more; here is what I'm thinking.
> 
> 1. Export symbols for bpf the same as modules, but into one or more
>    special namespaces.
> 
>    Exported symbols recently gained namespaces.
>      https://lore.kernel.org/linux-usb/20190906103235.197072-1-maennich@google.com/
>      Documentation/kbuild/namespaces.rst
> 
>    This makes the in-kernel changes needed for export super simple.
> 
>      #define EXPORT_SYMBOL_BPF(sym)     EXPORT_SYMBOL_NS(sym, BPF_PROG)
>      #define EXPORT_SYMBOL_BPF_GPL(sym) EXPORT_SYMBOL_NS_GPL(sym, BPF_PROG)
> 
>    BPF_PROG is our special namespace above. We can easily add
>    BPF_PROG_ACQUIRES and BPF_PROG_RELEASES for those types of
>    unstable helpers.
> 
>    Exports for bpf progs are then as simple as for modules.
> 
>      EXPORT_SYMBOL_BPF(bpf_icmp_send);
> 
>    Documenting these namespaces as not for use by modules should be
>    enough; an explicit import statement to use namespaced symbols is
>    already required. Explicitly preventing module use in
>    MODULE_IMPORT_NS or modpost are also options if we feel more is
>    needed.
> 
> 2. Teach pahole's (dwarves') dwarf loader to parse __ksymtab*.
> 
>    I've got a functional wip which retrieves the namespace from the
>    __kstrtab ELF section. Working to differentiate between __ksymtab
>    and __ksymtab_gpl symbols next. Good news is this info is readily
>    available in vmlinux and module .o files. The interface here will
>    probably end up similar to dwarves' elf_symtab__*, but with an
>    struct elf_ksymtab per __ksymtab* section (all pointing to the
>    same __kstrtab section though).
> 
> 3. Teach pahole's btf encoder to encode the following bools: export,
>    gpl_only, acquires, releases.
> 
>    I'm envisioning this info will end up in a new struct
>    btf_func_proto in btf.h. Perhaps like this.
> 
>      struct btf_func_proto {
>          /* "info" bits arrangement
>           * bit     0: exported (callable by bpf prog)
>           * bit     1: gpl_only (only callable from GPL licensed bpf prog)
>           * bit     2: acquires (acquires and returns a refcounted pointer)
>           * bit     3: releases (first argument, a refcounted pointer,
> is released)
>           * bits 4-31: unused
>           */
>          __u32    info;
>      };
> 
>    Currently, a "struct btf_type" of type BTF_KIND_FUNC_PROTO is
>    directly followed by vlen struct btf_param/s. I'm hoping we can
>    insert btf_func_proto before the first btf_param or after the
>    last. If that's not workable, adding a new type,
>    BTF_KIND_FUNC_EXPORT, is another idea.

I don't see why 1 and 2 are necessary.
What is the value of true export_symbol here?
What is the value of namespaced true export_symbol?
Imo it only adds memory overhead to vmlinux.
The same information is available in BTF as a _name_.
What is the point to replicate it into kcrc?
Imo kcrc is a poor protection mechanism that is already worse
that BTF. I really don't see a value going that route.

I think just encoding the intent to export into BTF is enough.
Option 3 above looks like overkill too. Just name convention would do.
We already use different prefixes to encode certain BTFs
(see struct_ops and btf_trace).
Just say when BTF func_proto starts with "export_" it means it's exported.
It would be trivial for users to grep as well:
bpftool btf dump file ./vmlinux |grep export_

> 
> The crcs could be used to improve the developer experience when
> using unstable helpers.

crc don't add any additional value on top of BTF. BTF types has to match exactly.
It's like C compiler checking that you can call a function with correct proto.