[PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Michael Lee]

When the link fails to use the channel, chandef's bandwidth will be 
downgraded without checking the control channel.
The issue happens when the STA of an extender with limited channel 
context associates with a root AP operating on a different channel.

Below is an example:

    ______________           ________________
   | RootAP(ch36) |         | Extender(ch44) |
   |              | (ASSOC) |       AP       |
   |      AP <-------------------- STA       |
   |______________|         |________________|

- RootAP is operating on channel 36, while Extender is operating
  on channel 44
- When associating with RootAP, Extender-STA downgrades the
  chandef's bandwidth to be compatible with any channels on the phy
- Finally, chandef's bandwidth is downgraded to 20MHz and 
  the association fails

In this patch, a control channel checking is added to avoid unnecessary
bandwidth downgrading

Signed-off-by: Michael Lee <michael-cy.lee@mediatek.com>
---
 net/mac80211/mlme.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 0aee2392dd29..bc435e8508e2 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4616,6 +4616,27 @@ ieee80211_verify_sta_he_mcs_support(struct ieee80211_sub_if_data *sdata,
 	return false;
 }
 
+static bool
+ieee80211_check_same_ctrl_channel(struct ieee80211_sub_if_data *sdata,
+				  const struct cfg80211_chan_def *chandef)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx *ctx;
+
+	mutex_lock(&local->chanctx_mtx);
+	list_for_each_entry(ctx, &local->chanctx_list, list) {
+		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
+			continue;
+		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
+			continue;
+		if (chandef->chan == ctx->conf.def.chan)
+			return true;
+	}
+
+	mutex_unlock(&local->chanctx_mtx);
+	return false;
+}
+
 static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
 				  struct ieee80211_link_data *link,
 				  struct cfg80211_bss *cbss,
@@ -4842,6 +4863,9 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
 	    chandef.width == NL80211_CHAN_WIDTH_10)
 		goto out;
 
+	if (!ret || !ieee80211_check_same_ctrl_channel(sdata, &chandef))
+		goto out;
+
 	while (ret && chandef.width != NL80211_CHAN_WIDTH_20_NOHT) {
 		*conn_flags |=
 			ieee80211_chandef_downgrade(&chandef);
-- 
2.25.1

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Johannes Berg]

Hi,

So I've looked at this patch a few times, but it just confuses me ...

On Thu, 2022-12-22 at 17:13 +0800, Michael Lee wrote:
> When the link fails to use the channel, chandef's bandwidth will be 
> downgraded without checking the control channel.
> The issue happens when the STA of an extender with limited channel 
> context associates with a root AP operating on a different channel.
> 
> Below is an example:
> 
>     ______________           ________________
>    | RootAP(ch36) |         | Extender(ch44) |
>    |              | (ASSOC) |       AP       |
>    |      AP <-------------------- STA       |
>    |______________|         |________________|
> 
> - RootAP is operating on channel 36, while Extender is operating
>   on channel 44

What does this matter? The extended is just a STA, no? Or are you saying
it's important that the extender has a concurrent AP interface that's on
channel 44?

And if you say "ch36" or "ch44" that's just the control channel (I
guess), but what's the actual complete channel configuration?

> - When associating with RootAP, Extender-STA downgrades the
>   chandef's bandwidth to be compatible with any channels on the phy

What do you mean by "on the phy" here? That's not mac80211 terminology,
so not sure.

> - Finally, chandef's bandwidth is downgraded to 20MHz and 
>   the association fails
> 
> In this patch, a control channel checking is added to avoid unnecessary
> bandwidth downgrading
> 
> Signed-off-by: Michael Lee <michael-cy.lee@mediatek.com>
> ---
>  net/mac80211/mlme.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 0aee2392dd29..bc435e8508e2 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -4616,6 +4616,27 @@ ieee80211_verify_sta_he_mcs_support(struct ieee80211_sub_if_data *sdata,
>  	return false;
>  }
>  
> +static bool
> +ieee80211_check_same_ctrl_channel(struct ieee80211_sub_if_data *sdata,
> +				  const struct cfg80211_chan_def *chandef)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_chanctx *ctx;
> +
> +	mutex_lock(&local->chanctx_mtx);
> +	list_for_each_entry(ctx, &local->chanctx_list, list) {
> +		if (ctx->replace_state == IEEE80211_CHANCTX_WILL_BE_REPLACED)
> +			continue;
> +		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
> +			continue;
> +		if (chandef->chan == ctx->conf.def.chan)
> +			return true;
> +	}
> +
> +	mutex_unlock(&local->chanctx_mtx);
> +	return false;
> +}
> +
>  static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>  				  struct ieee80211_link_data *link,
>  				  struct cfg80211_bss *cbss,
> @@ -4842,6 +4863,9 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata,
>  	    chandef.width == NL80211_CHAN_WIDTH_10)
>  		goto out;
>  
> +	if (!ret || !ieee80211_check_same_ctrl_channel(sdata, &chandef))
> +		goto out;

