netdev - Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for BTF-defined maps in .maps section

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3f3bcc5-be1a-6d11-0c6e-081fc30367c4@fb.com>
Date:   Wed, 14 Apr 2021 15:00:27 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Andrii Nakryiko <andrii@...nel.org>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>, <daniel@...earbox.net>
CC:     <kernel-team@...com>
Subject: Re: [PATCH bpf-next 12/17] libbpf: support extern resolution for
 BTF-defined maps in .maps section

On 4/14/21 1:01 PM, Andrii Nakryiko wrote:
> Add extra logic to handle map externs (only BTF-defined maps are supported for
> linking). Re-use the map parsing logic used during bpf_object__open(). Map
> externs are currently restricted to always and only specify map type, key
> type and/or size, and value type and/or size. Nothing extra is allowed. If any
> of those attributes are mismatched between extern and actual map definition,
> linker will report an error.

I don't get the motivation for this.
It seems cumbersome to force users to do:
+extern struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__type(key, key_type);
+	__type(value, value_type);
+	/* no max_entries on extern map definitions */
+} map1 SEC(".maps");

> The original intent was to allow for extern to specify attributes that matters
> (to user) to enforce. E.g., if you specify just key information and omit
> value, then any value fits. Similarly, it should have been possible to enforce
> map_flags, pinning, and any other possible map attribute. Unfortunately, that
> means that multiple externs can be only partially overlapping with each other,
> which means linker would need to combine their type definitions to end up with
> the most restrictive and fullest map definition.

but there is only one such full map definition.
Can all externs to be:
extern struct {} map1 SEC(".maps");

They can be in multiple .o files, but one true global map def
should have all the fields and will take the precedence during
the linking.

The map type, key size, value size, max entries are all irrelevant
during compilation. They're relevant during loading, but libbpf is
not going to load every .o individually. So "extern map" can
have any fields it wouldn't change the end result after linking.
May be enforce that 'extern struct {} map' doesn't have
any fields defined instead?
It seems asking users to copy-paste map defs in one file and in all
of extern is just extra hassle.
The users wouldn't want to copy-paste them for production code,
but will put map def into .h and include in multiple .c,
but adding "extern " in many .c-s and not
adding that "extern " is the main .c is another macro hassle.
Actually forcing "no max_entries in extern" is killing this idea.
So it's mandatory copy-paste or even more macro magic with partial
defs of maps?
What am I missing?