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

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

[Gustavo A. R. Silva]

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

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

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

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.

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]

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Use the struct_group() helper, and update the conflicting type
   (struct il4965_tx_resp -> struct il4965_tx_resp_hdr) in
   struct il4965_beacon_notif.

v1:
 - Link: https://lore.kernel.org/linux-hardening/aR2CtqZI3atH0HmE@kspp/

 drivers/net/wireless/intel/iwlegacy/3945.h    |  4 +-
 .../net/wireless/intel/iwlegacy/4965-mac.c    |  2 +-
 .../net/wireless/intel/iwlegacy/commands.h    | 41 +++++++++++--------
 drivers/net/wireless/intel/iwlegacy/common.h  |  4 +-
 4 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlegacy/3945.h b/drivers/net/wireless/intel/iwlegacy/3945.h
index fb1e33c89d0e..ed63b31fee9a 100644
--- a/drivers/net/wireless/intel/iwlegacy/3945.h
+++ b/drivers/net/wireless/intel/iwlegacy/3945.h
@@ -123,13 +123,15 @@ enum il3945_antenna {
 #define IEEE80211_FRAME_LEN             (IEEE80211_DATA_LEN + IEEE80211_HLEN)
 
 struct il3945_frame {
+	struct list_head list;
+
+	/* Must be last as it ends in a flexible-array member. */
 	union {
 		struct ieee80211_hdr frame;
 		struct il3945_tx_beacon_cmd beacon;
 		u8 raw[IEEE80211_FRAME_LEN];
 		u8 cmd[360];
 	} u;
-	struct list_head list;
 };
 
 #define SUP_RATE_11A_MAX_NUM_CHANNELS  8
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index 57fa866efd9f..f5a99a2ee95a 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -4076,7 +4076,7 @@ il4965_hdl_beacon(struct il_priv *il, struct il_rx_buf *rxb)
 	u8 rate = il4965_hw_get_rate(beacon->beacon_notify_hdr.rate_n_flags);
 
 	D_RX("beacon status %x retries %d iss %d tsf:0x%.8x%.8x rate %d\n",
-	     le32_to_cpu(beacon->beacon_notify_hdr.u.status) & TX_STATUS_MSK,
+	     le32_to_cpu(beacon->beacon_tx_status) & TX_STATUS_MSK,
 	     beacon->beacon_notify_hdr.failure_frame,
 	     le32_to_cpu(beacon->ibss_mgr_status),
 	     le32_to_cpu(beacon->high_tsf), le32_to_cpu(beacon->low_tsf), rate);
diff --git a/drivers/net/wireless/intel/iwlegacy/commands.h b/drivers/net/wireless/intel/iwlegacy/commands.h
index b61b8f377702..630bc12cd0d5 100644
--- a/drivers/net/wireless/intel/iwlegacy/commands.h
+++ b/drivers/net/wireless/intel/iwlegacy/commands.h
@@ -1691,22 +1691,25 @@ struct agg_tx_status {
 } __packed;
 
 struct il4965_tx_resp {
-	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 */
-	u8 failure_frame;	/* # failures due to no ACK (unused for agg) */
-
-	/* For non-agg:  Rate at which frame was successful.
-	 * For agg:  Rate at which all frames were transmitted. */
-	__le32 rate_n_flags;	/* RATE_MCS_*  */
-
-	/* For non-agg:  RTS + CTS + frame tx attempts time + ACK.
-	 * For agg:  RTS + CTS + aggregation tx time + block-ack time. */
-	__le16 wireless_media_time;	/* uSecs */
-
-	__le16 reserved;
-	__le32 pa_power1;	/* RF power amplifier measurement (not used) */
-	__le32 pa_power2;
+	/* New members MUST be added within the __struct_group() macro below. */
+	__struct_group(il4965_tx_resp_hdr, __hdr, __packed,
+		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 */
+		u8 failure_frame;	/* # failures due to no ACK (unused for agg) */
+
+		/* For non-agg:  Rate at which frame was successful.
+		 * For agg:  Rate at which all frames were transmitted. */
+		__le32 rate_n_flags;	/* RATE_MCS_*  */
+
+		/* For non-agg:  RTS + CTS + frame tx attempts time + ACK.
+		 * For agg:  RTS + CTS + aggregation tx time + block-ack time. */
+		__le16 wireless_media_time;	/* uSecs */
+
+		__le16 reserved;
+		__le32 pa_power1;	/* RF power amplifier measurement (not used) */
+		__le32 pa_power2;
+	);
 
 	/*
 	 * For non-agg:  frame status TX_STATUS_*
@@ -1726,6 +1729,9 @@ struct il4965_tx_resp {
 		DECLARE_FLEX_ARRAY(struct agg_tx_status, agg_status);	/* for each agg frame */
 	} u;
 } __packed;
