Re: [PATCH v2][next] iwlegacy: Avoid multiple -Wflex-array-member-not-at-end warnings

[Kees Cook <kees@kernel.org>]

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.

> 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")?

> 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".

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:

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?

> 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! :)

-- 
Kees Cook