netdev - Re: [PATCH bpf-next 1/5] libbpf: update BTF reloc support to latest Clang format

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: <4c10947e-5d75-a224-8b9c-6af81fc07324@fb.com>
Date:   Tue, 15 Oct 2019 15:15:03 +0000
From:   Alexei Starovoitov <ast@...com>
To:     Andrii Nakryiko <andriin@...com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>
CC:     "andrii.nakryiko@...il.com" <andrii.nakryiko@...il.com>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/5] libbpf: update BTF reloc support to latest
 Clang format

On 10/14/19 4:49 PM, Andrii Nakryiko wrote:
> BTF offset reloc was generalized in recent Clang into field relocation,
> capturing extra u32 field, specifying what aspect of captured field
> needs to be relocated. This changes .BTF.ext's record size for this
> relocation from 12 bytes to 16 bytes. Given these format changes
> happened in Clang before official released version, it's ok to not
> support outdated 12-byte record size w/o breaking ABI.
> 
> Signed-off-by: Andrii Nakryiko <andriin@...com>
...> -/* The minimum bpf_offset_reloc checked by the loader
> +/* bpf_field_reloc_kind encodes which aspect of captured field has to be
> + * adjusted by relocations. Currently supported values are:
> + *   - BPF_FI_BYTE_OFFSET: field offset (in bytes);
> + *   - BPF_FI_EXISTS: field existence (1 if field exists, 0 - otherwise);
> + */
> +enum bpf_field_reloc_kind {
> +	BPF_FRK_BYTE_OFFSET = 0,
> +	BPF_FRK_EXISTS = 2,
> +};

Comment above doesn't match the enum.
Also in patch 4 :
+enum bpf_field_info_kind {
+	BPF_FI_BYTE_OFFSET = 0,	  /* field byte offset */
+	BPF_FI_EXISTS = 2,	  /* whether field exists in target kernel */
+};

Do you expect that .btf.ext encoding to be different from
argument passed into __builtin_field_info() ?
May be better to design the interface that it always matches
and keep single enum for both?
I don't like either FRK or FI abbreviations.
May be
BPF_FIELD_BYTE_OFFSET
BPF_FIELD_EXISTS ?

These constants would need to be exposed in bpf_core_read.h and
will become uapi as soon as we release next libbpf at
the end of this bpf-next cycle. At that point llvm side would have
to stick to these constants as well.
It would have to understand them as arguments and use the same in .btf.ext.
Does it make sense?