When encoding concrete instances of optimized functions it is possible parameters get reordered, often due to a parameter being optimized out; in such cases the order of abstract origin references to the abstract function is different, and the parameters that are optimized out usually appear after all the non-optimized parameters with no DW_AT_location information [1]. As an example consider static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu); It has - as expected - an abstract representation as follows: <1><6392a2d>: Abbrev Number: 47 (DW_TAG_subprogram) <6392a2e> DW_AT_name : (indirect string, offset: 0x261e25): __blkcg_rstat_flush <6392a32> DW_AT_decl_file : 1 <6392a33> DW_AT_decl_line : 1043 <6392a35> DW_AT_decl_column : 13 <6392a36> DW_AT_prototyped : 1 <6392a36> DW_AT_inline : 1 (inlined) <6392a37> DW_AT_sibling : <0x6392bac> <2><6392a3b>: Abbrev Number: 38 (DW_TAG_formal_parameter) <6392a3c> DW_AT_name : (indirect string, offset: 0xa7a9f): blkcg <6392a40> DW_AT_decl_file : 1 <6392a41> DW_AT_decl_line : 1043 <6392a43> DW_AT_decl_column : 47 <6392a44> DW_AT_type : <0x638b611> <2><6392a48>: Abbrev Number: 20 (DW_TAG_formal_parameter) <6392a49> DW_AT_name : cpu <6392a4d> DW_AT_decl_file : 1 <6392a4e> DW_AT_decl_line : 1043 <6392a50> DW_AT_decl_column : 58 <6392a51> DW_AT_type : <0x6377f8f> However the concrete representation after optimization becomes: ffffffff8186d180 t __blkcg_rstat_flush.isra.0 and has a concrete representation with parameter order switched: <1><6399661>: Abbrev Number: 110 (DW_TAG_subprogram) <6399662> DW_AT_abstract_origin: <0x6392a2d> <6399666> DW_AT_low_pc : 0xffffffff8186d180 <639966e> DW_AT_high_pc : 0x169 <6399676> DW_AT_frame_base : 1 byte block: 9c (DW_OP_call_frame_cfa) <6399678> DW_AT_GNU_all_call_sites: 1 <6399678> DW_AT_sibling : <0x6399a8a> <2><639967c>: Abbrev Number: 4 (DW_TAG_formal_parameter) <639967d> DW_AT_abstract_origin: <0x6392a48> <6399681> DW_AT_location : 0x1fe21fb (location list) <6399685> DW_AT_GNU_locviews: 0x1fe21f5 <2><63996e4>: Abbrev Number: 4 (DW_TAG_formal_parameter) <63996e5> DW_AT_abstract_origin: <0x6392a3b> <63996e9> DW_AT_location : 0x1fe2387 (location list) <63996ed> DW_AT_GNU_locviews: 0x1fe2385 In other words we end up with static void __blkcg_rstat_flush.isra(int cpu, struct blkcg *blkcg); We are not detecting cases like this in pahole, so we need to catch it to exclude such cases since they could lead to incorrect fentry attachment. Future work around true function signatures will allow such functions with their "." suffixes, but even for such cases it is good to detect the reordering. In practice we just end up excluding a few more .isra/.constprop functions which we cannot fentry-attach by name anyway; see [2] for an example list from CI. [1] https://lore.kernel.org/bpf/101b74c9-949a-4bf4-a766-a5343b70bdd2@oracle.com/ [2] https://github.com/alan-maguire/dwarves/actions/runs/20031993822 Signed-off-by: Alan Maguire Acked-by: Yonghong Song --- btf_encoder.c | 29 ++++++++++++++++++++--------- dwarf_loader.c | 5 +++-- dwarves.h | 2 ++ 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/btf_encoder.c b/btf_encoder.c index ec6933e..9a567e4 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -87,6 +87,7 @@ struct btf_encoder_func_state { uint8_t unexpected_reg:1; uint8_t inconsistent_proto:1; uint8_t uncertain_parm_loc:1; + uint8_t reordered_parm:1; uint8_t ambiguous_addr:1; int ret_type_id; struct btf_encoder_func_parm *parms; @@ -1273,6 +1274,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi state->unexpected_reg = ftype->unexpected_reg; state->optimized_parms = ftype->optimized_parms; state->uncertain_parm_loc = ftype->uncertain_parm_loc; + state->reordered_parm = ftype->reordered_parm; ftype__for_each_parameter(ftype, param) { const char *name = parameter__name(param) ?: ""; @@ -1442,7 +1444,7 @@ static int saved_functions_combine(struct btf_encoder *encoder, struct btf_encoder_func_state *a, struct btf_encoder_func_state *b) { - uint8_t optimized, unexpected, inconsistent, uncertain_parm_loc; + uint8_t optimized, unexpected, inconsistent, uncertain_parm_loc, reordered_parm; if (a->elf != b->elf) return 1; @@ -1451,12 +1453,14 @@ static int saved_functions_combine(struct btf_encoder *encoder, unexpected = a->unexpected_reg | b->unexpected_reg; inconsistent = a->inconsistent_proto | b->inconsistent_proto; uncertain_parm_loc = a->uncertain_parm_loc | b->uncertain_parm_loc; - if (!unexpected && !inconsistent && !funcs__match(encoder, a, b)) + reordered_parm = a->reordered_parm | b->reordered_parm; + if (!unexpected && !inconsistent && !reordered_parm && !funcs__match(encoder, a, b)) inconsistent = 1; a->optimized_parms = b->optimized_parms = optimized; a->unexpected_reg = b->unexpected_reg = unexpected; a->inconsistent_proto = b->inconsistent_proto = inconsistent; a->uncertain_parm_loc = b->uncertain_parm_loc = uncertain_parm_loc; + a->reordered_parm = b->reordered_parm = reordered_parm; return 0; } @@ -1494,7 +1498,7 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e for (i = 0; i < nr_saved_fns; i = j) { struct btf_encoder_func_state *state = &saved_fns[i]; - bool add_to_btf = !skip_encoding_inconsistent_proto; + char *skip_reason = NULL; /* Compare across sorted functions that match by name/prefix; * share inconsistent/unexpected reg state between them. @@ -1510,14 +1514,21 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e * unexpected register use, multiple inconsistent prototypes or * uncertain parameters location */ - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc && !state->elf->ambiguous_addr; - + if (state->unexpected_reg) + skip_reason = "unexpected register usage for parameter\n"; + if (skip_encoding_inconsistent_proto && state->inconsistent_proto) + skip_reason = "inconsistet prototype\n"; if (state->uncertain_parm_loc) - btf_encoder__log_func_skip(encoder, saved_fns[i].elf, - "uncertain parameter location\n", - 0, 0); + skip_reason = "uncertain parameter location\n"; + if (state->reordered_parm) + skip_reason = "reordered parameters\n"; + if (state->elf->ambiguous_addr) + skip_reason = "ambiguous address\n"; - if (add_to_btf) { + if (skip_reason) { + btf_encoder__log_func_skip(encoder, saved_fns[i].elf, + skip_reason, 0, 0); + } else { if (is_kfunc_state(state)) err = btf_encoder__add_bpf_kfunc(encoder, state); else diff --git a/dwarf_loader.c b/dwarf_loader.c index 77aab8a..16fb7be 100644 --- a/dwarf_loader.c +++ b/dwarf_loader.c @@ -1262,7 +1262,7 @@ static struct parameter *parameter__new(Dwarf_Die *die, struct cu *cu, tag__init(&parm->tag, cu, die); parm->name = attr_string(die, DW_AT_name, conf); - + parm->idx = param_idx; if (param_idx >= cu->nr_register_params || param_idx < 0) return parm; /* Parameters which use DW_AT_abstract_origin to point at @@ -2636,6 +2636,8 @@ static void ftype__recode_dwarf_types(struct tag *tag, struct cu *cu) } opos = tag__parameter(dtag__tag(dtype)); pos->name = opos->name; + if (pos->idx != opos->idx) + type->reordered_parm = 1; pos->tag.type = dtag__tag(dtype)->type; /* share location information between parameter and * abstract origin; if neither have location, we will @@ -2838,7 +2840,6 @@ static int tag__recode_dwarf_type(struct tag *tag, struct cu *cu) lexblock__recode_dwarf_types(&fn->lexblock, cu); } /* Fall thru */ - case DW_TAG_subroutine_type: ftype__recode_dwarf_types(tag, cu); /* Fall thru, for the function return type */ diff --git a/dwarves.h b/dwarves.h index 21d4166..78bedf5 100644 --- a/dwarves.h +++ b/dwarves.h @@ -944,6 +944,7 @@ struct parameter { uint8_t optimized:1; uint8_t unexpected_reg:1; uint8_t has_loc:1; + uint8_t idx; }; static inline struct parameter *tag__parameter(const struct tag *tag) @@ -1023,6 +1024,7 @@ struct ftype { uint8_t processed:1; uint8_t inconsistent_proto:1; uint8_t uncertain_parm_loc:1; + uint8_t reordered_parm:1; struct list_head template_type_params; struct list_head template_value_params; struct template_parameter_pack *template_parameter_pack; -- 2.43.5 Currently we collate function information by name and add functions provided there are no inconsistencies across various representations. For true_signature support - where we wish to add the real signature of a function even if it differs from source level - we need to do a few things: 1. For "."-suffixed functions, we need to match from DWARF->ELF; we can do this via the address associated with the function. In doing this, we can then be confident that the debug info for foo.isra.0 is the right info for the function at that address. 2. When adding saved functions we need to look for such cases and provided they do not violate other constraints around BTF representation - unexpected reg usage for function, uncertain parameter location or ambiguous address - we add them with their "."-suffixed name. The latter can be used as a signal that the function is transformed from the original. Doing this adds 500 functions to BTF. These are traceable with their "."-suffix names and because we have excluded ambiguous address cases we know exactly which function address they refer to. Signed-off-by: Alan Maguire --- btf_encoder.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++----- dwarves.h | 1 + pahole.c | 1 + 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/btf_encoder.c b/btf_encoder.c index 9a567e4..c1002c3 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -77,9 +77,16 @@ struct btf_encoder_func_annot { int16_t component_idx; }; +struct elf_function_sym { + const char *name; + uint64_t addr; +}; + /* state used to do later encoding of saved functions */ struct btf_encoder_func_state { struct elf_function *elf; + struct elf_function_sym *sym; + uint64_t addr; uint32_t type_id_off; uint16_t nr_parms; uint16_t nr_annots; @@ -94,11 +101,6 @@ struct btf_encoder_func_state { struct btf_encoder_func_annot *annots; }; -struct elf_function_sym { - const char *name; - uint64_t addr; -}; - struct elf_function { char *name; struct elf_function_sym *syms; @@ -145,7 +147,8 @@ struct btf_encoder { skip_encoding_decl_tag, tag_kfuncs, gen_distilled_base, - encode_attributes; + encode_attributes, + true_signature; uint32_t array_index_id; struct elf_secinfo *secinfo; size_t seccnt; @@ -1270,14 +1273,36 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi goto out; } } + if (encoder->true_signature && fn->lexblock.ip.addr) { + int i; + + for (i = 0; i < func->sym_cnt; i++) { + if (fn->lexblock.ip.addr != func->syms[i].addr) + continue; + /* Only need to record address for '.'-suffixed + * functions, since we only currently need true + * signatures for them. + */ + if (!strchr(func->syms[i].name, '.')) + continue; + state->sym = &func->syms[i]; + break; + } + } state->inconsistent_proto = ftype->inconsistent_proto; state->unexpected_reg = ftype->unexpected_reg; state->optimized_parms = ftype->optimized_parms; state->uncertain_parm_loc = ftype->uncertain_parm_loc; state->reordered_parm = ftype->reordered_parm; ftype__for_each_parameter(ftype, param) { - const char *name = parameter__name(param) ?: ""; + const char *name; + /* No location info/optimized + reordered means optimized out. */ + if (ftype->reordered_parm && (!param->has_loc || param->optimized)) { + state->nr_parms--; + continue; + } + name = parameter__name(param) ?: ""; str_off = btf__add_str(btf, name); if (str_off < 0) { err = str_off; @@ -1366,6 +1391,9 @@ static int32_t btf_encoder__add_func(struct btf_encoder *encoder, btf_fnproto_id = btf_encoder__add_func_proto_for_state(encoder, state); name = func->name; + if (encoder->true_signature && state->sym) + name = state->sym->name; + if (btf_fnproto_id >= 0) btf_fn_id = btf_encoder__add_ref_type(encoder, BTF_KIND_FUNC, btf_fnproto_id, name, false); @@ -1508,6 +1536,39 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e while (j < nr_saved_fns && saved_functions_combine(encoder, &saved_fns[i], &saved_fns[j]) == 0) j++; + /* Add true signatures for case where we have an exact + * symbol match by address from DWARF->ELF and have a + * "." suffixed name. + */ + if (encoder->true_signature) { + bool true_added = false; + int k; + + for (k = i; k < nr_saved_fns; k++) { + struct btf_encoder_func_state *true_state = &saved_fns[k]; + + if (state->elf != true_state->elf) + break; + if (!true_state->sym) + continue; + /* Unexpected reg, uncertain parm loc and + * ambiguous address mean we cannot trust fentry. + */ + if (true_state->unexpected_reg || + true_state->uncertain_parm_loc || + true_state->ambiguous_addr) + continue; + err = btf_encoder__add_func(encoder, true_state); + if (err < 0) + goto out; + true_added = true; + break; + } + /* If true symbol was added, skip the below. */ + if (true_added) + continue; + } + /* do not exclude functions with optimized-out parameters; they * may still be _called_ with the right parameter values, they * just do not _use_ them. Only exclude functions with @@ -2584,6 +2645,7 @@ struct btf_encoder *btf_encoder__new(struct cu *cu, const char *detached_filenam encoder->tag_kfuncs = conf_load->btf_decl_tag_kfuncs; encoder->gen_distilled_base = conf_load->btf_gen_distilled_base; encoder->encode_attributes = conf_load->btf_attributes; + encoder->true_signature = conf_load->true_signature; encoder->verbose = verbose; encoder->has_index_type = false; encoder->need_index_type = false; diff --git a/dwarves.h b/dwarves.h index 78bedf5..d7c6474 100644 --- a/dwarves.h +++ b/dwarves.h @@ -101,6 +101,7 @@ struct conf_load { bool btf_decl_tag_kfuncs; bool btf_gen_distilled_base; bool btf_attributes; + bool true_signature; uint8_t hashtable_bits; uint8_t max_hashtable_bits; uint16_t kabi_prefix_len; diff --git a/pahole.c b/pahole.c index ef01e58..02a0d19 100644 --- a/pahole.c +++ b/pahole.c @@ -1234,6 +1234,7 @@ struct btf_feature { BTF_NON_DEFAULT_FEATURE(global_var, encode_btf_global_vars, false), BTF_NON_DEFAULT_FEATURE_CHECK(attributes, btf_attributes, false, attributes_check), + BTF_NON_DEFAULT_FEATURE(true_signature, true_signature, false), }; #define BTF_MAX_FEATURE_STR 1024 -- 2.43.5 Use Yonghong's true signature test program to verify signatures differ for function "foo" with optimized-out parameter. Signed-off-by: Alan Maguire --- tests/gcc_true_signatures.sh | 92 ++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100755 tests/gcc_true_signatures.sh diff --git a/tests/gcc_true_signatures.sh b/tests/gcc_true_signatures.sh new file mode 100755 index 0000000..57cbe3f --- /dev/null +++ b/tests/gcc_true_signatures.sh @@ -0,0 +1,92 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0-only + +outdir= + +fail() +{ + # Do not remove test dir; might be useful for analysis + trap - EXIT + if [[ -d "$outdir" ]]; then + echo "Test data is in $outdir" + fi + exit 1 +} + +cleanup() +{ + rm ${outdir}/* + rmdir $outdir +} + +outdir=$(mktemp -d /tmp/gcc_true.sh.XXXXXX) + +trap cleanup EXIT + +echo -n "Validation of BTF encoding of true_signatures: " + +gcc_true="${outdir}/gcc_true" +CC=$(which gcc 2>/dev/null) + +if [[ -z "$CC" ]]; then + echo "skip: gcc not available" + exit 2 +fi + +cat > ${gcc_true}.c << EOF +struct t { int a; }; +__attribute__((noinline)) char *tar(struct t *a, struct t *d) +{ + if (a->a == d->a) + return (char *)10; + else + return (char *)0; +} + +__attribute__((noinline)) static char * foo(struct t *a, int b, struct t *d) +{ + return tar(a, d); +} + +__attribute__((noinline)) char *bar(struct t *a, struct t *d) +{ + return foo(a, 1, d); +} + +struct t p1, p2; +int main() +{ + return !!bar(&p1, &p2); +} +EOF + +CFLAGS="$CFLAGS -g -O2" +${CC} ${CFLAGS} -o $gcc_true ${gcc_true}.c +if [[ $? -ne 0 ]]; then + echo "Could not compile ${gcc_true}.c" >& 2 + exit 1 +fi +LLVM_OBJCOPY=objcopy pahole -J --btf_features=+true_signature $gcc_true +if [[ $? -ne 0 ]]; then + echo "Could not encode BTF for $gcc_true" + exit 1 +fi + +btf_optimized=$(pfunct --all --format_path=btf $gcc_true |grep "foo\.") +if [[ -z "$btf_optimized" ]]; then + echo "skip: no optimizations applied." + exit 2 +fi +# Convert foo.[constprop|isra].0 to foo to allow comparison. +btf_cmp="$(echo $btf_optimized \ + awk '/foo/ {sub(/\.constprop.0/,""); sub(/\.isra.0/,""); print $0 }')" +dwarf=$(pfunct --all $gcc_true |grep "foo") + +test -n "$VERBOSE" && printf "\nBTF: $btf_optimized DWARF: $dwarf \n" + +if [[ "$btf_cmp" == "$dwarf" ]]; then + echo "BTF and DWARF signatures should be different and they are not: BTF: $btf_optimized ; DWARF $dwarf" + exit 1 +fi +echo "Ok" +exit 0 -- 2.43.5 Ensure non-default "true_signature" feature is documented in the manual page. Signed-off-by: Alan Maguire --- man-pages/pahole.1 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/man-pages/pahole.1 b/man-pages/pahole.1 index 3125de3..90a8f45 100644 --- a/man-pages/pahole.1 +++ b/man-pages/pahole.1 @@ -337,6 +337,11 @@ Supported non-standard features (not enabled for 'default') of split BTF with a possibly changed base, storing it in a .BTF.base ELF section. global_var Encode all global variables using BTF_KIND_VAR in BTF. + true_signature Encode functions ensuring that binary-level + (rather than source-level) signatures are used; + for gcc these are ".isra.0" and ".costprop.0" + optimized functions + .fi So for example, specifying \-\-btf_encode=var,enum64 will result in a BTF encoding that (as well as encoding basic BTF information) will contain variables and enum64 values. -- 2.43.5 From: Matt Bobrowski Currently, when a function has both a weak and a strong definition across different compilation units (CUs), the BTF encoder arbitrarily selects one to generate the BTF entry. This selection fundamentally is dependent on the order in which pahole processes the CUs. This indifference often leads to a mismatch where the generated BTF reflects the weak definition's prototype, even though the linker selected the strong definition for the final vmlinux binary. A notable example described in [0] involving function bpf_lsm_mmap_file(). Both weak and strong definitions exist, distinguished only by parameter names (e.g., file vs file__nullable). While the strong definition is linked into the vmlinux object, the generated BTF contained the prototype for the weak definition. This causes issues for BPF verifier (e.g., __nullable annotation semantics), or tools relying on accurate type information. To fix this, ensure the BTF encoder selects the function definition corresponding to the actual code linked into the binary. This is achieved by comparing the DWARF function address (DW_AT_low_pc) with the ELF symbol address (st_value). Only the DWARF entry for the strong definition will match the final resolved ELF symbol address. [0] https://lore.kernel.org/all/aVJY9H-e83T7ivT4@google.com/ Link: https://lore.kernel.org/all/aVJY9H-e83T7ivT4@google.com/ Signed-off-by: Matt Bobrowski --- btf_encoder.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/btf_encoder.c b/btf_encoder.c index c1002c3..4264f36 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -1263,6 +1263,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi if (!state) return -ENOMEM; + state->addr = function__addr(fn); state->elf = func; state->nr_parms = ftype->nr_parms + (ftype->unspec_parms ? 1 : 0); state->ret_type_id = ftype->tag.type == 0 ? 0 : encoder->type_id_off + ftype->tag.type; @@ -1510,6 +1511,29 @@ static void btf_encoder__delete_saved_funcs(struct btf_encoder *encoder) encoder->func_states.cap = 0; } +static struct btf_encoder_func_state *btf_encoder__select_canonical_state(struct btf_encoder_func_state *combined_states, + int combined_cnt) +{ + int i, j; + + /* + * The same elf_function is shared amongst combined functions, + * as per saved_functions_combine(). + */ + struct elf_function *elf = combined_states[0].elf; + + for (i = 0; i < combined_cnt; i++) { + struct btf_encoder_func_state *state = &combined_states[i]; + + for (j = 0; j < elf->sym_cnt; j++) { + if (state->addr == elf->syms[j].addr) + return state; + } + } + + return &combined_states[0]; +} + static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_encoding_inconsistent_proto) { struct btf_encoder_func_state *saved_fns = encoder->func_states.array; @@ -1590,6 +1614,16 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e btf_encoder__log_func_skip(encoder, saved_fns[i].elf, skip_reason, 0, 0); } else { + /* + * We're to add the current function within + * BTF. Although, from all functions that have + * possibly been combined via + * saved_functions_combine(), ensure to only + * select and emit BTF for the most canonical + * function definition. + */ + if (j - i > 1) + state = btf_encoder__select_canonical_state(state, j - i); if (is_kfunc_state(state)) err = btf_encoder__add_bpf_kfunc(encoder, state); else -- 2.43.5