Not sure I get how this is any different - you're describing a case
where "ret != 0" (because if ret == 0 nothing happens in the while
loop), so then you fail _anyway_? So what's the point?

johannes

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Michael-cy Lee (李峻宇)]

Hi, 

In the scenario mentioned in the previous mail, we expect the
association attempt from Extender’s STA interface would fail with the
following conditions:
1. Extender's STA and AP interfaces locate on the same ieee80211_local
which uses channel 44 and bandwidth 80MHz.
2. The channel configurations between RootAP’s and Extender's are
different. (RootAP: Ch36/80MHz; Extender: Ch44/80MHz)
3. Our device allows only one channel context to exist on each
ieee80211_local. (num_different_channels = 1 in mt76 driver, ref: 
https://github.com/openwrt/mt76/blob/master/mt7915/init.c#L30)

When Extender's STA interface tries to associate with RootAP, the
return value of ieee80211_link_use_channel is not 0 because the current
channel context (ch44/80M) is not compatible with RootAP’s channel
context (ch36/80M) and it cannot create a new channel context on
Extender’s ieee80211_local because of the device limitation
(num_different_channels = 1).
To deal with such an error case, Extender’s STA interface would
downgrade the channel bandwidth (from 80MHz) and run
ieee80211_link_use_channel to check again until the bandwidth becomes
the minimum 20MHz. 
Since the control channels are different, downgrading the bandwidth
cannot make ieee80211_link_use_channel to return 0. Finally, Extender’s
STA interface bandwidth (ifmgd->flags) would use 20MHz afterward. (In
the current kernel version, the flag is link->u.mgd.conn_flags)

At this moment, a hostapd_cli channel switch command might be issued,
making Extender's AP interface switch to RootAP's channel (ch36).
Because the control channel now synchronizes, Extender's STA
association attempt succeeds. However, Extender’s STA ifmgd->flags
remains in the fallback state and thus the operating bandwidth is 20MHz
instead of 80MHz we expect.

To avoid any undesirable bandwidth downgrading, when
ieee80211_link_use_channel returns non-zero, we check the control
channel before entering the loop of downgrading bandwidth. 

For other replies, please see the inline.

On Wed, 2023-01-18 at 10:40 +0100, Johannes Berg wrote:
> Hi,
> 
> So I've looked at this patch a few times, but it just confuses me ...
> 
> On Thu, 2022-12-22 at 17:13 +0800, Michael Lee wrote:
> > When the link fails to use the channel, chandef's bandwidth will
> > be 
> > downgraded without checking the control channel.
> > The issue happens when the STA of an extender with limited channel 
> > context associates with a root AP operating on a different channel.
> > 
> > Below is an example:
> > 
> >     ______________           ________________
> >    | RootAP(ch36) |         | Extender(ch44) |
> >    |              | (ASSOC) |       AP       |
> >    |      AP <-------------------- STA       |
> >    |______________|         |________________|
> > 
> > - RootAP is operating on channel 36, while Extender is operating
> >   on channel 44
> 
> What does this matter? The extended is just a STA, no? Or are you
> saying
> it's important that the extender has a concurrent AP interface that's
> on
> channel 44?
> 
> And if you say "ch36" or "ch44" that's just the control channel (I
> guess), but what's the actual complete channel configuration?

