| Message ID | 20240125125855.827619-2-quic_adisi@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: cfg80211/mac80211: add support to flush stations based on link ID | expand |
On Thu, 2024-01-25 at 18:28 +0530, Aditya Kumar Singh wrote: > Currently whenever NL80211_CMD_DEL_STATION command is called without any > MAC address, all stations present on that interface are flushed. True. > However with MLO there is a need to flush the stations from a particular > link in the interface, and not from all the links associated with the MLD > interface. Fair enough, I can get behind that. Edit: reading the code - I think I misunderstand that ... you're actually trying to remove all MLDs ("STATION") that have an active link on this link? So then maybe disregard all the below, and just write a better commit message? But I'll leave the below because I'm not really sure what you're trying to do here. > For example - 2 GHz and 5 GHz are part of an AP MLD. When 2 GHz BSS is > brought up, it sends flush command on the interface (MLD). Then eventually > 5 GHZ links comes up and that also sends the command on the same interface. > Now by the time 5 GHz link comes up, if any station gets connected to 2 GHz > link, it would be flushed while 5 GHz link is started which is wrong. Right. Though in this case - after bringup - you wouldn't really have to flush anyway, so it could just not do that, I guess? Feels a bit like a broken flow which is a bad justification, but I do understand there's justification for this. > Hence, add an option to pass link ID as well in the command so that if link > ID is passed, station using that passed link ID alone would be deleted > and others will not be removed. So first: Do you want some feature flag that indicates this? Or will we just eat the cost of kicking out everyone (without even sending deauth though, I think?) when running on older kernels? Secondly: why is this part of NL80211_CMD_DEL_STATION? I'm not convinced that makes sense. I actually kind of get why you're doing that - it's easier to retrofit into the existing hostapd, but I don't necessarily think that the hostap design (problems?) should influence this too much. IOW, it would feel much more appropriate to have this as part of NL80211_CMD_REMOVE_LINK_STA? After all, when going to MLD then "STATION" now represents a "peer MLD", and "LINK_STA" now represents an affiliated STA. And flushing all affiliated STAs is what you want. So I think it should be NL80211_CMD_REMOVE_LINK_STA without a NL80211_ATTR_MLD_ADDR. > A subsequent patch would add logic to delete only the station using the > passed link ID. Not sure I'd say that here - I mean, (1) yeah obviously, otherwise we won't apply this patch? and (2) it's not related to cfg80211. > case NL80211_IFTYPE_MESH_POINT: > @@ -7675,6 +7677,17 @@ static int nl80211_del_station(struct sk_buff *skb, struct genl_info *info) > params.reason_code = WLAN_REASON_PREV_AUTH_NOT_VALID; > } > > + /* Link ID not expected in case of non-ML operation */ > + if (!wdev->valid_links && link_id != -1) > + return -EINVAL; > + > + /* If given, a valid link ID should be passed during MLO */ > + if (wdev->valid_links && link_id >= 0 && > + !(wdev->valid_links & BIT(link_id))) > + return -EINVAL; Maybe refactor this with the NL80211_FLAG_MLO_VALID_LINK_ID checks? > @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = { > .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, > .doit = nl80211_del_station, > .flags = GENL_UNS_ADMIN_PERM, > + /* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on > + * MAC address > + */ > .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), Hmm? How does NL80211_FLAG_MLO_VALID_LINK_ID depend on the MAC address?! It ... doesn't? johannes
On 1/26/24 14:36, Johannes Berg wrote: > On Thu, 2024-01-25 at 18:28 +0530, Aditya Kumar Singh wrote: >> Currently whenever NL80211_CMD_DEL_STATION command is called without any >> MAC address, all stations present on that interface are flushed. > > True. > >> However with MLO there is a need to flush the stations from a particular >> link in the interface, and not from all the links associated with the MLD >> interface. > > Fair enough, I can get behind that. > > Edit: reading the code - I think I misunderstand that ... you're > actually trying to remove all MLDs ("STATION") that have an active link > on this link? Yes correct. The station might not be MLD station. It could be a legacy station (non EHT) as well. > So then maybe disregard all the below, and just write a > better commit message? > But I'll leave the below because I'm not really sure what you're trying > to do here. > Sure. > >> For example - 2 GHz and 5 GHz are part of an AP MLD. When 2 GHz BSS is >> brought up, it sends flush command on the interface (MLD). Then eventually >> 5 GHZ links comes up and that also sends the command on the same interface. >> Now by the time 5 GHz link comes up, if any station gets connected to 2 GHz >> link, it would be flushed while 5 GHz link is started which is wrong. > > Right. Though in this case - after bringup - you wouldn't really have to > flush anyway, so it could just not do that, I guess? Feels a bit like a > broken flow which is a bad justification, but I do understand there's > justification for this. > Correct, for the first bring up not required but one use case I see is - the hostapd interface was disabled for some reason. While going down, it would have cleared the stations on the kernel but what if for some reason kernel did not clear the station entries and there are some stale entries present? So at next bring up (during enable) it would send the command without any MAC address to flush all stale entries (probably as a safety so that kernel and hostapd would now be on par). >> Hence, add an option to pass link ID as well in the command so that if link >> ID is passed, station using that passed link ID alone would be deleted >> and others will not be removed. > > So first: Do you want some feature flag that indicates this? Or will we > just eat the cost of kicking out everyone (without even sending deauth > though, I think?) when running on older kernels? > If what I said above was the actual intention, then kicking out everyone without even sending deauth makes sense? Yes? If yes then we don't need a feature flag. > > Secondly: why is this part of NL80211_CMD_DEL_STATION? I'm not convinced > that makes sense. I actually kind of get why you're doing that - it's > easier to retrofit into the existing hostapd, but I don't necessarily > think that the hostap design (problems?) should influence this too much. > > IOW, it would feel much more appropriate to have this as part of > NL80211_CMD_REMOVE_LINK_STA? After all, when going to MLD then "STATION" > now represents a "peer MLD", and "LINK_STA" now represents an affiliated > STA. And flushing all affiliated STAs is what you want. > > So I think it should be NL80211_CMD_REMOVE_LINK_STA without a > NL80211_ATTR_MLD_ADDR. > At least as per the current way of NL80211_CMD_REMOVE_LINK_STA implementation, it did not made any sense to delete all link STAs if MLD_ADDR is not passed. So probably the command should be called as many times as there are active links in the STA? Still I feel that NL80211_CMD_DEL_STATION is the proper place to put this? Without the current change also, it used to flush all STAs whenever MAC address is not passed. With MLO, now we need to flush STAs only if it is using the given link ID. So that link STAs from other affiliated links of AP would not be flushed. Scenario I'm targeting is this - Pre-MLO ---------------------------- sdata -> 2 GHz AP interface sta_lists -> 1. sta -> connected 2 GHz AP sdata 2. sta -> connected 2 GHz AP sdata After NL80211_CMD_DEL_STATION is given without any MAC address, sta_lists -> No entry(ies) With MLO ----------------------------- sdata -> link_data -> 2 GHz AP link (link ID 0) link_data -> 5 GHz AP link (link ID 1) link_data -> 6 GHz AP link (link ID 2) sta_lists -> 1. sta -> connected AP MLD sdata link_sta 0 -> connected to 2 GHz link 2. sta -> connected AP MLD sdata link_sta 1 -> connected to 5 GHz link 3. sta -> connected AP MLD sdata link_sta 2 -> connected to 6 GHz link 4. sta -> connected AP MLD sdata link_sta 0 -> connected to 2 GHz link link_sta 1 -> connected to 5 GHz link link_sta 2 -> connected to 6 GHz link Assume 5 GHz goes down and it gives NL80211_CMD_DEL_STATION without any MAC address, sta_lists -> No entry(ies) This is not desirable since 5 GHz link went down, why 2/6 GHz STA also got flushed. Hence with the proposed change, only sta #2 and #4 would be flushed since only these two are using passed link ID (which would be 1). Hence after the command, sta_lists -> 1. sta -> connected AP MLD sdata link_sta 0 -> connected to 2 GHz link 3. sta -> connected AP MLD sdata link_sta 2 -> connected to 6 GHz link Now, if ML re-config support is present, then hostapd (or the user space controller for that matters), could first issue NL80211_CMD_REMOVE_LINK_STA for the MLD STA (#4) and remove link sta with ID 1 from it. So that when NL80211_CMD_DEL_STATION comes, it would not remove the 2/6 GHz link STA as well from the MLD STA and hence flush the whole entry. The above change is not there yet in hostapd, so for the time being, whole MLD STA would be flushed. >> A subsequent patch would add logic to delete only the station using the >> passed link ID. > > Not sure I'd say that here - I mean, (1) yeah obviously, otherwise we > won't apply this patch? and (2) it's not related to cfg80211. Sure got it. > >> case NL80211_IFTYPE_MESH_POINT: >> @@ -7675,6 +7677,17 @@ static int nl80211_del_station(struct sk_buff *skb, struct genl_info *info) >> params.reason_code = WLAN_REASON_PREV_AUTH_NOT_VALID; >> } >> >> + /* Link ID not expected in case of non-ML operation */ >> + if (!wdev->valid_links && link_id != -1) >> + return -EINVAL; >> + >> + /* If given, a valid link ID should be passed during MLO */ >> + if (wdev->valid_links && link_id >= 0 && >> + !(wdev->valid_links & BIT(link_id))) >> + return -EINVAL; > > Maybe refactor this with the NL80211_FLAG_MLO_VALID_LINK_ID checks? See comment below - > >> @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = { >> .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, >> .doit = nl80211_del_station, >> .flags = GENL_UNS_ADMIN_PERM, >> + /* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on >> + * MAC address >> + */ >> .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), > > Hmm? How does NL80211_FLAG_MLO_VALID_LINK_ID depend on the MAC address?! > It ... doesn't? > I mean intention was that if MAC addresses is passed then no need of link ID. That is why did not add the valid link flag since it would expect the link ID even when MAC address is passed.
On Sat, 2024-01-27 at 11:14 +0530, Aditya Kumar Singh wrote: > On 1/26/24 14:36, Johannes Berg wrote: > > On Thu, 2024-01-25 at 18:28 +0530, Aditya Kumar Singh wrote: > > > Currently whenever NL80211_CMD_DEL_STATION command is called without any > > > MAC address, all stations present on that interface are flushed. > > > > True. > > > > > However with MLO there is a need to flush the stations from a particular > > > link in the interface, and not from all the links associated with the MLD > > > interface. > > > > Fair enough, I can get behind that. > > > > Edit: reading the code - I think I misunderstand that ... you're > > actually trying to remove all MLDs ("STATION") that have an active link > > on this link? > > Yes correct. The station might not be MLD station. It could be a legacy > station (non EHT) as well. We pretty much treat that the same though as an MLD station with a single link, with some caveats of translations, no? > Correct, for the first bring up not required but one use case I see is - > the hostapd interface was disabled for some reason. While going down, it > would have cleared the stations on the kernel but what if for some > reason kernel did not clear the station entries and there are some stale > entries present? So at next bring up (during enable) it would send the > command without any MAC address to flush all stale entries (probably as > a safety so that kernel and hostapd would now be on par). I don't think this really makes much sense. The kernel can't keep track of those stations properly if they're there, and anyway that'd be a (pretty massive!) kernel bug? Anyway, I think there probably _is_ justification for this (link removal?), I'm just not sure this bringup flow really is a good justification. > > > Hence, add an option to pass link ID as well in the command so that if link > > > ID is passed, station using that passed link ID alone would be deleted > > > and others will not be removed. > > > > So first: Do you want some feature flag that indicates this? Or will we > > just eat the cost of kicking out everyone (without even sending deauth > > though, I think?) when running on older kernels? > > > > If what I said above was the actual intention, then kicking out everyone > without even sending deauth makes sense? Yes? If yes then we don't need > a feature flag. Does it though? Even if you're talking about init, you could have init of one link much delayed for CSA, for example, with stations already connected on the other(s). > > Secondly: why is this part of NL80211_CMD_DEL_STATION? I'm not convinced > > that makes sense. I actually kind of get why you're doing that - it's > > easier to retrofit into the existing hostapd, but I don't necessarily > > think that the hostap design (problems?) should influence this too much. > > > > IOW, it would feel much more appropriate to have this as part of > > NL80211_CMD_REMOVE_LINK_STA? After all, when going to MLD then "STATION" > > now represents a "peer MLD", and "LINK_STA" now represents an affiliated > > STA. And flushing all affiliated STAs is what you want. > > > > So I think it should be NL80211_CMD_REMOVE_LINK_STA without a > > NL80211_ATTR_MLD_ADDR. > > > > At least as per the current way of NL80211_CMD_REMOVE_LINK_STA > implementation, it did not made any sense to delete all link STAs if > MLD_ADDR is not passed. So probably the command should be called as many > times as there are active links in the STA? Not sure I understand this, we're doing a kind of flush here, so you could (conceptually) say "flush all link STAs on link 5", no? And obviously stations that have no link left after this need to be removed completely. Note this raises an interesting point in mac80211, in that there's one link ('deflink', the link the STA used to assoc) that cannot be removed from an MLD station even. But again this comes down to what you actually _want_, I think, so I'll keep reading for now. > Still I feel that NL80211_CMD_DEL_STATION is the proper place to put > this? Without the current change also, it used to flush all STAs > whenever MAC address is not passed. With MLO, now we need to flush STAs > only if it is using the given link ID. So that link STAs from other > affiliated links of AP would not be flushed. Right so this is coming to the point where I wasn't sure earlier what you actually meant, and I'm still not entirely positive I've understood it. Let me read on ... > Scenario I'm targeting is this - > > Pre-MLO > ---------------------------- > > sdata -> 2 GHz AP interface > sta_lists -> > 1. sta -> connected 2 GHz AP sdata > 2. sta -> connected 2 GHz AP sdata > > After NL80211_CMD_DEL_STATION is given without any MAC address, > > sta_lists -> > No entry(ies) Right. > With MLO > ----------------------------- > sdata -> > link_data -> 2 GHz AP link (link ID 0) > link_data -> 5 GHz AP link (link ID 1) > link_data -> 6 GHz AP link (link ID 2) > sta_lists -> > 1. sta -> connected AP MLD sdata > link_sta 0 -> connected to 2 GHz link > 2. sta -> connected AP MLD sdata > link_sta 1 -> connected to 5 GHz link > 3. sta -> connected AP MLD sdata > link_sta 2 -> connected to 6 GHz link > 4. sta -> connected AP MLD sdata > link_sta 0 -> connected to 2 GHz link > link_sta 1 -> connected to 5 GHz link > link_sta 2 -> connected to 6 GHz link > > Assume 5 GHz goes down and it gives NL80211_CMD_DEL_STATION without any > MAC address, > > sta_lists -> > No entry(ies) > > This is not desirable since 5 GHz link went down, why 2/6 GHz STA also > got flushed. > > Hence with the proposed change, only sta #2 and #4 would be flushed > since only these two are using passed link ID (which would be 1). > Hence after the command, > > sta_lists -> > 1. sta -> connected AP MLD sdata > link_sta 0 -> connected to 2 GHz link > 3. sta -> connected AP MLD sdata > link_sta 2 -> connected to 6 GHz link Right, OK. So you _are_ indeed wanting to remove all MLDs *entirely*, if they use a specific link. Agree that in this case, NL80211_CMD_DEL_STATION with link ID makes sense as implemented, but probably need to clarify a little bit overall that this is the operation, seeing how I was confused about whether you want to remove only the link STAs on on those links, or the entire MLD stations. (and yeah, our terminology here is confusing and doesn't help either, but that's because we didn't rename STATION to MLD or something everywhere) > Now, if ML re-config support is present, then hostapd (or the user space > controller for that matters), could first issue > NL80211_CMD_REMOVE_LINK_STA for the MLD STA (#4) and remove link sta > with ID 1 from it. So that when NL80211_CMD_DEL_STATION comes, it would > not remove the 2/6 GHz link STA as well from the MLD STA and hence flush > the whole entry. Right, OK! But see above - that NL80211_CMD_REMOVE_LINK_STA as described here may or may not be possible today in mac80211. > The above change is not there yet in hostapd, so for the time being, > whole MLD STA would be flushed. OK. > > > @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = { > > > .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, > > > .doit = nl80211_del_station, > > > .flags = GENL_UNS_ADMIN_PERM, > > > + /* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on > > > + * MAC address > > > + */ > > > .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), > > > > Hmm? How does NL80211_FLAG_MLO_VALID_LINK_ID depend on the MAC address?! > > It ... doesn't? > > > I mean intention was that if MAC addresses is passed then no need of > link ID. That is why did not add the valid link flag since it would > expect the link ID even when MAC address is passed. > Ah, OK, that makes sense. Maybe rephrase that comment? I was also thinking of just refactoring the logic into a helper function, but that may be difficult, not sure. It just looked similar. johannes
On 1/30/24 16:16, Johannes Berg wrote: > On Sat, 2024-01-27 at 11:14 +0530, Aditya Kumar Singh wrote: >> On 1/26/24 14:36, Johannes Berg wrote: >>> On Thu, 2024-01-25 at 18:28 +0530, Aditya Kumar Singh wrote: >>>> Currently whenever NL80211_CMD_DEL_STATION command is called without any >>>> MAC address, all stations present on that interface are flushed. >>> >>> True. >>> >>>> However with MLO there is a need to flush the stations from a particular >>>> link in the interface, and not from all the links associated with the MLD >>>> interface. >>> >>> Fair enough, I can get behind that. >>> >>> Edit: reading the code - I think I misunderstand that ... you're >>> actually trying to remove all MLDs ("STATION") that have an active link >>> on this link? >> >> Yes correct. The station might not be MLD station. It could be a legacy >> station (non EHT) as well. > > We pretty much treat that the same though as an MLD station with a > single link, with some caveats of translations, no? > Yes correct! >> Correct, for the first bring up not required but one use case I see is - >> the hostapd interface was disabled for some reason. While going down, it >> would have cleared the stations on the kernel but what if for some >> reason kernel did not clear the station entries and there are some stale >> entries present? So at next bring up (during enable) it would send the >> command without any MAC address to flush all stale entries (probably as >> a safety so that kernel and hostapd would now be on par). > > I don't think this really makes much sense. The kernel can't keep track > of those stations properly if they're there, and anyway that'd be a > (pretty massive!) kernel bug? > Yeah :). Even I haven't seen kernel not removing the entries while the interface was going down. Was just thinking out loud here. Tbh, even I don't know the exact reason it was written in that way. Was guessing. > Anyway, I think there probably _is_ justification for this (link > removal?), I'm just not sure this bringup flow really is a good > justification. > Yes we have. >>>> Hence, add an option to pass link ID as well in the command so that if link >>>> ID is passed, station using that passed link ID alone would be deleted >>>> and others will not be removed. >>> >>> So first: Do you want some feature flag that indicates this? Or will we >>> just eat the cost of kicking out everyone (without even sending deauth >>> though, I think?) when running on older kernels? >>> >> >> If what I said above was the actual intention, then kicking out everyone >> without even sending deauth makes sense? Yes? If yes then we don't need >> a feature flag. > > Does it though? Even if you're talking about init, you could have init > of one link much delayed for CSA, for example, with stations already > connected on the other(s). > hmm.. correct. >>> Secondly: why is this part of NL80211_CMD_DEL_STATION? I'm not convinced >>> that makes sense. I actually kind of get why you're doing that - it's >>> easier to retrofit into the existing hostapd, but I don't necessarily >>> think that the hostap design (problems?) should influence this too much. >>> >>> IOW, it would feel much more appropriate to have this as part of >>> NL80211_CMD_REMOVE_LINK_STA? After all, when going to MLD then "STATION" >>> now represents a "peer MLD", and "LINK_STA" now represents an affiliated >>> STA. And flushing all affiliated STAs is what you want. >>> >>> So I think it should be NL80211_CMD_REMOVE_LINK_STA without a >>> NL80211_ATTR_MLD_ADDR. >>> >> >> At least as per the current way of NL80211_CMD_REMOVE_LINK_STA >> implementation, it did not made any sense to delete all link STAs if >> MLD_ADDR is not passed. So probably the command should be called as many >> times as there are active links in the STA? > > Not sure I understand this, we're doing a kind of flush here, so you > could (conceptually) say "flush all link STAs on link 5", no? And > obviously stations that have no link left after this need to be removed > completely. > > Note this raises an interesting point in mac80211, in that there's one > link ('deflink', the link the STA used to assoc) that cannot be removed > from an MLD station even. > Good point :) I did not think about this. Let me think again how to handle this case. Thanks. > But again this comes down to what you actually _want_, I think, so I'll > keep reading for now. > >> Still I feel that NL80211_CMD_DEL_STATION is the proper place to put >> this? Without the current change also, it used to flush all STAs >> whenever MAC address is not passed. With MLO, now we need to flush STAs >> only if it is using the given link ID. So that link STAs from other >> affiliated links of AP would not be flushed. > > > Right so this is coming to the point where I wasn't sure earlier what > you actually meant, and I'm still not entirely positive I've understood > it. Let me read on ... > >> Scenario I'm targeting is this - >> >> Pre-MLO >> ---------------------------- >> >> sdata -> 2 GHz AP interface >> sta_lists -> >> 1. sta -> connected 2 GHz AP sdata >> 2. sta -> connected 2 GHz AP sdata >> >> After NL80211_CMD_DEL_STATION is given without any MAC address, >> >> sta_lists -> >> No entry(ies) > > Right. > >> With MLO >> ----------------------------- >> sdata -> >> link_data -> 2 GHz AP link (link ID 0) >> link_data -> 5 GHz AP link (link ID 1) >> link_data -> 6 GHz AP link (link ID 2) >> sta_lists -> >> 1. sta -> connected AP MLD sdata >> link_sta 0 -> connected to 2 GHz link >> 2. sta -> connected AP MLD sdata >> link_sta 1 -> connected to 5 GHz link >> 3. sta -> connected AP MLD sdata >> link_sta 2 -> connected to 6 GHz link >> 4. sta -> connected AP MLD sdata >> link_sta 0 -> connected to 2 GHz link >> link_sta 1 -> connected to 5 GHz link >> link_sta 2 -> connected to 6 GHz link >> >> Assume 5 GHz goes down and it gives NL80211_CMD_DEL_STATION without any >> MAC address, >> >> sta_lists -> >> No entry(ies) >> >> This is not desirable since 5 GHz link went down, why 2/6 GHz STA also >> got flushed. >> >> Hence with the proposed change, only sta #2 and #4 would be flushed >> since only these two are using passed link ID (which would be 1). >> Hence after the command, >> >> sta_lists -> >> 1. sta -> connected AP MLD sdata >> link_sta 0 -> connected to 2 GHz link >> 3. sta -> connected AP MLD sdata >> link_sta 2 -> connected to 6 GHz link > > Right, OK. > > So you _are_ indeed wanting to remove all MLDs *entirely*, if they use a > specific link. > Yes correct. > Agree that in this case, NL80211_CMD_DEL_STATION with link ID makes > sense as implemented, but probably need to clarify a little bit overall > that this is the operation, seeing how I was confused about whether you > want to remove only the link STAs on on those links, or the entire MLD > stations. > okay sure, I will try to explain in the commit message as well as in code if needed. > (and yeah, our terminology here is confusing and doesn't help either, > but that's because we didn't rename STATION to MLD or something > everywhere) > >> Now, if ML re-config support is present, then hostapd (or the user space >> controller for that matters), could first issue >> NL80211_CMD_REMOVE_LINK_STA for the MLD STA (#4) and remove link sta >> with ID 1 from it. So that when NL80211_CMD_DEL_STATION comes, it would >> not remove the 2/6 GHz link STA as well from the MLD STA and hence flush >> the whole entry. > > Right, OK! > But see above - that NL80211_CMD_REMOVE_LINK_STA as described here may > or may not be possible today in mac80211. Sure will check that. > >> The above change is not there yet in hostapd, so for the time being, >> whole MLD STA would be flushed. > > OK. > >>>> @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = { >>>> .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, >>>> .doit = nl80211_del_station, >>>> .flags = GENL_UNS_ADMIN_PERM, >>>> + /* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on >>>> + * MAC address >>>> + */ >>>> .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), >>> >>> Hmm? How does NL80211_FLAG_MLO_VALID_LINK_ID depend on the MAC address?! >>> It ... doesn't? >>> >> I mean intention was that if MAC addresses is passed then no need of >> link ID. That is why did not add the valid link flag since it would >> expect the link ID even when MAC address is passed. >> > > Ah, OK, that makes sense. > > Maybe rephrase that comment? I was also thinking of just refactoring the > logic into a helper function, but that may be difficult, not sure. It > just looked similar. > Sure, I will see what I can do here. Thanks for your inputs. Will send v2 soon :)
On Tue, 2024-01-30 at 16:41 +0530, Aditya Kumar Singh wrote: > Yeah :). Even I haven't seen kernel not removing the entries while the > interface was going down. Was just thinking out loud here. Tbh, even I > don't know the exact reason it was written in that way. Was guessing. It's probably just old ... :) > > Note this raises an interesting point in mac80211, in that there's one > > link ('deflink', the link the STA used to assoc) that cannot be removed > > from an MLD station even. > > > > Good point :) I did not think about this. Let me think again how to > handle this case. Thanks. You don't actually have to now though - you're _not_ removing just links, you're removing the whole station with all links, as long as it has a link with the relevant link ID. johannes
On 1/30/24 17:24, Johannes Berg wrote: > On Tue, 2024-01-30 at 16:41 +0530, Aditya Kumar Singh wrote: > >> Yeah :). Even I haven't seen kernel not removing the entries while the >> interface was going down. Was just thinking out loud here. Tbh, even I >> don't know the exact reason it was written in that way. Was guessing. > > It's probably just old ... :) > >>> Note this raises an interesting point in mac80211, in that there's one >>> link ('deflink', the link the STA used to assoc) that cannot be removed >>> from an MLD station even. >>> >> >> Good point :) I did not think about this. Let me think again how to >> handle this case. Thanks. > > You don't actually have to now though - you're _not_ removing just > links, you're removing the whole station with all links, as long as it > has a link with the relevant link ID. > Yeah that is true. Not for this change. But the other one I'm working on is the link removal area. So there I need to be care full.
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index cf79656ce09c..2e194638717d 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1766,11 +1766,14 @@ struct station_parameters { * @subtype: Management frame subtype to use for indicating removal * (10 = Disassociation, 12 = Deauthentication) * @reason_code: Reason code for the Disassociation/Deauthentication frame + * @link_id: Link ID on which station should be connected at least in order + * to delete its entry. Valid only during MLO. */ struct station_del_parameters { const u8 *mac; u8 subtype; u16 reason_code; + int link_id; }; /** diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h index 1ccdcae24372..4197d7097591 100644 --- a/include/uapi/linux/nl80211.h +++ b/include/uapi/linux/nl80211.h @@ -438,7 +438,8 @@ * %NL80211_ATTR_REASON_CODE can optionally be used to specify which type * of disconnection indication should be sent to the station * (Deauthentication or Disassociation frame and reason code for that - * frame). + * frame). %NL80211_ATTR_MLO_LINK_ID can be used optionally to remove + * stations connected and using at least that link. * * @NL80211_CMD_GET_MPATH: Get mesh path attributes for mesh path to * destination %NL80211_ATTR_MAC on the interface identified by diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 60877b532993..a99537828978 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -7627,14 +7627,16 @@ static int nl80211_del_station(struct sk_buff *skb, struct genl_info *info) { struct cfg80211_registered_device *rdev = info->user_ptr[0]; struct net_device *dev = info->user_ptr[1]; + struct wireless_dev *wdev = dev->ieee80211_ptr; struct station_del_parameters params; + int link_id = nl80211_link_id_or_invalid(info->attrs); memset(¶ms, 0, sizeof(params)); if (info->attrs[NL80211_ATTR_MAC]) params.mac = nla_data(info->attrs[NL80211_ATTR_MAC]); - switch (dev->ieee80211_ptr->iftype) { + switch (wdev->iftype) { case NL80211_IFTYPE_AP: case NL80211_IFTYPE_AP_VLAN: case NL80211_IFTYPE_MESH_POINT: @@ -7675,6 +7677,17 @@ static int nl80211_del_station(struct sk_buff *skb, struct genl_info *info) params.reason_code = WLAN_REASON_PREV_AUTH_NOT_VALID; } + /* Link ID not expected in case of non-ML operation */ + if (!wdev->valid_links && link_id != -1) + return -EINVAL; + + /* If given, a valid link ID should be passed during MLO */ + if (wdev->valid_links && link_id >= 0 && + !(wdev->valid_links & BIT(link_id))) + return -EINVAL; + + params.link_id = link_id; + return rdev_del_station(rdev, dev, ¶ms); } @@ -16827,6 +16840,9 @@ static const struct genl_small_ops nl80211_small_ops[] = { .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP, .doit = nl80211_del_station, .flags = GENL_UNS_ADMIN_PERM, + /* cannot use NL80211_FLAG_MLO_VALID_LINK_ID, depends on + * MAC address + */ .internal_flags = IFLAGS(NL80211_FLAG_NEED_NETDEV_UP), }, { diff --git a/net/wireless/trace.h b/net/wireless/trace.h index 1f374c8a17a5..838107186b91 100644 --- a/net/wireless/trace.h +++ b/net/wireless/trace.h @@ -859,6 +859,7 @@ DECLARE_EVENT_CLASS(station_del, MAC_ENTRY(sta_mac) __field(u8, subtype) __field(u16, reason_code) + __field(int, link_id) ), TP_fast_assign( WIPHY_ASSIGN; @@ -866,11 +867,13 @@ DECLARE_EVENT_CLASS(station_del, MAC_ASSIGN(sta_mac, params->mac); __entry->subtype = params->subtype; __entry->reason_code = params->reason_code; + __entry->link_id = params->link_id; ), TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", station mac: %pM" - ", subtype: %u, reason_code: %u", + ", subtype: %u, reason_code: %u, link_id: %d", WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->sta_mac, - __entry->subtype, __entry->reason_code) + __entry->subtype, __entry->reason_code, + __entry->link_id) ); DEFINE_EVENT(station_del, rdev_del_station,
Currently whenever NL80211_CMD_DEL_STATION command is called without any MAC address, all stations present on that interface are flushed. However with MLO there is a need to flush the stations from a particular link in the interface, and not from all the links associated with the MLD interface. For example - 2 GHz and 5 GHz are part of an AP MLD. When 2 GHz BSS is brought up, it sends flush command on the interface (MLD). Then eventually 5 GHZ links comes up and that also sends the command on the same interface. Now by the time 5 GHz link comes up, if any station gets connected to 2 GHz link, it would be flushed while 5 GHz link is started which is wrong. Hence, add an option to pass link ID as well in the command so that if link ID is passed, station using that passed link ID alone would be deleted and others will not be removed. A subsequent patch would add logic to delete only the station using the passed link ID. Signed-off-by: Aditya Kumar Singh <quic_adisi@quicinc.com> --- include/net/cfg80211.h | 3 +++ include/uapi/linux/nl80211.h | 3 ++- net/wireless/nl80211.c | 18 +++++++++++++++++- net/wireless/trace.h | 7 +++++-- 4 files changed, 27 insertions(+), 4 deletions(-)