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