Our Extender contains one AP interface and one STA interface on the
same ieee80211_local, and since our device allows only one channel
context on each ieee80211_local, the AP and STA interface must operate
on the same control channel.
RootAP and Extender’s AP interface operate on different control
channels (ch36 and ch44), with other channel configurations being the
same (bandwidth 80MHz and center frequency 5210MHz)

> 
> > - When associating with RootAP, Extender-STA downgrades the
> >   chandef's bandwidth to be compatible with any channels on the phy
> 
> What do you mean by "on the phy" here? That's not mac80211
> terminology,
> so not sure.

Sorry for the ambiguous word, we mean the ieee80211_local structure.

> 
> > - Finally, chandef's bandwidth is downgraded to 20MHz and 
> >   the association fails
> > 
> > In this patch, a control channel checking is added to avoid
> > unnecessary
> > bandwidth downgrading
> > 
> > Signed-off-by: Michael Lee <michael-cy.lee@mediatek.com>
> > ---
> >  net/mac80211/mlme.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> > index 0aee2392dd29..bc435e8508e2 100644
> > --- a/net/mac80211/mlme.c
> > +++ b/net/mac80211/mlme.c
> > @@ -4616,6 +4616,27 @@ ieee80211_verify_sta_he_mcs_support(struct
> > ieee80211_sub_if_data *sdata,
> >  	return false;
> >  }
> >  
> > +static bool
> > +ieee80211_check_same_ctrl_channel(struct ieee80211_sub_if_data
> > *sdata,
> > +				  const struct cfg80211_chan_def
> > *chandef)
> > +{
> > +	struct ieee80211_local *local = sdata->local;
> > +	struct ieee80211_chanctx *ctx;
> > +
> > +	mutex_lock(&local->chanctx_mtx);
> > +	list_for_each_entry(ctx, &local->chanctx_list, list) {
> > +		if (ctx->replace_state ==
> > IEEE80211_CHANCTX_WILL_BE_REPLACED)
> > +			continue;
> > +		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE)
> > +			continue;
> > +		if (chandef->chan == ctx->conf.def.chan)
> > +			return true;
> > +	}
> > +
> > +	mutex_unlock(&local->chanctx_mtx);
> > +	return false;
> > +}
> > +
> >  static int ieee80211_prep_channel(struct ieee80211_sub_if_data
> > *sdata,
> >  				  struct ieee80211_link_data *link,
> >  				  struct cfg80211_bss *cbss,
> > @@ -4842,6 +4863,9 @@ static int ieee80211_prep_channel(struct
> > ieee80211_sub_if_data *sdata,
> >  	    chandef.width == NL80211_CHAN_WIDTH_10)
> >  		goto out;
> >  
> > +	if (!ret || !ieee80211_check_same_ctrl_channel(sdata,
> > &chandef))
> > +		goto out;
> 
> Not sure I get how this is any different - you're describing a case
> where "ret != 0" (because if ret == 0 nothing happens in the while
> loop), so then you fail _anyway_? So what's the point?

As described above, the first association fails as expected. However,
after Extender's AP interface switch to the same control channel as
RootAP, the Extender's STA associates with RootAP by only 20MHz instead
of 80Mhz we expect.

> 
> johannes
> 

Best, 
Michael

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Johannes Berg]

Hi,

> To deal with such an error case, Extender’s STA interface would
> downgrade the channel bandwidth (from 80MHz) and run
> ieee80211_link_use_channel to check again until the bandwidth becomes
> the minimum 20MHz. 

Correct, and then it fails.

> Since the control channels are different, downgrading the bandwidth
> cannot make ieee80211_link_use_channel to return 0.

Right.

> Finally, Extender’s
> STA interface bandwidth (ifmgd->flags) would use 20MHz afterward. (In
> the current kernel version, the flag is link->u.mgd.conn_flags)

Also correct, but does it matter?

> At this moment, a hostapd_cli channel switch command might be issued,
> making Extender's AP interface switch to RootAP's channel (ch36).
> Because the control channel now synchronizes, Extender's STA
> association attempt succeeds. However, Extender’s STA ifmgd->flags
> remains in the fallback state and thus the operating bandwidth is 20MHz
> instead of 80MHz we expect.

So ... still not sure I understand.

Are you saying that

 1) you have a race, and hostapd switches channels while the client-side
    code is in the middle of this loop?

