| Message ID | ZrJmjM4izqDqwIrc@cute (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] wifi: radiotap: Avoid -Wflex-array-member-not-at-end warnings | expand |
On Tue, Aug 06, 2024 at 12:08:12PM -0600, 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. > > So, in order to avoid ending up with a flexible-array member in the > middle of multiple other structs, we use the `__struct_group()` > helper to create a new tagged `struct ieee80211_radiotap_header_hdr`. > This structure groups together all the members of the flexible > `struct ieee80211_radiotap_header` except the flexible array. > > As a result, the array is effectively separated from the rest of the > members without modifying the memory layout of the flexible structure. > We then change the type of the middle struct members currently causing > trouble from `struct ieee80211_radiotap_header` to `struct > ieee80211_radiotap_header_hdr`. > > We also want to ensure that in case new members need to be added to the > flexible structure, they are always included within the newly created > tagged struct. For this, we use `static_assert()`. This ensures that the > memory layout for both the flexible structure and the new tagged struct > is the same after any changes. > > This approach avoids having to implement `struct ieee80211_radiotap_header_hdr` > as a completely separate structure, thus preventing having to maintain > two independent but basically identical structures, closing the door > to potential bugs in the future. > > So, with these changes, fix the following warnings: > drivers/net/wireless/ath/wil6210/txrx.c:309:50: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/intel/ipw2x00/ipw2100.c:2521:50: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/intel/ipw2x00/ipw2200.h:1146:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/intel/ipw2x00/libipw.h:595:36: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/marvell/libertas/radiotap.h:34:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/marvell/libertas/radiotap.h:5:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/microchip/wilc1000/mon.c:10:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/microchip/wilc1000/mon.c:15:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/virtual/mac80211_hwsim.c:758:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/virtual/mac80211_hwsim.c:767:42: 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> Reviewed-by: Simon Horman <horms@kernel.org>
On Tue, 2024-08-06 at 12:08 -0600, Gustavo A. R. Silva wrote: > > +++ b/include/net/ieee80211_radiotap.h > @@ -24,31 +24,36 @@ > * struct ieee80211_radiotap_header - base radiotap header > */ > struct ieee80211_radiotap_header { > - /** > - * @it_version: radiotap version, always 0 > - */ > - uint8_t it_version; > - > - /** > - * @it_pad: padding (or alignment) > - */ > - uint8_t it_pad; > - > - /** > - * @it_len: overall radiotap header length > - */ > - __le16 it_len; > - > - /** > - * @it_present: (first) present word > - */ > - __le32 it_present; > + /* New members MUST be added within the __struct_group() macro below. */ There cannot be new members without breaking interop with the radiotap spec, that comment isn't really useful ;-) > + __struct_group(ieee80211_radiotap_header_hdr, hdr, __packed, And also, "header header" sounds odd to me really. The header of the header? What's what? I think perhaps better to call this ieee80211_radiotap_hdr_fixed or so? johannes
On 27/08/24 02:22, Johannes Berg wrote: > On Tue, 2024-08-06 at 12:08 -0600, Gustavo A. R. Silva wrote: >> >> +++ b/include/net/ieee80211_radiotap.h >> @@ -24,31 +24,36 @@ >> * struct ieee80211_radiotap_header - base radiotap header >> */ >> struct ieee80211_radiotap_header { >> - /** >> - * @it_version: radiotap version, always 0 >> - */ >> - uint8_t it_version; >> - >> - /** >> - * @it_pad: padding (or alignment) >> - */ >> - uint8_t it_pad; >> - >> - /** >> - * @it_len: overall radiotap header length >> - */ >> - __le16 it_len; >> - >> - /** >> - * @it_present: (first) present word >> - */ >> - __le32 it_present; >> + /* New members MUST be added within the __struct_group() macro below. */ > > There cannot be new members without breaking interop with the radiotap > spec, that comment isn't really useful ;-) Ah yes; the comment is not meant to be an invitation to add members to the struct just for the sake of it. It's there to indicate where they should be added if needed. > >> + __struct_group(ieee80211_radiotap_header_hdr, hdr, __packed, > > And also, "header header" sounds odd to me really. The header of the > header? What's what? > > I think perhaps better to call this ieee80211_radiotap_hdr_fixed or so? Sounds good to me. :) Thanks for the feedback! -- Gustavo
diff --git a/drivers/net/wireless/ath/wil6210/txrx.c b/drivers/net/wireless/ath/wil6210/txrx.c index f29ac6de7139..b7cc29939d59 100644 --- a/drivers/net/wireless/ath/wil6210/txrx.c +++ b/drivers/net/wireless/ath/wil6210/txrx.c @@ -306,7 +306,7 @@ static void wil_rx_add_radiotap_header(struct wil6210_priv *wil, struct sk_buff *skb) { struct wil6210_rtap { - struct ieee80211_radiotap_header rthdr; + struct ieee80211_radiotap_header_hdr rthdr; /* fields should be in the order of bits in rthdr.it_present */ /* flags */ u8 flags; diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2100.c b/drivers/net/wireless/intel/ipw2x00/ipw2100.c index b6636002c7d2..9d5524c8e95d 100644 --- a/drivers/net/wireless/intel/ipw2x00/ipw2100.c +++ b/drivers/net/wireless/intel/ipw2x00/ipw2100.c @@ -2518,7 +2518,7 @@ static void isr_rx_monitor(struct ipw2100_priv *priv, int i, * to build this manually element by element, we can write it much * more efficiently than we can parse it. ORDER MATTERS HERE */ struct ipw_rt_hdr { - struct ieee80211_radiotap_header rt_hdr; + struct ieee80211_radiotap_header_hdr rt_hdr; s8 rt_dbmsignal; /* signal in dbM, kluged to signed */ } *ipw_rt; diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2200.h b/drivers/net/wireless/intel/ipw2x00/ipw2200.h index 8ebf09121e17..becdd3bc5235 100644 --- a/drivers/net/wireless/intel/ipw2x00/ipw2200.h +++ b/drivers/net/wireless/intel/ipw2x00/ipw2200.h @@ -1143,7 +1143,7 @@ struct ipw_prom_priv { * structure is provided regardless of any bits unset. */ struct ipw_rt_hdr { - struct ieee80211_radiotap_header rt_hdr; + struct ieee80211_radiotap_header_hdr rt_hdr; u64 rt_tsf; /* TSF */ /* XXX */ u8 rt_flags; /* radiotap packet flags */ u8 rt_rate; /* rate in 500kb/s */ diff --git a/drivers/net/wireless/marvell/libertas/radiotap.h b/drivers/net/wireless/marvell/libertas/radiotap.h index 1ed5608d353f..95b427196f5a 100644 --- a/drivers/net/wireless/marvell/libertas/radiotap.h +++ b/drivers/net/wireless/marvell/libertas/radiotap.h @@ -2,7 +2,7 @@ #include <net/ieee80211_radiotap.h> struct tx_radiotap_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; u8 rate; u8 txpower; u8 rts_retries; @@ -31,7 +31,7 @@ struct tx_radiotap_hdr { #define IEEE80211_FC_DSTODS 0x0300 struct rx_radiotap_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; u8 flags; u8 rate; u8 antsignal; diff --git a/drivers/net/wireless/microchip/wilc1000/mon.c b/drivers/net/wireless/microchip/wilc1000/mon.c index 03b7229a0ff5..56cdc7a193dd 100644 --- a/drivers/net/wireless/microchip/wilc1000/mon.c +++ b/drivers/net/wireless/microchip/wilc1000/mon.c @@ -7,12 +7,12 @@ #include "cfg80211.h" struct wilc_wfi_radiotap_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; u8 rate; } __packed; struct wilc_wfi_radiotap_cb_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; u8 rate; u8 dump; u16 tx_flags; diff --git a/drivers/net/wireless/virtual/mac80211_hwsim.c b/drivers/net/wireless/virtual/mac80211_hwsim.c index d86e6ff4523d..6ab0f3a6f26c 100644 --- a/drivers/net/wireless/virtual/mac80211_hwsim.c +++ b/drivers/net/wireless/virtual/mac80211_hwsim.c @@ -763,7 +763,7 @@ static const struct rhashtable_params hwsim_rht_params = { }; struct hwsim_radiotap_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; __le64 rt_tsft; u8 rt_flags; u8 rt_rate; @@ -772,7 +772,7 @@ struct hwsim_radiotap_hdr { } __packed; struct hwsim_radiotap_ack_hdr { - struct ieee80211_radiotap_header hdr; + struct ieee80211_radiotap_header_hdr hdr; u8 rt_flags; u8 pad; __le16 rt_channel; diff --git a/include/net/ieee80211_radiotap.h b/include/net/ieee80211_radiotap.h index 91762faecc13..029137023779 100644 --- a/include/net/ieee80211_radiotap.h +++ b/include/net/ieee80211_radiotap.h @@ -24,31 +24,36 @@ * struct ieee80211_radiotap_header - base radiotap header */ struct ieee80211_radiotap_header { - /** - * @it_version: radiotap version, always 0 - */ - uint8_t it_version; - - /** - * @it_pad: padding (or alignment) - */ - uint8_t it_pad; - - /** - * @it_len: overall radiotap header length - */ - __le16 it_len; - - /** - * @it_present: (first) present word - */ - __le32 it_present; + /* New members MUST be added within the __struct_group() macro below. */ + __struct_group(ieee80211_radiotap_header_hdr, hdr, __packed, + /** + * @it_version: radiotap version, always 0 + */ + uint8_t it_version; + + /** + * @it_pad: padding (or alignment) + */ + uint8_t it_pad; + + /** + * @it_len: overall radiotap header length + */ + __le16 it_len; + + /** + * @it_present: (first) present word + */ + __le32 it_present; + ); /** * @it_optional: all remaining presence bitmaps */ __le32 it_optional[]; } __packed; +static_assert(offsetof(struct ieee80211_radiotap_header, it_optional) == sizeof(struct ieee80211_radiotap_header_hdr), + "struct member likely outside of __struct_group()"); /* version is always 0 */ #define PKTHDR_RADIOTAP_VERSION 0
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. So, in order to avoid ending up with a flexible-array member in the middle of multiple other structs, we use the `__struct_group()` helper to create a new tagged `struct ieee80211_radiotap_header_hdr`. This structure groups together all the members of the flexible `struct ieee80211_radiotap_header` except the flexible array. As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. We then change the type of the middle struct members currently causing trouble from `struct ieee80211_radiotap_header` to `struct ieee80211_radiotap_header_hdr`. We also want to ensure that in case new members need to be added to the flexible structure, they are always included within the newly created tagged struct. For this, we use `static_assert()`. This ensures that the memory layout for both the flexible structure and the new tagged struct is the same after any changes. This approach avoids having to implement `struct ieee80211_radiotap_header_hdr` as a completely separate structure, thus preventing having to maintain two independent but basically identical structures, closing the door to potential bugs in the future. So, with these changes, fix the following warnings: drivers/net/wireless/ath/wil6210/txrx.c:309:50: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/intel/ipw2x00/ipw2100.c:2521:50: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/intel/ipw2x00/ipw2200.h:1146:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/intel/ipw2x00/libipw.h:595:36: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/marvell/libertas/radiotap.h:34:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/marvell/libertas/radiotap.h:5:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/microchip/wilc1000/mon.c:10:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/microchip/wilc1000/mon.c:15:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/virtual/mac80211_hwsim.c:758:42: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/virtual/mac80211_hwsim.c:767:42: 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> --- drivers/net/wireless/ath/wil6210/txrx.c | 2 +- drivers/net/wireless/intel/ipw2x00/ipw2100.c | 2 +- drivers/net/wireless/intel/ipw2x00/ipw2200.h | 2 +- .../net/wireless/marvell/libertas/radiotap.h | 4 +- drivers/net/wireless/microchip/wilc1000/mon.c | 4 +- drivers/net/wireless/virtual/mac80211_hwsim.c | 4 +- include/net/ieee80211_radiotap.h | 43 +++++++++++-------- 7 files changed, 33 insertions(+), 28 deletions(-)