From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-180.mta0.migadu.com (out-180.mta0.migadu.com [91.218.175.180]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 557D1219EB for ; Mon, 7 Jul 2025 17:45:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.180 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751910358; cv=none; b=LD0WmaU1z9WmaVQ1c/6OmBbcdnYn7ra3Dd5Vzd5V05c3SNg92jLrEzGf0Xp5i2I80gt5l3Vj/iyJS1uLo4pKH2Z92LOagIUy3hX7/HOTFvlw6gSiGGuy74AOqYA/2qRvbA1crRRP2jIJyxPAiDV/oQZ/vQI5orsBr/NcGt527Rs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1751910358; c=relaxed/simple; bh=XL/L5BqyMAPcoVY/4Vxv2KfmmgF09PyG7gtrsVlrSsA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ZDyVvzA9RpqIKw4eH/Mqty72GJwjm1r6ZmELFWCYLkOJjgxMr1FGpQbABkX9k8VnPG5Wl2X8CntIN2/mTK4e48xK+Gz8MTJ95Lm/OCbufJrr3Mmpy7pgkfHhiTPnXCPZmp7YwP5NHK/5xgH38Uu003lwUXU6kg6Z/iE+v1HMrwQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=HnRhYoGo; arc=none smtp.client-ip=91.218.175.180 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="HnRhYoGo" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1751910354; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=o/I7b4ObseyNUkhPmIqIc+OoZ+dFxFq1PXe/5W4ZG5A=; b=HnRhYoGo29nNqwEKLsr6bRRGHBDYJ6QCDMOYC1AUKtfx3QCXEIrCupPo+eWvO4uBzltsWz MFB+XPxo4JiTkUbSW/icUL4d3ndSjeoio/YBxIDy19T+zylR9YmSzH8nFYuN+otqvhst6Y +OaGQd/cHlviHvZmHyqXF6+dlTa/QNU= Date: Mon, 7 Jul 2025 10:45:41 -0700 Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v3 1/3] btf_encoder: skip functions consuming packed structs passed by value on stack To: =?UTF-8?Q?Alexis_Lothor=C3=A9_=28eBPF_Foundation=29?= , dwarves@vger.kernel.org Cc: bpf@vger.kernel.org, Alan Maguire , Arnaldo Carvalho de Melo , Alexei Starovoitov , Thomas Petazzoni , Bastien Curutchet , ebpf@linuxfoundation.org References: <20250707-btf_skip_structs_on_stack-v3-0-29569e086c12@bootlin.com> <20250707-btf_skip_structs_on_stack-v3-1-29569e086c12@bootlin.com> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Ihor Solodrai In-Reply-To: <20250707-btf_skip_structs_on_stack-v3-1-29569e086c12@bootlin.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 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) > --- > 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 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; >