* [PATCH v2 0/3] btf_encoder: do not encode functions consuming packed structs on stack @ 2025-07-03 9:02 Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 UTC (permalink / raw) To: dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf, Alexis Lothoré (eBPF Foundation) Hello, this series is a follow-up to the RFC sent in [1], which aimed to make sure that functions consuming struct by value on stack were not encoded in BTF. This new series comes with a less "aggressive" strategy, and only skips function encoding if: - the function consumes a struct by value - the struct is passed on the stack - it is detected as packed (see class__infer_packed_attributes) The detection is not done anymore in parameter__new, as it may be done too early to have all the relevant info to properly infer the attributes; it is now done in btf_encoder__encode_cu. The series also comes with a simple test, but as the kernel don't have such case exposed, this new test comes with a small out-of-tree module. The logic is kept simple and the test is by default skipped (it needs to be provided with a path to some kernel sources), as I am not sure how it should integrate with the current CI effort done by Alan. [1] https://lore.kernel.org/dwarves/20250618-btf_skip_structs_on_stack-v1-1-e70be639cc53@bootlin.com/ Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- Changes in v2: - infer structs attributes - skip function encoded if some consumed struct (passed on stack) is marked as packed - add some tests in btf_functions.sh - drop RFC prefix - Link to v1: https://lore.kernel.org/r/20250618-btf_skip_structs_on_stack-v1-1-e70be639cc53@bootlin.com --- Alexis Lothoré (eBPF Foundation) (3): btf_encoder: skip functions consuming packed structs passed by value on stack tests: add some tests validating skipped functions due to uncertain arg location gitignore: ignore all the test kmod build-related files .gitignore | 3 ++ btf_encoder.c | 50 ++++++++++++++++++++++++- dwarves.h | 1 + tests/btf_functions.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/kmod/Makefile | 1 + tests/kmod/kmod.c | 69 +++++++++++++++++++++++++++++++++++ 6 files changed, 221 insertions(+), 2 deletions(-) --- base-commit: 042d73962d35fdd1466e056f1ea14590b1cdbb9b change-id: 20250617-btf_skip_structs_on_stack-006adf457d50 Best regards, -- Alexis Lothoré, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-03 9:02 [PATCH v2 0/3] btf_encoder: do not encode functions consuming packed structs on stack Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 ` Alexis Lothoré (eBPF Foundation) 2025-07-03 18:17 ` Ihor Solodrai 2025-07-04 20:05 ` Ihor Solodrai 2025-07-03 9:02 ` [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 3/3] gitignore: ignore all the test kmod build-related files Alexis Lothoré (eBPF Foundation) 2 siblings, 2 replies; 12+ messages in thread From: Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 UTC (permalink / raw) To: dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf, Alexis Lothoré (eBPF Foundation) Most ABIs allow functions to receive structs passed by value, if they fit in a register or a pair of registers, depending on the exact ABI. However, when there is a struct passed by value but all registers are already used for parameters passing, the struct is still passed by value but on the stack. This becomes an issue if the passed struct is defined with some attributes like __attribute__((packed)) or __attribute__((aligned(X)), as its location on the stack is altered, but this change is not reflected in dwarf information. The corresponding BTF data generated from this can lead to incorrect BPF trampolines generation (eg to attach bpf tracing programs to kernel functions) in the Linux kernel. Prevent those wrong cases by not encoding functions consuming structs passed by value on stack, when those structs do not have the expected alignment due to some attribute usage. Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- Changes in v2: - do not deny any struct passed by value, only those passed on stack AND with some attribute alteration - use the existing class__infer_packed_attributes to deduce is a struct is "altered". As a consequence, move the function filtering from parameter__new to btf_encoder__encode_cu, to make sure that all the needed data has been parsed from debug info --- btf_encoder.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- dwarves.h | 1 + 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/btf_encoder.c b/btf_encoder.c index 0bc23349b5d740c3ddab8208b2e15cdbdd139b9d..16739066caae808aea77175e6c221afbe37b7c70 100644 --- a/btf_encoder.c +++ b/btf_encoder.c @@ -87,6 +87,7 @@ struct btf_encoder_func_state { uint8_t optimized_parms:1; uint8_t unexpected_reg:1; uint8_t inconsistent_proto:1; + uint8_t uncertain_parm_loc:1; int ret_type_id; struct btf_encoder_func_parm *parms; struct btf_encoder_func_annot *annots; @@ -1203,6 +1204,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi 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; ftype__for_each_parameter(ftype, param) { const char *name = parameter__name(param) ?: ""; @@ -1430,9 +1432,15 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e /* 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 - * unexpected register use or multiple inconsistent prototypes. + * unexpected register use, multiple inconsistent prototypes or + * uncertain parameters location */ - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; + + if (state->uncertain_parm_loc) + btf_encoder__log_func_skip(encoder, saved_fns[i].elf, + "uncertain parameter location\n", + 0, 0); if (add_to_btf) { err = btf_encoder__add_func(state->encoder, state); @@ -2553,6 +2561,39 @@ void btf_encoder__delete(struct btf_encoder *encoder) free(encoder); } +static bool ftype__has_uncertain_arg_loc(struct cu *cu, struct ftype *ftype) +{ + struct parameter *param; + int param_idx = 0; + + if (ftype->nr_parms < cu->nr_register_params) + return false; + + ftype__for_each_parameter(ftype, param) { + if (param_idx++ < cu->nr_register_params) + continue; + + struct tag *type = cu__type(cu, param->tag.type); + + if (type == NULL || !tag__is_struct(type)) + continue; + + struct type *ctype = tag__type(type); + if (ctype->namespace.name == 0) + continue; + + struct class *class = tag__class(type); + + class__find_holes(class); + class__infer_packed_attributes(class, cu); + + if (class->is_packed) + return true; + } + + return false; +} + int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load) { struct llvm_annotation *annot; @@ -2647,6 +2688,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co * Skip functions that: * - are marked as declarations * - do not have full argument names + * - have arguments with uncertain locations, e.g packed + * structs passed by value on stack * - are not in ftrace list (if it's available) * - are not external (in case ftrace filter is not available) */ @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co if (!func) continue; + if (ftype__has_uncertain_arg_loc(cu, &fn->proto)) + fn->proto.uncertain_parm_loc = 1; + err = btf_encoder__save_func(encoder, fn, func); if (err) goto out; diff --git a/dwarves.h b/dwarves.h index 36c689847ebf29a1ab9936f9d0f928dd46514547..d689aee5910f4b40dc13b3e9dc596dfbe6a2c3d0 100644 --- a/dwarves.h +++ b/dwarves.h @@ -1021,6 +1021,7 @@ struct ftype { uint8_t unexpected_reg:1; uint8_t processed:1; uint8_t inconsistent_proto:1; + uint8_t uncertain_parm_loc:1; struct list_head template_type_params; struct list_head template_value_params; struct template_parameter_pack *template_parameter_pack; -- 2.50.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) @ 2025-07-03 18:17 ` Ihor Solodrai 2025-07-04 9:01 ` Alexis Lothoré 2025-07-04 20:05 ` Ihor Solodrai 1 sibling, 1 reply; 12+ messages in thread From: Ihor Solodrai @ 2025-07-03 18:17 UTC (permalink / raw) To: Alexis Lothoré (eBPF Foundation), dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: > Most ABIs allow functions to receive structs passed by value, if they > fit in a register or a pair of registers, depending on the exact ABI. > However, when there is a struct passed by value but all registers are > already used for parameters passing, the struct is still passed by value > but on the stack. This becomes an issue if the passed struct is defined > with some attributes like __attribute__((packed)) or > __attribute__((aligned(X)), as its location on the stack is altered, but > this change is not reflected in dwarf information. The corresponding BTF > data generated from this can lead to incorrect BPF trampolines > generation (eg to attach bpf tracing programs to kernel functions) in > the Linux kernel. > > Prevent those wrong cases by not encoding functions consuming structs > passed by value on stack, when those structs do not have the expected > alignment due to some attribute usage. > > Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> > --- > Changes in v2: > - do not deny any struct passed by value, only those passed on stack AND > with some attribute alteration > - use the existing class__infer_packed_attributes to deduce is a struct > is "altered". As a consequence, move the function filtering from > parameter__new to btf_encoder__encode_cu, to make sure that all the > needed data has been parsed from debug info > --- > btf_encoder.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > dwarves.h | 1 + > 2 files changed, 49 insertions(+), 2 deletions(-) > Hi Alexis. A couple of comments below. > diff --git a/btf_encoder.c b/btf_encoder.c > index 0bc23349b5d740c3ddab8208b2e15cdbdd139b9d..16739066caae808aea77175e6c221afbe37b7c70 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -87,6 +87,7 @@ struct btf_encoder_func_state { > uint8_t optimized_parms:1; > uint8_t unexpected_reg:1; > uint8_t inconsistent_proto:1; > + uint8_t uncertain_parm_loc:1; > int ret_type_id; > struct btf_encoder_func_parm *parms; > struct btf_encoder_func_annot *annots; > @@ -1203,6 +1204,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi > 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; > ftype__for_each_parameter(ftype, param) { > const char *name = parameter__name(param) ?: ""; > > @@ -1430,9 +1432,15 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e > /* 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 > - * unexpected register use or multiple inconsistent prototypes. > + * unexpected register use, multiple inconsistent prototypes or > + * uncertain parameters location > */ > - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; > + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; Is it possible for a function to have uncertain_parm_loc in one CU, but not in another? If yes, we still don't want the function in BTF, right? > + > + if (state->uncertain_parm_loc) > + btf_encoder__log_func_skip(encoder, saved_fns[i].elf, > + "uncertain parameter location\n", > + 0, 0); > > if (add_to_btf) { > err = btf_encoder__add_func(state->encoder, state); > @@ -2553,6 +2561,39 @@ void btf_encoder__delete(struct btf_encoder *encoder) > free(encoder); > } > > +static bool ftype__has_uncertain_arg_loc(struct cu *cu, struct ftype *ftype) > +{ > + struct parameter *param; > + int param_idx = 0; > + > + if (ftype->nr_parms < cu->nr_register_params) > + return false; > + > + ftype__for_each_parameter(ftype, param) { > + if (param_idx++ < cu->nr_register_params) > + continue; > + > + struct tag *type = cu__type(cu, param->tag.type); > + > + if (type == NULL || !tag__is_struct(type)) > + continue; > + > + struct type *ctype = tag__type(type); > + if (ctype->namespace.name == 0) > + continue; > + > + struct class *class = tag__class(type); > + > + class__find_holes(class); > + class__infer_packed_attributes(class, cu); > + > + if (class->is_packed) > + return true; > + } > + > + return false; > +} > + > int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load) > { > struct llvm_annotation *annot; > @@ -2647,6 +2688,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co > * Skip functions that: > * - are marked as declarations > * - do not have full argument names > + * - have arguments with uncertain locations, e.g packed > + * structs passed by value on stack > * - are not in ftrace list (if it's available) > * - are not external (in case ftrace filter is not available) > */ > @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co > if (!func) > continue; > > + if (ftype__has_uncertain_arg_loc(cu, &fn->proto)) > + fn->proto.uncertain_parm_loc = 1; > + > err = btf_encoder__save_func(encoder, fn, func); I think checking and setting uncertain_parm_loc flag should be done inside btf_encoder__save_func(), because that's where we inspect DWARF function prototype and add a new btf_encoder_func_state. > if (err) > goto out; > diff --git a/dwarves.h b/dwarves.h > index 36c689847ebf29a1ab9936f9d0f928dd46514547..d689aee5910f4b40dc13b3e9dc596dfbe6a2c3d0 100644 > --- a/dwarves.h > +++ b/dwarves.h > @@ -1021,6 +1021,7 @@ struct ftype { > uint8_t unexpected_reg:1; > uint8_t processed:1; > uint8_t inconsistent_proto:1; > + uint8_t uncertain_parm_loc:1; > struct list_head template_type_params; > struct list_head template_value_params; > struct template_parameter_pack *template_parameter_pack; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-03 18:17 ` Ihor Solodrai @ 2025-07-04 9:01 ` Alexis Lothoré 2025-07-04 19:59 ` Ihor Solodrai 0 siblings, 1 reply; 12+ messages in thread From: Alexis Lothoré @ 2025-07-04 9:01 UTC (permalink / raw) To: Ihor Solodrai, dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf Hello Ihor, thanks for the prompt feedback and testing ! On Thu Jul 3, 2025 at 8:17 PM CEST, Ihor Solodrai wrote: > On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: [...] >> /* 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 >> - * unexpected register use or multiple inconsistent prototypes. >> + * unexpected register use, multiple inconsistent prototypes or >> + * uncertain parameters location >> */ >> - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; >> + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; > > > Is it possible for a function to have uncertain_parm_loc in one CU, > but not in another? > > If yes, we still don't want the function in BTF, right? TBH, my understanding about those discrepancies between CUs about the same functions and how pahole handle them is still a bit fragile. Have you got any example about how it could be the case ? If it _can_ happen, I guess you are suggesting to make sure that copies are compared in saved_functions_combine and their uncertain_loc_parm flag are aligned. Something like this: uncertain_parm_loc = a->uncertain_parm_loc | b->uncertain_parm_loc; [...] a->uncertain_parm_loc = b->uncertain_parm_loc = uncertain_parm_loc; >> @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co >> if (!func) >> continue; >> >> + if (ftype__has_uncertain_arg_loc(cu, &fn->proto)) >> + fn->proto.uncertain_parm_loc = 1; >> + >> err = btf_encoder__save_func(encoder, fn, func); > > I think checking and setting uncertain_parm_loc flag should be done > inside btf_encoder__save_func(), because that's where we inspect DWARF > function prototype and add a new btf_encoder_func_state. ACK, it can be moved there Thanks, Alexis -- Alexis Lothoré, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-04 9:01 ` Alexis Lothoré @ 2025-07-04 19:59 ` Ihor Solodrai 2025-07-04 21:10 ` Alexis Lothoré 0 siblings, 1 reply; 12+ messages in thread From: Ihor Solodrai @ 2025-07-04 19:59 UTC (permalink / raw) To: Alexis Lothoré, dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On 7/4/25 2:01 AM, Alexis Lothoré wrote: > Hello Ihor, > > thanks for the prompt feedback and testing ! > > On Thu Jul 3, 2025 at 8:17 PM CEST, Ihor Solodrai wrote: >> On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: > > [...] > >>> /* 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 >>> - * unexpected register use or multiple inconsistent prototypes. >>> + * unexpected register use, multiple inconsistent prototypes or >>> + * uncertain parameters location >>> */ >>> - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; >>> + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; >> >> >> Is it possible for a function to have uncertain_parm_loc in one CU, >> but not in another? >> >> If yes, we still don't want the function in BTF, right? > > TBH, my understanding about those discrepancies between CUs about the same > functions and how pahole handle them is still a bit fragile. Have you got > any example about how it could be the case ? I would hope stuff like this doesn't happen often in the real world, but in principle you could have the following situation: #ifdef ENABLE_PACKING struct some_data { char header; int payload; short footer; } __attribute__((packed)); #else struct some_data { char header; int payload; short footer; }; #endif void read_data(/* lots of args */, struct some_data data) { ... } And then one user has #define ENABLE_PACKING and the other doesn't, for example: #define ENABLE_PACKING // or not #include "some_data.h" void user() { struct some_data = get_some_data(); ... read_data(/* args */, some_data); } And then you compile a binary with both users: $ gcc -g -O0 user1.c user2.c DWARF will contain both packed and not packed instances of struct some_data and two corresponding read_data() funcs. > > If it _can_ happen, I guess you are suggesting to make sure that copies are > compared in saved_functions_combine and their uncertain_loc_parm flag are > aligned. Something like this: > > uncertain_parm_loc = a->uncertain_parm_loc | b->uncertain_parm_loc; > [...] > a->uncertain_parm_loc = b->uncertain_parm_loc = uncertain_parm_loc; I asked out of curiosity, but you're right that this piece is needed. > >>> @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co >>> if (!func) >>> continue; >>> >>> + if (ftype__has_uncertain_arg_loc(cu, &fn->proto)) >>> + fn->proto.uncertain_parm_loc = 1; >>> + >>> err = btf_encoder__save_func(encoder, fn, func); >> >> I think checking and setting uncertain_parm_loc flag should be done >> inside btf_encoder__save_func(), because that's where we inspect DWARF >> function prototype and add a new btf_encoder_func_state. > > ACK, it can be moved there > > Thanks, > > Alexis > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-04 19:59 ` Ihor Solodrai @ 2025-07-04 21:10 ` Alexis Lothoré 0 siblings, 0 replies; 12+ messages in thread From: Alexis Lothoré @ 2025-07-04 21:10 UTC (permalink / raw) To: Ihor Solodrai, dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On Fri Jul 4, 2025 at 9:59 PM CEST, Ihor Solodrai wrote: > On 7/4/25 2:01 AM, Alexis Lothoré wrote: >> Hello Ihor, >> >> thanks for the prompt feedback and testing ! >> >> On Thu Jul 3, 2025 at 8:17 PM CEST, Ihor Solodrai wrote: >>> On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: >> >> [...] >> >>>> /* 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 >>>> - * unexpected register use or multiple inconsistent prototypes. >>>> + * unexpected register use, multiple inconsistent prototypes or >>>> + * uncertain parameters location >>>> */ >>>> - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; >>>> + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; >>> >>> >>> Is it possible for a function to have uncertain_parm_loc in one CU, >>> but not in another? >>> >>> If yes, we still don't want the function in BTF, right? >> >> TBH, my understanding about those discrepancies between CUs about the same >> functions and how pahole handle them is still a bit fragile. Have you got >> any example about how it could be the case ? > > I would hope stuff like this doesn't happen often in the real > world, but in principle you could have the following situation: > > #ifdef ENABLE_PACKING > struct some_data { > char header; > int payload; > short footer; > } __attribute__((packed)); > #else > struct some_data { > char header; > int payload; > short footer; > }; > #endif > > void read_data(/* lots of args */, struct some_data data) { ... } > > And then one user has #define ENABLE_PACKING and the other doesn't, > for example: > > #define ENABLE_PACKING // or not > #include "some_data.h" > > void user() { > struct some_data = get_some_data(); > ... > read_data(/* args */, some_data); > } > > And then you compile a binary with both users: > > $ gcc -g -O0 user1.c user2.c > > DWARF will contain both packed and not packed instances of struct > some_data and two corresponding read_data() funcs. Got it, thanks for the clarification ! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) 2025-07-03 18:17 ` Ihor Solodrai @ 2025-07-04 20:05 ` Ihor Solodrai 2025-07-04 21:12 ` Alexis Lothoré 1 sibling, 1 reply; 12+ messages in thread From: Ihor Solodrai @ 2025-07-04 20:05 UTC (permalink / raw) To: Alexis Lothoré (eBPF Foundation), dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: > Most ABIs allow functions to receive structs passed by value, if they > fit in a register or a pair of registers, depending on the exact ABI. > However, when there is a struct passed by value but all registers are > already used for parameters passing, the struct is still passed by value > but on the stack. This becomes an issue if the passed struct is defined > with some attributes like __attribute__((packed)) or > __attribute__((aligned(X)), as its location on the stack is altered, but > this change is not reflected in dwarf information. The corresponding BTF > data generated from this can lead to incorrect BPF trampolines > generation (eg to attach bpf tracing programs to kernel functions) in > the Linux kernel. > > Prevent those wrong cases by not encoding functions consuming structs > passed by value on stack, when those structs do not have the expected > alignment due to some attribute usage. > > Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> > --- > Changes in v2: > - do not deny any struct passed by value, only those passed on stack AND > with some attribute alteration > - use the existing class__infer_packed_attributes to deduce is a struct > is "altered". As a consequence, move the function filtering from > parameter__new to btf_encoder__encode_cu, to make sure that all the > needed data has been parsed from debug info > --- > btf_encoder.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > dwarves.h | 1 + > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/btf_encoder.c b/btf_encoder.c > index 0bc23349b5d740c3ddab8208b2e15cdbdd139b9d..16739066caae808aea77175e6c221afbe37b7c70 100644 > --- a/btf_encoder.c > +++ b/btf_encoder.c > @@ -87,6 +87,7 @@ struct btf_encoder_func_state { > uint8_t optimized_parms:1; > uint8_t unexpected_reg:1; > uint8_t inconsistent_proto:1; > + uint8_t uncertain_parm_loc:1; > int ret_type_id; > struct btf_encoder_func_parm *parms; > struct btf_encoder_func_annot *annots; > @@ -1203,6 +1204,7 @@ static int32_t btf_encoder__save_func(struct btf_encoder *encoder, struct functi > 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; > ftype__for_each_parameter(ftype, param) { > const char *name = parameter__name(param) ?: ""; > > @@ -1430,9 +1432,15 @@ static int btf_encoder__add_saved_funcs(struct btf_encoder *encoder, bool skip_e > /* 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 > - * unexpected register use or multiple inconsistent prototypes. > + * unexpected register use, multiple inconsistent prototypes or > + * uncertain parameters location > */ > - add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto; > + add_to_btf |= !state->unexpected_reg && !state->inconsistent_proto && !state->uncertain_parm_loc; > + > + if (state->uncertain_parm_loc) > + btf_encoder__log_func_skip(encoder, saved_fns[i].elf, > + "uncertain parameter location\n", > + 0, 0); > > if (add_to_btf) { > err = btf_encoder__add_func(state->encoder, state); > @@ -2553,6 +2561,39 @@ void btf_encoder__delete(struct btf_encoder *encoder) > free(encoder); > } > > +static bool ftype__has_uncertain_arg_loc(struct cu *cu, struct ftype *ftype) > +{ > + struct parameter *param; > + int param_idx = 0; > + > + if (ftype->nr_parms < cu->nr_register_params) > + return false; > + > + ftype__for_each_parameter(ftype, param) { > + if (param_idx++ < cu->nr_register_params) > + continue; > + > + struct tag *type = cu__type(cu, param->tag.type); > + > + if (type == NULL || !tag__is_struct(type)) > + continue; > + > + struct type *ctype = tag__type(type); > + if (ctype->namespace.name == 0) > + continue; > + > + struct class *class = tag__class(type); > + > + class__find_holes(class); > + class__infer_packed_attributes(class, cu); I just noticed that class__infer_packed_attributes() already does call class__find_holes() [1], so calling it here is unnecessary. Although there is already a flag to detect repeated calls [2]. [1] https://github.com/acmel/dwarves/blob/master/dwarves.c#L1859 [2] https://github.com/acmel/dwarves/blob/master/dwarves.c#L1604-L1605 > + > + if (class->is_packed) > + return true; > + } > + > + return false; > +} > + > int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct conf_load *conf_load) > { > struct llvm_annotation *annot; > @@ -2647,6 +2688,8 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co > * Skip functions that: > * - are marked as declarations > * - do not have full argument names > + * - have arguments with uncertain locations, e.g packed > + * structs passed by value on stack > * - are not in ftrace list (if it's available) > * - are not external (in case ftrace filter is not available) > */ > @@ -2693,6 +2736,9 @@ int btf_encoder__encode_cu(struct btf_encoder *encoder, struct cu *cu, struct co > if (!func) > continue; > > + if (ftype__has_uncertain_arg_loc(cu, &fn->proto)) > + fn->proto.uncertain_parm_loc = 1; > + > err = btf_encoder__save_func(encoder, fn, func); > if (err) > goto out; > diff --git a/dwarves.h b/dwarves.h > index 36c689847ebf29a1ab9936f9d0f928dd46514547..d689aee5910f4b40dc13b3e9dc596dfbe6a2c3d0 100644 > --- a/dwarves.h > +++ b/dwarves.h > @@ -1021,6 +1021,7 @@ struct ftype { > uint8_t unexpected_reg:1; > uint8_t processed:1; > uint8_t inconsistent_proto:1; > + uint8_t uncertain_parm_loc:1; > struct list_head template_type_params; > struct list_head template_value_params; > struct template_parameter_pack *template_parameter_pack; > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack 2025-07-04 20:05 ` Ihor Solodrai @ 2025-07-04 21:12 ` Alexis Lothoré 0 siblings, 0 replies; 12+ messages in thread From: Alexis Lothoré @ 2025-07-04 21:12 UTC (permalink / raw) To: Ihor Solodrai, dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On Fri Jul 4, 2025 at 10:05 PM CEST, Ihor Solodrai wrote: > On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: [...] >> +static bool ftype__has_uncertain_arg_loc(struct cu *cu, struct ftype *ftype) >> +{ >> + struct parameter *param; >> + int param_idx = 0; >> + >> + if (ftype->nr_parms < cu->nr_register_params) >> + return false; >> + >> + ftype__for_each_parameter(ftype, param) { >> + if (param_idx++ < cu->nr_register_params) >> + continue; >> + >> + struct tag *type = cu__type(cu, param->tag.type); >> + >> + if (type == NULL || !tag__is_struct(type)) >> + continue; >> + >> + struct type *ctype = tag__type(type); >> + if (ctype->namespace.name == 0) >> + continue; >> + >> + struct class *class = tag__class(type); >> + >> + class__find_holes(class); >> + class__infer_packed_attributes(class, cu); > > I just noticed that class__infer_packed_attributes() already does call > class__find_holes() [1], so calling it here is unnecessary. Although > there is already a flag to detect repeated calls [2]. > > [1] https://github.com/acmel/dwarves/blob/master/dwarves.c#L1859 > [2] https://github.com/acmel/dwarves/blob/master/dwarves.c#L1604-L1605 ACK, I'll remove the duplicate class__find_holes then. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location 2025-07-03 9:02 [PATCH v2 0/3] btf_encoder: do not encode functions consuming packed structs on stack Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 ` Alexis Lothoré (eBPF Foundation) 2025-07-03 18:31 ` Ihor Solodrai 2025-07-03 9:02 ` [PATCH v2 3/3] gitignore: ignore all the test kmod build-related files Alexis Lothoré (eBPF Foundation) 2 siblings, 1 reply; 12+ messages in thread From: Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 UTC (permalink / raw) To: dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf, Alexis Lothoré (eBPF Foundation) Add a small kernel module representing specific cases likely absent from standard vmlinux files. As a starter, the introduced module exposes a few functions consuming structs passed by value, some passed by register, some passed on the stack: int kmod_test_init(void); int test_kmod_func_ok(int, void *, char, short int); int test_kmod_func_struct_ok(int, void *, char, struct kmod_struct); int test_kmod_func_struct_on_stack_ok(int, void *, char, short int, int, \ void *, char, short int, struct kmod_struct); int test_kmod_func_struct_on_stack_ko(int, void *, char, short int, int, \ void *, char, short int, struct kmod_struct_packed); Then enrich btf_functions.sh to make it perform the following steps: - build the module - generate BTF info and pfunct listing, both with dwarf and the generated BTF - check that any function encoded in BTF is found in DWARF - check that any function announced as skipped is indeed absent from BTF - check that any skipped function has been skipped due to uncertain parameter location Those new tests are executed only if a kernel directory is provided as script's second argument, they are otherwise skipped. Example of the new test execution: Encoding...Matched 4 functions exactly. Ok Validation of skipped function logic... Skipped encoding 1 functions in BTF. Ok Validating skipped functions have uncertain parameter location... Found 1 legitimately skipped function due to uncertain loc Ok Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- Changes in v2: - new patch --- tests/btf_functions.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/kmod/Makefile | 1 + tests/kmod/kmod.c | 69 +++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+) diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh index c92e5ae906f90badfede86eb530108894fbc8c93..64810b7eb51e7f2693929fbf66e0641a9d4e0277 100755 --- a/tests/btf_functions.sh +++ b/tests/btf_functions.sh @@ -193,4 +193,103 @@ if [[ -n "$VERBOSE" ]]; then fi echo "Ok" +# Some specific cases can not be tested directly with a standard kernel. +# We can use the kernel module in kmod/ to test those cases, like packed +# structs passed on the stack. Run this test only if we have the needed +# dependencies (eg: some kernel sources directory passed as argument; those +# must match the used vmlinux file) + +KDIR=${KDIR:-$2} +if [ -z "$KDIR" ] ; then + echo "Skipping kmod tests" + exit 0 +fi + +echo -n "Validation of BTF encoding corner cases with kmod functions; this may take some time: " + +test -n "$VERBOSE" && printf "\nBuilding kmod..." +tests_dir=$(dirname $0) +make -C ${KDIR} M=${tests_dir}/kmod + +test -n "$VERBOSE" && printf "\nEncoding..." +pahole --btf_features=default --lang_exclude=rust --btf_encode_detached=$outdir/kmod.btf \ + --verbose ${tests_dir}/kmod/kmod.ko | grep "skipping BTF encoding of function" \ + > ${outdir}/kmod_skipped_fns + +funcs=$(pfunct --format_path=btf $outdir/kmod.btf 2>/dev/null|sort) +pfunct --all --no_parm_names --format_path=dwarf kmod/kmod.ko | \ + sort|uniq > $outdir/kmod_dwarf.funcs +pfunct --all --no_parm_names --format_path=btf $outdir/kmod.btf 2>/dev/null|\ + awk '{ gsub("^(bpf_kfunc |bpf_fastcall )+",""); print $0}'|sort|uniq > $outdir/kmod_btf.funcs + +exact=0 +while IFS= read -r btf ; do + # Matching process can be kept simpler as the tested binary is + # specifically tailored for tests + dwarf=$(grep -F "$btf" $outdir/kmod_dwarf.funcs) + if [[ "$btf" != "$dwarf" ]]; then + echo "ERROR: mismatch : BTF '$btf' not found; DWARF '$dwarf'" + fail + else + exact=$((exact+1)) + fi +done < $outdir/kmod_btf.funcs + +if [[ -n "$VERBOSE" ]]; then + echo "Matched $exact functions exactly." + echo "Ok" + echo "Validation of skipped function logic..." +fi + +skipped_cnt=$(wc -l ${outdir}/kmod_skipped_fns | awk '{ print $1}') +if [[ "$skipped_cnt" == "0" ]]; then + echo "No skipped functions. Done." + exit 0 +fi + +skipped_fns=$(awk '{print $1}' $outdir/kmod_skipped_fns) +for s in $skipped_fns ; do + # Ensure the skipped function are not in BTF + inbtf=$(grep " $s(" $outdir/kmod_btf.funcs) + if [[ -n "$inbtf" ]]; then + echo "ERROR: '${s}()' was added incorrectly to BTF: '$inbtf'" + fail + fi +done + +if [[ -n "$VERBOSE" ]]; then + echo "Skipped encoding $skipped_cnt functions in BTF." + echo "Ok" + echo "Validating skipped functions have uncertain parameter location..." +fi + +uncertain_loc=$(awk '/due to uncertain parameter location/ { print $1 }' $outdir/kmod_skipped_fns) +legitimate_skip=0 + +for f in $uncertain_loc ; do + # Extract parameters types + raw_params=$(grep ${f} $outdir/kmod_dwarf.funcs|sed -n 's/^[^(]*(\([^)]*\)).*/\1/p') + IFS=',' read -ra params <<< "${raw_params}" + for param in "${params[@]}" + do + # Search any param that could be a struct + struct_type=$(echo ${param}|grep -E '^struct [^*]' | sed -E 's/^struct //') + if [ -n "${struct_type}" ]; then + # Check with pahole if the struct is detected as + # packed + if pahole -C "${struct_type}" kmod/kmod.ko|grep -q __packed__ + then + legitimate_skip=$((legitimate_skip+1)) + continue 2 + fi + fi + done + echo "ERROR: '${f}()' should not have been skipped; it has no parameter with uncertain location" + fail +done + +if [[ -n "$VERBOSE" ]]; then + echo "Found ${legitimate_skip} legitimately skipped function due to uncertain loc" +fi +echo "Ok" exit 0 diff --git a/tests/kmod/Makefile b/tests/kmod/Makefile new file mode 100644 index 0000000000000000000000000000000000000000..e7c2ed929eaf81e91429f744c3778156ed2be2d2 --- /dev/null +++ b/tests/kmod/Makefile @@ -0,0 +1 @@ +obj-m += kmod.o diff --git a/tests/kmod/kmod.c b/tests/kmod/kmod.c new file mode 100644 index 0000000000000000000000000000000000000000..5b93614b6156b05925a6cb48809ad63533ccba3e --- /dev/null +++ b/tests/kmod/kmod.c @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/module.h> +#include <linux/printk.h> + +struct kmod_struct { + char a; + short b; + int c; + unsigned long long d; +}; + +struct kmod_struct_packed { + char a; + short b; + int c; + unsigned long long d; +}__packed; + +int test_kmod_func_ok(int a, void *b, char c, short d); +int test_kmod_func_struct_ok(int a, void *b, char c, struct kmod_struct d); +int test_kmod_func_struct_on_stack_ok(int a, void *b, char c, short d, int e, + void *f, char g, short h, + struct kmod_struct i); +int test_kmod_func_struct_on_stack_ko(int a, void *b, char c, short d, int e, + void *f, char g, short h, + struct kmod_struct_packed i); + +noinline int test_kmod_func_ok(int a, void *b, char c, short d) +{ + return a + (long)b + c + d; +} + +noinline int test_kmod_func_struct_ok(int a, void *b, char c, + struct kmod_struct d) +{ + return a + (long)b + c + d.a + d.b + d.c + d.d; +} + +noinline int test_kmod_func_struct_on_stack_ok(int a, void *b, char c, short d, + int e, void *f, char g, short h, + struct kmod_struct i) +{ + return a + (long)b + c + d + e + (long)f + g + h + i.a + i.b + i.c + i.d; +} + +noinline int test_kmod_func_struct_on_stack_ko(int a, void *b, char c, short d, + int e, void *f, char g, short h, + struct kmod_struct_packed i) +{ + return a + (long)b + c + d + e + (long)f + g + h + i.a + i.b + i.c + i.d; +} + +static int kmod_test_init(void) +{ + struct kmod_struct test; + struct kmod_struct_packed test_bis; + + test_kmod_func_ok(0, NULL, 0, 0); + test_kmod_func_struct_ok(0, NULL, 0, test); + test_kmod_func_struct_on_stack_ok(0, NULL, 0, 0, 0, NULL, 0, 0, test); + test_kmod_func_struct_on_stack_ko(0, NULL, 0, 0, 0, NULL, 0, 0, test_bis); + return 0; +} + +module_init(kmod_test_init); + +MODULE_AUTHOR("Alexis Lothoré"); +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Pahole testing module"); -- 2.50.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location 2025-07-03 9:02 ` [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location Alexis Lothoré (eBPF Foundation) @ 2025-07-03 18:31 ` Ihor Solodrai 2025-07-04 9:06 ` Alexis Lothoré 0 siblings, 1 reply; 12+ messages in thread From: Ihor Solodrai @ 2025-07-03 18:31 UTC (permalink / raw) To: Alexis Lothoré (eBPF Foundation), dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: > Add a small kernel module representing specific cases likely absent from > standard vmlinux files. As a starter, the introduced module exposes a > few functions consuming structs passed by value, some passed by > register, some passed on the stack: > > int kmod_test_init(void); > int test_kmod_func_ok(int, void *, char, short int); > int test_kmod_func_struct_ok(int, void *, char, struct kmod_struct); > int test_kmod_func_struct_on_stack_ok(int, void *, char, short int, int, \ > void *, char, short int, struct kmod_struct); > int test_kmod_func_struct_on_stack_ko(int, void *, char, short int, int, \ > void *, char, short int, struct kmod_struct_packed); > > Then enrich btf_functions.sh to make it perform the following steps: > - build the module > - generate BTF info and pfunct listing, both with dwarf and the > generated BTF > - check that any function encoded in BTF is found in DWARF > - check that any function announced as skipped is indeed absent from BTF > - check that any skipped function has been skipped due to uncertain > parameter location > > Those new tests are executed only if a kernel directory is provided as > script's second argument, they are otherwise skipped. While this shouldn't be a problem for CI, since it checks out a kernel tree to test vmlinux as input, I wonder if there is a way to do the same test without this dependency. We need to generate a binary with DWARF, containing function prototypes with packed/aligned attributes. Give it to pahole and see that those functions were skipped. Any reason it must be a kernel module? Am I missing something? > > Example of the new test execution: > Encoding...Matched 4 functions exactly. > Ok > Validation of skipped function logic... > Skipped encoding 1 functions in BTF. > Ok > Validating skipped functions have uncertain parameter location... > Found 1 legitimately skipped function due to uncertain loc > Ok > > Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> > --- > Changes in v2: > - new patch > --- > tests/btf_functions.sh | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/kmod/Makefile | 1 + > tests/kmod/kmod.c | 69 +++++++++++++++++++++++++++++++++++ > 3 files changed, 169 insertions(+) > > diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh > index c92e5ae906f90badfede86eb530108894fbc8c93..64810b7eb51e7f2693929fbf66e0641a9d4e0277 100755 > --- a/tests/btf_functions.sh > +++ b/tests/btf_functions.sh > @@ -193,4 +193,103 @@ if [[ -n "$VERBOSE" ]]; then > fi > echo "Ok" > > +# Some specific cases can not be tested directly with a standard kernel. > +# We can use the kernel module in kmod/ to test those cases, like packed > +# structs passed on the stack. Run this test only if we have the needed > +# dependencies (eg: some kernel sources directory passed as argument; those > +# must match the used vmlinux file) > + > +KDIR=${KDIR:-$2} > +if [ -z "$KDIR" ] ; then > + echo "Skipping kmod tests" > + exit 0 > +fi > + > +echo -n "Validation of BTF encoding corner cases with kmod functions; this may take some time: " > + > +test -n "$VERBOSE" && printf "\nBuilding kmod..." > +tests_dir=$(dirname $0) > +make -C ${KDIR} M=${tests_dir}/kmod This part fails for me: isolodrai@isolodrai-fedora-PC2K40WQ:~/pahole/tests$ KDIR=/home/isolodrai/kernels/bpf-next vmlinux=/home/isolodrai/kernels/bpf-next/vmlinux ./btf_functions.sh Validation of BTF encoding of functions; this may take some time: Ok Validation of BTF encoding corner cases with kmod functions; this may take some time: make: Entering directory '/home/isolodrai/kernels/bpf-next' Makefile:199: *** specified external module directory "./kmod" does not exist. Stop. make: Leaving directory '/home/isolodrai/kernels/bpf-next' No skipped functions. Done. Maybe: diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh index 64810b7..fcb1591 100755 --- a/tests/btf_functions.sh +++ b/tests/btf_functions.sh @@ -208,7 +208,7 @@ fi echo -n "Validation of BTF encoding corner cases with kmod functions; this may take some time: " test -n "$VERBOSE" && printf "\nBuilding kmod..." -tests_dir=$(dirname $0) +tests_dir=$(realpath $(dirname $0)) make -C ${KDIR} M=${tests_dir}/kmod test -n "$VERBOSE" && printf "\nEncoding..." Also, in case kernel is built with LLVM, one must set LLVM=1. Not sure if this is detectable by the test. > + > +test -n "$VERBOSE" && printf "\nEncoding..." > +pahole --btf_features=default --lang_exclude=rust --btf_encode_detached=$outdir/kmod.btf \ > + --verbose ${tests_dir}/kmod/kmod.ko | grep "skipping BTF encoding of function" \ > + > ${outdir}/kmod_skipped_fns > + > +funcs=$(pfunct --format_path=btf $outdir/kmod.btf 2>/dev/null|sort) > +pfunct --all --no_parm_names --format_path=dwarf kmod/kmod.ko | \ > + sort|uniq > $outdir/kmod_dwarf.funcs > +pfunct --all --no_parm_names --format_path=btf $outdir/kmod.btf 2>/dev/null|\ > + awk '{ gsub("^(bpf_kfunc |bpf_fastcall )+",""); print $0}'|sort|uniq > $outdir/kmod_btf.funcs > + > +exact=0 > +while IFS= read -r btf ; do > + # Matching process can be kept simpler as the tested binary is > + # specifically tailored for tests > + dwarf=$(grep -F "$btf" $outdir/kmod_dwarf.funcs) > + if [[ "$btf" != "$dwarf" ]]; then > + echo "ERROR: mismatch : BTF '$btf' not found; DWARF '$dwarf'" > + fail > + else > + exact=$((exact+1)) > + fi > +done < $outdir/kmod_btf.funcs > + > +if [[ -n "$VERBOSE" ]]; then > + echo "Matched $exact functions exactly." > + echo "Ok" > + echo "Validation of skipped function logic..." > +fi > + > +skipped_cnt=$(wc -l ${outdir}/kmod_skipped_fns | awk '{ print $1}') > +if [[ "$skipped_cnt" == "0" ]]; then > + echo "No skipped functions. Done." > + exit 0 > +fi > + > +skipped_fns=$(awk '{print $1}' $outdir/kmod_skipped_fns) > +for s in $skipped_fns ; do > + # Ensure the skipped function are not in BTF > + inbtf=$(grep " $s(" $outdir/kmod_btf.funcs) > + if [[ -n "$inbtf" ]]; then > + echo "ERROR: '${s}()' was added incorrectly to BTF: '$inbtf'" > + fail > + fi > +done > + > +if [[ -n "$VERBOSE" ]]; then > + echo "Skipped encoding $skipped_cnt functions in BTF." > + echo "Ok" > + echo "Validating skipped functions have uncertain parameter location..." > +fi > + > +uncertain_loc=$(awk '/due to uncertain parameter location/ { print $1 }' $outdir/kmod_skipped_fns) > +legitimate_skip=0 > + > +for f in $uncertain_loc ; do > + # Extract parameters types > + raw_params=$(grep ${f} $outdir/kmod_dwarf.funcs|sed -n 's/^[^(]*(\([^)]*\)).*/\1/p') > + IFS=',' read -ra params <<< "${raw_params}" > + for param in "${params[@]}" > + do > + # Search any param that could be a struct > + struct_type=$(echo ${param}|grep -E '^struct [^*]' | sed -E 's/^struct //') > + if [ -n "${struct_type}" ]; then > + # Check with pahole if the struct is detected as > + # packed > + if pahole -C "${struct_type}" kmod/kmod.ko|grep -q __packed__ > + then > + legitimate_skip=$((legitimate_skip+1)) > + continue 2 > + fi > + fi > + done > + echo "ERROR: '${f}()' should not have been skipped; it has no parameter with uncertain location" > + fail > +done > + > +if [[ -n "$VERBOSE" ]]; then > + echo "Found ${legitimate_skip} legitimately skipped function due to uncertain loc" > +fi > +echo "Ok" > exit 0 > diff --git a/tests/kmod/Makefile b/tests/kmod/Makefile > new file mode 100644 > index 0000000000000000000000000000000000000000..e7c2ed929eaf81e91429f744c3778156ed2be2d2 > --- /dev/null > +++ b/tests/kmod/Makefile > @@ -0,0 +1 @@ > +obj-m += kmod.o > diff --git a/tests/kmod/kmod.c b/tests/kmod/kmod.c > new file mode 100644 > index 0000000000000000000000000000000000000000..5b93614b6156b05925a6cb48809ad63533ccba3e > --- /dev/null > +++ b/tests/kmod/kmod.c > @@ -0,0 +1,69 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/module.h> > +#include <linux/printk.h> > + > +struct kmod_struct { > + char a; > + short b; > + int c; > + unsigned long long d; > +}; > + > +struct kmod_struct_packed { > + char a; > + short b; > + int c; > + unsigned long long d; > +}__packed; > + > +int test_kmod_func_ok(int a, void *b, char c, short d); > +int test_kmod_func_struct_ok(int a, void *b, char c, struct kmod_struct d); > +int test_kmod_func_struct_on_stack_ok(int a, void *b, char c, short d, int e, > + void *f, char g, short h, > + struct kmod_struct i); > +int test_kmod_func_struct_on_stack_ko(int a, void *b, char c, short d, int e, > + void *f, char g, short h, > + struct kmod_struct_packed i); > + > +noinline int test_kmod_func_ok(int a, void *b, char c, short d) > +{ > + return a + (long)b + c + d; > +} > + > +noinline int test_kmod_func_struct_ok(int a, void *b, char c, > + struct kmod_struct d) > +{ > + return a + (long)b + c + d.a + d.b + d.c + d.d; > +} > + > +noinline int test_kmod_func_struct_on_stack_ok(int a, void *b, char c, short d, > + int e, void *f, char g, short h, > + struct kmod_struct i) > +{ > + return a + (long)b + c + d + e + (long)f + g + h + i.a + i.b + i.c + i.d; > +} > + > +noinline int test_kmod_func_struct_on_stack_ko(int a, void *b, char c, short d, > + int e, void *f, char g, short h, > + struct kmod_struct_packed i) > +{ > + return a + (long)b + c + d + e + (long)f + g + h + i.a + i.b + i.c + i.d; > +} > + > +static int kmod_test_init(void) > +{ > + struct kmod_struct test; > + struct kmod_struct_packed test_bis; > + > + test_kmod_func_ok(0, NULL, 0, 0); > + test_kmod_func_struct_ok(0, NULL, 0, test); > + test_kmod_func_struct_on_stack_ok(0, NULL, 0, 0, 0, NULL, 0, 0, test); > + test_kmod_func_struct_on_stack_ko(0, NULL, 0, 0, 0, NULL, 0, 0, test_bis); > + return 0; > +} > + > +module_init(kmod_test_init); > + > +MODULE_AUTHOR("Alexis Lothoré"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Pahole testing module"); > ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location 2025-07-03 18:31 ` Ihor Solodrai @ 2025-07-04 9:06 ` Alexis Lothoré 0 siblings, 0 replies; 12+ messages in thread From: Alexis Lothoré @ 2025-07-04 9:06 UTC (permalink / raw) To: Ihor Solodrai, dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf On Thu Jul 3, 2025 at 8:31 PM CEST, Ihor Solodrai wrote: > On 7/3/25 2:02 AM, Alexis Lothoré (eBPF Foundation) wrote: >> Add a small kernel module representing specific cases likely absent from >> standard vmlinux files. As a starter, the introduced module exposes a >> few functions consuming structs passed by value, some passed by >> register, some passed on the stack: >> >> int kmod_test_init(void); >> int test_kmod_func_ok(int, void *, char, short int); >> int test_kmod_func_struct_ok(int, void *, char, struct kmod_struct); >> int test_kmod_func_struct_on_stack_ok(int, void *, char, short int, int, \ >> void *, char, short int, struct kmod_struct); >> int test_kmod_func_struct_on_stack_ko(int, void *, char, short int, int, \ >> void *, char, short int, struct kmod_struct_packed); >> >> Then enrich btf_functions.sh to make it perform the following steps: >> - build the module >> - generate BTF info and pfunct listing, both with dwarf and the >> generated BTF >> - check that any function encoded in BTF is found in DWARF >> - check that any function announced as skipped is indeed absent from BTF >> - check that any skipped function has been skipped due to uncertain >> parameter location >> >> Those new tests are executed only if a kernel directory is provided as >> script's second argument, they are otherwise skipped. > > While this shouldn't be a problem for CI, since it checks out a kernel > tree to test vmlinux as input, I wonder if there is a way to do the > same test without this dependency. > > We need to generate a binary with DWARF, containing function > prototypes with packed/aligned attributes. Give it to pahole and see > that those functions were skipped. > > Any reason it must be a kernel module? Am I missing something? I guess I have no valid reason, I just focused too much on a specific use case :) It would indeed be simpler with a bare userspace binary, I'll check further and change it. >> Example of the new test execution: >> Encoding...Matched 4 functions exactly. >> Ok >> Validation of skipped function logic... >> Skipped encoding 1 functions in BTF. >> Ok >> Validating skipped functions have uncertain parameter location... >> Found 1 legitimately skipped function due to uncertain loc >> Ok > This part fails for me: > > isolodrai@isolodrai-fedora-PC2K40WQ:~/pahole/tests$ > KDIR=/home/isolodrai/kernels/bpf-next > vmlinux=/home/isolodrai/kernels/bpf-next/vmlinux ./btf_functions.sh > Validation of BTF encoding of functions; this may take some time: Ok > Validation of BTF encoding corner cases with kmod functions; this may > take some time: make: Entering directory '/home/isolodrai/kernels/bpf-next' > Makefile:199: *** specified external module directory "./kmod" does not > exist. Stop. > make: Leaving directory '/home/isolodrai/kernels/bpf-next' > No skipped functions. Done. > > Maybe: > > diff --git a/tests/btf_functions.sh b/tests/btf_functions.sh > index 64810b7..fcb1591 100755 > --- a/tests/btf_functions.sh > +++ b/tests/btf_functions.sh > @@ -208,7 +208,7 @@ fi > echo -n "Validation of BTF encoding corner cases with kmod functions; > this may take some time: " > > test -n "$VERBOSE" && printf "\nBuilding kmod..." > -tests_dir=$(dirname $0) > +tests_dir=$(realpath $(dirname $0)) > make -C ${KDIR} M=${tests_dir}/kmod > > test -n "$VERBOSE" && printf "\nEncoding..." > > > Also, in case kernel is built with LLVM, one must set LLVM=1. > Not sure if this is detectable by the test. Yeah, the tests_dir computation is a bit fragile. I saw it in tests.sh, and so I assumed use cases were simple enough to keep this simple logic. I'll update it to make it more robust. Alexis -- Alexis Lothoré, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] gitignore: ignore all the test kmod build-related files 2025-07-03 9:02 [PATCH v2 0/3] btf_encoder: do not encode functions consuming packed structs on stack Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 ` Alexis Lothoré (eBPF Foundation) 2 siblings, 0 replies; 12+ messages in thread From: Alexis Lothoré (eBPF Foundation) @ 2025-07-03 9:02 UTC (permalink / raw) To: dwarves Cc: bpf, Alan Maguire, Arnaldo Carvalho de Melo, Alexei Starovoitov, Thomas Petazzoni, Bastien Curutchet, ebpf, Alexis Lothoré (eBPF Foundation) The kmod module generates quite a lot of intermediate build files, so ignore those in git. Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- Changes in v2: - new patch --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 96e05c7842624067ed5571bccbaae76122a66567..b82205d4ee2d0a83ac736c3c879d46d44e0212d6 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,5 @@ /build /config.h +tests/kmod/* +!tests/kmod/kmod.c +!tests/kmod/Makefile -- 2.50.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-07-04 21:12 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-03 9:02 [PATCH v2 0/3] btf_encoder: do not encode functions consuming packed structs on stack Alexis Lothoré (eBPF Foundation) 2025-07-03 9:02 ` [PATCH v2 1/3] btf_encoder: skip functions consuming packed structs passed by value " Alexis Lothoré (eBPF Foundation) 2025-07-03 18:17 ` Ihor Solodrai 2025-07-04 9:01 ` Alexis Lothoré 2025-07-04 19:59 ` Ihor Solodrai 2025-07-04 21:10 ` Alexis Lothoré 2025-07-04 20:05 ` Ihor Solodrai 2025-07-04 21:12 ` Alexis Lothoré 2025-07-03 9:02 ` [PATCH v2 2/3] tests: add some tests validating skipped functions due to uncertain arg location Alexis Lothoré (eBPF Foundation) 2025-07-03 18:31 ` Ihor Solodrai 2025-07-04 9:06 ` Alexis Lothoré 2025-07-03 9:02 ` [PATCH v2 3/3] gitignore: ignore all the test kmod build-related files Alexis Lothoré (eBPF Foundation)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).