or

 2) the ifmgd->flags are not reset correctly for a new association?


johannes

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Michael-cy Lee (李峻宇)]

Hi, 

Please see the inline.

On Tue, 2023-03-14 at 08:42 +0100, Johannes Berg wrote:
> Hi,
> 
> > To deal with such an error case, Extender’s STA interface would
> > downgrade the channel bandwidth (from 80MHz) and run
> > ieee80211_link_use_channel to check again until the bandwidth
> > becomes
> > the minimum 20MHz. 
> 
> Correct, and then it fails.
> 
> > Since the control channels are different, downgrading the bandwidth
> > cannot make ieee80211_link_use_channel to return 0.
> 
> Right.
> 
> > Finally, Extender’s
> > STA interface bandwidth (ifmgd->flags) would use 20MHz afterward.
> > (In
> > the current kernel version, the flag is link->u.mgd.conn_flags)
> 
> Also correct, but does it matter?

It matters because Extender's STA interface's ifmgd->flags is changed
during bandwidth downgrading and remains changed to the next
association.

> 
> > At this moment, a hostapd_cli channel switch command might be
> > issued,
> > making Extender's AP interface switch to RootAP's channel (ch36).
> > Because the control channel now synchronizes, Extender's STA
> > association attempt succeeds. However, Extender’s STA ifmgd->flags
> > remains in the fallback state and thus the operating bandwidth is
> > 20MHz
> > instead of 80MHz we expect.
> 
> So ... still not sure I understand.
> 
> Are you saying that
> 
>  1) you have a race, and hostapd switches channels while the client-
> side
>     code is in the middle of this loop?

No, we think there is no race concern since the locks are well-
maintained.
So the steps are:
First association fails -> hostapd switches channel -> second
association succeeds (with STA operating on 20MHz)

Every step starts after the previous step has finished. 

> 
> or
> 
>  2) the ifmgd->flags are not reset correctly for a new association?

Yes, our patch wants to solve this problem.
Since there is no way to reset ifmgd->flags after bandwidth
downgrading, we check the control channel before downgrading the
bandwidth. If the control channels are different, we directly fail the
association.

> 
> 
> johannes

Best, 
Michael

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Johannes Berg]

Hi,

> It matters because Extender's STA interface's ifmgd->flags is changed
> during bandwidth downgrading and remains changed to the next
> association.

OK but then _that_ seems like the problem here, no?

> Since there is no way to reset ifmgd->flags after bandwidth
> downgrading, we check the control channel before downgrading the
> bandwidth. If the control channels are different, we directly fail the
> association.
> 

Right, but what if you had an association to some other AP, or two
chanctxes were allowed, or something else?

I feel we should probably instead just reset these ifmgd->flags before
association, or so?

johannes

Re: [PATCH] wifi: mac80211: check the control channel before downgrading the bandwidth

[Michael-cy Lee (李峻宇)]

Hi, 

On Tue, 2023-03-14 at 09:40 +0100, Johannes Berg wrote:
> Hi,
> 
> > It matters because Extender's STA interface's ifmgd->flags is
> > changed
> > during bandwidth downgrading and remains changed to the next
> > association.
> 
> OK but then _that_ seems like the problem here, no?

Right.

> 
> > Since there is no way to reset ifmgd->flags after bandwidth
> > downgrading, we check the control channel before downgrading the
> > bandwidth. If the control channels are different, we directly fail
> > the
> > association.
> > 
> 
> Right, but what if you had an association to some other AP, or two
> chanctxes were allowed, or something else?

In our scenario, if two chanctxs were allowed,
ieee80211_link_use_channel would not return non-zero, and the
association should succeed.

And if Extender's STA interface had an association with some other AP,
it would first de-authenticate with that AP, then scan and associate
with another new AP. The same issue still happens in this case.

To deal with other cases, the new function
ieee80211_check_same_ctrl_channel iterates through ieee80211_local's
chanctx_list to find a chanctx with the same cfg80211_chan_def as
RootAP.

> 
> I feel we should probably instead just reset these ifmgd->flags
> before
> association, or so?

Yes, it might also be a solution.

> 
> johannes


Best, 
Michael