| Message ID | ZrZB3Rjswe0ZXtug@cute (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [next] wifi: ath11k: Avoid -Wflex-array-member-not-at-end warnings | expand |
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. > }; > > 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>
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(-)