linux-kernel - Re: [PATCH 3/8][next] xfs: Avoid -Wflex-array-member-not-at-end warnings

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: <Z7zohueO2E3ZSpfj@dread.disaster.area>
Date: Tue, 25 Feb 2025 08:45:42 +1100
From: Dave Chinner <david@...morbit.com>
To: "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Carlos Maiolino <cem@...nel.org>, "Darrick J. Wong" <djwong@...nel.org>,
	linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH 3/8][next] xfs: Avoid -Wflex-array-member-not-at-end
 warnings

On Mon, Feb 24, 2025 at 08:27:44PM +1030, 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.
> 
> Change the type of the middle struct members currently causing trouble
> from `struct bio` to `struct bio_hdr`.

What's this bio_hdr thing? You haven't sent the patch to the XFS
list, so we cannot review this for correctness. Please cc the
-entire- patch set to -all- recipients so we can see the whole
change in it's full context.

> We also use `container_of()` whenever we need to retrieve a pointer to
> the flexible structure `struct bio`, through which we can access the
> flexible-array member in it, if necessary.
> 
> With these changes fix 27 of the following warnings:
> 
> fs/xfs/xfs_log_priv.h:208:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  fs/xfs/xfs_log.c      | 15 +++++++++------
>  fs/xfs/xfs_log_priv.h |  2 +-
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index f8851ff835de..7e8b71f64a46 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1245,7 +1245,7 @@ xlog_ioend_work(
>  	}
>  
>  	xlog_state_done_syncing(iclog);
> -	bio_uninit(&iclog->ic_bio);
> +	bio_uninit(container_of(&iclog->ic_bio, struct bio, __hdr));

This is a pretty nasty conversion. The code is obviously correct
right now - the bio uses an external bvec table, so has no
associated allocated bvec space and so the bio isn't flexibly
sized.

Making the code more complex for humans to understand and get
right because "the compiler is dumb" is a bad tradeoff.

I also think this is a really nasty way of fixing the problem;
casting the fixed size structure to a flex sized structure that can
overlap other parent structure fields really isn't a good solution.
IMO, it is a recipe for unexpected memory corruption when the bio
isn't set up properly by the caller or there are bugs in the way the
bio is iterated/processed....

>  	return;
>  shutdown:
>  	xlog_force_shutdown(log, SHUTDOWN_LOG_IO_ERROR);
> diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
> index f3d78869e5e5..32abc48aef24 100644
> --- a/fs/xfs/xfs_log_priv.h
> +++ b/fs/xfs/xfs_log_priv.h
> @@ -205,7 +205,7 @@ typedef struct xlog_in_core {
>  #endif
>  	struct semaphore	ic_sema;
>  	struct work_struct	ic_end_io_work;
> -	struct bio		ic_bio;
> +	struct bio_hdr		ic_bio;
>  	struct bio_vec          ic_bvec[];

But then there is the bigger question: ic_bvec is a *fixed size*
that is allocated at mount time. The fact it is defined as a
flexible array is the problem here, not the embedding of a struct
bio. i.e. we've used a flex array to optimise away an extra
allocation in a non-performance sensitive path.

Hence if we had done it this way:

	struct bvec		*ic_bvecs;
	struct bio		ic_bio;		// must be last
};

and use another allocation for ic_bvecs in xlog_alloc_log() (similar
to how we do the separate allocation of the *ic_data buffer that is
mapped into ic_bio at IO time), then none of the code that accesses
ic_bio for IO purposes needs to change and the compiler warnings go
away. That's a much cleaner solution that requires no extra
cognitive load on developers to maintain in correct working order.

-Dave.
-- 
Dave Chinner
david@...morbit.com