diff mbox series

[RFC,2/2] wifi: mac80211: use link_id from ieee80211_rx_status to retrieve rx link

Message ID 20220802065019.20791-3-quic_vthiagar@quicinc.com (mailing list archive)
State RFC
Delegated to: Johannes Berg
Headers show
Series wifi: mac80211: extend rx API with link_id for MLO connection | expand

Commit Message

Vasanthakumar Thiagarajan Aug. 2, 2022, 6:50 a.m. UTC 
Fill rx.link with respective data_link from the reported link_id
in rx_status. Any link_id > 15 is invalid. Non-MLO connections
can use either 0 or 15 as the link_id. Please note that link_id
0 is used with non-MLO connections to avoid changes in the
drivers not supporting MLO. For a 802.11 MLD address translated
frame, driver must report the right link_id for the
frame to get processed. When processing 802.3 frame format,
link_id is not that critical, used only with stats update.
In such case, all the stats will be updated for the deflink.

Signed-off-by: Vasanthakumar Thiagarajan <quic_vthiagar@quicinc.com>
---
 net/mac80211/rx.c | 133 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 124 insertions(+), 9 deletions(-)

Comments

Johannes Berg Aug. 9, 2022, 6:15 p.m. UTC | #1 
On Tue, 2022-08-02 at 12:20 +0530, Vasanthakumar Thiagarajan wrote:
> Fill rx.link with respective data_link from the reported link_id
> in rx_status. Any link_id > 15 is invalid. Non-MLO connections
> can use either 0 or 15 as the link_id. Please note that link_id
> 0 is used with non-MLO connections to avoid changes in the
> drivers not supporting MLO. For a 802.11 MLD address translated
> frame, driver must report the right link_id for the
> frame to get processed. When processing 802.3 frame format,
> link_id is not that critical, used only with stats update.
> In such case, all the stats will be updated for the deflink.

I think it might be worth splitting this patch a bit different - putting
some parts into the first patch already (that fill rx.link_id), and
keeping some others (statistics etc.) in this patch, which is then more
related to statistics?

