diff mbox series

wifi: mac80211: validate link status before deciding on off channel Tx

Message ID 20240312154620.242773-1-quic_adisi@quicinc.com
State New
Headers show
Series wifi: mac80211: validate link status before deciding on off channel Tx | expand

Commit Message

Aditya Kumar Singh March 12, 2024, 3:46 p.m. UTC 
From: Sriram R <quic_srirrama@quicinc.com>

Currently we check the status of bss active flag to see if the AP is
active. But in case of an AP MLD, when some of the links are getting
teardown and some are active, management Tx (like deauthentication) can
be sent on some links before they are brought down as well.

In such cases, the bss active flag might not provide the exact
status of the MLD links. Hence check if any of the links can
be used for management Tx before returning error status.

While at it, when source address is same as the link conf's address and
if userspace requested Tx on a specific link, add changes to use the same
link id if the link bss is matching the requested channel.

Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com>
---
 net/mac80211/offchannel.c | 46 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)


base-commit: c2b25092864a16c7865e406badedece5cc25fc2b

Comments

Johannes Berg March 25, 2024, 3:21 p.m. UTC | #1 
On Tue, 2024-03-12 at 21:16 +0530, Aditya Kumar Singh wrote:

> In such cases, the bss active flag might not provide the exact
> status of the MLD links.

That's ... true I guess, but then I'm suspicious, why are you sending
this patch? Does that mean 'active' is managed incorrectly and actually
becomes false when one link is removed, rather than all? Can you fix
that too? And if you fix that ... yeah we probably still should have
this patch but ... _without_ this:

> +	/* This is consolidated status of the MLD or non ML bss */
> +	if (sdata->bss->active)
> +		return true;

I'd think?

> While at it, when source address is same as the link conf's address and
> if userspace requested Tx on a specific link, add changes to use the same
> link id if the link bss is matching the requested channel.

Why not separate that? It's really not related much?

> +	if (link_id < 0)
> +		return false;
> +
> +	if (!sdata->vif.valid_links)
> +		return false;
> +
> +	if (!(sdata->vif.valid_links & BIT(link_id)))
> +		return false;

The second condition seems useless then?

But probably better to check *active* links, and then might as well use
ieee80211_vif_link_active()?

> +	link = sdata_dereference(sdata->link[link_id], sdata);
> +	if (!link)
> +		return false;

That might be a WARN_ON()? After all, if links are valid (or active per
above) that really should be there?

>  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
>  {
> @@ -817,7 +850,7 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  	case NL80211_IFTYPE_P2P_GO:
>  		if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
>  		    !ieee80211_vif_is_mesh(&sdata->vif) &&
> -		    !sdata->bss->active)
> +		    !ieee80211_is_link_bss_active(sdata, params->link_id))
>  			need_offchan = true;
>  
>  		rcu_read_lock();
> @@ -897,8 +930,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>  				break;
>  			}
>  
> -			if (ether_addr_equal(conf->addr, mgmt->sa))
> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
> +				/* If userspace requested Tx on a specific link
> +				 * use the same link id if the link bss is matching
> +				 * the requested chan.
> +				 */
> +				if (sdata->vif.valid_links &&
> +				    params->link_id >= 0 && params->link_id == i &&
> +				    params->chan == chanctx_conf->def.chan)
> +					link_id = i;
>  				break;
> +			}
>  
>  			chanctx_conf = NULL;
>  		}
> 
> base-commit: c2b25092864a16c7865e406badedece5cc25fc2b
Aditya Kumar Singh March 26, 2024, 4:28 a.m. UTC | #2 
On 3/25/24 20:51, Johannes Berg wrote:
> On Tue, 2024-03-12 at 21:16 +0530, Aditya Kumar Singh wrote:
> 
>> In such cases, the bss active flag might not provide the exact
>> status of the MLD links.
> 
> That's ... true I guess, but then I'm suspicious, why are you sending
> this patch? Does that mean 'active' is managed incorrectly and actually
> becomes false when one link is removed, rather than all?

Yes I believe. I would say it is managing the pre-MLO time data 
structures. As of today, the active flag is set at assign beacon. But 
since it is maintained at sdata level, as soon as first link is assigned 
beacon, the flag would be set. Similarly, at stop_ap() it is set to 
false, so as soon as first link is brought down, it is set to false.

