On Mon, Dec 5, 2022 at 5:54 AM Per Sundström XP
<per.xp.sundstrom@xxxxxxxxxxxx> wrote:
>
> On Wed, 2022-11-30 at 15:11 -0800, Andrii Nakryiko wrote:
> > On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > > On Wed, 2022-11-30 at 14:49 -0800, Andrii Nakryiko wrote:
> > > > On Tue, Nov 29, 2022 at 6:29 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > > > > On Tue, 2022-11-29 at 16:27 -0800, Andrii Nakryiko wrote:
> > > > > > On Tue, Nov 29, 2022 at 9:38 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > > > > > > On Wed, 2022-11-23 at 18:37 -0800, Andrii Nakryiko wrote:
> > > > > > > > On Fri, Nov 18, 2022 at 9:26 AM Per Sundström XP
> > > > > > > > <per.xp.sundstrom@xxxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > > > ============ Vanilla ==========
> > > > > > > > > > > struct foo {
> > > > > > > > > > > struct {
> > > > > > > > > > > int aa;
> > > > > > > > > > > char ab;
> > > > > > > > > > > } a;
> > > > > > > > > > > long :64;
> > > > > > > > > > > int :4;
> > > > > > > > > > > char b;
> > > > > > > > > > > short c;
> > > > > > > > > > > };
> > > > > > > > > > > offsetof(struct foo, c)=18
> > > > > > > > > > >
> > > > > > > > > > > ============ Custom ==========
> > > > > > > > > > > struct foo {
> > > > > > > > > > > long: 8;
> > > > > > > > > > > long: 64;
> > > > > > > > > > > long: 64;
> > > > > > > > > > > char b;
> > > > > > > > > > > short c;
> > > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > so I guess the issue is that the first 'long: 8' is padded to full
> > > > > > > > > > long: 64 ?
> > > > > > > > > >
> > > > > > > > > > looks like btf_dump_emit_bit_padding did not take into accout the gap
> > > > > > > > > > on the
> > > > > > > > > > begining of the struct
> > > > > > > > > >
> > > > > > > > > > on the other hand you generated that header file from 'min_core_btf'
> > > > > > > > > > btf data,
> > > > > > > > > > which takes away all the unused fields.. it might not beeen
> > > > > > > > > > considered as a
> > > > > > > > > > use case before
> > > > > > > > > >
> > > > > > > > > > jirka
> > > > > > > > > >
> > > > > > > > > > That could be the case, but I think the 'emit_bit_padding()' will not
> > > > > > > > > > really have a
> > > > > > > > > > lot to do for the non sparse headers ..
> > > > > > > > > > /Per
> > > > > > > > >
> > > > > > > > > Looks like something like this makes tings a lot better:
> > > > > > > >
> > > > > > > > yep, this helps, though changes output with padding to more verbose
> > > > > > > > version, quite often unnecessarily. I need to thing a bit more on
> > > > > > > > this, but the way we currently calculate alignment of a type is not
> > > > > > > > always going to be correct. E.g., just because there is an int field,
> > > > > > > > doesn't mean that struct actually has 4-byte alignment.
> > > > > > > >
> > > > > > > > We must take into account natural alignment, but also actual
> > > > > > > > alignment, which might be different due to __attribute__((packed)).
> > > > > > > >
> > > > > > > > Either way, thanks for reporting!
> > > > > > >
> > > > > > > Hi everyone,
> > > > > > >
> > > > > > > I think the fix is simpler:
> > > > > > >
> > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> > > > > > > index deb2bc9a0a7b..23a00818854b 100644
> > > > > > > --- a/tools/lib/bpf/btf_dump.c
> > > > > > > +++ b/tools/lib/bpf/btf_dump.c
> > > > > > > @@ -860,7 +860,7 @@ static bool btf_is_struct_packed(const struct btf *btf, __u32 id,
> > > > > > >
> > > > > > > static int chip_away_bits(int total, int at_most)
> > > > > > > {
> > > > > > > - return total % at_most ? : at_most;
> > > > > > > + return total > at_most ? at_most : total;
> > > > > > > }
> > > > > > >
> > > > > > > It changes the order in which btf_dump_emit_bit_padding() prints field
> > > > > > > sizes. Right now it returns the division remainder on a first call and
> > > > > > > full 'at_most' values at subsequent calls. For this particular example
> > > > > > > the bit offset of 'b' is 136, so the output looks as follows:
> > > > > > >
> > > > > > > struct foo {
> > > > > > > long: 8; // first call pad_bits = 136 % 64 ? : 64; off_diff -= 8;
> > > > > > > long: 64; // second call pad_bits = 128 % 64 ? : 64; off_diff -= 64; ...
> > > > > > > long: 64;
> > > > > > > char b;
> > > > > > > short c;
> > > > > > > };
> > > > > > >
> > > > > > > This is incorrect, because compiler would always add padding between
> > > > > > > the first and second members to account for the second member alignment.
> > > > > > >
> > > > > > > However, my change inverts the order, which avoids the accidental
> > > > > > > padding and gets the desired output:
> > > > > > >
> > > > > > > ============ Custom ==========
> > > > > > > struct foo {
> > > > > > > long: 64;
> > > > > > > long: 64;
> > > > > > > char: 8;
> > > > > > > char b;
> > > > > > > short c;
> > > > > > > };
> > > > > > > offsetof(struct foo, c)=18
> > > > > > >
> > > > > > > === BTF offsets ===
> > > > > > > full : 'c' type_id=6 bits_offset=144
> > > > > > > custom : 'c' type_id=3 bits_offset=144
> > > > > > >
> > > > > > > wdyt?
> > > > > >
> > > > > > There were at least two issues I realized when I was thinking about
> > > > > > fixing this, and I think you are missing at least one of them.
> > > > > >
> > > > > > 1. Adding `long :xxx` as padding makes struct at least 8-byte aligned.
> > > > > > If the struct originally had a smaller alignment requirement, you are
> > > > > > now potentially breaking the struct layout by changing its layout.
> > > > > >
> > > > > > 2. The way btf__align_of() is calculating alignment doesn't work
> > > > > > correctly for __attribute__((packed)) structs.
> > > > >
> > > > > Missed these point, sorry.
> > > > > On the other hand isn't this information lost in the custom.btf?
> > > > >
> > > > > $ bpftool btf dump file custom.btf
> > > > > [1] STRUCT 'foo' size=20 vlen=2
> > > > > 'b' type_id=2 bits_offset=136
> > > > > 'c' type_id=3 bits_offset=144
> > > > > [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
> > > > > [3] INT 'short' size=2 bits_offset=0 nr_bits=16 encoding=SIGNED
> > > > >
> > > > > This has no info that 'foo' had fields of size 'long'. It does not
> > > > > matter for structs used inside BTF because 'bits_offset' is specified
> > > > > everywhere, but would matter if STRUCT 'foo' is used as a member of a
> > > > > non-BTF struct.
> > > >
> > > > Yes, the latter is important, though, right?
> > >
> > > Do you want to do anything about this at the custom BTF creation stage?
> >
> > No, absolutely not. We just need to teach btf_dump.c to not introduce
> > any new alignment requirements while taking advantage of existing
> > ones. We can derive enough information from BTF to achieve this.
> >
> > > E.g. leave one real member / create a synthetic member to force a specific
> > > struct alignment in the minimized version.
> > >
> > > > So I think ideally we determine "maximum allowable alignment" and use
> > > > that to determine what's the allowable set of padding types is. WDYT?
> > >
> > > Yes, I agree.
> > > I think that a change in the btf__align_of() should be minimal, just check
> > > if structure is packed and if so return 1, otherwise logic should remain
> > > unchanged, this would match what LLVM does ([1]).
> > > Also the flip of the order of chip_away_bits() should remain.
> >
> > Let's come up with a few tricky examples trying to break existing
> > logic and then fix it. I suspect just chip_away_bits() changes are not
> > sufficient.
>
> I have been using this python script to produce code that verifies
> offsets for struct members for some various kernel 'btf's.
> It compares the offsets from 'bits_offset' generated with 'bpftool
> btf dump <file>' (without 'format c') and the offsets computed by
> 'gcc' from header files generated with 'format c'.
>
> Use as: './verify_header_offsets.py <path to btf>'
>
> It will by default skip 50% of the members to make it harder for
> bpftool to produce correct offsets (can be changed with environment
> variable "RANDOM_SKIP_MEMBERS=<value between 0.0 and 1.0>"
>
> 'clang' does not play well with these big files, so I need to divide
> the generated files into batches. Default is 1000 structs.
> (can be controlled with environment variable 'MAX_STRUCTS')
>
Hi Per,
Your script was very useful and instrumental to get to the final state
of the fixes I just submitted ([0]). Thanks to it I also discovered
for myself mode(byte) and mode(word) attributes, which influences
sizing of enums. All the issues I found were fixed in my patch set.
That said, your script isn't always correct, it seems. I've noticed
that often when there are two structs involved, one of which is
embedded in another as a field, offset assumptions will be incorrect.
I suspect it has something to do with your script modifying nested
struct definition in a way that actually changes its size, but offset
checks don't take this into the account.
I haven't spent time trying to understand or fix it, though. The
script itself is great, and it would be nice to have it in
selftests/bpf available at least for manual testing. So if you can,
please improve it and submit it for inclusion.
Oh, another issue is that sometimes clang will crash due to some
condition violation when related to bitfields. It would be nice to
either avoid this, or at least not spam the output. Please check that
as well.
But overall, I've found multiple issues as I worked on this, so it
definitely was very useful. Thank you!
[0] https://patchwork.kernel.org/project/netdevbpf/list/?series=703071&state=*
> /Per
>
>
> ---------- verify_header_offsets.py ------------
> #!/usr/bin/env python3
>
> import os
> import sys
> import time
> import random
> import tempfile
> import subprocess as sp
>
> structs_count = {}
> n_files = 0
> done = False
>
> class AppError(Exception):
> """
> Class used for application generated exceptions
> """
> pass
>
>
> def shell_cmd(command, **kwargs):
> print(f"COMMAND: {command}")
> res = sp.run(["bash", "-c", command], **kwargs)
> if res.returncode != 0:
> raise AppError(f'shell command "{command} failed')
> return res
>
>
> def find_struct_members(btf_file):
> struct_list = []
> members = []
> found = False
> name = ""
>
> res = shell_cmd(f"bpftool btf dump file {btf_file}", universal_newlines=True, stdout=sp.PIPE)
>
> for line in res.stdout.splitlines():
> #print(line, flush=True)
> if found and line.startswith("\t"):
> member = line.split()[0].replace("'", "")
> # Get "Error: error recording relocations for <file>.o: Invalid argument" in 'bpftool'
> # for some structs. Skip for now
> if member in ['(anon)', 'context', 'inflate_state', 'dma_fence_array', 'net_generic']:
> continue
> bit_offset = int(line.split()[2].replace("bits_offset=", ""))
> bitfield_size = int(line.split()[-1].replace("bitfield_size=", "")) if line.find("bitfield_size") > 0 else 0
> if random.random() > float(os.environ.get('RANDOM_SKIP_MEMBERS', '0.5')):
> members.append((member, bitfield_size, bit_offset))
> else:
> # flag skipped members with (0,0) so we can log them later
> members.append((member, 0, 0))
> elif found:
> found = False
> struct_list.append((name, members))
> name = ""
> members = []
> if line.find(" STRUCT ") > 0:
> name = line.split()[2].replace("'", "")
> if name in ["(anon)"]:
> continue
> structs_count[name] = structs_count.get(name, 0) + 1
> found = True
>
> # Due to limitations in "clang" we need to split the
> # verification into batches
> split_n_structs = int(os.environ.get('MAX_STRUCTS', 1000))
> structs = []
> batches = []
> for n, struct in enumerate(struct_list):
> if n and n % split_n_structs == 0:
> batches.append(structs)
> structs = []
> structs.append(struct)
>
> batches.append(structs)
> return batches
>
> def generate_header(dir, btf_file):
> shell_cmd(f"bpftool btf dump file {btf_file} format c > {dir}/test.h")
>
> def generate_verification_code(dir, btf_file, struct_batch):
> code = ""
> main_body = ""
>
> code += f'#include "test_{n_files}.h"\n'
> code += 'int printf(const char *format, ...);\n'
> code += 'int sprintf(char *str, const char *format, ...);\n'
> code += '#define offsetof(TYPE, MEMBER) ((long) &((TYPE*)0)->MEMBER)\n'
> for name, members in struct_batch:
> if structs_count[name] > 1:
> # structs seen more than one time will be called 'struct foo___<n>' in
> # the generated header file. Only that the '<n>' seems arbitrary, so skip
> # them for now
> continue
> if name in ['context']:
> # for some reason, there are missing structs in the generated header file
> # skip them
> continue
>
> code += f"int __ref_func_struct_{name}() {{\n"
> code += f" int ret = 0;\n"
> code += f" char data[100];\n"
> for member, bitfield_size, bit_offset in members:
> if bitfield_size:
> code += f' ret += ((struct {name}*)&data)->{member}; /* bit_offset={bit_offset}, bitfield_size={bitfield_size} */\n'
> else:
> if bitfield_size == 0 and bit_offset == 0:
> # Skip verifying non bitfield member at offset 0, will always be correct
> code += f' /* ret += offsetof(struct {name}, {member}); Skipped */\n'
> continue
> code += f' ret += offsetof(struct {name}, {member});\n'
> main_body += f' offset = offsetof(struct {name}, {member});\n'
> main_body += f' sprintf(line, "offsetof(struct {name}, {member}) = %d", offset);\n'
> main_body += f' printf("%-80.80s %s\\n", line, offset == {int(bit_offset/8)} ? "OK" : "Not OK (should be {int(bit_offset/8)})");\n'
> code += " return ret;\n"
> code += "}\n"
> code += 'int main() {\n'
> code += '#ifdef VERIFY\n'
> code += ' char line[200];\n'
> code += ' int offset = 0;\n'
> code += ' int dummy = 0;\n'
> code += main_body
> code += '#endif\n'
> code += ' return 0;\n'
> code += '}\n'
> with open(f"{dir}/test_{n_files}.c", "w") as f:
> f.write(code)
>
> def compile_btf_object(dir, btf_file):
> shell_cmd(f"cp {dir}/test.h {dir}/test_{n_files}.h")
> shell_cmd(f"clang -c -I{dir} -ggdb -gdwarf -fpie -target bpf -D__TARGET_ARCH_x86 -o {dir}/test_{n_files}.o {dir}/test_{n_files}.c")
> shell_cmd(f"bpftool gen min_core_btf {btf_file} {dir}/test.btf {dir}/test_{n_files}.o")
> shell_cmd(f"bpftool btf dump file {dir}/test.btf format c > {dir}/test_{n_files}.h")
>
> def compile_and_run_verification(dir):
> shell_cmd(f"gcc -DVERIFY -I{dir} -o {dir}/test_{n_files} {dir}/test_{n_files}.c")
> shell_cmd(f"{dir}/test_{n_files}")
>
> def main():
> global n_files
> global done
> if len(sys.argv) > 1 and os.path.exists(sys.argv[1]):
> btf_file = sys.argv[1]
> print(f"Verifying btf file {btf_file}", flush=True)
> #with tempfile.TemporaryDirectory() as dir:
> dir="/tmp"
> try:
> generate_header(dir, btf_file)
> for batch in find_struct_members(btf_file):
> generate_verification_code(dir, btf_file, batch)
> try:
> compile_btf_object(dir, btf_file)
> except AppError as fault:
> print(f"Error: {fault}", file=sys.stderr)
> print(f".. ignore ..", file=sys.stderr)
> continue
> compile_and_run_verification(dir)
> if done:
> break
> n_files += 1
> except AppError as fault:
> print(f"Error: {fault}", file=sys.stderr)
> sys.exit(1)
>
> if __name__ == "__main__":
> main()
>
> >
> > > [1]
> > > https://protect2.fireeye.com/v1/url?k=31323334-501cfaf3-313273af-454445554331-e6381a6a39d24e8d&q=1&e=50f6402e-fdb7-4512-8c16-8ce450e943f7&u=https%3A%2F%2Fgithub.com%2Feddyz87%2Fllvm-project%2Fblob%2Fmain%2Fllvm%2Flib%2FIR%2FDataLayout.cpp%23L764
> > > > > > So we need to fix btf__align_of() first. What btf__align_of() is
> > > > > > calculating right now is a natural alignment requirement if we ignore
> > > > > > actual field offsets. This might be useful (at the very least to
> > > > > > determine if the struct is packed or not), so maybe we should have a
> > > > > > separate btf__natural_align_of() or something along those lines?
> > > > > >
> > > > > > And then we need to fix btf_dump_emit_bit_padding() to better handle
> > > > > > alignment and padding rules. This is what Per Sundström is trying to
> > > > > > do, I believe, but I haven't carefully thought about his latest code
> > > > > > suggestion.
> > > > > >
> > > > > > In general, the most obvious solution would be to pad with `char :8;`
> > > > > > everywhere, but that's very ugly and I'd prefer us to have as
> > > > > > "natural" output as possible. That is, only emit strictly necessary
> > > > > > padding fields and rely on natural alignment otherwise.
> > > > > >
> > > > > > >
> > > > > > > > > diff --git a/src/btf_dump.c b/src/btf_dump.c
> > > > > > > > > index 12f7039..a8bd52a 100644
> > > > > > > > > --- a/src/btf_dump.c
> > > > > > > > > +++ b/src/btf_dump.c
> > > > > > > > > @@ -881,13 +881,13 @@ static void btf_dump_emit_bit_padding(const
> > > > > > > > > struct btf_dump *d,
> > > > > > > > > const char *pad_type;
> > > > > > > > > int pad_bits;
> > > > > > > > >
> > > > > > > > > - if (ptr_bits > 32 && off_diff > 32) {
> > > > > > > > > + if (align > 4 && ptr_bits > 32 && off_diff > 32) {
> > > > > > > > > pad_type = "long";
> > > > > > > > > pad_bits = chip_away_bits(off_diff, ptr_bits);
> > > > > > > > > - } else if (off_diff > 16) {
> > > > > > > > > + } else if (align > 2 && off_diff > 16) {
> > > > > > > > > pad_type = "int";
> > > > > > > > > pad_bits = chip_away_bits(off_diff, 32);
> > > > > > > > > - } else if (off_diff > 8) {
> > > > > > > > > + } else if (align > 1 && off_diff > 8) {
> > > > > > > > > pad_type = "short";
> > > > > > > > > pad_bits = chip_away_bits(off_diff, 16);
> > > > > > > > > } else {
> > > > > > > > > /Per
