| Message ID | ZrZB3Rjswe0ZXtug@cute |
|---|---|
| State | New |
| Headers | show |
| Series | [next] wifi: ath11k: Avoid -Wflex-array-member-not-at-end warnings | expand |
On 09/08/24 20:21, Jeff Johnson wrote: > On 8/9/2024 9:20 AM, 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 declaration to the end of the structure. Notice >> that `struct ieee80211_chanctx_conf` is a flexible structure --a >> structure that contains a flexible-array member. >> >> Also, remove a couple of unused structures. >> >> Fix the following warnings: >> drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/net/wireless/ath/ath11k/dp.h:1368:24: 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/ath11k/core.h | 4 +++- >> drivers/net/wireless/ath/ath11k/dp.h | 23 ----------------------- >> 2 files changed, 3 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h >> index df24f0e409af..e283415dccf3 100644 >> --- a/drivers/net/wireless/ath/ath11k/core.h >> +++ b/drivers/net/wireless/ath/ath11k/core.h >> @@ -406,11 +406,13 @@ struct ath11k_vif { >> bool wpaie_present; >> bool bcca_zero_sent; >> bool do_not_send_tmpl; >> - struct ieee80211_chanctx_conf chanctx; >> struct ath11k_arp_ns_offload arp_ns_offload; >> struct ath11k_rekey_data rekey_data; >> >> struct ath11k_reg_tpc_power_info reg_tpc_info; >> + >> + /* Must be last - ends in a flexible-array member. */ >> + struct ieee80211_chanctx_conf chanctx; > > there is something illogical about this since the vif is allocated using > sizeof() and hence there will never be memory allocated for the flexible > array, and it is assigned using either struct assignment or memcpy using the > struct size which (fortunately) would not transfer the flexible array contents: > arvif->chanctx = *ctx; > > memcpy(&arvif->chanctx, ctx, sizeof(*ctx)); Yes; this is why I didn't include a 'Fixes' tag. In any case, middle-flex-arrays are deprecated and should either be removed/refactored[1][2] or placed at the very bottom in any nested structures. :) > > since ath11k doesn't actually use the drv_priv[] I guess this change is OK, it > is just strange to me. > > also makes me wonder why ath11k keeps a copy of the chanctx instead of just > getting it from the underlying ieee80211_link_data. but that is outside the > scope of this discussion. > >> }; >> >> struct ath11k_vif_iter { >> diff --git a/drivers/net/wireless/ath/ath11k/dp.h b/drivers/net/wireless/ath/ath11k/dp.h >> index 2f6dd69d3be2..65d2bc0687c8 100644 >> --- a/drivers/net/wireless/ath/ath11k/dp.h >> +++ b/drivers/net/wireless/ath/ath11k/dp.h >> @@ -1305,18 +1305,6 @@ struct htt_ppdu_stats_user_rate { >> #define HTT_TX_INFO_PEERID(_flags) \ >> FIELD_GET(HTT_PPDU_STATS_TX_INFO_FLAGS_PEERID_M, _flags) >> >> -struct htt_tx_ppdu_stats_info { >> - struct htt_tlv tlv_hdr; >> - u32 tx_success_bytes; >> - u32 tx_retry_bytes; >> - u32 tx_failed_bytes; >> - u32 flags; /* %HTT_PPDU_STATS_TX_INFO_FLAGS_ */ >> - u16 tx_success_msdus; >> - u16 tx_retry_msdus; >> - u16 tx_failed_msdus; >> - u16 tx_duration; /* united in us */ >> -} __packed; >> - >> enum htt_ppdu_stats_usr_compln_status { >> HTT_PPDU_STATS_USER_STATUS_OK, >> HTT_PPDU_STATS_USER_STATUS_FILTERED, >> @@ -1364,17 +1352,6 @@ struct htt_ppdu_stats_usr_cmpltn_ack_ba_status { >> u32 success_bytes; >> } __packed; >> >> -struct htt_ppdu_stats_usr_cmn_array { >> - struct htt_tlv tlv_hdr; >> - u32 num_ppdu_stats; >> - /* tx_ppdu_stats_info is filled by multiple struct htt_tx_ppdu_stats_info >> - * elements. >> - * tx_ppdu_stats_info is variable length, with length = >> - * number_of_ppdu_stats * sizeof (struct htt_tx_ppdu_stats_info) >> - */ >> - struct htt_tx_ppdu_stats_info tx_ppdu_info[]; >> -} __packed; >> - >> struct htt_ppdu_user_stats { >> u16 peer_id; >> u32 tlv_flags; > > the second part if definitely ok. > > Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> Thanks -- Gustavo [1] https://lore.kernel.org/linux-hardening/ZrOQa2gew5yadyt3@cute/ [2] https://lore.kernel.org/linux-hardening/ZrJqtUpCI+uCeb4D@cute/
Jeff Johnson <quic_jjohnson@quicinc.com> writes: > On 8/9/2024 9:20 AM, 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 declaration to the end of the structure. Notice >> that `struct ieee80211_chanctx_conf` is a flexible structure --a >> structure that contains a flexible-array member. >> >> Also, remove a couple of unused structures. >> >> Fix the following warnings: >> drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] >> drivers/net/wireless/ath/ath11k/dp.h:1368:24: 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/ath11k/core.h | 4 +++- >> drivers/net/wireless/ath/ath11k/dp.h | 23 ----------------------- >> 2 files changed, 3 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h >> index df24f0e409af..e283415dccf3 100644 >> --- a/drivers/net/wireless/ath/ath11k/core.h >> +++ b/drivers/net/wireless/ath/ath11k/core.h >> @@ -406,11 +406,13 @@ struct ath11k_vif { >> bool wpaie_present; >> bool bcca_zero_sent; >> bool do_not_send_tmpl; >> - struct ieee80211_chanctx_conf chanctx; >> struct ath11k_arp_ns_offload arp_ns_offload; >> struct ath11k_rekey_data rekey_data; >> >> struct ath11k_reg_tpc_power_info reg_tpc_info; >> + >> + /* Must be last - ends in a flexible-array member. */ >> + struct ieee80211_chanctx_conf chanctx; > > there is something illogical about this since the vif is allocated using > sizeof() and hence there will never be memory allocated for the flexible > array, and it is assigned using either struct assignment or memcpy using the > struct size which (fortunately) would not transfer the flexible array contents: > arvif->chanctx = *ctx; > > memcpy(&arvif->chanctx, ctx, sizeof(*ctx)); > > since ath11k doesn't actually use the drv_priv[] I guess this change is OK, it > is just strange to me. > > also makes me wonder why ath11k keeps a copy of the chanctx instead of just > getting it from the underlying ieee80211_link_data. but that is outside the > scope of this discussion. Yeah, this doesn't look right. I don't think a driver should be copying struct ieee80211_chanctx_conf like that. I think I'll add a comment about this to the code: /* FIXME: Driver should not copy struct ieee80211_chanctx_conf, * especially because it has a flexible array. Find a better way. */ Thoughts?
"Gustavo A. R. Silva" <gustavoars@kernel.org> wrote: > -Wflex-array-member-not-at-end was introduced in GCC-14, and we are > getting ready to enable it, globally. > > Move the conflicting declaration to the end of the structure. Notice > that `struct ieee80211_chanctx_conf` is a flexible structure --a > structure that contains a flexible-array member. > > Also, remove a couple of unused structures. > > Fix the following warnings: > drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/net/wireless/ath/ath11k/dp.h:1368:24: 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> > Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com> > Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com> Patch applied to ath-next branch of ath.git, thanks. 820aa897837f wifi: ath11k: Avoid -Wflex-array-member-not-at-end warnings
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index df24f0e409af..e283415dccf3 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -406,11 +406,13 @@ struct ath11k_vif { bool wpaie_present; bool bcca_zero_sent; bool do_not_send_tmpl; - struct ieee80211_chanctx_conf chanctx; struct ath11k_arp_ns_offload arp_ns_offload; struct ath11k_rekey_data rekey_data; struct ath11k_reg_tpc_power_info reg_tpc_info; + + /* Must be last - ends in a flexible-array member. */ + struct ieee80211_chanctx_conf chanctx; }; struct ath11k_vif_iter { diff --git a/drivers/net/wireless/ath/ath11k/dp.h b/drivers/net/wireless/ath/ath11k/dp.h index 2f6dd69d3be2..65d2bc0687c8 100644 --- a/drivers/net/wireless/ath/ath11k/dp.h +++ b/drivers/net/wireless/ath/ath11k/dp.h @@ -1305,18 +1305,6 @@ struct htt_ppdu_stats_user_rate { #define HTT_TX_INFO_PEERID(_flags) \ FIELD_GET(HTT_PPDU_STATS_TX_INFO_FLAGS_PEERID_M, _flags) -struct htt_tx_ppdu_stats_info { - struct htt_tlv tlv_hdr; - u32 tx_success_bytes; - u32 tx_retry_bytes; - u32 tx_failed_bytes; - u32 flags; /* %HTT_PPDU_STATS_TX_INFO_FLAGS_ */ - u16 tx_success_msdus; - u16 tx_retry_msdus; - u16 tx_failed_msdus; - u16 tx_duration; /* united in us */ -} __packed; - enum htt_ppdu_stats_usr_compln_status { HTT_PPDU_STATS_USER_STATUS_OK, HTT_PPDU_STATS_USER_STATUS_FILTERED, @@ -1364,17 +1352,6 @@ struct htt_ppdu_stats_usr_cmpltn_ack_ba_status { u32 success_bytes; } __packed; -struct htt_ppdu_stats_usr_cmn_array { - struct htt_tlv tlv_hdr; - u32 num_ppdu_stats; - /* tx_ppdu_stats_info is filled by multiple struct htt_tx_ppdu_stats_info - * elements. - * tx_ppdu_stats_info is variable length, with length = - * number_of_ppdu_stats * sizeof (struct htt_tx_ppdu_stats_info) - */ - struct htt_tx_ppdu_stats_info tx_ppdu_info[]; -} __packed; - struct htt_ppdu_user_stats { u16 peer_id; u32 tlv_flags;
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are getting ready to enable it, globally. Move the conflicting declaration to the end of the structure. Notice that `struct ieee80211_chanctx_conf` is a flexible structure --a structure that contains a flexible-array member. Also, remove a couple of unused structures. Fix the following warnings: drivers/net/wireless/ath/ath11k/core.h:409:39: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/ath/ath11k/dp.h:1309:24: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/net/wireless/ath/ath11k/dp.h:1368:24: 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/ath11k/core.h | 4 +++- drivers/net/wireless/ath/ath11k/dp.h | 23 ----------------------- 2 files changed, 3 insertions(+), 24 deletions(-)