Hence checking that sdata level flag during MLO scenario might be 
misleading.

> Can you fix
> that too? And if you fix that ... yeah we probably still should have
> this patch but ... _without_ this:
> 

Sure let me try to fix that as well. So here's what Im planning -
1. Separate the ether_addr changes into a separate independent patch.
2. Patch series to fix the active flag handling at link level.


>> +	/* This is consolidated status of the MLD or non ML bss */
>> +	if (sdata->bss->active)
>> +		return true;
> 
> I'd think?

Yes :)

> 
>> While at it, when source address is same as the link conf's address and
>> if userspace requested Tx on a specific link, add changes to use the same
>> link id if the link bss is matching the requested channel.
> 
> Why not separate that? It's really not related much?

Yeah will put this in separate patch. Thanks.

> 
>> +	if (link_id < 0)
>> +		return false;
>> +
>> +	if (!sdata->vif.valid_links)
>> +		return false;
>> +
>> +	if (!(sdata->vif.valid_links & BIT(link_id)))
>> +		return false;
> 
> The second condition seems useless then?

Yeah. I would say "if (!sdata->vif.valid_links)" this check can be 
removed. Will remove it.
> But probably better to check *active* links, and then might as well use
> ieee80211_vif_link_active()?

Sure, I will see what I can do here. Thanks for the suggestion.

> 
>> +	link = sdata_dereference(sdata->link[link_id], sdata);
>> +	if (!link)
>> +		return false;
> 
> That might be a WARN_ON()? After all, if links are valid (or active per
> above) that really should be there?
> 

Sure, will do.

>>   int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
>>   {
>> @@ -817,7 +850,7 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   	case NL80211_IFTYPE_P2P_GO:
>>   		if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
>>   		    !ieee80211_vif_is_mesh(&sdata->vif) &&
>> -		    !sdata->bss->active)
>> +		    !ieee80211_is_link_bss_active(sdata, params->link_id))
>>   			need_offchan = true;
>>   
>>   		rcu_read_lock();
>> @@ -897,8 +930,17 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>   				break;
>>   			}
>>   
>> -			if (ether_addr_equal(conf->addr, mgmt->sa))
>> +			if (ether_addr_equal(conf->addr, mgmt->sa)) {
>> +				/* If userspace requested Tx on a specific link
>> +				 * use the same link id if the link bss is matching
>> +				 * the requested chan.
>> +				 */
>> +				if (sdata->vif.valid_links &&
>> +				    params->link_id >= 0 && params->link_id == i &&
>> +				    params->chan == chanctx_conf->def.chan)
>> +					link_id = i;
>>   				break;
>> +			}
>>   
>>   			chanctx_conf = NULL;
>>   		}
>>
>> base-commit: c2b25092864a16c7865e406badedece5cc25fc2b
>
Aditya Kumar Singh March 26, 2024, 5:57 a.m. UTC | #3 
On 3/26/24 09:58, Aditya Kumar Singh wrote:
>> Can you fix
>> that too? And if you fix that ... yeah we probably still should have
>> this patch but ... _without_ this:
>>
> 
> Sure let me try to fix that as well. So here's what Im planning -
> 1. Separate the ether_addr changes into a separate independent patch.
> 2. Patch series to fix the active flag handling at link level.

Upon checking further, I see -

If we fix the setting of the flag only when first link comes up and 
reset it only when last link is removed, then probably there is no need 
to add separate handler - ieee80211_is_link_bss_active() to check if 
any one link is active or not.

