netdev - Re: [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs

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: <20210311023417.vhwe4avhvri7gcr5@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 10 Mar 2021 18:34:17 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...com,
        daniel@...earbox.net, kernel-team@...com
Subject: Re: [PATCH bpf-next 05/10] libbpf: add BPF static linker APIs

On Tue, Mar 09, 2021 at 08:04:26PM -0800, Andrii Nakryiko wrote:
> +
> +	struct btf *strtab_btf; /* we use struct btf to manage strings */
...
> +	str_off = btf__add_str(linker->strtab_btf, sec->sec_name);
> +	sec->shdr->sh_name = str_off;

That bit took me an hour to grok.
That single line comment above is far far from obvious.
What the logic is relying on is that string section in BTF format
has the same zero terminated set of strings as ELF's .strtab section.
There is no BTF anywhere here in this 'strtab_btf'.
The naming choice made it double hard.
My understanding that you're using that instead of renaming btf_add_mem()
into something generic to rely on string hashmap for string dedup?

The commit log in patch 2 that introduces btf_raw_strs() sort of talks about
this code puzzle, but I would never guessed that's what you meant based
on patch 2 alone.

Did you consider some renaming/generalizing of string management to
avoid btf__add_str() through out the patch 5?
The "btf_" prefix makes things challenging to read.
Especially when patch 6 is using btf__add_str() to add to real BTF.

Mainly pointing it out for others who might be looking at the patches.