Re: Sv: Bad padding with bpftool btf dump .. format c

["Per Sundström XP" <per.xp.sundstrom@ericsson.com>]

On Wed, 2022-11-30 at 15:11 -0800, Andrii Nakryiko wrote:
> On Wed, Nov 30, 2022 at 3:06 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com> 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@gmail.com> 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@ericsson.com> 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')

  /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