| Message ID | 20230119221306.24526-1-quic_srirrama@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: mac80211: validate link status before deciding on mgmt tx | expand |
On Fri, 2023-01-20 at 03:43 +0530, Sriram R wrote: > Currently we check the status of bss active flag to see if the > AP is active. > Following so far :) > But in case of a MLD AP, when some of the links > are getting teardown "getting torn down"? > and some are active, mgmt Tx(like deauth) > can be sent on some links before they are brought down as well. Makes sense. > In such cases, the bss active flag might not provide the exact > status of the MLD links, which becomes false on first link deleted. Wait, isn't that already a bug? > Hence check if any of the links can be used for mgmt tx > before returning error status. > > Also, use the link id passed from userspace when the link bss > address matches the mgmt SA and the chan params match the request. > This will avoid scenario where the link id from userspace > gets reset. "gets reset"?? > > +static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data *sdata, > + int link_id) > +{ [...] > + sdata_lock(sdata); > + link = sdata_dereference(sdata->link[link_id], sdata); > + if (!link) { > + sdata_unlock(sdata); > + return false; > + } > + > + if (sdata_dereference(link->u.ap.beacon, sdata)) { > + sdata_unlock(sdata); > + return true; > + } > + > + sdata_unlock(sdata); The locking here is ... decidedly odd. It feels like with all the wdev_lock()ing going on in cfg80211_mlme_mgmt_tx() we should probably just lock around the *entire* thing in cfg80211, including the driver/mac80211 call? > @@ -883,8 +920,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; > + } Not sure I get it, if it's bad (link ID doesn't match BSS) then shouldn't we just reject it? johannes
>-----Original Message----- >From: Johannes Berg <johannes@sipsolutions.net> >Sent: Tuesday, February 14, 2023 6:16 PM >To: Sriram R (QUIC) <quic_srirrama@quicinc.com> >Cc: linux-wireless@vger.kernel.org >Subject: Re: [PATCH v2] wifi: mac80211: validate link status before deciding on >mgmt tx > >On Fri, 2023-01-20 at 03:43 +0530, Sriram R wrote: >> Currently we check the status of bss active flag to see if the AP is >> active. >> > >Following so far :) > >> But in case of a MLD AP, when some of the links are getting teardown > >"getting torn down"? Right, I'll update in the next revision. > >> and some are active, mgmt Tx(like deauth) can be sent on some links >> before they are brought down as well. > >Makes sense. > >> In such cases, the bss active flag might not provide the exact status >> of the MLD links, which becomes false on first link deleted. > >Wait, isn't that already a bug? This commit ("commit bfd8403adddd09f32033a14bf25be398291e7881") introduced this flag for whole AP(MLD) and replaced beacon since it is link specific. Hence I thought It was brought in to represent a MLD whereas the beacon ptr can be checked if a certain link is active. Hence used both bss->active and beacon checks to see if atleast one link is active. > >> Hence check if any of the links can be used for mgmt tx before >> returning error status. >> >> Also, use the link id passed from userspace when the link bss address >> matches the mgmt SA and the chan params match the request. >> This will avoid scenario where the link id from userspace gets reset. > >"gets reset"?? In ieee80211_mgmt_tx() the link id which was passed from userspace is Ignored if its not an action frame. Hence the below change was done to check if one of the Link bss matches with the link id passed Also, in __ieee80211_tx_skb_tid_band() the below elseif condition might not be appropriate in case the MLD address and one of the link address is same. } else if (memcmp(sdata->vif.addr, hdr->addr2, ETH_ALEN) == 0) { /* address from the MLD */ link = IEEE80211_LINK_UNSPECIFIED; } else { Hence wanted to fix in the ieee80211_mgmt_tx() itself to use the proper link id passed from userspace and avoid getting reset to -1. > >> >> +static bool ieee80211_is_link_bss_active(struct ieee80211_sub_if_data >*sdata, >> + int link_id) >> +{ >[...] >> + sdata_lock(sdata); >> + link = sdata_dereference(sdata->link[link_id], sdata); >> + if (!link) { >> + sdata_unlock(sdata); >> + return false; >> + } >> + >> + if (sdata_dereference(link->u.ap.beacon, sdata)) { >> + sdata_unlock(sdata); >> + return true; >> + } >> + >> + sdata_unlock(sdata); > >The locking here is ... decidedly odd. It feels like with all the wdev_lock()ing >going on in cfg80211_mlme_mgmt_tx() we should probably just lock around >the *entire* thing in cfg80211, including the >driver/mac80211 call? Sure, let me revisit this in the next version. > >> @@ -883,8 +920,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; >> + } > > >Not sure I get it, if it's bad (link ID doesn't match BSS) then shouldn't we just >reject it? As mentioned above the link id gets set to -1 since the switch case for NL80211_IFTYPE_AP sets the link id from params->link_id only when it’s an action frame. We might have to honor the link id passed in params->link_id for any management frame , right?, so that the address translation is done properly in driver for all mgmt. frames Please let me know if something is misunderstood here. Thanks, Sriram.R
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c index d78c82d6b696..5e312860a976 100644 --- a/net/mac80211/offchannel.c +++ b/net/mac80211/offchannel.c @@ -763,6 +763,43 @@ 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; + + 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; + + sdata_lock(sdata); + link = sdata_dereference(sdata->link[link_id], sdata); + if (!link) { + sdata_unlock(sdata); + return false; + } + + if (sdata_dereference(link->u.ap.beacon, sdata)) { + sdata_unlock(sdata); + return true; + } + + sdata_unlock(sdata); + return false; +} + int ieee80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params, u64 *cookie) { @@ -804,7 +841,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(); @@ -883,8 +920,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; }
Currently we check the status of bss active flag to see if the AP is active. But in case of a MLD AP, when some of the links are getting teardown and some are active, mgmt Tx(like deauth) 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, which becomes false on first link deleted. Hence check if any of the links can be used for mgmt tx before returning error status. Also, use the link id passed from userspace when the link bss address matches the mgmt SA and the chan params match the request. This will avoid scenario where the link id from userspace gets reset. Signed-off-by: Sriram R <quic_srirrama@quicinc.com> --- v2: added wifi prefix in commit title net/mac80211/offchannel.c | 50 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)