> 
>  	if (rx->link_id >= 0) {
> -		link = rcu_dereference(rx->sdata->link[rx->link_id]);
> -
> +		link =  rcu_dereference(sdata->link[rx->link_id]);

that has some spurious whitespace changes

> +	/*
> +	 * TODO: In MLO, should the frame be dropped if the right link_id is not
> +	 * available? Or may be it is fine in the current form to proceed with
> +	 * the frame processing because with frame being in 802.3 format,
> +	 * link_id is used only for stats purpose and updating the stats on
> +	 * the deflink is fine?
> +	 */
> +	if (pubsta->mlo && status->link_id != IEEE80211_LINK_UNSPECIFIED)
> +		rx.link_id = status->link_id;

If the driver *does* give a link ID, it better be valid?

OTOH, could there be races, e.g. while disabling a link?

johannes
Vasanthakumar Thiagarajan Aug. 11, 2022, 10:28 a.m. UTC | #2 
On 8/9/2022 11:45 PM, Johannes Berg wrote:
> On Tue, 2022-08-02 at 12:20 +0530, Vasanthakumar Thiagarajan wrote:
>> Fill rx.link with respective data_link from the reported link_id
>> in rx_status. Any link_id > 15 is invalid. Non-MLO connections
>> can use either 0 or 15 as the link_id. Please note that link_id
>> 0 is used with non-MLO connections to avoid changes in the
>> drivers not supporting MLO. For a 802.11 MLD address translated
>> frame, driver must report the right link_id for the
>> frame to get processed. When processing 802.3 frame format,
>> link_id is not that critical, used only with stats update.
>> In such case, all the stats will be updated for the deflink.
> 
> I think it might be worth splitting this patch a bit different - putting
> some parts into the first patch already (that fill rx.link_id), and
> keeping some others (statistics etc.) in this patch, which is then more
> related to statistics?

Sure.

> 
>>
>>   	if (rx->link_id >= 0) {
>> -		link = rcu_dereference(rx->sdata->link[rx->link_id]);
>> -
>> +		link =  rcu_dereference(sdata->link[rx->link_id]);
> 
> that has some spurious whitespace changes
> 
>> +	/*
>> +	 * TODO: In MLO, should the frame be dropped if the right link_id is not
>> +	 * available? Or may be it is fine in the current form to proceed with
>> +	 * the frame processing because with frame being in 802.3 format,
>> +	 * link_id is used only for stats purpose and updating the stats on
>> +	 * the deflink is fine?
>> +	 */
>> +	if (pubsta->mlo && status->link_id != IEEE80211_LINK_UNSPECIFIED)
>> +		rx.link_id = status->link_id;
> 
> If the driver *does* give a link ID, it better be valid?

Sure, we can remove IEEE80211_LINK_UNSPECIFIED. If now valid
link_id is set for an MLO link, we still try to retrieve it from
the addr2 in case the MLD address translation is not done in the
lower layers but when this happens with already translated packets
then we may need to drop the packet?

> 
> OTOH, could there be races, e.g. while disabling a link?

I assume this is for the packets where the received on a
link which is disabled? May be driver sets that link but
that link will not be valid in mac80211 while processing
this packet and the packet will be dropped? Or may be
a different race you are mentioning?

Vasanth
Johannes Berg Jan. 19, 2023, 8:57 p.m. UTC | #3 
So I'm digging through old email before I declare myself bankrupt on
that ...

> > 
> > OTOH, could there be races, e.g. while disabling a link?
> 
> I assume this is for the packets where the received on a
> link which is disabled? May be driver sets that link but
> that link will not be valid in mac80211 while processing
> this packet and the packet will be dropped? Or may be
> a different race you are mentioning?
> 

Yes that's what I was thinking of.

johannes
diff mbox series

Patch

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 57df21e2170a..87aa81bc6595 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4508,6 +4508,16 @@  void ieee80211_check_fast_rx_iface(struct ieee80211_sub_if_data *sdata)
 	mutex_unlock(&local->sta_mtx);
 }
 
+static bool
+ieee80211_rx_is_valid_sta_link_id(struct ieee80211_sta *sta, u8 link_id)
+{
+	if (!sta->mlo && (link_id && link_id != IEEE80211_LINK_UNSPECIFIED))
+		return false;
+
+	return !(link_id != IEEE80211_LINK_UNSPECIFIED &&
+		 (sta->mlo && !(sta->valid_links & BIT(link_id))));
+}
+
 static void ieee80211_rx_8023(struct ieee80211_rx_data *rx,
 			      struct ieee80211_fast_rx *fast_rx,
 			      int orig_len)
@@ -4515,19 +4525,30 @@  static void ieee80211_rx_8023(struct ieee80211_rx_data *rx,
 	struct ieee80211_sta_rx_stats *stats;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
 	struct sta_info *sta = rx->sta;
+	struct link_sta_info *link_sta;
 	struct sk_buff *skb = rx->skb;
 	void *sa = skb->data + ETH_ALEN;
 	void *da = skb->data;
 
-	stats = &sta->deflink.rx_stats;
+	if (rx->link_id >= 0) {
+		link_sta = rcu_dereference(sta->link[rx->link_id]);
+		if (WARN_ON_ONCE(!link_sta)) {
+			dev_kfree_skb(rx->skb);
+			return;
+		}
+	} else {
+		link_sta = &sta->deflink;
+	}
+
+	stats = &link_sta->rx_stats;
 	if (fast_rx->uses_rss)
-		stats = this_cpu_ptr(sta->deflink.pcpu_rx_stats);
+		stats = this_cpu_ptr(link_sta->pcpu_rx_stats);
 
 	/* statistics part of ieee80211_rx_h_sta_process() */
 	if (!(status->flag & RX_FLAG_NO_SIGNAL_VAL)) {
 		stats->last_signal = status->signal;
 		if (!fast_rx->uses_rss)
-			ewma_signal_add(&sta->deflink.rx_stats_avg.signal,
+			ewma_signal_add(&link_sta->rx_stats_avg.signal,
 					-status->signal);
 	}
 
@@ -4543,7 +4564,7 @@  static void ieee80211_rx_8023(struct ieee80211_rx_data *rx,
 
 			stats->chain_signal_last[i] = signal;
 			if (!fast_rx->uses_rss)
-				ewma_signal_add(&sta->deflink.rx_stats_avg.chain_signal[i],
+				ewma_signal_add(&link_sta->rx_stats_avg.chain_signal[i],
 						-signal);
 		}
 	}
@@ -4619,7 +4640,8 @@  static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 		u8 da[ETH_ALEN];
 		u8 sa[ETH_ALEN];
 	} addrs __aligned(2);
-	struct ieee80211_sta_rx_stats *stats = &sta->deflink.rx_stats;
+	struct link_sta_info *link_sta;
+	struct ieee80211_sta_rx_stats *stats;
 
 	/* for parallel-rx, we need to have DUP_VALIDATED, otherwise we write
 	 * to a common data structure; drivers can implement that per queue
@@ -4720,8 +4742,19 @@  static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 	return true;
  drop:
 	dev_kfree_skb(skb);
+
+	if (rx->link_id >= 0) {
+		link_sta = rcu_dereference(sta->link[rx->link_id]);
+		if (!link_sta)
+			return true;
+	} else {
+		link_sta = &sta->deflink;
+	}
+
 	if (fast_rx->uses_rss)
-		stats = this_cpu_ptr(sta->deflink.pcpu_rx_stats);
+		stats = this_cpu_ptr(link_sta->pcpu_rx_stats);
+	else
+		stats = &link_sta->rx_stats;
 
 	stats->dropped++;
 	return true;
@@ -4763,8 +4796,7 @@  static bool ieee80211_prepare_and_rx_handle(struct ieee80211_rx_data *rx,
 		return false;
 
 	if (rx->link_id >= 0) {
-		link = rcu_dereference(rx->sdata->link[rx->link_id]);
-
+		link =  rcu_dereference(sdata->link[rx->link_id]);
 		/* we might race link removal */
 		if (!link)
 			return true;
@@ -4827,6 +4859,7 @@  static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw,
 				       struct list_head *list)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
+	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_fast_rx *fast_rx;
 	struct ieee80211_rx_data rx;
 
@@ -4847,7 +4880,30 @@  static void __ieee80211_rx_handle_8023(struct ieee80211_hw *hw,
 
 	rx.sta = container_of(pubsta, struct sta_info, sta);
 	rx.sdata = rx.sta->sdata;
-	rx.link = &rx.sdata->deflink;
+
+	if (!ieee80211_rx_is_valid_sta_link_id(pubsta, status->link_id))
+		goto drop;
+
+	/*
+	 * TODO: In MLO, should the frame be dropped if the right link_id is not
+	 * available? Or may be it is fine in the current form to proceed with
+	 * the frame processing because with frame being in 802.3 format,
+	 * link_id is used only for stats purpose and updating the stats on
+	 * the deflink is fine?
+	 */
+	if (pubsta->mlo && status->link_id != IEEE80211_LINK_UNSPECIFIED)
+		rx.link_id = status->link_id;
+
+	if (rx.link_id >= 0) {
+		struct ieee80211_link_data *link;
+
+		link =  rcu_dereference(rx.sdata->link[rx.link_id]);
+		if (!link)
+			goto drop;
+		rx.link = link;
+	} else {
+		rx.link = &rx.sdata->deflink;
+	}
 
 	fast_rx = rcu_dereference(rx.sta->fast_rx);
 	if (!fast_rx)
@@ -4877,7 +4933,18 @@  static bool ieee80211_rx_for_interface(struct ieee80211_rx_data *rx,
 		rx->sta = link_sta->sta;
 		rx->link_id = link_sta->link_id;
 	} else {
+		struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
+
 		rx->sta = sta_info_get_bss(rx->sdata, hdr->addr2);
+		if (rx->sta) {
+			if ((status->link_id == IEEE80211_LINK_UNSPECIFIED &&
+			     rx->sta->sta.mlo) ||
+			    !ieee80211_rx_is_valid_sta_link_id(&rx->sta->sta,
+							       status->link_id))
+				return false;
+
+			rx->link_id = rx->sta->sta.mlo ? status->link_id : -1;
+		}
 	}
 
 	return ieee80211_prepare_and_rx_handle(rx, skb, consume);
@@ -4893,6 +4960,7 @@  static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 					 struct list_head *list)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
+	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_hdr *hdr;
 	__le16 fc;
@@ -4941,6 +5009,36 @@  static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 		if (pubsta) {
 			rx.sta = container_of(pubsta, struct sta_info, sta);
 			rx.sdata = rx.sta->sdata;
+
+			if (!ieee80211_rx_is_valid_sta_link_id(pubsta,
+							       status->link_id))
+				goto out;
+
+			if (pubsta->mlo &&
+			    status->link_id != IEEE80211_LINK_UNSPECIFIED)
+				rx.link_id = status->link_id;
+
+			/*
+			 * In MLO connection, fetch the link_id using addr2
+			 * when the driver passes unspecified link_id in status.
+			 * When the address translation is already performed by
+			 * driver/hw, the right link_id must be passed in
+			 * status.
+			 */
+
+			if (status->link_id == IEEE80211_LINK_UNSPECIFIED &&
+			    pubsta->mlo) {
+				struct ieee80211_hdr *hdr = (void *)skb->data;
+				struct link_sta_info *link_sta;
+
+				link_sta = link_sta_info_get_bss(rx.sdata,
+								 hdr->addr2);
+				if (!link_sta)
+					goto out;
+
+				rx.link_id = link_sta->link_id;
+			}
+
 			if (ieee80211_prepare_and_rx_handle(&rx, skb, true))
 				return;
 			goto out;
@@ -4954,6 +5052,13 @@  static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 				continue;
 			}
 
+			if ((status->link_id == IEEE80211_LINK_UNSPECIFIED &&
+			     sta->sta.mlo) ||
+			    !ieee80211_rx_is_valid_sta_link_id(&sta->sta,
+							       status->link_id))
+				continue;
+
+			rx.link_id = sta->sta.mlo ? status->link_id : -1;
 			rx.sta = prev_sta;
 			rx.sdata = prev_sta->sdata;
 			ieee80211_prepare_and_rx_handle(&rx, skb, false);
@@ -4962,6 +5067,13 @@  static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 		}
 
 		if (prev_sta) {
+			if ((status->link_id == IEEE80211_LINK_UNSPECIFIED &&
+			     prev_sta->sta.mlo) ||
+			    !ieee80211_rx_is_valid_sta_link_id(&prev_sta->sta,
+							       status->link_id))
+				goto out;
+
+			rx.link_id = sta->sta.mlo ? status->link_id : -1;
 			rx.sta = prev_sta;
 			rx.sdata = prev_sta->sdata;
 
@@ -5104,6 +5216,9 @@  void ieee80211_rx_list(struct ieee80211_hw *hw, struct ieee80211_sta *pubsta,
 		}
 	}
 
+	if (WARN_ON_ONCE(status->link_id > IEEE80211_LINK_UNSPECIFIED))
+		goto drop;
+
 	status->rx_flags = 0;
 
 	kcov_remote_start_common(skb_get_kcov_handle(skb));