Skip to content

Conversation

@opsiff
Copy link
Member

@opsiff opsiff commented Feb 18, 2025

Commit(3):
wifi: mac80211: improve CSA/ECSA connection refusal
wifi: cfg80211: detect stuck ECSA element in probe resp
wifi: mac80211: don't connect to an AP while it's in a CSA process

Summary by Sourcery

This pull request improves the mac80211 driver's handling of Channel Switch Announcements (CSA) and Extended Channel Switch Announcements (ECSA). It prevents the driver from attempting to connect to an AP while it is in a CSA process and detects stuck ECSA elements in probe responses.

Bug Fixes:

  • Fixes an issue where the driver could attempt to connect to an AP while it is in the middle of a Channel Switch Announcement (CSA) process, which could lead to connection failures or instability.
  • Improves the reliability of CSA/ECSA handling by detecting and ignoring 'stuck' ECSA elements in probe responses, preventing the driver from misinterpreting outdated channel switch information.

Enhancements:

  • Improves the robustness of Wi-Fi connections by preventing connections to APs during CSA, ensuring a smoother transition during channel switches.

mainline inclusion
from mainline-v6.7-rc1
category: bugfix

Connection to an AP that is running a CSA flow may end up with a
failure as the AP might change its channel during the connection
flow while we do not track the channel change yet.
Avoid that by rejecting a connection to such an AP.

Signed-off-by: Ayala Beker <ayala.beker@intel.com>
Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
Link: https://lore.kernel.org/r/20230920211508.e5001a762a4a.I9745c695f3403b259ad000ce94110588a836c04a@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
(cherry picked from commit c09c4f3)
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
@sourcery-ai
Copy link

sourcery-ai bot commented Feb 18, 2025

Reviewer's Guide by Sourcery

This pull request enhances Wi-Fi connection stability by addressing issues related to Channel Switch Announcements (CSA) and Extended Channel Switch Announcements (ECSA). It prevents connections to APs during CSA processes and improves detection of stuck ECSA elements.

Sequence diagram for rejecting authentication during CSA process

sequenceDiagram
    participant STA as Station
    participant MGD as Management Daemon
    participant AP as Access Point

    STA->>MGD: Auth Request
    MGD->>MGD: ieee80211_mgd_csa_in_process(req->bss)
    alt AP is in CSA process
        MGD-->>STA: Auth Reject (-EINVAL)
    else AP is not in CSA process
        MGD->>AP: Auth Request
        AP-->>MGD: Auth Response
        MGD-->>STA: Auth Confirm
    end
Loading

Sequence diagram for rejecting association during CSA process

sequenceDiagram
    participant STA as Station
    participant MGD as Management Daemon
    participant AP as Access Point

    STA->>MGD: Assoc Request
    MGD->>MGD: ieee80211_mgd_csa_in_process(cbss)
    alt AP is in CSA process
        MGD-->>STA: Assoc Reject (-EINVAL)
    else AP is not in CSA process
        MGD->>AP: Assoc Request
        AP-->>MGD: Assoc Response
        MGD-->>STA: Assoc Confirm
    end
Loading

Updated class diagram for cfg80211_bss

classDiagram
    class cfg80211_bss {
        +u8 chains
        +s8 chain_signal[IEEE80211_MAX_CHAINS]
        +u8 proberesp_ecsa_stuck
        +u8 bssid_index
        +u8 max_bssid_indicator
    }
    note for cfg80211_bss "Added proberesp_ecsa_stuck field"
Loading

File-Level Changes

Change Details Files
Implements a mechanism to detect and ignore 'stuck' Extended Channel Switch Announcement (ECSA) elements in probe responses.
  • Adds a proberesp_ecsa_stuck flag to the cfg80211_bss struct to indicate a stuck ECSA element.
  • Introduces cfg80211_check_stuck_ecsa to detect stuck ECSA elements by comparing consecutive probe responses.
  • Sets the proberesp_ecsa_stuck flag if the ECSA element remains unchanged for more than one second.
  • The driver will ignore the ECSA element if the proberesp_ecsa_stuck flag is set.
net/wireless/scan.c
include/net/cfg80211.h
Prevents the driver from attempting to connect to an AP while it is in the middle of a Channel Switch Announcement (CSA) or Extended Channel Switch Announcement (ECSA) process.
  • Introduces the ieee80211_mgd_csa_present function to check for the presence of CSA/ECSA elements in beacon or probe response IEs.
  • Introduces the ieee80211_mgd_csa_in_process function to determine if an AP is currently in a CSA process.
  • The driver will now refuse authentication or association with an AP if it detects that the AP is in a CSA process.
net/mac80211/mlme.c

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @opsiff - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a comment explaining why ieee80211_mgd_csa_present ignores ECSA elements when ignore_ecsa is true.
  • It might be helpful to add a comment to cfg80211_check_stuck_ecsa explaining why the function returns early if known->pub.proberesp_ecsa_stuck is already set.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

return err;
}

