Re: [RFC PACTH v2 wireless-next 1/3] wifi: mac80211: Get link_id from freq for received management frame

[Johannes Berg <johannes@sipsolutions.net>]

On Fri, 2025-07-11 at 12:03 +0200, Remi Pommarel wrote:
> 
> +static int ieee80211_rx_get_link_from_freq(struct ieee80211_rx_data *rx,
> +					   struct sk_buff *skb,
> +					   struct link_sta_info *link_sta)
> +{
> +	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
> +	struct ieee80211_sta *sta = &link_sta->sta->sta;
> +	struct ieee80211_link_data *link;
> +	struct ieee80211_bss_conf *bss_conf;
> +	struct ieee80211_chanctx_conf *conf;
> +
> +	if (!status->freq)
> +		return link_sta->link_id;
> +
> +	for_each_link_data(rx->sdata, link) {


..._rcu()

> +		bss_conf = link->conf;
> +		if (!bss_conf)
> +			continue;
> +		conf = rcu_dereference(bss_conf->chanctx_conf);
> +		if (!conf || !conf->def.chan)
> +			continue;
> +
> +		if (conf->def.chan->center_freq != status->freq)
> +			continue;
> +
> +		if (ieee80211_rx_is_valid_sta_link_id(sta, link->link_id))
> +			return link->link_id;
> +	}

But this is now almost the same as the code added via
https://patch.msgid.link/20250721062929.1662700-1-michael-cy.lee@mediatek.com
so I think it should be refactored/combined.

> @@ -5131,7 +5162,15 @@ static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
>  	link_sta = link_sta_info_get_bss(rx->sdata, hdr->addr2);
>  	if (link_sta) {
>  		sta = link_sta->sta;
> -		link_id = link_sta->link_id;
> +
> +		/* Use freq to get link id information on management frames to
> +		 * allow for offchannel scan, roaming, etc.
> +		 */
> +		if (ieee80211_is_mgmt(hdr->frame_control))
> +			link_id = ieee80211_rx_get_link_from_freq(rx, skb,
> +								  link_sta);

It seems to me taht _iff_ the link ID ends up not being link_sta-
>link_id here, we should set link_sta=NULL. Otherwise we think we're
actually receiving from that STA but we aren't really, and if we then
use sta->link[link_id] we'd crash, and I'd be very surprised if this
were impossible.

I'm not sure what consequences that has, but OTOH it really should not
be sending anything other than probe requests, authentication frames and
some few public action frames on another link. We need not handle other
frames as if we didn't realise the link was different, it's fine to even
just drop them because we don't recognise the STA there.

So I think that's what we should do. As written this seems really
problematic to have a link_sta != NULL and thus sta != NULL but then
sta->link[link_id] == NULL and not == link_sta.


Arguably, given the code we already have, we should perhaps implement
this in another way and after doing the link_sta lookup via
link_sta_info_get_bss() simply reject the link_sta (set it to NULL) if
the RX link doesn't match the link it's expected to be on. Then we'd
fall back to the existing code I linked to above anyway, which seems
almost better.

johannes