Re: [PATCH 1/3] wifi: cfg80211: add support for link id attribute in NL80211_CMD_DEL_STATION

[Johannes Berg <johannes@sipsolutions.net>]

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