| Message ID | 20210212021312.40486-1-skhan@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Felix Fietkau |
| Headers | show |
| Series | mt76: hold RCU lock when calling ieee80211_find_sta_by_ifaddr() | expand |
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 2/11/21 10:36 PM, Felix Fietkau wrote: > > 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. > Yes. You are right. I checked the caller of this routine and didn't go further up. :) thanks, -- Shuah
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,
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(-)