Re: [PATCH v3 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack

[Ihor Solodrai <ihor.solodrai@xxxxxxxxx>]

On 7/7/25 7: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@xxxxxxxxxxx>
> ---
> Changes in v3:
> - remove unneeded class__find_holes (already done by
>    class__infer_packed_attributes)
> - add uncertain parm loc in saved_functions_combine

Acked-by: Ihor Solodrai <ihor.solodrai@xxxxxxxxx>

Thank you!

> 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 | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>   dwarves.h     |  1 +
>   2 files changed, 51 insertions(+), 3 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index 0bc23349b5d740c3ddab8208b2e15cdbdd139b9d..3f040fe03d7a208aa742914513bacde9782aabcf 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) ?: "";
>   
> @@ -1365,7 +1367,7 @@ static int saved_functions_cmp(const void *_a, const void *_b)
>   
>   static int saved_functions_combine(struct btf_encoder_func_state *a, struct btf_encoder_func_state *b)
>   {
> -	uint8_t optimized, unexpected, inconsistent;
> +	uint8_t optimized, unexpected, inconsistent, uncertain_parm_loc;
>   	int ret;
>   
>   	ret = strncmp(a->elf->name, b->elf->name,
> @@ -1375,11 +1377,13 @@ static int saved_functions_combine(struct btf_encoder_func_state *a, struct btf_
>   	optimized = a->optimized_parms | b->optimized_parms;
>   	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(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;
>   
>   	return 0;
>   }
> @@ -1430,9 +1434,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 +2563,38 @@ 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__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 +2689,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 +2737,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;