-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;
 };
 
 struct mac80211_qos_map {
-- 
2.34.1

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