wifi: mac80211: ieee80211_i: Avoid dozens of -Wflex-array-member-not-at-end warnings

PATCH Superseded Gustavo A. R. Silva 3 participants 9 messages Oct 25, 2024 mac80211

lore

Share & tools

lore

Gustavo A. R. Silva

author

2024-10-25 20:10 UTC #0 lore raw

PATCH 1/1 wifi: mac80211: ieee80211_i: Avoid dozens of -Wflex-array-member-not-at-end warnings

-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 <[email protected]>

---
 net/mac80211/ieee80211_i.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

net/mac80211/ieee80211_i.h +3 -2

index e7815ffeaf30..c65adbdf2166 100644
@@ -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 {

Signature · 1 line

2.34.1

Johannes Berg

maintainer

2024-10-25 20:15 UTC #1 lore raw

Quoted 21 lines · Gustavo A. R. Silva

-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 <[email protected]>
---
 net/mac80211/ieee80211_i.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
net/mac80211/ieee80211_i.h +0 -0
index e7815ffeaf30..c65adbdf2166 100644

net/mac80211/ieee80211_i.h struct ieee80211_chanctx { · line 899

Quoted 10 lines

net/mac80211/ieee80211_i.h +3 -2

@@ -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

Gustavo A. R. Silva

2024-10-25 20:23 UTC #2 lore raw

Quoted 38 lines · Johannes Berg

Quoted 21 lines · Gustavo A. R. Silva
-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 <[email protected]>
---
  net/mac80211/ieee80211_i.h | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
net/mac80211/ieee80211_i.h +0 -0
index e7815ffeaf30..c65adbdf2166 100644
net/mac80211/ieee80211_i.h struct ieee80211_chanctx { · line 899
Quoted 10 lines
net/mac80211/ieee80211_i.h +3 -2
@@ -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.

Quoted 3 lines

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.

Quoted 2 lines

please? Or I can do it myself I guess, but ...

Sure thing. I can CC stable as well.

Quoted 2 lines

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

Johannes Berg

maintainer

2024-10-25 20:25 UTC #3 lore raw

Quoted 14 lines · Gustavo A. R. Silva

Quoted 14 lines
net/mac80211/ieee80211_i.h struct ieee80211_chanctx { · line 899
Quoted 8 lines
net/mac80211/ieee80211_i.h +3 -2
-	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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 3 lines

Quoted 1 line
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

Gustavo A. R. Silva

2024-10-25 20:36 UTC #4 lore raw

Quoted 19 lines · Johannes Berg

Quoted 18 lines · Gustavo A. R. Silva
Quoted 17 lines
net/mac80211/ieee80211_i.h struct ieee80211_chanctx { · line 899
Quoted 8 lines
net/mac80211/ieee80211_i.h +3 -2
-	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")

Quoted 7 lines

Quoted 4 lines
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;

struct bar *ptr = (void *)foo->conf->drv_priv;
then write something into *ptr...

In the above cases the code will indeed overwrite radar_detected.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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.

--
Gustavo

Johannes Berg

maintainer

2024-10-25 20:48 UTC #5 lore raw

Quoted 14 lines · Gustavo A. R. Silva

Quoted 8 lines
Quoted 5 lines
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;
...

Quoted 8 lines

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

Gustavo A. R. Silva

2024-10-25 21:10 UTC #6 lore raw

Quoted 19 lines · Johannes Berg

Quoted 18 lines · Gustavo A. R. Silva
Quoted 8 lines
Quoted 5 lines
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

Quoted 8 lines

Quoted 4 lines
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. :)

Quoted 4 lines

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

Johannes Berg

maintainer

2024-10-25 21:16 UTC #7 lore raw

Quoted 24 lines · Gustavo A. R. Silva

Quoted 22 lines · Johannes Berg
Quoted 21 lines · Gustavo A. R. Silva
Quoted 8 lines
Quoted 5 lines
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...

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
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

Gustavo A. R. Silva

2024-10-25 21:38 UTC #8 lore raw

Quoted 87 lines · Johannes Berg

Quoted 24 lines · Gustavo A. R. Silva
Quoted 22 lines · Johannes Berg
Quoted 21 lines · Gustavo A. R. Silva
Quoted 8 lines
Quoted 5 lines
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...
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
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.

Quoted 5 lines

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