Re: [PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()
On 4/5/21 9:34 AM, Pavel Machek wrote:
Hi!
Fix ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU lock before it
calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
pointer is no longer needed.
It does that. But is also does the unlock even if it did not take the
lock:
There is only one path after it takes the lock.
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
case WMI_TDLS_TEARDOWN_REASON_TX:
case WMI_TDLS_TEARDOWN_REASON_RSSI:
case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
+ rcu_read_lock();
Takes the lock here:
station = ieee80211_find_sta_by_ifaddr(ar->hw,
ev->peer_macaddr.addr,
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer
event");
- kfree(tb);
- return;
+ goto exit;
Releases it if something fails
}
arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
ieee80211_tdls_oper_request(
@@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
);
break;
}
+
falls through here.
+exit:
+ rcu_read_unlock();
kfree(tb);
}
The switch only takes the lock in 3 branches, but it is released
unconditionally at the end.
I don't see any other switch cases. However, default is missing:
It could be done this way perhaps: (simpler than what you proposed)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index d97b33f789e4..7efbe03fbca8 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -592,6 +592,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k
*ar, struct sk_buff *skb)
GFP_ATOMIC
);
break;
+ default:
+ kfree(tb);
+ return;
}
exit:
thanks,
-- Shuah
Re: [PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()
Hi!
> Fix ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU lock before it
> calls ieee80211_find_sta_by_ifaddr() and release it when the resulting
> pointer is no longer needed.
It does that. But is also does the unlock even if it did not take the
lock:
> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k
> *ar, struct sk_buff *skb)
> case WMI_TDLS_TEARDOWN_REASON_TX:
> case WMI_TDLS_TEARDOWN_REASON_RSSI:
> case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
> + rcu_read_lock();
> station = ieee80211_find_sta_by_ifaddr(ar->hw,
> ev->peer_macaddr.addr,
> NULL);
> if (!station) {
> ath10k_warn(ar, "did not find station from tdls peer
> event");
> - kfree(tb);
> - return;
> + goto exit;
> }
> arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
> ieee80211_tdls_oper_request(
> @@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
> struct sk_buff *skb)
> );
> break;
> }
> +
> +exit:
> + rcu_read_unlock();
> kfree(tb);
> }
The switch only takes the lock in 3 branches, but it is released
unconditionally at the end.
Something like this?
Best regards,
Pavel
Signed-off-by: Pavel Machek (CIP)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index e7072fc4f487..e03ff56d938b 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -582,20 +582,19 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer
event");
- goto exit;
- }
- arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
- ieee80211_tdls_oper_request(
+ } else {
+ arvif = ath10k_get_arvif(ar,
__le32_to_cpu(ev->vdev_id));
+ ieee80211_tdls_oper_request(
arvif->vif, station->addr,
NL80211_TDLS_TEARDOWN,
WLAN_REASON_TDLS_TEARDOWN_UNREACHABLE,
GFP_ATOMIC
);
+ }
+ rcu_read_unlock();
break;
}
-exit:
- rcu_read_unlock();
kfree(tb);
}
--
http://www.livejournal.com/~pavelmachek
signature.asc
Description: Digital signature
[PATCH 5.10 047/126] ath10k: hold RCU lock when calling ieee80211_find_sta_by_ifaddr()
From: Shuah Khan
[ Upstream commit 09078368d516918666a0122f2533dc73676d3d7e ]
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 ath10k_wmi_tlv_op_pull_peer_stats_info() to hold RCU 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().
Link:
https://lore.kernel.org/linux-wireless/7230c9e5-2632-b77e-c4f9-10eca557a...@linuxfoundation.org/
Signed-off-by: Shuah Khan
Signed-off-by: Kalle Valo
Link: https://lore.kernel.org/r/20210210212107.40373-1-sk...@linuxfoundation.org
Signed-off-by: Sasha Levin
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index e6135795719a..e7072fc4f487 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -576,13 +576,13 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
case WMI_TDLS_TEARDOWN_REASON_TX:
case WMI_TDLS_TEARDOWN_REASON_RSSI:
case WMI_TDLS_TEARDOWN_REASON_PTR_TIMEOUT:
+ rcu_read_lock();
station = ieee80211_find_sta_by_ifaddr(ar->hw,
ev->peer_macaddr.addr,
NULL);
if (!station) {
ath10k_warn(ar, "did not find station from tdls peer
event");
- kfree(tb);
- return;
+ goto exit;
}
arvif = ath10k_get_arvif(ar, __le32_to_cpu(ev->vdev_id));
ieee80211_tdls_oper_request(
@@ -593,6 +593,9 @@ static void ath10k_wmi_event_tdls_peer(struct ath10k *ar,
struct sk_buff *skb)
);
break;
}
+
+exit:
+ rcu_read_unlock();
kfree(tb);
}
--
2.30.1
