Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2017-03-08 21:20, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> If the user rate mask results in no (basic) rates being usable,
> clear it. Also, if we're already operating when it's set, reject
> it instead.
> 
> Technically, selecting basic rates as the criterion is a bit too
> restrictive, but calculating the usable rates over all stations
> (e.g. in AP mode) is harder, and all stations must support the
> basic rates. Similarly, in client mode, the basic rates will be
> used anyway for control frames.
> 
> This fixes the "no supported rates (...) in rate_mask ..." warning
> that occurs on TX when you've selected a rate mask that's not
> compatible with the connection (e.g. an AP that enables only the
> rates 36, 48, 54 and you've selected only 6, 9, 12.)
> 
> Reported-by: Kirtika Ruchandani <kirtika@google.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  net/mac80211/cfg.c  | 18 +++++++++++++++++-
>  net/mac80211/mlme.c |  2 ++
>  net/mac80211/rate.c | 27 +++++++++++++++++++++++++++
>  net/mac80211/rate.h |  2 ++
>  4 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 9c7490cb2243..8bc3d3669348 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright 2006-2010	Johannes Berg <johannes@sipsolutions.net>
>   * Copyright 2013-2015  Intel Mobile Communications GmbH
> - * Copyright (C) 2015-2016 Intel Deutschland GmbH
> + * Copyright (C) 2015-2017 Intel Deutschland GmbH
>   *
>   * This file is GPLv2 as found in COPYING.
>   */
> @@ -2042,6 +2042,7 @@ static int ieee80211_change_bss(struct wiphy 
> *wiphy,
>  					 params->basic_rates_len,
>  					 &sdata->vif.bss_conf.basic_rates);
>  		changed |= BSS_CHANGED_BASIC_RATES;
> +		ieee80211_check_rate_mask(sdata);
>  	}
> 
>  	if (params->ap_isolate >= 0) {
> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
> wiphy *wiphy,
>  			return ret;
>  	}
> 
> +	/*
> +	 * If active validate the setting and reject it if it doesn't leave
> +	 * at least one basic rate usable, since we really have to be able
> +	 * to send something, and if we're an AP we have to be able to do
> +	 * so at a basic rate so that all clients can receive it.
> +	 */
> +	if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
> +	    sdata->vif.bss_conf.chandef.chan) {
> +		u32 basic_rates = sdata->vif.bss_conf.basic_rates;
> +		enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
> +
> +		if (!(mask->control[band].legacy & basic_rates))
> +			return -EINVAL;
If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to 
fixed in 54M and 54M is
not basic rate in AP's becaon as example of below, then the iw command 
will fail.

Tag: Supported Rates 6(B), 9, 12(B), 18, 24(B), 36, 48, 54, [Mbit/sec]
     Tag Number: Supported Rates (1)
     Tag length: 8
     Supported Rates: 6(B) (0x8c)
     Supported Rates: 9 (0x12)
     Supported Rates: 12(B) (0x98)
     Supported Rates: 18 (0x24)
     Supported Rates: 24(B) (0xb0)
     Supported Rates: 36 (0x48)
     Supported Rates: 48 (0x60)
     Supported Rates: 54 (0x6c)

> +	}
> +
>  	for (i = 0; i < NUM_NL80211_BANDS; i++) {
>  		struct ieee80211_supported_band *sband = wiphy->bands[i];
>  		int j;
...

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Arend Van Spriel]

