Message-ID: <20260216184033.GB10063@wp.pl>
Date: Mon, 16 Feb 2026 19:40:33 +0100
From: Stanislaw Gruszka <stf_xl@...pl>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc: Kees Cook <kees@...nel.org>,
	"Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Johannes Berg <johannes@...solutions.net>,
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2][next] iwlegacy: Avoid multiple
 -Wflex-array-member-not-at-end warnings

Hi

On Mon, Feb 09, 2026 at 03:23:59PM +0900, Gustavo A. R. Silva wrote:

When you will repost, please use 'wifi: iwlegacy:' prefix in the title.

Regards
Stanislaw

> On 2/10/26 05:46, Kees Cook wrote:
> > On Mon, Feb 09, 2026 at 01:38:15PM +0900, 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.
> > > 
> > > Move the conflicting declarations (which in a couple of cases happen
> > > to be in a union, so the entire unions are moved) to the end of the
> > > corresponding structures. Notice that `struct il_tx_beacon_cmd`,
> > > `struct il4965_tx_resp`, and `struct il3945_tx_beacon_cmd` are flexible
> > > structures, this is structures that contain a flexible-array member.
> > 
> > I think explicit mention of il3945_frame and il_frame should be included
> > in the commit log (probably after the above), as they are the ones that
> > contain the mentioned il*_tx_beacon_cmd structs that have a trailing
> > flex array.
> 
> Okay.
> 
> > 
> > > The case for struct il4965_beacon_notif is different. Since this
> > > structure is defined by hardware, we use the struct_group() helper
> > > to create the new `struct il4965_tx_resp_hdr` type. We then use this newly
> > > created type to replace the obhect type of  causing trouble in
> > > struct il4965_beacon_notif, namely `stryct il4965_tx_resp`.
> > 
> > Above two lines have typos and maybe a missing name (between "of" and
> > "causing")?
> 
> Aggh.. yes, I had fixed this before, but somehow I ended up using the
> wrong changelog text. Thanks for the catch!
> 
> > 
> > > In order to preserve the memory layout in struct il4965_beacon_notif,
> > > add member `__le32 beacon_tx_status`, which was previously included
> > > by `struct il4965_tx_resp` (as `__le32 status`), but it's not present
> > > in the newly created type `struct il4965_tx_resp_hdr`.
> > 
> > It may help to explicitly mention how the union exists to provide the
> > "status" member to anything using struct il4965_tx_resp, but there's no
> > sane way to do the overlap across structs, so anything using
> > il4965_beacon_notif needs have its own view of "status".
> 
> Okay.
> 
> > 
> > It does feel like accessing il4965_tx_resp's agg_status should be using
> > a different struct (like happens for other things that want bytes beyond
> > "status"), but okay.
> > 
> > Anyway, it's another situation of a header with a trailing flex array
> > that needs to overlap with differing deserializations of the trailing
> > bytes. The complication here is the kind of "layering violation" of
> > agg_status and status.
> > 
> > > Notice that after this changes, the size of struct il4965_beacon_notif
> > > along with its member's offsets remain the same, hence the memory
> > > layout doesn't change:
> > > 
> > > Before changes:
> > > struct il4965_beacon_notif {
> > > 	struct il4965_tx_resp      beacon_notify_hdr;    /*     0    24 */
> > > 	__le32                     low_tsf;              /*    24     4 */
> > > 	__le32                     high_tsf;             /*    28     4 */
> > > 	__le32                     ibss_mgr_status;      /*    32     4 */
> > > 
> > > 	/* size: 36, cachelines: 1, members: 4 */
> > > 	/* last cacheline: 36 bytes */
> > > };
> > > 
> > > After changes:
> > > struct il4965_beacon_notif {
> > > 	struct il4965_tx_resp_hdr  beacon_notify_hdr;    /*     0    20 */
> > > 	__le32                     beacon_tx_status;     /*    20     4 */
> > > 	__le32                     low_tsf;              /*    24     4 */
> > > 	__le32                     high_tsf;             /*    28     4 */
> > > 	__le32                     ibss_mgr_status;      /*    32     4 */
> > > 
> > > 	/* size: 36, cachelines: 1, members: 5 */
> > > 	/* last cacheline: 36 bytes */
> > > };
> > > 
> > > We also want to ensure that when new members are added to the flexible
> > > structure `struct il4965_tx_resp` (if any), they are always included
> > > within the newly created struct type. To enforce this, we use
> > > `static_assert()` (which is intentionally placed right after the struct,
> > > this is, no blank line in between). This ensures that the memory layout
> > > of both the flexible structure and the new `struct il4965_tx_resp_hdr`
> > > type remains consistent after any changes.
> > > 
> > > Lastly, refactor the rest of the code, accordingly.
> > 
> > I think the changes look consistent with other similar logical changes
> > that have been made for -Wfamnae.
> > 
> > Since enabling -fms-extensions I'm on the look-out for cases where
> > we can use transparent struct members (i.e. define the header struct
> > separately and then use it transparently) instead of using the struct
> > group when we don't need to make the interior explicitly addressable
> > (as we have in this case), as it makes the diff way smaller:
> 
> Ah yes, I can do this. The only thing is that I'd have to change every
> place where members in struct il4965_tx_resp are used, e.g.
> 
> s/frame_count/hdr.frame_count
> 
> and so on...
> 
> Another thing to take into account (fortunately, not in this case) is
> when the FAM needs to be annotated with __counted_by(). If we use a
> separate struct for the header portion of the flexible structure, GCC
> currently cannot _see_ the _counter_ if it's included in a non-anonymous
> structure. However, this will be possible in the near future, correct?
> 
> > 
> > diff --git a/drivers/net/wireless/intel/iwlegacy/commands.h b/drivers/net/wireless/intel/iwlegacy/commands.h
> > index b61b8f377702..440dff2a4ad9 100644
> > --- a/drivers/net/wireless/intel/iwlegacy/commands.h
> > +++ b/drivers/net/wireless/intel/iwlegacy/commands.h
> > @@ -1690,7 +1690,7 @@ struct agg_tx_status {
> >   	__le16 sequence;
> >   } __packed;
> > -struct il4965_tx_resp {
> > +struct il4965_tx_resp_hdr {
> >   	u8 frame_count;		/* 1 no aggregation, >1 aggregation */
> >   	u8 bt_kill_count;	/* # blocked by bluetooth (unused for agg) */
> >   	u8 failure_rts;		/* # failures due to unsuccessful RTS */
> > @@ -1707,6 +1707,10 @@ struct il4965_tx_resp {
> >   	__le16 reserved;
> >   	__le32 pa_power1;	/* RF power amplifier measurement (not used) */
> >   	__le32 pa_power2;
> > +} __packed;
> > +
> > +struct il4965_tx_resp {
> > +	struct il4965_tx_resp_hdr;
> >   	/*
> >   	 * For non-agg:  frame status TX_STATUS_*
> > @@ -2664,7 +2668,8 @@ struct il3945_beacon_notif {
> >   } __packed;
> >   struct il4965_beacon_notif {
> > -	struct il4965_tx_resp beacon_notify_hdr;
> > +	struct il4965_tx_resp_hdr beacon_notify_hdr;
> > +	__le32 status;
> >   	__le32 low_tsf;
> >   	__le32 high_tsf;
> >   	__le32 ibss_mgr_status;
> > 
> > 
> > What do folks think?
> 
> I'll wait for maintainers to chime in.
> 
> > 
> > > With these changes fix the following warnings:
> > > 
> > > 11 drivers/net/wireless/intel/iwlegacy/common.h:526:11: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> > > 11 drivers/net/wireless/intel/iwlegacy/commands.h:2667:31: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> > > 4 drivers/net/wireless/intel/iwlegacy/3945.h:131:11: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> > 
> > Yay for getting these gone! :)
> > 
> 
> We're getting there! \o/
> 
> Thanks
> -Gustavo
> 

Powered by blists - more mailing lists