>-----Original Message-----
>From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
>Sent: Tuesday, February 14, 2023 6:16 PM
>To: Sriram R (QUIC) <quic_srirrama@xxxxxxxxxxx>
>Cc: linux-wireless@xxxxxxxxxxxxxxx
>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
