[PATCH 1/2] ath9k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix ath_tx_process_buffer() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/ath/ath9k/xmit.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index e60d4737fc6e..1d36aae3f7b6 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c

@@ -701,6 +701,9 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 					     ts->ts_rateindex);
 
 	hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
 	if (sta) {
 		struct ath_node *an = (struct ath_node *)sta->drv_priv;

@@ -725,6 +728,8 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 
 	if (!flush)
 		ath_txq_schedule(sc, txq);
+
+	rcu_read_unlock();
 }
 
 static bool ath_lookup_legacy(struct ath_buf *bf)

-- 
2.27.0

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/mediatek/mt76/mac80211.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index a840396f2c74..3c732da2a53f 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c

@@ -867,6 +867,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	bool ps;
 
 	hw = mt76_phy_hw(dev, status->ext_phy);
+
+	rcu_read_lock();
+
 	if (ieee80211_is_pspoll(hdr->frame_control) && !wcid) {
 		sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr2, NULL);
 		if (sta)

@@ -876,7 +879,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	mt76_airtime_check(dev, skb);
 
 	if (!wcid || !wcid->sta)
-		return;
+		goto exit;
 
 	sta = container_of((void *)wcid, struct ieee80211_sta, drv_priv);
 

@@ -886,17 +889,17 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 	wcid->inactive_count = 0;
 
 	if (!test_bit(MT_WCID_FLAG_CHECK_PS, &wcid->flags))
-		return;
+		goto exit;
 
 	if (ieee80211_is_pspoll(hdr->frame_control)) {
 		ieee80211_sta_pspoll(sta);
-		return;
+		goto exit;
 	}
 
 	if (ieee80211_has_morefrags(hdr->frame_control) ||
 	    !(ieee80211_is_mgmt(hdr->frame_control) ||
 	      ieee80211_is_data(hdr->frame_control)))
-		return;
+		goto exit;
 
 	ps = ieee80211_has_pm(hdr->frame_control);
 

@@ -905,7 +908,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 		ieee80211_sta_uapsd_trigger(sta, status->tid);
 
 	if (!!test_bit(MT_WCID_FLAG_PS, &wcid->flags) == ps)
-		return;
+		goto exit;
 
 	if (ps)
 		set_bit(MT_WCID_FLAG_PS, &wcid->flags);

@@ -914,6 +917,9 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
 
 	dev->drv->sta_ps(dev, sta, ps);
 	ieee80211_sta_ps_transition(sta, ps);
+
+exit:
+	rcu_read_unlock();
 }
 
 void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,

-- 
2.27.0

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
- Note: This patch is compile tested. I don't have access to
  hardware.

 drivers/net/wireless/realtek/rtw88/rx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/rx.c b/drivers/net/wireless/realtek/rtw88/rx.c
index 7087e385a9b3..4ab3d3e2bfab 100644
--- a/drivers/net/wireless/realtek/rtw88/rx.c
+++ b/drivers/net/wireless/realtek/rtw88/rx.c

@@ -111,6 +111,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 		return;
 
 	rtw_rx_phy_stat(rtwdev, pkt_stat, hdr);
+
+	rcu_read_lock();
+
 	sta = ieee80211_find_sta_by_ifaddr(rtwdev->hw, hdr->addr2,
 					   vif->addr);
 	if (!sta)

@@ -118,6 +121,9 @@ static void rtw_rx_addr_match_iter(void *data, u8 *mac,
 
 	si = (struct rtw_sta_info *)sta->drv_priv;
 	ewma_rssi_add(&si->avg_rssi, pkt_stat->rssi);
+
+exit:
+	rcu_read_unlock();
 }
 
 static void rtw_rx_addr_match(struct rtw_dev *rtwdev,

-- 
2.27.0

On 2021-02-12 03:13, Shuah Khan wrote:

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only
called from mt76_rx_poll_complete, which itself is only called under RCU
lock.

- Felix

On 2021-02-12 03:13, Shuah Khan wrote:

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix ath_tx_process_buffer() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

This patch seems unnecessary as well. All callers of
ath_tx_process_buffer seem to hold the RCU read lock already.

- Felix

On 2021-02-12 03:13, Shuah Khan wrote:

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix rtw_rx_addr_match_iter() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

This one also seems unnecessary. rtw_rx_addr_match_iter is called by
ieee80211_iterate_active_interfaces_atomic, which acquires the RCU read
lock before calling it.

- Felix

On 2/11/21 10:36 PM, Felix Fietkau wrote:

On 2021-02-12 03:13, Shuah Khan wrote:

quoted

ieee80211_find_sta_by_ifaddr() must be called under the RCU lock and
the resulting pointer is only valid under RCU lock as well.

Fix mt76_check_sta() to hold RCU read lock before it calls
ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.

This problem was found while reviewing code to debug RCU warn from
ath10k_wmi_tlv_parse_peer_stats_info() and a subsequent manual audit
of other callers of ieee80211_find_sta_by_ifaddr() that don't hold
RCU read lock.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

If I'm not mistaken, this patch is unnecessary. mt76_check_sta is only
called from mt76_rx_poll_complete, which itself is only called under RCU
lock.

Yes. You are right. I checked the caller of this routine and didn't
go further up. :)

thanks,
-- Shuah