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 :)
