Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <545227cf18baac94ea8aa24dc08b250c47949541.camel@sipsolutions.net>
Subject: Re: [PATCH 10/27] wifi: mac80211: isolate driver from inactive links
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Wen Gong <quic_wgong@quicinc.com>, linux-wireless@vger.kernel.org
Cc:     ath11k@lists.infradead.org
Date:   Fri, 09 Sep 2022 09:28:16 +0200
In-Reply-To: <c05780bc-864c-9323-499d-a8b1ba1c2ef2@quicinc.com>
References: <20220902141259.377789-1-johannes@sipsolutions.net>
         <20220902161143.5ce3dad3be7c.I92e9f7a6c120cd4a3631baf486ad8b6aafcd796f@changeid>
         <5d82e564-86bf-c26b-077a-d0bc14e2d3c3@quicinc.com>
         <74f3eb848326607b15336c31a02bdd861ccafb47.camel@sipsolutions.net>
         <2de44394-cb93-7be4-481f-2d92788b8d28@quicinc.com>
         <351f74e0e1cd6e9724f97dbd042bdc5e04c44842.camel@sipsolutions.net>
         <c05780bc-864c-9323-499d-a8b1ba1c2ef2@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

Hi,

> > No, they aren't, and shouldn't be.

> IEEE P802.11be=E2=84=A2/D2.0
> 35.3.3 Multi-link device addressing

> An MLD has an MLD MAC address that singly identifies the MLD.
> Each STA affiliated with an MLD shall have a different MAC address.
> NOTE 1=E2=80=94The MLD MAC address of an MLD might be the same as the MAC=
=20
> address of one affiliated STA or different
> from the MAC address of any affiliated STA.

Right. I was over-simplifying, that was basically the "tl;dr" version of
my statement, without the longer one ;-)

> This means the MLD address can be same with one link.

True.

> I suggest to set primary link local addr same with MLD address for statio=
n.

I wouldn't suggest that, but YMMV.

> reason is:
> When station up, one link interface of driver will be created with the=
=20
> addr of struct ieee80211_vif,
> it is used for scan and non-MLO connection.
> If station start to do MLO connection now, then random local link addr=
=20
> will be generated by below call stack.
> for the 1st link. This lead driver must change the link interface local=
=20
> address to this random addr.

Well, that depends how you treat "address of an interface", no? I don't
think there's really any need to "install" a MAC address to the NIC
until you even start any kind of operation.

True, if you cannot scan using the MLD address while you also have a
different link address, you might be in trouble - but I find this
unrealistic because you would want to be able to scan on any part of the
hardware that is doing any of the links?


In any case, changing this makes the receive logic a bit different. You
would have to ensure that your driver does indeed indicate the link a
frame was received on, I think? Also, ieee80211_rx_for_interface() might
have to change, something like the below maybe?

If we just change the first link's address to the same as the MLD
address without any changes then the code without the changes below
would overwrite the link ID because it can find the link STA address,
even if the device already did address translation. Of course this is
only relevant if it does address translation w/o indicating the link,
which it shouldn't ... hence the patch.

In any case, I expect this will end up being some kind of driver policy,
so I can imagine that we could make a relatively simple patch with a new
method to let drivers set the link address that gets used. It cannot be
changing the link address when it's added to the driver since this patch
that this thread is based on means the driver doesn't get to know about
the links until it's far too late (and even before this patch, the links
were only created after assoc, when the link addresses were already sent
to the AP)

johannes


diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ac2bad57933f..648b2de8dd3e 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1482,7 +1482,8 @@ enum mac80211_rx_encoding {
  * @ampdu_delimiter_crc: A-MPDU delimiter CRC
  * @zero_length_psdu_type: radiotap type of the 0-length PSDU
  * @link_valid: if the link which is identified by @link_id is valid. This=
 flag
- *	is set only when connection is MLO.
+ *	is set only when connection is MLO. Note that setting this also implies
+ *	address translation was done.
  * @link_id: id of the link used to receive the packet. This is used along=
 with
  *	@link_valid.
  */
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a57811372027..963de5d880d7 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4946,22 +4946,24 @@ static void __ieee80211_rx_handle_8023(struct ieee8=
0211_hw *hw,
 static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
 				       struct sk_buff *skb, bool consume)
 {
-	struct link_sta_info *link_sta;
+	struct ieee80211_rx_status *status =3D IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_hdr *hdr =3D (void *)skb->data;
+	struct link_sta_info *link_sta =3D NULL;
=20
 	/*
-	 * Look up link station first, in case there's a
-	 * chance that they might have a link address that
-	 * is identical to the MLD address, that way we'll
-	 * have the link information if needed.
+	 * Unless the driver did addr translation and provided the link
+	 * ID, look up link station first. Note that if we get a frame
+	 * without link ID in the status and the device happens to use
+	 * identical addresses for one of the links and the MLD, then
+	 * we cannot identify whether it was translated already or not.
 	 */
-	link_sta =3D link_sta_info_get_bss(rx->sdata, hdr->addr2);
+	if (!status->link_valid)
+		link_sta =3D link_sta_info_get_bss(rx->sdata, hdr->addr2);
+
 	if (link_sta) {
 		rx->sta =3D link_sta->sta;
 		rx->link_id =3D link_sta->link_id;
 	} else {
-		struct ieee80211_rx_status *status =3D IEEE80211_SKB_RXCB(skb);
-
 		rx->sta =3D sta_info_get_bss(rx->sdata, hdr->addr2);
 		if (rx->sta) {
 			if (status->link_valid &&