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