+static_assert(offsetof(struct il4965_tx_resp, u.agg_status) ==
+	      sizeof(struct il4965_tx_resp_hdr),
+	      "struct member likely outside of __struct_group()");
 
 /*
  * N_COMPRESSED_BA = 0xc5 (response only, not a command)
@@ -2664,7 +2670,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 beacon_tx_status;
 	__le32 low_tsf;
 	__le32 high_tsf;
 	__le32 ibss_mgr_status;
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 4c9836ab11dd..21f1c7702add 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -518,13 +518,15 @@ struct il_channel_info {
 #define IEEE80211_FRAME_LEN             (IEEE80211_DATA_LEN + IEEE80211_HLEN)
 
 struct il_frame {
+	struct list_head list;
+
+	/* Must be last as it ends in a flexible-array member. */
 	union {
 		struct ieee80211_hdr frame;
 		struct il_tx_beacon_cmd beacon;
 		u8 raw[IEEE80211_FRAME_LEN];
 		u8 cmd[360];
 	} u;
-	struct list_head list;
 };
 
 enum {
-- 
2.43.0

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

[Gustavo A. R. Silva]

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

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

[Gustavo A. R. Silva]

On 2/10/26 07:20, Kees Cook wrote:
> On Mon, Feb 09, 2026 at 03:23:59PM +0900, Gustavo A. R. Silva wrote:
>> 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
> 
> Hm? No, that's what transparent struct members avoid: there is no
> sub-struct name, the members of the struct are transparently visible in
> the surrounding struct:

Ah yes, that's why it's defined like

+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;

and not like

+struct il4965_tx_resp {
+	struct il4965_tx_resp_hdr hdr;

  	/*
  	 * For non-agg:  frame status TX_STATUS_*
@@ -2664,7 +2668,8 @@ struct il3945_beacon_notif {
  } __packed;

> 
> struct inside {
> 	int a;
> 	int b;
> };
> 
> struct foo {
> 	struct inside;
> 	int c;
> } *p;
> 
> "p->a" is valid.

Yes, gotcha!

> 
>> 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?
> 
> Right, that's still in progress. I don't expect it soon, though. :(
> 

Okay.

-Gustavo

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

[Kees Cook]

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

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

[Kees Cook]

On Mon, Feb 09, 2026 at 03:23:59PM +0900, Gustavo A. R. Silva wrote:
> 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

Hm? No, that's what transparent struct members avoid: there is no
sub-struct name, the members of the struct are transparently visible in
the surrounding struct:

struct inside {
	int a;
	int b;
};

struct foo {
	struct inside;
	int c;
} *p;

"p->a" is valid.

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

Right, that's still in progress. I don't expect it soon, though. :(

-- 
Kees Cook

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

[Gustavo A. R. Silva]

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

Sure thing.

Thanks for the feedback
-Gustavo

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

[Stanislaw Gruszka]

On Mon, Feb 09, 2026 at 12:46:23PM -0800, Kees Cook wrote:
> @@ -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?

Both options, with __struct_group(), and this one, are ok for me.

Regards
Stanislaw

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

[Stanislaw Gruszka]

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
>