wifi: mac80211: fix puncturing bitmap handling in CSA

Commit Message

Johannes Berg Aug. 16, 2023, 8:04 a.m. UTC

From: Johannes Berg <[e-mail]>

Code inspection reveals that we switch the puncturing bitmap
before the real channel switch, since that happens only in
the second round of the worker after the channel context is
switched by ieee80211_link_use_reserved_context().

Fixes: 2cc25e4b2a04 ("wifi: mac80211: configure puncturing bitmap")
Signed-off-by: Johannes Berg <[e-mail]>
---
 net/mac80211/cfg.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Aloka Dixit Aug. 16, 2023, 5:22 p.m. UTC | #1

On 8/16/2023 1:04 AM, Johannes Berg wrote:
> From: Johannes Berg <[e-mail]>
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index e7ac24603892..1657ff09a83a 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -3648,12 +3648,6 @@ static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>   	lockdep_assert_held(&local->mtx);
>   	lockdep_assert_held(&local->chanctx_mtx);
>   
> -	if (sdata->vif.bss_conf.eht_puncturing != sdata->vif.bss_conf.csa_punct_bitmap) {
> -		sdata->vif.bss_conf.eht_puncturing =
> -					sdata->vif.bss_conf.csa_punct_bitmap;
> -		changed |= BSS_CHANGED_EHT_PUNCTURING;
> -	}
> -
>   	/*
>   	 * using reservation isn't immediate as it may be deferred until later
>   	 * with multi-vif. once reservation is complete it will re-schedule the
> @@ -3683,6 +3677,12 @@ static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
>   	if (err)
>   		return err;
>   
> +	if (link_data->conf->eht_puncturing != link_data->conf->csa_punct_bitmap) {
> +		link_data->conf->eht_puncturing =
> +					link_data->conf->csa_punct_bitmap;
> +		changed |= BSS_CHANGED_EHT_PUNCTURING;
> +	}
> +
>   	ieee80211_link_info_change_notify(sdata, &sdata->deflink, changed);
>   
>   	if (sdata->deflink.csa_block_tx) {

Looks okay, though I remember there was a reason I had put it at the 
top. If this causes any issue I will send a follow-up later.
Thanks!

Johannes Berg Aug. 16, 2023, 7:40 p.m. UTC | #2

On Wed, 2023-08-16 at 10:22 -0700, Aloka Dixit wrote:
> 
> Looks okay, though I remember there was a reason I had put it at the 
> top.

Odd. I wonder why? It seems wrong here - in some cases it might apply it
before it even goes to the new channel?

To be fair, we should maybe split this work and not use it for the whole
"once reservation is complete" part?

> If this causes any issue I will send a follow-up later.

Oh, I can wait with this too - clearly puncturing with CSA isn't really
even on my list - I was just trying to make sense of this code due to
some other (syzbot maybe, I was looking at those a bit) issue, and this
stood out as "looks wrong".

So if you have any tests you'd like to run with the patch first, just
let me know and I can just hold off applying this.

johannes

Patch

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index e7ac24603892..1657ff09a83a 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -3648,12 +3648,6 @@  static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	lockdep_assert_held(&local->mtx);
 	lockdep_assert_held(&local->chanctx_mtx);
 
-	if (sdata->vif.bss_conf.eht_puncturing != sdata->vif.bss_conf.csa_punct_bitmap) {
-		sdata->vif.bss_conf.eht_puncturing =
-					sdata->vif.bss_conf.csa_punct_bitmap;
-		changed |= BSS_CHANGED_EHT_PUNCTURING;
-	}
-
 	/*
 	 * using reservation isn't immediate as it may be deferred until later
 	 * with multi-vif. once reservation is complete it will re-schedule the
@@ -3683,6 +3677,12 @@  static int __ieee80211_csa_finalize(struct ieee80211_sub_if_data *sdata)
 	if (err)
 		return err;
 
+	if (link_data->conf->eht_puncturing != link_data->conf->csa_punct_bitmap) {
+		link_data->conf->eht_puncturing =
+					link_data->conf->csa_punct_bitmap;
+		changed |= BSS_CHANGED_EHT_PUNCTURING;
+	}
+
 	ieee80211_link_info_change_notify(sdata, &sdata->deflink, changed);
 
 	if (sdata->deflink.csa_block_tx) {