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')
/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