[-- Attachment #1.1: Type: text/plain, Size: 1347 bytes --]

On 11/12/2020 11:55 AM, Wen Gong wrote:
> On 2017-03-08 21:20, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>>

[...]

>> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
>> wiphy *wiphy,
>>              return ret;
>>      }
>>
>> +    /*
>> +     * If active validate the setting and reject it if it doesn't leave
>> +     * at least one basic rate usable, since we really have to be able
>> +     * to send something, and if we're an AP we have to be able to do
>> +     * so at a basic rate so that all clients can receive it.
>> +     */
>> +    if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>> +        sdata->vif.bss_conf.chandef.chan) {
>> +        u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>> +        enum nl80211_band band = sdata->vif.bss_conf.chandef.chan->band;
>> +
>> +        if (!(mask->control[band].legacy & basic_rates))
>> +            return -EINVAL;
> If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to 
> fixed in 54M and 54M is
> not basic rate in AP's becaon as example of below, then the iw command 
> will fail.

Which was the intent of this change, wasn't it? You want to allow anyway 
or you want a clear error message as to why it fails?

Regards,
Arend

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4176 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-12 20:49, Arend Van Spriel wrote:
> On 11/12/2020 11:55 AM, Wen Gong wrote:
>> On 2017-03-08 21:20, Johannes Berg wrote:
>>> From: Johannes Berg <johannes.berg@intel.com>
>>> 
> 
> [...]
> 
>>> @@ -2685,6 +2686,21 @@ static int ieee80211_set_bitrate_mask(struct
>>> wiphy *wiphy,
>>>              return ret;
>>>      }
>>> 
>>> +    /*
>>> +     * If active validate the setting and reject it if it doesn't 
>>> leave
>>> +     * at least one basic rate usable, since we really have to be 
>>> able
>>> +     * to send something, and if we're an AP we have to be able to 
>>> do
>>> +     * so at a basic rate so that all clients can receive it.
>>> +     */
>>> +    if (rcu_access_pointer(sdata->vif.chanctx_conf) &&
>>> +        sdata->vif.bss_conf.chandef.chan) {
>>> +        u32 basic_rates = sdata->vif.bss_conf.basic_rates;
>>> +        enum nl80211_band band = 
>>> sdata->vif.bss_conf.chandef.chan->band;
>>> +
>>> +        if (!(mask->control[band].legacy & basic_rates))
>>> +            return -EINVAL;
>> If user want to use “iw wlan0 set bitrates legacy-5 54” to set it to 
>> fixed in 54M and 54M is
>> not basic rate in AP's becaon as example of below, then the iw command 
>> will fail.
> 
> Which was the intent of this change, wasn't it? You want to allow
> anyway or you want a clear error message as to why it fails?
We need to set the tx rate to fixed at a single rate, e.g., 
54M/48M/36M... for a test case.
I do not want a clear error message, I want to the 54M rate pass/set 
success to lower wlan driver.
Then lower wlan driver can handle it.
> 
> Regards,
> Arend

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
> 
> > Which was the intent of this change, wasn't it?

Indeed. Permitting this leads to warnings later.

> We need to set the tx rate to fixed at a single rate, e.g., 
> 54M/48M/36M... for a test case.
> I do not want a clear error message, I want to the 54M rate pass/set 
> success to lower wlan driver.
> Then lower wlan driver can handle it.

No, that will not happen. You should configure your test AP to actually
support 54M.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-13 15:38, Johannes Berg wrote:
> On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
>> 
>> > Which was the intent of this change, wasn't it?
> 
> Indeed. Permitting this leads to warnings later.
> 
>> We need to set the tx rate to fixed at a single rate, e.g.,
>> 54M/48M/36M... for a test case.
>> I do not want a clear error message, I want to the 54M rate pass/set
>> success to lower wlan driver.
>> Then lower wlan driver can handle it.
> 
> No, that will not happen. You should configure your test AP to actually
> support 54M.
Yes, the AP support 54M, but it is not basic rate, so 
ieee80211_set_bitrate_mask will reject 54M
because fail for check (mask->control[band].legacy & basic_rates).

suppor rates of my AP:
Tag: Supported Rates 6(B), 9, 12(B), 18, 24(B), 36, 48, 54, [Mbit/sec]
     Tag Number: Supported Rates (1)
     Tag length: 8
     Supported Rates: 6(B) (0x8c)
     Supported Rates: 9 (0x12)
     Supported Rates: 12(B) (0x98)
     Supported Rates: 18 (0x24)
     Supported Rates: 24(B) (0xb0)
     Supported Rates: 36 (0x48)
     Supported Rates: 48 (0x60)
     Supported Rates: 54 (0x6c)

> 
> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

On Fri, 2020-11-13 at 16:14 +0800, Wen Gong wrote:
> On 2020-11-13 15:38, Johannes Berg wrote:
> > On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
> > > > Which was the intent of this change, wasn't it?
> > 
> > Indeed. Permitting this leads to warnings later.
> > 
> > > We need to set the tx rate to fixed at a single rate, e.g.,
> > > 54M/48M/36M... for a test case.
> > > I do not want a clear error message, I want to the 54M rate pass/set
> > > success to lower wlan driver.
> > > Then lower wlan driver can handle it.
> > 
> > No, that will not happen. You should configure your test AP to actually
> > support 54M.
> Yes, the AP support 54M, but it is not basic rate, so 
> ieee80211_set_bitrate_mask will reject 54M
> because fail for check (mask->control[band].legacy & basic_rates).

Ah. So this is what I said in the original commit message even:

> Technically, selecting basic rates as the criterion is a bit too
> restrictive, but calculating the usable rates over all stations
> (e.g. in AP mode) is harder, and all stations must support the
> basic rates. Similarly, in client mode, the basic rates will be
> used anyway for control frames.

I guess if we really want to redefine the user rate mask to not apply to
control frames, then we can relax this?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-13 16:16, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:14 +0800, Wen Gong wrote:
>> On 2020-11-13 15:38, Johannes Berg wrote:
>> > On Fri, 2020-11-13 at 10:08 +0800, Wen Gong wrote:
>> > > > Which was the intent of this change, wasn't it?
>> >
>> > Indeed. Permitting this leads to warnings later.
>> >
>> > > We need to set the tx rate to fixed at a single rate, e.g.,
>> > > 54M/48M/36M... for a test case.
>> > > I do not want a clear error message, I want to the 54M rate pass/set
>> > > success to lower wlan driver.
>> > > Then lower wlan driver can handle it.
>> >
>> > No, that will not happen. You should configure your test AP to actually
>> > support 54M.
>> Yes, the AP support 54M, but it is not basic rate, so
>> ieee80211_set_bitrate_mask will reject 54M
>> because fail for check (mask->control[band].legacy & basic_rates).
> 
> Ah. So this is what I said in the original commit message even:
> 
>> Technically, selecting basic rates as the criterion is a bit too
>> restrictive, but calculating the usable rates over all stations
>> (e.g. in AP mode) is harder, and all stations must support the
>> basic rates. Similarly, in client mode, the basic rates will be
>> used anyway for control frames.
> 
> I guess if we really want to redefine the user rate mask to not apply 
> to
> control frames, then we can relax this?
> 
Yes, for AP mode, it is hard to calculate the usable rates over all 
stations.
But for STATION mode, it can set 54M because AP support it, so it should 
not reject it.
If add a check for nl80211_iftype of ieee80211_vif in 
ieee80211_set_bitrate_mask, it can
solve this like this:
if (sdata->vif.type != NL80211_IFTYPE_STATION && 
!(mask->control[band].legacy & basic_rates))
> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

On Fri, 2020-11-13 at 16:35 +0800, Wen Gong wrote:
> 
> > I guess if we really want to redefine the user rate mask to not apply 
> > to
> > control frames, then we can relax this?
> > 
> Yes, for AP mode, it is hard to calculate the usable rates over all 
> stations.
> But for STATION mode, it can set 54M because AP support it, so it should 
> not reject it.
> If add a check for nl80211_iftype of ieee80211_vif in 
> ieee80211_set_bitrate_mask, it can
> solve this like this:
> if (sdata->vif.type != NL80211_IFTYPE_STATION && 
> !(mask->control[band].legacy & basic_rates))


That would forgo the check completely - we'd still need to check against
the *supported* rates.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-13 16:35, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:35 +0800, Wen Gong wrote:
>> 
>> > I guess if we really want to redefine the user rate mask to not apply
>> > to
>> > control frames, then we can relax this?
>> >
>> Yes, for AP mode, it is hard to calculate the usable rates over all
>> stations.
>> But for STATION mode, it can set 54M because AP support it, so it 
>> should
>> not reject it.
>> If add a check for nl80211_iftype of ieee80211_vif in
>> ieee80211_set_bitrate_mask, it can
>> solve this like this:
>> if (sdata->vif.type != NL80211_IFTYPE_STATION &&
>> !(mask->control[band].legacy & basic_rates))
> 
> 
> That would forgo the check completely - we'd still need to check 
> against
> the *supported* rates.
> 
yes.
It can add check with supp_rates[band] of ieee80211_sta for 
NL80211_IFTYPE_STATION type.
for others, check with sdata->vif.bss_conf.basic_rates
> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:

> yes.
> It can add check with supp_rates[band] of ieee80211_sta for 
> NL80211_IFTYPE_STATION type.
> for others, check with sdata->vif.bss_conf.basic_rates

Right.

Though, might need to check that only if there's no TDLS station or
something?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-13 16:51, Johannes Berg wrote:
> On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
> 
>> yes.
>> It can add check with supp_rates[band] of ieee80211_sta for
>> NL80211_IFTYPE_STATION type.
>> for others, check with sdata->vif.bss_conf.basic_rates
> 
> Right.
> 
> Though, might need to check that only if there's no TDLS station or
> something?
> 
yes, I think it should do like that for TDLS:
test_sta_flag(sta, WLAN_STA_TDLS_PEER)
> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

On Fri, 2020-11-13 at 17:09 +0800, Wen Gong wrote:
> On 2020-11-13 16:51, Johannes Berg wrote:
> > On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
> > 
> > > yes.
> > > It can add check with supp_rates[band] of ieee80211_sta for
> > > NL80211_IFTYPE_STATION type.
> > > for others, check with sdata->vif.bss_conf.basic_rates
> > 
> > Right.
> > 
> > Though, might need to check that only if there's no TDLS station or
> > something?
> > 
> yes, I think it should do like that for TDLS:
> test_sta_flag(sta, WLAN_STA_TDLS_PEER)

Yes but you have to iterate all the stations and check they belong to
the interface and all that I think?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Wen Gong]

On 2020-11-13 17:10, Johannes Berg wrote:
> On Fri, 2020-11-13 at 17:09 +0800, Wen Gong wrote:
>> On 2020-11-13 16:51, Johannes Berg wrote:
>> > On Fri, 2020-11-13 at 16:51 +0800, Wen Gong wrote:
>> >
>> > > yes.
>> > > It can add check with supp_rates[band] of ieee80211_sta for
>> > > NL80211_IFTYPE_STATION type.
>> > > for others, check with sdata->vif.bss_conf.basic_rates
>> >
>> > Right.
>> >
>> > Though, might need to check that only if there's no TDLS station or
>> > something?
>> >
>> yes, I think it should do like that for TDLS:
>> test_sta_flag(sta, WLAN_STA_TDLS_PEER)
> 
> Yes but you have to iterate all the stations and check they belong to
> the interface and all that I think?
> 
Maybe not need.
if NL80211_IFTYPE_STATION and NON-WLAN_STA_TDLS_PEER, it has only 1 
station for the ieee80211_vif
by my understand.
> johannes

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2] mac80211: reject/clear user rate mask if not usable

[Johannes Berg]

> > Yes but you have to iterate all the stations and check they belong to
> > the interface and all that I think?
> > 
> Maybe not need.
> if NL80211_IFTYPE_STATION and NON-WLAN_STA_TDLS_PEER, it has only 1 
> station for the ieee80211_vif
> by my understand.

Well, yes, but there's no counter.

So you need to iterate to see if there are any additional TDLS peers (in
addition to the STA entry for the AP).

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k