Re: [PATCH v2] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Zhongqiu Han <quic_zhonhan@quicinc.com>]

On 4/23/2025 11:26 PM, Johannes Berg wrote:
> On Fri, 2025-03-14 at 20:06 +0800, Zhongqiu Han wrote:
>>
>> +++ b/net/mac80211/mlme.c
>> @@ -4433,6 +4433,10 @@ static void ieee80211_report_disconnect(struct ieee80211_sub_if_data *sdata,
>>   		.u.mlme.data = tx ? DEAUTH_TX_EVENT : DEAUTH_RX_EVENT,
>>   		.u.mlme.reason = reason,
>>   	};
>> +	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
>> +
>> +	if (WARN_ON(!ap_sta))
>> +		return;
> 
> You're adding a WARN_ON() that's now guaranteed to trigger, no?
> Shouldn't the caller (also) be fixed?

Thanks Johannes for the review~

yes, the caller can first check if ap_sta is null, thereby preventing
further calls to ieee80211_set_disassoc and ieee80211_report_disconnect,
maybe just like the condition check for ifmgd->associated in the example
below:

static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
				   u16 stype, u16 reason, bool tx,
				   u8 *frame_buf)
{
	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
	struct ieee80211_local *local = sdata->local;
	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
	unsigned int link_id;
	u64 changed = 0;
	struct ieee80211_prep_tx_info info = {
		.subtype = stype,
		.was_assoc = true,
		.link_id = ffs(sdata->vif.active_links) - 1,
	};

	lockdep_assert_wiphy(local->hw.wiphy);

	if (WARN_ON(!ap_sta))--------> introduced since 687a7c8a7227
		return;

	if (WARN_ON_ONCE(tx && !frame_buf))
		return;

	if (WARN_ON(!ifmgd->associated))
		return;
...
...
}

---example 1.
----------------------------------------------------------------
void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata)
{
	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;

	lockdep_assert_wiphy(sdata->local->hw.wiphy);

	if (!ifmgd->associated)
		return;

	if (sdata->flags & IEEE80211_SDATA_DISCONNECT_RESUME) {
		sdata->flags &= ~IEEE80211_SDATA_DISCONNECT_RESUME;
		mlme_dbg(sdata, "driver requested disconnect after resume\n");
		ieee80211_sta_connection_lost(sdata,
					      WLAN_REASON_UNSPECIFIED,
					      true);
		return;
	}

	if (sdata->flags & IEEE80211_SDATA_DISCONNECT_HW_RESTART) {
		sdata->flags &= ~IEEE80211_SDATA_DISCONNECT_HW_RESTART;
		mlme_dbg(sdata, "driver requested disconnect after hardware restart\n");
		ieee80211_sta_connection_lost(sdata,
					      WLAN_REASON_UNSPECIFIED,
					      true);
		return;
	}
}
----------------------------------------------------------------------


---example 2.
----------------------------------------------------------------------
static void ieee80211_rx_mgmt_deauth(struct ieee80211_sub_if_data 
*sdata, struct ieee80211_mgmt *mgmt, size_t len)
{
	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
	u16 reason_code = le16_to_cpu(mgmt->u.deauth.reason_code);

	lockdep_assert_wiphy(sdata->local->hw.wiphy);

	if (len < 24 + 2)
		return;

	if (!ether_addr_equal(mgmt->bssid, mgmt->sa)) {
		ieee80211_tdls_handle_disconnect(sdata, mgmt->sa, reason_code);
		return;
	}

	if (ifmgd->associated &&
	    ether_addr_equal(mgmt->bssid, sdata->vif.cfg.ap_addr)) {
		sdata_info(sdata, "deauthenticated from %pM (Reason: %u=%s)\n", 
sdata->vif.cfg.ap_addr, reason_code,
			   ieee80211_get_reason_code_string(reason_code));

		ieee80211_set_disassoc(sdata, 0, 0, false, NULL);

		ieee80211_report_disconnect(sdata, (u8 *)mgmt, len, false,
					    reason_code, false);
		return;
	}
...
...
...
}
---------------------------------------------------------------------


However, sorry I'm not sure if !ifmgd->associated and !ap_sta have
exactly the same context and treatment, especially in one situations
like the below example3:


---example3.
-----------------------------------------------------------------------
static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
{
	struct ieee80211_local *local = sdata->local;
	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];

	lockdep_assert_wiphy(local->hw.wiphy);

	if (!ifmgd->associated)
		return;

	if (!ifmgd->driver_disconnect) {
		unsigned int link_id;

		/*
		 * AP is probably out of range (or not reachable for another
		 * reason) so remove the bss structs for that AP. In the case
		 * of multi-link, it's not clear that all of them really are
		 * out of range, but if they weren't the driver likely would
		 * have switched to just have a single link active?
		 */
		for (link_id = 0;
		     link_id < ARRAY_SIZE(sdata->link);
		     link_id++) {
			struct ieee80211_link_data *link;

			link = sdata_dereference(sdata->link[link_id], sdata);
			if (!link || !link->conf->bss)
				continue;
			cfg80211_unlink_bss(local->hw.wiphy, link->conf->bss);
			link->conf->bss = NULL;
		}
	}

	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
			       ifmgd->driver_disconnect ?
					WLAN_REASON_DEAUTH_LEAVING :
					WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
			       true, frame_buf);
	/* the other links will be destroyed */
	sdata->vif.bss_conf.csa_active = false;
	sdata->deflink.u.mgd.csa.waiting_bcn = false;
	sdata->deflink.u.mgd.csa.blocked_tx = false;
	ieee80211_vif_unblock_queues_csa(sdata);

	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
				    WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
				    ifmgd->reconnect);
	ifmgd->reconnect = false;
}
------------------------------------------------------------------------

My next step plan is change the code to as below:
For example1 and example2 pattern, I want add more check about ap_sta
such as:
---------------------------------------------------------------------
	if (ifmgd->associated &&
	    ether_addr_equal(mgmt->bssid, sdata->vif.cfg.ap_addr && ap_sta)) {
---------------------------------------------------------------------
becasue if this condition meets, the if condition {} only execute
ieee80211_set_disassoc and ieee80211_report_disconnect.


For example3 pattern, i want to add check before the call of
ieee80211_report_disconnect, but it is indeed not perfect.
Thanks for your time and discussion~


> 
>> @@ -8090,7 +8094,7 @@ static void ieee80211_sta_timer(struct timer_list *t)
>>   void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata,
>>   				   u8 reason, bool tx)
>>   {
>> -	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
>> +	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN] = {0};
>>
> 
> And that's not needed then? And perhaps should be {} if it is.
Yes, it is not needed even {}.
> 
> johannes


-- 
Thx and BRs,
Zhongqiu Han