2021-08-20 12:22:50

by Wen Gong

[permalink] [raw]
Subject: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

This is to save the transmit power envelope element and power
constraint in struct ieee80211_bss_conf for 6 GHz. Lower driver
will use this info to calculate the power limit.

Signed-off-by: Wen Gong <[email protected]>
---
include/net/mac80211.h | 6 ++++++
net/mac80211/chan.c | 9 +++++++++
net/mac80211/mlme.c | 26 ++++++++++++++++++++++++++
3 files changed, 41 insertions(+)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index e89530d0d9c6..6e11e122e364 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -631,6 +631,9 @@ struct ieee80211_fils_discovery {
* @s1g: BSS is S1G BSS (affects Association Request format).
* @beacon_tx_rate: The configured beacon transmit rate that needs to be passed
* to driver when rate control is offloaded to firmware.
+ * @tx_pwr_env: transmit power envelope array of BSS.
+ * @tx_pwr_env_num: number of @tx_pwr_env.
+ * @pwr_reduction: power constraint of BSS.
*/
struct ieee80211_bss_conf {
const u8 *bssid;
@@ -700,6 +703,9 @@ struct ieee80211_bss_conf {
u32 unsol_bcast_probe_resp_interval;
bool s1g;
struct cfg80211_bitrate_mask beacon_tx_rate;
+ struct ieee80211_tx_pwr_env tx_pwr_env[IEEE80211_TPE_MAX_IE_COUNT];
+ u8 tx_pwr_env_num;
+ u8 pwr_reduction;
};

/**
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 907bb1f748a1..149d4ac798f6 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -721,6 +721,15 @@ static int ieee80211_assign_vif_chanctx(struct ieee80211_sub_if_data *sdata,
lockdep_is_held(&local->chanctx_mtx));

if (conf) {
+ if (conf->def.chan->band == NL80211_BAND_6GHZ) {
+ struct ieee80211_bss_conf *bss_conf;
+
+ bss_conf = &sdata->vif.bss_conf;
+ bss_conf->pwr_reduction = 0;
+ bss_conf->tx_pwr_env_num = 0;
+ memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env));
+ }
+
curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);

drv_unassign_vif_chanctx(local, sdata, curr_ctx);
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 2480bd0577bb..a6d66b4ad7ee 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -5070,6 +5070,7 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,

if (!(ifmgd->flags & IEEE80211_STA_DISABLE_HE)) {
const struct cfg80211_bss_ies *ies;
+ struct ieee80211_bss_conf *bss_conf;
const u8 *he_oper_ie;

ies = rcu_dereference(cbss->ies);
@@ -5081,6 +5082,31 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
else
he_oper = NULL;

+ if (is_6ghz) {
+ struct ieee802_11_elems elems;
+ u8 i, j = 0;
+
+ ieee802_11_parse_elems(ies->data, ies->len, false, &elems,
+ NULL, NULL);
+
+ if (elems.pwr_constr_elem)
+ bss_conf->pwr_reduction = *elems.pwr_constr_elem;
+
+ BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
+ ARRAY_SIZE(elems.tx_pwr_env));
+
+ for (i = 0; i < elems.tx_pwr_env_num; i++) {
+ if (elems.tx_pwr_env_len[i] >
+ sizeof(bss_conf->tx_pwr_env[j]))
+ continue;
+
+ bss_conf->tx_pwr_env_num++;
+ memcpy(&bss_conf->tx_pwr_env[j], elems.tx_pwr_env[i],
+ elems.tx_pwr_env_len[i]);
+ j++;
+ }
+ }
+
if (!ieee80211_verify_sta_he_mcs_support(sband, he_oper))
ifmgd->flags |= IEEE80211_STA_DISABLE_HE;
}
--
2.31.1


2021-08-26 08:32:03

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-20 at 08:20 -0400, Wen Gong wrote:
>
>   if (!(ifmgd->flags & IEEE80211_STA_DISABLE_HE)) {
>   const struct cfg80211_bss_ies *ies;
> + struct ieee80211_bss_conf *bss_conf;

am I missing where you set this?

> + if (is_6ghz) {
> + struct ieee802_11_elems elems;

This is pretty big, not sure we want it on the stack (causes warnings
for me in build). Also, if we're doing this anyway, then we can change
the code above (perhaps as a separate patch) to not do
cfg80211_find_ext_ie() but rather take it out of the parsed.

> + u8 i, j = 0;
> +
> + ieee802_11_parse_elems(ies->data, ies->len, false, &elems,

(line too long)

> + NULL, NULL);
> +
> + if (elems.pwr_constr_elem)
> + bss_conf->pwr_reduction = *elems.pwr_constr_elem;

before using it?

> +
> + BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
> + ARRAY_SIZE(elems.tx_pwr_env));
> +
> + for (i = 0; i < elems.tx_pwr_env_num; i++) {
> + if (elems.tx_pwr_env_len[i] >
> + sizeof(bss_conf->tx_pwr_env[j]))
> + continue;

I did that in the parsing itself.

> +
> + bss_conf->tx_pwr_env_num++;
> + memcpy(&bss_conf->tx_pwr_env[j], elems.tx_pwr_env[i],
> + elems.tx_pwr_env_len[i]);

You're never resetting any of this for the next connection (if it's not
6 GHz for example, or doesn't have any data) - should probably memset
all the new members to 0 before the "if (is_6ghz)" or so?

johannes

2021-08-26 10:51:51

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-26 16:29, Johannes Berg wrote:
> On Fri, 2021-08-20 at 08:20 -0400, Wen Gong wrote:
>>
>>   if (!(ifmgd->flags & IEEE80211_STA_DISABLE_HE)) {
>>   const struct cfg80211_bss_ies *ies;
>> + struct ieee80211_bss_conf *bss_conf;
>
> am I missing where you set this?
sorry, i lost it in v2 patch.
bss_conf = &sdata->vif.bss_conf;
>
>> + if (is_6ghz) {
>> + struct ieee802_11_elems elems;
>
> This is pretty big, not sure we want it on the stack (causes warnings
> for me in build). Also, if we're doing this anyway, then we can change
> the code above (perhaps as a separate patch) to not do
> cfg80211_find_ext_ie() but rather take it out of the parsed.
will change it.
>
>> + u8 i, j = 0;
>> +
>> + ieee802_11_parse_elems(ies->data, ies->len, false, &elems,
>
> (line too long)
>
>> + NULL, NULL);
>> +
>> + if (elems.pwr_constr_elem)
>> + bss_conf->pwr_reduction = *elems.pwr_constr_elem;
>
> before using it?
>
>> +
>> + BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) !=
>> + ARRAY_SIZE(elems.tx_pwr_env));
>> +
>> + for (i = 0; i < elems.tx_pwr_env_num; i++) {
>> + if (elems.tx_pwr_env_len[i] >
>> + sizeof(bss_conf->tx_pwr_env[j]))
>> + continue;
>
> I did that in the parsing itself.
>
>> +
>> + bss_conf->tx_pwr_env_num++;
>> + memcpy(&bss_conf->tx_pwr_env[j], elems.tx_pwr_env[i],
>> + elems.tx_pwr_env_len[i]);
>
> You're never resetting any of this for the next connection (if it's not
> 6 GHz for example, or doesn't have any data) - should probably memset
> all the new members to 0 before the "if (is_6ghz)" or so?
>
it is memset here i this patch:
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -721,6 +721,15 @@ static int ieee80211_assign_vif_chanctx(struct
ieee80211_sub_if_data *sdata,

lockdep_is_held(&local->chanctx_mtx));

if (conf) {
+ if (conf->def.chan->band == NL80211_BAND_6GHZ) {
+ struct ieee80211_bss_conf *bss_conf;
+
+ bss_conf = &sdata->vif.bss_conf;
+ bss_conf->pwr_reduction = 0;
+ bss_conf->tx_pwr_env_num = 0;
+ memset(bss_conf->tx_pwr_env, 0,
sizeof(bss_conf->tx_pwr_env));
+ }
+
curr_ctx = container_of(conf, struct ieee80211_chanctx,
conf);

drv_unassign_vif_chanctx(local, sdata, curr_ctx);

> johannes

2021-08-26 10:59:36

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Thu, 2021-08-26 at 18:50 +0800, Wen Gong wrote:
> >
> it is memset here i this patch:

Oops, missed that.

But is that really a good place for it? Doesn't really seem to belong to
assigning a channel context - maybe put it into set_disassoc()?

johannes



2021-08-26 11:06:32

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-26 18:57, Johannes Berg wrote:
> On Thu, 2021-08-26 at 18:50 +0800, Wen Gong wrote:
>> >
>> it is memset here i this patch:
>
> Oops, missed that.
>
> But is that really a good place for it? Doesn't really seem to belong
> to
> assigning a channel context - maybe put it into set_disassoc()?
>
it is correct.
you can see it is place together with "drv_unassign_vif_chanctx(local,
sdata, curr_ctx)"
in ieee80211_assign_vif_chanctx(), it is for disconnect.
> johannes

2021-08-26 11:10:59

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Thu, 2021-08-26 at 19:00 +0800, Wen Gong wrote:
> On 2021-08-26 18:57, Johannes Berg wrote:
> > On Thu, 2021-08-26 at 18:50 +0800, Wen Gong wrote:
> > > >
> > > it is memset here i this patch:
> >
> > Oops, missed that.
> >
> > But is that really a good place for it? Doesn't really seem to belong
> > to
> > assigning a channel context - maybe put it into set_disassoc()?
> >
> it is correct.
> you can see it is place together with "drv_unassign_vif_chanctx(local,
> sdata, curr_ctx)"
> in ieee80211_assign_vif_chanctx(), it is for disconnect.

Yes, I know it's *correct*, but that doesn't mean it's *good*?

Look at that code - it does nothing with bss_conf. Nobody is ever going
to look there.

johannes
>

2021-08-27 02:06:24

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-26 19:10, Johannes Berg wrote:
> On Thu, 2021-08-26 at 19:00 +0800, Wen Gong wrote:
>> On 2021-08-26 18:57, Johannes Berg wrote:
>> > On Thu, 2021-08-26 at 18:50 +0800, Wen Gong wrote:
>> > > >
>> > > it is memset here i this patch:
>> >
>> > Oops, missed that.
>> >
>> > But is that really a good place for it? Doesn't really seem to belong
>> > to
>> > assigning a channel context - maybe put it into set_disassoc()?
>> >
>> it is correct.
>> you can see it is place together with "drv_unassign_vif_chanctx(local,
>> sdata, curr_ctx)"
>> in ieee80211_assign_vif_chanctx(), it is for disconnect.
>
> Yes, I know it's *correct*, but that doesn't mean it's *good*?
>
> Look at that code - it does nothing with bss_conf. Nobody is ever going
> to look there.
>
Yes,
seems it is better do memset() in ieee80211_set_disassoc().
I will change it in next verion.
> johannes
>>

2021-08-27 02:12:18

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-26 16:29, Johannes Berg wrote:
...
>
>> + if (is_6ghz) {
>> + struct ieee802_11_elems elems;
>
> This is pretty big, not sure we want it on the stack (causes warnings
> for me in build). Also, if we're doing this anyway, then we can change
> the code above (perhaps as a separate patch) to not do
> cfg80211_find_ext_ie() but rather take it out of the parsed.
do you mean NOT use cfg80211_find_ext_ie()/cfg80211_find_ie() and still
use "struct ieee802_11_elems elems" here and
move this code to a separate function/patch?
it has more than one tx_pwr_env in one beacon, if we use
cfg80211_find_ext_ie()/cfg80211_find_ie(),
it need add more logic.
>
...
>
> johannes

2021-08-27 06:46:39

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-27 at 10:11 +0800, Wen Gong wrote:
> > Also, if we're doing this anyway, then we can change
> > the code above (perhaps as a separate patch) to not do
> > cfg80211_find_ext_ie() but rather take it out of the parsed.
> do you mean NOT use cfg80211_find_ext_ie()/cfg80211_find_ie() and still
> use "struct ieee802_11_elems elems" here and
> move this code to a separate function/patch?

Well, there's an existing place in this function that uses
cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
it feels like if we're going to do the full parsing, we should switch
all the existing "look up an element" to also use the parsed data
instead.

Not the other way around :)

johannes

2021-08-27 06:55:42

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-27 at 14:53 +0800, Wen Gong wrote:
> >
> > Well, there's an existing place in this function that uses
> > cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
> > it feels like if we're going to do the full parsing, we should switch
> > all the existing "look up an element" to also use the parsed data
> > instead.

> ok.
> so it ha 2 way to change, right?
> 1.
> change ieee802_11_parse_elems() to ieee80211_bss_get_ie()

No why?

I think we should make a first patch (that doesn't add TPE yet) that
changes the function to ieee80211_parse_elems() and removes all the
ieee80211_bss_get_ie() / cfg80211_find_ext_ie() calls in favour of just
parsing once, and then looking at the elements there.

Then your TPE patch becomes trivial since the elems are already there?

>
> 2.
> still use ieee802_11_parse_elems(), and change others
> ieee80211_bss_get_ie()/cfg80211_find_ext_ie()
> to use the result of ieee802_11_parse_elems()
>

Right!

johannes

2021-08-27 07:13:30

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-27 14:55, Johannes Berg wrote:
> On Fri, 2021-08-27 at 14:53 +0800, Wen Gong wrote:
>> >
>> > Well, there's an existing place in this function that uses
>> > cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
>> > it feels like if we're going to do the full parsing, we should switch
>> > all the existing "look up an element" to also use the parsed data
>> > instead.
>
>> ok.
>> so it ha 2 way to change, right?
>> 1.
>> change ieee802_11_parse_elems() to ieee80211_bss_get_ie()
>
> No why?
>
> I think we should make a first patch (that doesn't add TPE yet) that
> changes the function to ieee80211_parse_elems() and removes all the
> ieee80211_bss_get_ie() / cfg80211_find_ext_ie() calls in favour of just
> parsing once, and then looking at the elements there.
>
> Then your TPE patch becomes trivial since the elems are already there?
this patch still needed, because the lower driver need the info.
and this patch is save the info to "struct ieee80211_bss_conf *bss_conf"
and
pass it to lower driver.

>
>>
>> 2.
>> still use ieee802_11_parse_elems(), and change others
>> ieee80211_bss_get_ie()/cfg80211_find_ext_ie()
>> to use the result of ieee802_11_parse_elems()
>>
>
> Right!
>
> johannes

2021-08-27 07:41:41

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-27 at 15:12 +0800, Wen Gong wrote:
> On 2021-08-27 14:55, Johannes Berg wrote:
> > On Fri, 2021-08-27 at 14:53 +0800, Wen Gong wrote:
> > > >
> > > > Well, there's an existing place in this function that uses
> > > > cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
> > > > it feels like if we're going to do the full parsing, we should switch
> > > > all the existing "look up an element" to also use the parsed data
> > > > instead.
> >
> > > ok.
> > > so it ha 2 way to change, right?
> > > 1.
> > > change ieee802_11_parse_elems() to ieee80211_bss_get_ie()
> >
> > No why?
> >
> > I think we should make a first patch (that doesn't add TPE yet) that
> > changes the function to ieee80211_parse_elems() and removes all the
> > ieee80211_bss_get_ie() / cfg80211_find_ext_ie() calls in favour of just
> > parsing once, and then looking at the elements there.
> >
> > Then your TPE patch becomes trivial since the elems are already there?
> this patch still needed, because the lower driver need the info.
> and this patch is save the info to "struct ieee80211_bss_conf *bss_conf"
> and
> pass it to lower driver.

Of course, but you don't have to deal with parsing etc. in that patch
then.

johannes

2021-08-27 07:47:10

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-27 14:46, Johannes Berg wrote:
> On Fri, 2021-08-27 at 10:11 +0800, Wen Gong wrote:
>> > Also, if we're doing this anyway, then we can change
>> > the code above (perhaps as a separate patch) to not do
>> > cfg80211_find_ext_ie() but rather take it out of the parsed.
>> do you mean NOT use cfg80211_find_ext_ie()/cfg80211_find_ie() and
>> still
>> use "struct ieee802_11_elems elems" here and
>> move this code to a separate function/patch?
>
> Well, there's an existing place in this function that uses
> cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
> it feels like if we're going to do the full parsing, we should switch
> all the existing "look up an element" to also use the parsed data
> instead.
>
> Not the other way around :)
ok.
so it ha 2 way to change, right?
1.
change ieee802_11_parse_elems() to ieee80211_bss_get_ie()

2.
still use ieee802_11_parse_elems(), and change others
ieee80211_bss_get_ie()/cfg80211_find_ext_ie()
to use the result of ieee802_11_parse_elems()
>
> johannes

2021-08-27 08:21:13

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-27 at 16:18 +0800, Wen Gong wrote:
>
> then should I use "struct ieee802_11_elems elems = {0}" or "struct
> ieee802_11_elems *elems = kzalloc(sizeof(*elems))"
> in the parsing patch?

Yeah, it's a good question ...

We keep adding stuff here, so it'll be safer to alloc it.

I get a fair number of stack size warnings on the build now (possibly
due to the addition of the TPE fields?), and while they're probably fine
for now (we get there from nl80211, so no deep stack), it's only going
to increase - we have EHT patches already now, for example.

johannes

2021-08-27 08:21:57

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-27 15:38, Johannes Berg wrote:
> On Fri, 2021-08-27 at 15:12 +0800, Wen Gong wrote:
>> On 2021-08-27 14:55, Johannes Berg wrote:
>> > On Fri, 2021-08-27 at 14:53 +0800, Wen Gong wrote:
>> > > >
>> > > > Well, there's an existing place in this function that uses
>> > > > cfg80211_find_ext_ie(), and various uses of ieee80211_bss_get_ie(), so
>> > > > it feels like if we're going to do the full parsing, we should switch
>> > > > all the existing "look up an element" to also use the parsed data
>> > > > instead.
>> >
>> > > ok.
>> > > so it ha 2 way to change, right?
>> > > 1.
>> > > change ieee802_11_parse_elems() to ieee80211_bss_get_ie()
>> >
>> > No why?
>> >
>> > I think we should make a first patch (that doesn't add TPE yet) that
>> > changes the function to ieee80211_parse_elems() and removes all the
>> > ieee80211_bss_get_ie() / cfg80211_find_ext_ie() calls in favour of just
>> > parsing once, and then looking at the elements there.
>> >
>> > Then your TPE patch becomes trivial since the elems are already there?
>> this patch still needed, because the lower driver need the info.
>> and this patch is save the info to "struct ieee80211_bss_conf
>> *bss_conf"
>> and
>> pass it to lower driver.
>
> Of course, but you don't have to deal with parsing etc. in that patch
> then.
>
yes.
then should I use "struct ieee802_11_elems elems = {0}" or "struct
ieee802_11_elems *elems = kzalloc(sizeof(*elems))"
in the parsing patch?

> johannes

2021-08-27 08:30:12

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-27 16:20, Johannes Berg wrote:
> On Fri, 2021-08-27 at 16:18 +0800, Wen Gong wrote:
>>
>> then should I use "struct ieee802_11_elems elems = {0}" or "struct
>> ieee802_11_elems *elems = kzalloc(sizeof(*elems))"
>> in the parsing patch?
>
> Yeah, it's a good question ...
>
> We keep adding stuff here, so it'll be safer to alloc it.
>
> I get a fair number of stack size warnings on the build now (possibly
> due to the addition of the TPE fields?), and while they're probably
> fine
> for now (we get there from nl80211, so no deep stack), it's only going
> to increase - we have EHT patches already now, for example.
>

the TPE is only 8 pointer in the struct ieee802_11_elems.
so stack size warnings should not caused by TPE.

#define IEEE80211_TPE_MAX_IE_COUNT 8
const struct ieee80211_tx_pwr_env
*tx_pwr_env[IEEE80211_TPE_MAX_IE_COUNT];

> johannes

2021-08-27 08:30:52

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On Fri, 2021-08-27 at 16:28 +0800, Wen Gong wrote:
>
> the TPE is only 8 pointer in the struct ieee802_11_elems.


I know, but it's showing _just_ above the threshold for the warning now,
so if it was _just_ below the threshold before, adding 64 bytes could
still do that. 64 bytes is a lot, after all, the threshold is only 512 I
think.

johannes

2021-08-27 08:47:54

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

On 2021-08-27 16:30, Johannes Berg wrote:
> On Fri, 2021-08-27 at 16:28 +0800, Wen Gong wrote:
>>
>> the TPE is only 8 pointer in the struct ieee802_11_elems.
>
>
> I know, but it's showing _just_ above the threshold for the warning
> now,
> so if it was _just_ below the threshold before, adding 64 bytes could
> still do that. 64 bytes is a lot, after all, the threshold is only 512
> I
> think.
>
Yes, I will change to below in next version.
"struct ieee802_11_elems *elems = kzalloc(sizeof(*elems))"
> johannes

2023-07-19 04:22:29

by Wen Gong

[permalink] [raw]
Subject: Re: [PATCH v2 8/8] mac80211: save transmit power envelope element and power constraint

Hi Johannes,
On 8/26/2021 6:57 PM, Johannes Berg wrote:
> On Thu, 2021-08-26 at 18:50 +0800, Wen Gong wrote:
>> it is memset here i this patch:
> Oops, missed that.
>
> But is that really a good place for it? Doesn't really seem to belong to
> assigning a channel context - maybe put it into set_disassoc()?
>
> johannes
We hit buffer overflow issue while connecting to 6 GHz AP fail and fail.
Will you fix it? Or do you have any suggestion to fix it?

[  227.539928] wlp90s0: authenticate with 02:03:7f:12:66:66
[  227.601846] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  227.633902] wlp90s0: authenticate with 02:03:7f:12:66:66
[  227.633906] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  227.657203] wlp90s0: 02:03:7f:12:66:66 denied authentication (status 1)
...
[  263.014661] wlp90s0: authenticate with 02:03:7f:12:66:66
[  263.075667] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  263.112427] wlp90s0: authenticate with 02:03:7f:12:66:66
[  263.112433] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  263.132507] wlp90s0: 02:03:7f:12:66:66 denied authentication (status 1)
[  279.668551] wlp90s0: authenticate with 02:03:7f:12:66:66
[  279.728848] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  279.763685] wlp90s0: authenticate with 02:03:7f:12:66:66
[  279.763696] wlp90s0: send auth to 02:03:7f:12:66:66 (try 1/3)
[  279.790867] wlp90s0: 02:03:7f:12:66:66 denied authentication (status 1)

for above fail, ieee80211_set_disassoc() is not called, so the
bss_conf->tx_pwr_env_num never reset and it is increased each
time in ieee80211_prep_channel().

Finally the bss_conf->tx_pwr_env_num arrived to 20+(it should be 1 for
correct
value), it has exceeded the max value IEEE80211_TPE_MAX_IE_COUNT(8), and
lead access bss_conf->tx_pwr_env[IEEE80211_TPE_MAX_IE_COUNT] overflow.

[  327.391621] wlp90s0: authenticate with 02:03:7f:12:66:66
[  327.434036] BUG: kernel NULL pointer dereference, address:
0000000000000018
[  327.434039] #PF: supervisor read access in kernel mode
[  327.434040] #PF: error_code(0x0000) - not-present page
[  327.434042] PGD 0 P4D 0
[  327.434044] Oops: 0000 [#1] PREEMPT SMP NOPTI
[  327.434047] CPU: 2 PID: 804 Comm: wpa_supplicant Kdump: loaded
Tainted: G        W          6.2.0-rc8-wt-ath+ #13
[  327.434050] Hardware name: Intel(R) Client Systems
NUC11PHi7/NUC11PHBi7, BIOS PHTGL579.0063.2021.0707.1057 07/07/2021
[  327.434052] RIP: 0010:ath12k_mac_fill_reg_tpc_info+0x292/0x3b0 [ath12k]
[  327.434080] RSP: 0018:ffffb7330160f448 EFLAGS: 00010246
[  327.434081] RAX: 0000000000000000 RBX: 0000000000000006 RCX:
00000000005a8f98
[  327.434082] RDX: ffff9c7de1cb7f00 RSI: 00000000006cdf18 RDI:
ffff9c7de2000508
[  327.434084] RBP: ffffb7330160f4b8 R08: 0000000000000000 R09:
ffff9c7de1cb7f00
[  327.434084] R10: ffff9c7de2000508 R11: ffffb7330160f0e0 R12:
ffff9c7dda376090
[  327.434085] R13: 0000000000000001 R14: 0000000000000010 R15:
ffff9c7de2002080
[  327.434086] FS:  00007f8258ee9c00(0000) GS:ffff9c8170480000(0000)
knlGS:0000000000000000
[  327.434087] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  327.434088] CR2: 0000000000000018 CR3: 0000000108fec006 CR4:
0000000000770ee0
[  327.434090] PKRU: 55555554
[  327.434090] Call Trace:
[  327.434091]  <TASK>
[  327.434094]  ath12k_mac_vdev_start_restart+0x6c8/0x7f0 [ath12k]
[  327.434104]  ? crypto_alloc_tfm_node+0x60/0x130
[  327.434108]  ath12k_mac_station_add+0x163/0x440 [ath12k]
[  327.434117]  ath12k_mac_handle_link_sta_state.isra.79+0x76/0x420 [ath12k]
[  327.434126]  ath12k_mac_op_sta_state+0x19f/0x350 [ath12k]
[  327.434136]  drv_sta_state+0x89/0x5d0 [mac80211]
[  327.434160]  sta_info_insert_rcu+0x222/0x580 [mac80211]
[  327.434176]  sta_info_insert+0xf/0x20 [mac80211]
[  327.434190]  ieee80211_prep_connection+0x200/0x490 [mac80211]
[  327.434214]  ieee80211_mgd_auth+0x2aa/0x4f0 [mac80211]
[  327.434237]  ? __local_bh_enable_ip+0x3b/0x80
[  327.434239]  ? _raw_spin_unlock_bh+0x1d/0x30
[  327.434243]  ieee80211_auth+0x18/0x20 [mac80211]
[  327.434263]  cfg80211_mlme_auth+0x94/0x180 [cfg80211]
[  327.434297]  nl80211_authenticate+0x392/0x3f0 [cfg80211]
[  327.434315]  genl_family_rcv_msg_doit.isra.19+0xf4/0x120
[  327.434318]  genl_rcv_msg+0x1a5/0x2a0
[  327.434320]  ? __cfg80211_rdev_from_attrs+0x1f0/0x1f0 [cfg80211]
[  327.434336]  ? cfg80211_prepare_cqm.isra.79+0x170/0x170 [cfg80211]
[  327.434352]  ? nl80211_put_signal.part.56+0xd0/0xd0 [cfg80211]
[  327.434368]  ? genl_get_cmd_both+0x60/0x60
[  327.434370]  netlink_rcv_skb+0x5a/0x110
[  327.434372]  genl_rcv+0x28/0x40
[  327.434374]  netlink_unicast+0x1be/0x290
[  327.434375]  netlink_sendmsg+0x377/0x4e0
[  327.434377]  sock_sendmsg+0x9a/0xa0
[  327.434380]  ____sys_sendmsg+0x22b/0x2f0
[  327.434382]  ___sys_sendmsg+0x88/0xd0
[  327.434384]  ? dput+0x5f/0x2e0
[  327.434386]  ? __fsnotify_parent+0x109/0x350
[  327.434389]  __sys_sendmsg+0x6c/0xc0
[  327.434391]  ? __sys_sendmsg+0x6c/0xc0
[  327.434393]  __x64_sys_sendmsg+0x1f/0x30
[  327.434395]  do_syscall_64+0x37/0x90
[  327.434398]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[  327.434400] RIP: 0033:0x7f8258927b17
>
>
>
>