static bool ieee80211_mgd_csa_present(struct ieee80211_sub_if_data *sdata,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider refactoring the CSA/ECSA validation checks into a helper function to reduce code duplication.

Consider removing duplicate CSA/ECSA validation code by refactoring the common checks into a helper function. For example, you could create a helper that validates an element’s data and returns whether that element should be ignored. Then call that helper for both CSA and ECSA. For instance:

---
static bool validate_csa_element(const struct element *elem, size_t expected_len,
				 u8 cur_channel)
{
	void *data;
	/* Check element exists and is the expected length */
	if (!elem || elem->datalen != expected_len)
		return false;

	/* Common parsing and validation for CSA element */
	data = (void *)elem->data;
	/* Replace `struct ieee80211_channel_sw_ie` with appropriate type for this element */
	if (((struct ieee80211_channel_sw_ie *)data)->count == 0 ||
	    (!((struct ieee80211_channel_sw_ie *)data)->mode &&
	     ((struct ieee80211_channel_sw_ie *)data)->new_ch_num == cur_channel))
		return false;

	return true;
}
---

You can then simplify `ieee80211_mgd_csa_present()` as follows (similarly for the ECSA path, using the correct type and expected length):

---
static bool ieee80211_mgd_csa_present(struct ieee80211_sub_if_data *sdata,
				      const struct cfg80211_bss_ies *ies,
				      u8 cur_channel, bool ignore_ecsa)
{
	bool has_csa = false, has_ecsa = false;
	const struct element *csa_elem, *ecsa_elem;

	if (!ies)
		return false;

	csa_elem = cfg80211_find_elem(WLAN_EID_CHANNEL_SWITCH, ies->data, ies->len);
	has_csa = validate_csa_element(csa_elem, sizeof(struct ieee80211_channel_sw_ie), cur_channel);

	ecsa_elem = cfg80211_find_elem(WLAN_EID_EXT_CHANSWITCH_ANN, ies->data, ies->len);
	has_ecsa = validate_csa_element(ecsa_elem, sizeof(struct ieee80211_ext_chansw_ie), cur_channel);

	if (ignore_ecsa && has_ecsa) {
		sdata_info(sdata,
			   "Ignoring ECSA in probe response - was considered stuck!\n");
		return has_csa;
	}

	return has_csa || has_ecsa;
}
---

Additionally, in `ieee80211_mgd_csa_in_process()`, you can combine the conditions without using goto:

---
static bool ieee80211_mgd_csa_in_process(struct ieee80211_sub_if_data *sdata,
					 struct cfg80211_bss *bss)
{
	u8 cur_channel = ieee80211_frequency_to_channel(bss->channel->center_freq);
	bool in_process;

	rcu_read_lock();
	in_process = ieee80211_mgd_csa_present(sdata, rcu_dereference(bss->beacon_ies),
						 cur_channel, false) ||
		     ieee80211_mgd_csa_present(sdata, rcu_dereference(bss->proberesp_ies),
						 cur_channel, bss->proberesp_ecsa_stuck);
	rcu_read_unlock();
	return in_process;
}
---

By factoring out the repetitive logic and avoiding goto in favor of clear boolean expressions, the complexity is reduced while the functionality remains intact.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sourcery-ai[bot]
Once this PR has been reviewed and has the lgtm label, please ask for approval from opsiff. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mainline inclusion
from mainline-v6.8-rc4
category: bugfix

We recently added some validation that we don't try to
connect to an AP that is currently in a channel switch
process, since that might want the channel to be quiet
or we might not be able to connect in time to hear the
switching in a beacon. This was in commit c09c4f3
("wifi: mac80211: don't connect to an AP while it's in
a CSA process").

However, we promptly got a report that this caused new
connection failures, and it turns out that the AP that
we now cannot connect to is permanently advertising an
extended channel switch announcement, even with quiet.
The AP in question was an Asus RT-AC53, with firmware
3.0.0.4.380_10760-g21a5898.

As a first step, attempt to detect that we're dealing
with such a situation, so mac80211 can use this later.

Reported-by: coldolt <andypalmadi@gmail.com>
Closes: https://lore.kernel.org/linux-wireless/CAJvGw+DQhBk_mHXeu6RTOds5iramMW2FbMB01VbKRA4YbHHDTA@mail.gmail.com/
Fixes: c09c4f3 ("wifi: mac80211: don't connect to an AP while it's in a CSA process")
Reviewed-by: Miriam Rachel Korenblit <miriam.rachel.korenblit@intel.com>
Link: https://msgid.link/20240129131413.246972c8775e.Ibf834d7f52f9951a353b6872383da710a7358338@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
(cherry picked from commit 177fbbc)
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
mainline inclusion
from mainline-v6.8-rc4
category: bugfix

As mentioned in the previous commit, we pretty quickly found
that some APs have ECSA elements stuck in their probe response,
so using that to not attempt to connect while CSA is happening
we never connect to such an AP.

Improve this situation by checking more carefully and ignoring
the ECSA if cfg80211 has previously detected the ECSA element
being stuck in the probe response.

Additionally, allow connecting to an AP that's switching to a
channel it's already using, unless it's using quiet mode. In
this case, we may just have to adjust bandwidth later. If it's
actually switching channels, it's better not to try to connect
in the middle of that.

Reported-by: coldolt <andypalmadi@gmail.com>
Closes: https://lore.kernel.org/linux-wireless/CAJvGw+DQhBk_mHXeu6RTOds5iramMW2FbMB01VbKRA4YbHHDTA@mail.gmail.com/
Fixes: c09c4f3 ("wifi: mac80211: don't connect to an AP while it's in a CSA process")
Reviewed-by: Miriam Rachel Korenblit <miriam.rachel.korenblit@intel.com>
Link: https://msgid.link/20240129131413.cc2d0a26226e.I682c016af76e35b6c47007db50e8554c5a426910@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
(cherry picked from commit 35e2385)
Signed-off-by: Wentao Guan <guanwentao@uniontech.com>
@opsiff opsiff force-pushed the linux-6.6.y-2025-02-19-mac80211 branch from 24248c4 to 9cc899a Compare February 18, 2025 17:08
@opsiff
Copy link
Member Author

opsiff commented Feb 19, 2025

/ok-to-test

@opsiff opsiff merged commit af775d4 into deepin-community:linux-6.6.y Feb 19, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants