| Message ID | Zxv7KtPEy1kvnTPM@kspp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [next] wifi: mac80211: ieee80211_i: Avoid dozens of -Wflex-array-member-not-at-end warnings | expand |
On Fri, 2024-10-25 at 14:10 -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. > > Move the conflicting declaration to the end of the structure and add > a code comment. Notice that `struct ieee80211_chanctx_conf` is a > flexible structure --a structure that contains a flexible-array member. > > Fix 50 of the following warnings: > > net/mac80211/ieee80211_i.h:895:39: 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> > --- > net/mac80211/ieee80211_i.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h > index e7815ffeaf30..c65adbdf2166 100644 > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -892,9 +892,10 @@ struct ieee80211_chanctx { > /* temporary data for search algorithm etc. */ > struct ieee80211_chan_req req; > > - struct ieee80211_chanctx_conf conf; > - > bool radar_detected; > + > + /* MUST be last - ends in a flexible-array member. */ > + struct ieee80211_chanctx_conf conf; > }; Oi. That's not just a warnings problem, that's actually a pretty stupid bug, this will surely get used and radar_detected will alias stuff that the driver puts there - at least for drivers using chanctx_data_size, which is a couple: ath9k, iwlmvm, mt792x, rwt89 and hwsim. Could you resend with a description that this is a bugfix and Fixes: bca8bc0399ac ("wifi: mac80211: handle ieee80211_radar_detected() for MLO") please? Or I can do it myself I guess, but ... This shouldn't go to next, it should go to 6.12 since that broke it... johannes
On 25/10/24 14:14, Johannes Berg wrote: > On Fri, 2024-10-25 at 14:10 -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. >> >> Move the conflicting declaration to the end of the structure and add >> a code comment. Notice that `struct ieee80211_chanctx_conf` is a >> flexible structure --a structure that contains a flexible-array member. >> >> Fix 50 of the following warnings: >> >> net/mac80211/ieee80211_i.h:895:39: 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> >> --- >> net/mac80211/ieee80211_i.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h >> index e7815ffeaf30..c65adbdf2166 100644 >> --- a/net/mac80211/ieee80211_i.h >> +++ b/net/mac80211/ieee80211_i.h >> @@ -892,9 +892,10 @@ struct ieee80211_chanctx { >> /* temporary data for search algorithm etc. */ >> struct ieee80211_chan_req req; >> >> - struct ieee80211_chanctx_conf conf; >> - >> bool radar_detected; >> + >> + /* MUST be last - ends in a flexible-array member. */ >> + struct ieee80211_chanctx_conf conf; >> }; > > Oi. That's not just a warnings problem, that's actually a pretty stupid > bug, this will surely get used and radar_detected will alias stuff that > the driver puts there - at least for drivers using chanctx_data_size, > which is a couple: ath9k, iwlmvm, mt792x, rwt89 and hwsim. > > Could you resend with a description that this is a bugfix and > > Fixes: bca8bc0399ac ("wifi: mac80211: handle ieee80211_radar_detected() for MLO") Yeah, I was actually going to mention this commit, as it's the one that introduced that `bool radar_detected` to the flex struct. However, it wasn't obvious to me how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. > > please? Or I can do it myself I guess, but ... Sure thing. I can CC stable as well. > > This shouldn't go to next, it should go to 6.12 since that broke it... OK, in that case I just remove the `[next]` part from the subject line. Thanks -- Gustavo
On Fri, 2024-10-25 at 14:22 -0600, Gustavo A. R. Silva wrote: > > > > - struct ieee80211_chanctx_conf conf; > > > - > > > bool radar_detected; > > > + > > > + /* MUST be last - ends in a flexible-array member. */ > > > + struct ieee80211_chanctx_conf conf; > > > }; > > > > Oi. That's not just a warnings problem, that's actually a pretty stupid > > bug, this will surely get used and radar_detected will alias stuff that > > the driver puts there - at least for drivers using chanctx_data_size, > > which is a couple: ath9k, iwlmvm, mt792x, rwt89 and hwsim. > > > > Could you resend with a description that this is a bugfix and > > > > Fixes: bca8bc0399ac ("wifi: mac80211: handle ieee80211_radar_detected() for MLO") > > Yeah, I was actually going to mention this commit, as it's the one that introduced > that `bool radar_detected` to the flex struct. However, it wasn't obvious to me > how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't > see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. You have to look at the drivers, see hwsim_clear_chanctx_magic() for example; I wonder why hwsim_check_chanctx_magic() never caught this. > > please? Or I can do it myself I guess, but ... > > Sure thing. I can CC stable as well. Thanks! No need for stable, it got introduced in 6.12-rc1 only. johannes
On 25/10/24 14:25, Johannes Berg wrote: > On Fri, 2024-10-25 at 14:22 -0600, Gustavo A. R. Silva wrote: >> >>>> - struct ieee80211_chanctx_conf conf; >>>> - >>>> bool radar_detected; >>>> + >>>> + /* MUST be last - ends in a flexible-array member. */ >>>> + struct ieee80211_chanctx_conf conf; >>>> }; >>> >>> Oi. That's not just a warnings problem, that's actually a pretty stupid >>> bug, this will surely get used and radar_detected will alias stuff that >>> the driver puts there - at least for drivers using chanctx_data_size, >>> which is a couple: ath9k, iwlmvm, mt792x, rwt89 and hwsim. >>> >>> Could you resend with a description that this is a bugfix and >>> >>> Fixes: bca8bc0399ac ("wifi: mac80211: handle ieee80211_radar_detected() for MLO") >> >> Yeah, I was actually going to mention this commit, as it's the one that introduced >> that `bool radar_detected` to the flex struct. However, it wasn't obvious to me >> how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't >> see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. > > You have to look at the drivers, see hwsim_clear_chanctx_magic() for > example; I wonder why hwsim_check_chanctx_magic() never caught this. Sorry, I actually meant through `struct ieee80211_chanctx`. Something like: struct ieee80211_chanctx *foo; ... foo->conf.drv_priv[i] = something; or struct bar *ptr = (void *)foo->conf->drv_priv; then write something into *ptr... In the above cases the code will indeed overwrite `radar_detected`. > >>> please? Or I can do it myself I guess, but ... >> >> Sure thing. I can CC stable as well. > > Thanks! > > No need for stable, it got introduced in 6.12-rc1 only. OK -- Gustavo
On Fri, 2024-10-25 at 14:36 -0600, Gustavo A. R. Silva wrote: > > > > > > Yeah, I was actually going to mention this commit, as it's the one that introduced > > > that `bool radar_detected` to the flex struct. However, it wasn't obvious to me > > > how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't > > > see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. > > > > You have to look at the drivers, see hwsim_clear_chanctx_magic() for > > example; I wonder why hwsim_check_chanctx_magic() never caught this. > > Sorry, I actually meant through `struct ieee80211_chanctx`. Something like: > > struct ieee80211_chanctx *foo; > ... > > foo->conf.drv_priv[i] = something; > > or > > struct bar *ptr = (void *)foo->conf->drv_priv; > then write something into *ptr... > > In the above cases the code will indeed overwrite `radar_detected`. Right, that's what it does though, no? Except it doesn't have, in the driver, "foo->conf." because mac80211 only gives it a pointer to conf, so it's only "conf->drv_priv" (and it's the "struct bar" example.) So yeah, pretty sure it will overwrite that, and I do wonder why it wasn't caught. I guess no radar detection tests with MLO yet. johannes
On 25/10/24 14:48, Johannes Berg wrote: > On Fri, 2024-10-25 at 14:36 -0600, Gustavo A. R. Silva wrote: >>>> >>>> Yeah, I was actually going to mention this commit, as it's the one that introduced >>>> that `bool radar_detected` to the flex struct. However, it wasn't obvious to me >>>> how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't >>>> see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. >>> >>> You have to look at the drivers, see hwsim_clear_chanctx_magic() for >>> example; I wonder why hwsim_check_chanctx_magic() never caught this. >> >> Sorry, I actually meant through `struct ieee80211_chanctx`. Something like: >> >> struct ieee80211_chanctx *foo; >> ... >> >> foo->conf.drv_priv[i] = something; >> >> or >> >> struct bar *ptr = (void *)foo->conf->drv_priv; >> then write something into *ptr... >> >> In the above cases the code will indeed overwrite `radar_detected`. > > Right, that's what it does though, no? Except it doesn't have, in the > driver, "foo->conf." because mac80211 only gives it a pointer to conf, > so it's only "conf->drv_priv" (and it's the "struct bar" example.) OK, so do you mean that pointer to `conf` is actually coming from `foo->conf`? This is probably a dumb question but, where is that pointer to `conf` coming from exactly? I'd really like to understand this better so I can determine whether I'm dealing with a bug when analyzing similar instances in the future. :) > > So yeah, pretty sure it will overwrite that, and I do wonder why it > wasn't caught. I guess no radar detection tests with MLO yet. > -- Gustavo
On Fri, 2024-10-25 at 15:10 -0600, Gustavo A. R. Silva wrote: > > On 25/10/24 14:48, Johannes Berg wrote: > > On Fri, 2024-10-25 at 14:36 -0600, Gustavo A. R. Silva wrote: > > > > > > > > > > Yeah, I was actually going to mention this commit, as it's the one that introduced > > > > > that `bool radar_detected` to the flex struct. However, it wasn't obvious to me > > > > > how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't > > > > > see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. > > > > > > > > You have to look at the drivers, see hwsim_clear_chanctx_magic() for > > > > example; I wonder why hwsim_check_chanctx_magic() never caught this. > > > > > > Sorry, I actually meant through `struct ieee80211_chanctx`. Something like: > > > > > > struct ieee80211_chanctx *foo; > > > ... > > > > > > foo->conf.drv_priv[i] = something; > > > > > > or > > > > > > struct bar *ptr = (void *)foo->conf->drv_priv; > > > then write something into *ptr... > > > > > > In the above cases the code will indeed overwrite `radar_detected`. > > > > Right, that's what it does though, no? Except it doesn't have, in the > > driver, "foo->conf." because mac80211 only gives it a pointer to conf, > > so it's only "conf->drv_priv" (and it's the "struct bar" example.) > > OK, so do you mean that pointer to `conf` is actually coming from > `foo->conf`? Well depends what code you're looking at? I guess we should get more concrete now. Let's say hwsim: struct hwsim_chanctx_priv { u32 magic; }; ... static inline void hwsim_set_chanctx_magic(struct ieee80211_chanctx_conf *c) { struct hwsim_chanctx_priv *cp = (void *)c->drv_priv; cp->magic = HWSIM_CHANCTX_MAGIC; } probably shouldn't be marked 'inline' now that I look at it :-) This is being called in hwsim itself, of course: static int mac80211_hwsim_add_chanctx(struct ieee80211_hw *hw, struct ieee80211_chanctx_conf *ctx) { hwsim_set_chanctx_magic(ctx); ... which is only referenced as a function pointer in the ops: static const struct ieee80211_ops mac80211_hwsim_mchan_ops = { ... .add_chanctx = mac80211_hwsim_add_chanctx, (via some macros) And that's called by mac80211: static inline int drv_add_chanctx(struct ieee80211_local *local, struct ieee80211_chanctx *ctx) { int ret = -EOPNOTSUPP; might_sleep(); lockdep_assert_wiphy(local->hw.wiphy); trace_drv_add_chanctx(local, ctx); if (local->ops->add_chanctx) ret = local->ops->add_chanctx(&local->hw, &ctx->conf); so you see that the struct ieee80211_chanctx is never passed to the driver, but instead &ctx->conf, which is the struct with the flex array for driver priv. So in this example, struct hwsim_chanctx_priv::magic overlaps the radar_detected value. (The allocation happens via chanctx_data_size.) johannes
On 25/10/24 15:16, Johannes Berg wrote: > On Fri, 2024-10-25 at 15:10 -0600, Gustavo A. R. Silva wrote: >> >> On 25/10/24 14:48, Johannes Berg wrote: >>> On Fri, 2024-10-25 at 14:36 -0600, Gustavo A. R. Silva wrote: >>>>>> >>>>>> Yeah, I was actually going to mention this commit, as it's the one that introduced >>>>>> that `bool radar_detected` to the flex struct. However, it wasn't obvious to me >>>>>> how `struct ieee80211_chanctx_conf conf` could overwrite `radar_detected` as I didn't >>>>>> see `conf->drv_priv` being accessed through `struct struct ieee80211_chanctx_conf`. >>>>> >>>>> You have to look at the drivers, see hwsim_clear_chanctx_magic() for >>>>> example; I wonder why hwsim_check_chanctx_magic() never caught this. >>>> >>>> Sorry, I actually meant through `struct ieee80211_chanctx`. Something like: >>>> >>>> struct ieee80211_chanctx *foo; >>>> ... >>>> >>>> foo->conf.drv_priv[i] = something; >>>> >>>> or >>>> >>>> struct bar *ptr = (void *)foo->conf->drv_priv; >>>> then write something into *ptr... >>>> >>>> In the above cases the code will indeed overwrite `radar_detected`. >>> >>> Right, that's what it does though, no? Except it doesn't have, in the >>> driver, "foo->conf." because mac80211 only gives it a pointer to conf, >>> so it's only "conf->drv_priv" (and it's the "struct bar" example.) >> >> OK, so do you mean that pointer to `conf` is actually coming from >> `foo->conf`? > > Well depends what code you're looking at? I guess we should get more > concrete now. Let's say hwsim: > > struct hwsim_chanctx_priv { > u32 magic; > }; > > ... > > static inline void hwsim_set_chanctx_magic(struct ieee80211_chanctx_conf *c) > { > struct hwsim_chanctx_priv *cp = (void *)c->drv_priv; > cp->magic = HWSIM_CHANCTX_MAGIC; > } > > probably shouldn't be marked 'inline' now that I look at it :-) > > This is being called in hwsim itself, of course: > > static int mac80211_hwsim_add_chanctx(struct ieee80211_hw *hw, > struct ieee80211_chanctx_conf *ctx) > { > hwsim_set_chanctx_magic(ctx); > ... > > which is only referenced as a function pointer in the ops: > > static const struct ieee80211_ops mac80211_hwsim_mchan_ops = { > ... > .add_chanctx = mac80211_hwsim_add_chanctx, > > (via some macros) > > > And that's called by mac80211: > > static inline int drv_add_chanctx(struct ieee80211_local *local, > struct ieee80211_chanctx *ctx) > { > int ret = -EOPNOTSUPP; > > might_sleep(); > lockdep_assert_wiphy(local->hw.wiphy); > > trace_drv_add_chanctx(local, ctx); > if (local->ops->add_chanctx) > ret = local->ops->add_chanctx(&local->hw, &ctx->conf); > > > so you see that the struct ieee80211_chanctx is never passed to the > driver, but instead &ctx->conf, which is the struct with the flex array > for driver priv. > > So in this example, struct hwsim_chanctx_priv::magic overlaps the > radar_detected value. > > > (The allocation happens via chanctx_data_size.) Ah, I see now. Thanks so much for this! It really obscures the whole thing when pointers to flex structs are passed to functions, and then the flex-array member is finally accessed after a few more calls. This is precisely why -Wfamnae is needed, because it's not that obvious for people to immediately notice when they are introducing this kinds of bugs. OK, I'll send v2, shortly. Thanks! -- Gustavo
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index e7815ffeaf30..c65adbdf2166 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -892,9 +892,10 @@ struct ieee80211_chanctx { /* temporary data for search algorithm etc. */ struct ieee80211_chan_req req; - struct ieee80211_chanctx_conf conf; - bool radar_detected; + + /* MUST be last - ends in a flexible-array member. */ + struct ieee80211_chanctx_conf conf; }; struct mac80211_qos_map {
-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 and add a code comment. Notice that `struct ieee80211_chanctx_conf` is a flexible structure --a structure that contains a flexible-array member. Fix 50 of the following warnings: net/mac80211/ieee80211_i.h:895:39: 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> --- net/mac80211/ieee80211_i.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)