Message-ID: <4bf43164-b130-4643-9f4f-761f49bd0dc9@embeddedor.com>
Date: Mon, 9 Feb 2026 15:23:59 +0900
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Kees Cook <kees@...nel.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc: Stanislaw Gruszka <stf_xl@...pl>,
 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



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