From: Ziyi Guo <[email protected]> [ Upstream commit 820ba7dd6859ef8b1eaf6014897e7aa4756fc65d ]
ath10k_wmi_event_peer_sta_ps_state_chg() uses lockdep_assert_held() to assert that ar->data_lock should be held by the caller, but neither ath10k_wmi_10_2_op_rx() nor ath10k_wmi_10_4_op_rx() acquire this lock before calling this function. The field arsta->peer_ps_state is documented as protected by ar->data_lock in core.h, and other accessors (ath10k_peer_ps_state_disable, ath10k_dbg_sta_read_peer_ps_state) properly acquire this lock. Add spin_lock_bh()/spin_unlock_bh() around the peer_ps_state update, and remove the lockdep_assert_held() to be aligned with new locking, following the pattern used by other WMI event handlers in the driver. Signed-off-by: Ziyi Guo <[email protected]> Reviewed-by: Baochen Qiang <[email protected]> Link: https://patch.msgid.link/[email protected] [removed excess blank line] Signed-off-by: Jeff Johnson <[email protected]> Signed-off-by: Sasha Levin <[email protected]> --- LLM Generated explanations, may be completely bogus: ## Analysis of wifi: ath10k: fix lock protection in ath10k_wmi_event_peer_sta_ps_state_chg() ### 1. Commit Message Analysis The commit message is clear and well-structured: - **Subject**: Explicitly says "fix lock protection" — this is a locking/synchronization bug fix - **Body**: Explains that `lockdep_assert_held()` asserted `ar->data_lock` should be held by callers, but **no caller actually held it**. This means the assertion was always wrong (or always disabled), and the field `arsta->peer_ps_state` was being accessed without the required lock protection. - **Reviewed-by**: Baochen Qiang from Qualcomm reviewed it, lending credibility - The commit references that `core.h` documents `peer_ps_state` as protected by `ar->data_lock`, and other accessors properly acquire it — meaning this was the only broken path. ### 2. Code Change Analysis The change is minimal and surgical: 1. **Removes** `lockdep_assert_held(&ar->data_lock)` — the callers never held this lock, so the assertion was incorrect (and likely only checked with CONFIG_LOCKDEP enabled, which is why it didn't always trigger) 2. **Adds** `spin_lock_bh(&ar->data_lock)` / `spin_unlock_bh(&ar->data_lock)` around the single line `arsta->peer_ps_state = __le32_to_cpu(ev->peer_ps_state)` — this is the actual fix, properly protecting the field with the documented lock The lock scope is minimal — only around the single write to `peer_ps_state`, placed after the RCU-protected station lookup and before the RCU read unlock. This is clean and correct. ### 3. Bug Classification This is a **data race / missing synchronization** bug: - The field `peer_ps_state` is documented as requiring `ar->data_lock` protection - Other readers/writers of this field properly acquire the lock - This WMI event handler was the only path that didn't hold the lock - Without the lock, concurrent reads (from `ath10k_dbg_sta_read_peer_ps_state`) and writes could race, leading to torn reads or inconsistent state ### 4. Scope and Risk Assessment - **Lines changed**: ~4 lines effective (remove 2 lines, add 3 lines including lock/unlock) - **Files changed**: 1 file (`drivers/net/wireless/ath/ath10k/wmi.c`) - **Risk**: Very low. The change adds proper locking around a single field access, following the established pattern used by all other accessors. The lock is `spin_lock_bh`, which is safe in this softirq context. - **Could it break something?** Extremely unlikely — it adds a lock that was already supposed to be held, and uses the same locking pattern as other paths in the driver. ### 5. User Impact - **Affected hardware**: ath10k WiFi devices (Qualcomm 802.11ac chipsets — very common in laptops and embedded systems) - **Trigger**: WMI peer power-save state change events from firmware - **Consequence of bug**: Data race on `peer_ps_state` field. While this is a 32-bit field and the race may not always cause visible corruption on most architectures, it violates the documented locking contract and could cause issues with lockdep-enabled kernels (warnings/splats). On architectures without atomic 32-bit writes, it could cause torn reads. - **Severity**: Medium — it's a real locking bug in a commonly-used WiFi driver ### 6. Stability Indicators - Reviewed by Qualcomm engineer (Baochen Qiang) - Accepted by the ath10k maintainer (Jeff Johnson) - Small, obvious, and following established patterns in the same driver ### 7. Dependency Check - No dependencies on other commits - The code being modified has existed in ath10k for a long time (the `lockdep_assert_held` suggests the locking was always intended but never correctly implemented in this path) - Should apply cleanly to any stable tree that has the ath10k driver with this function ### 8. Stable Kernel Criteria - **Obviously correct?** Yes — adds the documented lock around a field access - **Fixes a real bug?** Yes — data race / missing synchronization - **Small and contained?** Yes — 4 lines in 1 file - **No new features?** Correct — pure bug fix - **Tested?** Reviewed by subsystem experts, accepted by maintainer ### Conclusion This is a textbook stable-worthy fix: a small, surgical correction to a locking bug in a widely-used WiFi driver. It properly adds the lock that was always documented as required but never acquired in this code path. The risk is minimal and the fix follows established patterns in the same driver. **YES** drivers/net/wireless/ath/ath10k/wmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index b4aad6604d6d9..ce22141e5efd9 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -5289,8 +5289,6 @@ ath10k_wmi_event_peer_sta_ps_state_chg(struct ath10k *ar, struct sk_buff *skb) struct ath10k_sta *arsta; u8 peer_addr[ETH_ALEN]; - lockdep_assert_held(&ar->data_lock); - ev = (struct wmi_peer_sta_ps_state_chg_event *)skb->data; ether_addr_copy(peer_addr, ev->peer_macaddr.addr); @@ -5305,7 +5303,9 @@ ath10k_wmi_event_peer_sta_ps_state_chg(struct ath10k *ar, struct sk_buff *skb) } arsta = (struct ath10k_sta *)sta->drv_priv; + spin_lock_bh(&ar->data_lock); arsta->peer_ps_state = __le32_to_cpu(ev->peer_ps_state); + spin_unlock_bh(&ar->data_lock); exit: rcu_read_unlock(); -- 2.51.0
