lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240806105941.GV37996@noisy.programming.kicks-ass.net>
Date: Tue, 6 Aug 2024 12:59:41 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	"Liang, Kan" <kan.liang@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] perf: Avoid -Wflex-array-member-not-at-end warnings

On Mon, Aug 05, 2024 at 04:42:35PM -0600, Gustavo A. R. Silva wrote:
> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
> getting ready to enable it, globally.
> 
> So, in order to avoid ending up with a flexible-array member in the
> middle of multiple other structs, we use the `struct_group_tagged()`
> helper to create a new tagged `struct perf_branch_stack_hdr`.
> This structure groups together all the members of the flexible
> `struct perf_branch_stack` except the flexible array.
> 
> As a result, the array is effectively separated from the rest of the
> members without modifying the memory layout of the flexible structure.
> We then change the type of the middle struct members currently causing
> trouble from `struct perf_branch_stack` to `struct perf_branch_stack_hdr`.
> 
> We also want to ensure that when new members need to be added to the
> flexible structure, they are always included within the newly created
> tagged struct. For this, we use `static_assert()`. This ensures that the
> memory layout for both the flexible structure and the new tagged struct
> is the same after any changes.
> 
> This approach avoids having to implement `struct perf_branch_stack_hdr`
> as a completely separate structure, thus preventing having to maintain
> two independent but basically identical structures, closing the door
> to potential bugs in the future.
> 
> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure, through which the flexible-array member can be
> accessed, if necessary.
> 
> So, with these changes, fix the following warnings:
> arch/x86/events/amd/../perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> arch/x86/events/intel/../perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> arch/x86/events/perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> arch/x86/events/zhaoxin/../perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> ./arch/x86/include/generated/../../events/perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> arch/x86/xen/../events/perf_event.h:289:41: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Urgh, this fugly.. And you're breaking coding style :-(

Let me see if this can't be done sanely...

> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  arch/x86/events/amd/core.c   | 4 +++-
>  arch/x86/events/core.c       | 4 +++-
>  arch/x86/events/intel/ds.c   | 4 +++-
>  arch/x86/events/intel/lbr.c  | 8 ++++++--
>  arch/x86/events/perf_event.h | 2 +-
>  include/linux/perf_event.h   | 9 +++++++--
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 920e3a640cad..37adf2fcf508 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -1001,7 +1001,9 @@ static int amd_pmu_v2_handle_irq(struct pt_regs *regs)
>  			continue;
>  
>  		if (has_branch_stack(event))
> -			perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> +			perf_sample_save_brstack(&data, event,
> +				container_of(&cpuc->lbr_stack, struct perf_branch_stack, hdr),
> +				NULL);
>  
>  		if (perf_event_overflow(event, &data, regs))
>  			x86_pmu_stop(event, 0);
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 12f2a0c14d33..e7faf7eedfc4 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1704,7 +1704,9 @@ int x86_pmu_handle_irq(struct pt_regs *regs)
>  		perf_sample_data_init(&data, 0, event->hw.last_period);
>  
>  		if (has_branch_stack(event))
> -			perf_sample_save_brstack(&data, event, &cpuc->lbr_stack, NULL);
> +			perf_sample_save_brstack(&data, event,
> +				container_of(&cpuc->lbr_stack, struct perf_branch_stack, hdr),
> +				NULL);
>  
>  		if (perf_event_overflow(event, &data, regs))
>  			x86_pmu_stop(event, 0);
> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index fa5ea65de0d0..d5c86edbeaa9 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c
> @@ -1869,7 +1869,9 @@ static void setup_pebs_fixed_sample_data(struct perf_event *event,
>  		setup_pebs_time(event, data, pebs->tsc);
>  
>  	if (has_branch_stack(event))
> -		perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> +		perf_sample_save_brstack(data, event,
> +			container_of(&cpuc->lbr_stack, struct perf_branch_stack, hdr),
> +			NULL);
>  }
>  
>  static void adaptive_pebs_save_regs(struct pt_regs *regs,
> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
> index dc641b50814e..79ca1a2d6ff4 100644
> --- a/arch/x86/events/intel/lbr.c
> +++ b/arch/x86/events/intel/lbr.c
> @@ -974,11 +974,15 @@ void intel_pmu_lbr_save_brstack(struct perf_sample_data *data,
>  {
>  	if (is_branch_counters_group(event)) {
>  		intel_pmu_lbr_counters_reorder(cpuc, event);
> -		perf_sample_save_brstack(data, event, &cpuc->lbr_stack, cpuc->lbr_counters);
> +		perf_sample_save_brstack(data, event,
> +			container_of(&cpuc->lbr_stack, struct perf_branch_stack, hdr),
> +			cpuc->lbr_counters);
>  		return;
>  	}
>  
> -	perf_sample_save_brstack(data, event, &cpuc->lbr_stack, NULL);
> +	perf_sample_save_brstack(data, event,
> +		container_of(&cpuc->lbr_stack, struct perf_branch_stack, hdr),
> +		NULL);
>  }
>  
>  static void intel_pmu_arch_lbr_read(struct cpu_hw_events *cpuc)
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index ac1182141bf6..bf05cdc3f22f 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -286,7 +286,7 @@ struct cpu_hw_events {
>  	 */
>  	int				lbr_users;
>  	int				lbr_pebs_users;
> -	struct perf_branch_stack	lbr_stack;
> +	struct perf_branch_stack_hdr	lbr_stack;
>  	struct perf_branch_entry	lbr_entries[MAX_LBR_ENTRIES];
>  	u64				lbr_counters[MAX_LBR_ENTRIES]; /* branch stack extra */
>  	union {
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 1a8942277dda..eb4413917b39 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -121,10 +121,15 @@ static __always_inline bool perf_raw_frag_last(const struct perf_raw_frag *frag)
>   * already stored in age order, the hw_idx should be 0.
>   */
>  struct perf_branch_stack {
> -	__u64				nr;
> -	__u64				hw_idx;
> +	/* New members MUST be added within the struct_group() macro below. */
> +	struct_group_tagged(perf_branch_stack_hdr, hdr,
> +		__u64				nr;
> +		__u64				hw_idx;
> +	);
>  	struct perf_branch_entry	entries[];
>  };
> +static_assert(offsetof(struct perf_branch_stack, entries) == sizeof(struct perf_branch_stack_hdr),
> +	      "struct member likely outside of struct_group_tagged()");
>  
>  struct task_struct;
>  
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