FWIW, the purpose of the new function introduced is to check if at least 
one of the link is active. And now if the flag is set, this ultimately 
means that one link is at least active. So we do not need to go and 
check in each link again right?
Johannes Berg March 26, 2024, 7:42 a.m. UTC | #4 
On Tue, 2024-03-26 at 11:27 +0530, Aditya Kumar Singh wrote:
> On 3/26/24 09:58, Aditya Kumar Singh wrote:
> > > Can you fix
> > > that too? And if you fix that ... yeah we probably still should have
> > > this patch but ... _without_ this:
> > > 
> > 
> > Sure let me try to fix that as well. So here's what Im planning -
> > 1. Separate the ether_addr changes into a separate independent patch.
> > 2. Patch series to fix the active flag handling at link level.
> 
> Upon checking further, I see -
> 
> If we fix the setting of the flag only when first link comes up and 
> reset it only when last link is removed, then probably there is no need 
> to add separate handler - ieee80211_is_link_bss_active() to check if 
> any one link is active or not.
> 
> FWIW, the purpose of the new function introduced is to check if at least 
> one of the link is active. And now if the flag is set, this ultimately 
> means that one link is at least active. So we do not need to go and 
> check in each link again right?
> 
Yes, which is why I even noticed the whole mess with 'active'.

johannes
Aditya Kumar Singh March 26, 2024, 8:38 a.m. UTC | #5 
On 3/26/24 13:12, Johannes Berg wrote:
> On Tue, 2024-03-26 at 11:27 +0530, Aditya Kumar Singh wrote:
>> On 3/26/24 09:58, Aditya Kumar Singh wrote:
>>>> Can you fix
>>>> that too? And if you fix that ... yeah we probably still should have
>>>> this patch but ... _without_ this:
>>>>
>>>
>>> Sure let me try to fix that as well. So here's what Im planning -
>>> 1. Separate the ether_addr changes into a separate independent patch.
>>> 2. Patch series to fix the active flag handling at link level.
>>
>> Upon checking further, I see -
>>
>> If we fix the setting of the flag only when first link comes up and
>> reset it only when last link is removed, then probably there is no need
>> to add separate handler - ieee80211_is_link_bss_active() to check if
>> any one link is active or not.
>>
>> FWIW, the purpose of the new function introduced is to check if at least
>> one of the link is active. And now if the flag is set, this ultimately
>> means that one link is at least active. So we do not need to go and
>> check in each link again right?
>>
> Yes, which is why I even noticed the whole mess with 'active'.
> 
> johannes

Sure, then I will just try to fix the sdata->bss->active flag usage and 
send a patch soon.
diff mbox series

Patch

diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index 221695d841fd..83f6229fe33b 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -774,6 +774,39 @@  int ieee80211_cancel_remain_on_channel(struct wiphy *wiphy,
 	return ieee80211_cancel_roc(local, cookie, false);
 }
 
+static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data *sdata,
+					 int link_id)
+{
+	struct ieee80211_link_data *link;
+
+	lockdep_assert_wiphy(sdata->local->hw.wiphy);
+
+	if (!sdata->bss)
+		return false;
+
+	/* This is consolidated status of the MLD or non ML bss */
+	if (sdata->bss->active)
+		return true;
+
+	if (link_id < 0)
+		return false;
+
+	if (!sdata->vif.valid_links)
+		return false;
+
+	if (!(sdata->vif.valid_links & BIT(link_id)))
+		return false;
+
+	link = sdata_dereference(sdata->link[link_id], sdata);
+	if (!link)
+		return false;
+
+	if (sdata_dereference(link->u.ap.beacon, sdata))
+		return true;
+
+	return false;
+}
+
 int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 		      struct cfg80211_mgmt_tx_params *params, u64 *cookie)
 {
@@ -817,7 +850,7 @@  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 	case NL80211_IFTYPE_P2P_GO:
 		if (sdata->vif.type != NL80211_IFTYPE_ADHOC &&
 		    !ieee80211_vif_is_mesh(&sdata->vif) &&
-		    !sdata->bss->active)
+		    !ieee80211_is_link_bss_active(sdata, params->link_id))
 			need_offchan = true;
 
 		rcu_read_lock();
@@ -897,8 +930,17 @@  int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 				break;
 			}
 
-			if (ether_addr_equal(conf->addr, mgmt->sa))
+			if (ether_addr_equal(conf->addr, mgmt->sa)) {
+				/* If userspace requested Tx on a specific link
+				 * use the same link id if the link bss is matching
+				 * the requested chan.
+				 */
+				if (sdata->vif.valid_links &&
+				    params->link_id >= 0 && params->link_id == i &&
+				    params->chan == chanctx_conf->def.chan)
+					link_id = i;
 				break;
+			}
 
 			chanctx_conf = NULL;
 		}