| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <nycvar.YFH.7.76.1909111238470.473@cbobk.fhfr.pm>
Date: Wed, 11 Sep 2019 12:42:38 +0100 (WEST)
From: Jiri Kosina <jikos@...nel.org>
To: Johannes Berg <johannes@...solutions.net>
cc: Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
Luca Coelho <luciano.coelho@...el.com>,
Intel Linux Wireless <linuxwifi@...el.com>,
Kalle Valo <kvalo@...eaurora.org>,
"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu()
BH-safe
On Mon, 15 Apr 2019, Johannes Berg wrote:
> > If there are other reasons why disable BH for the whole function (are
> > there?), then this bigger hammer works as well of course.
>
> I thought there are, but seeing the commit log here I'm not sure.
>
> In any case, even if not, the function itself is part of the TX fast
> path, but the caller from the workqueue is very uncommon (basically only
> happens for a handful of packets on each new RA/TID), so I'd say that'd
> be a good reason to use the slightly bigger hammer (it's not that much
> different really, if you look at how much code is covered by the lock)
> and avoid doing it all the time when we know it to be not needed.
So this now popped up on me with current Linus' tree from a different
codepath, see below. Therefore I'd like to propose bringing my previous
patch back to life.
Thanks.
From: Jiri Kosina <jkosina@...e.cz>
Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
iwl_mvm_sta->lock can be taken from BH, and therefore BH must be disabled if
taking it from other contexts.
This fixes the lockdep warning below.
================================
WARNING: inconsistent lock state
5.3.0-rc8 #3 Tainted: G W
--------------------------------
inconsistent {IN-SOFTIRQ-W} -> {SOFTIRQ-ON-W} usage.
kworker/u8:2/28401 [HC0[0]:SC0[0]:HE1:SE1] takes:
000000009c020e13 (&(&mvm_sta->lock)->rlock){+.?.}, at: iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
{IN-SOFTIRQ-W} state was registered at:
lock_acquire+0xbd/0x1e0
_raw_spin_lock+0x35/0x50
iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
iwl_mvm_tx_skb+0x1f8/0x460 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xcc/0x200 [iwlmvm]
ieee80211_queue_skb+0x290/0x4c0 [mac80211]
ieee80211_xmit_fast+0x814/0xa70 [mac80211]
__ieee80211_subif_start_xmit+0x132/0x380 [mac80211]
ieee80211_subif_start_xmit+0x49/0x370 [mac80211]
dev_hard_start_xmit+0xaf/0x340
__dev_queue_xmit+0x98d/0xd20
ip6_finish_output2+0x323/0x960
ip6_output+0x19d/0x2d0
mld_sendpack+0x361/0x3a0
mld_ifc_timer_expire+0x19f/0x2e0
call_timer_fn+0x97/0x300
run_timer_softirq+0x233/0x590
__do_softirq+0x12c/0x448
irq_exit+0xa5/0xb0
smp_apic_timer_interrupt+0xac/0x2a0
apic_timer_interrupt+0xf/0x20
cpuidle_enter_state+0xbf/0x450
cpuidle_enter+0x29/0x40
do_idle+0x1cc/0x290
cpu_startup_entry+0x19/0x20
start_secondary+0x15f/0x1a0
secondary_startup_64+0xa4/0xb0
[ ... snip ... ]
stack backtrace:
CPU: 1 PID: 28401 Comm: kworker/u8:2 Tainted: G W 5.3.0-rc8 #3
Hardware name: LENOVO 20K5S22R00/20K5S22R00, BIOS R0IET38W (1.16 ) 05/31/2017
Workqueue: phy0 ieee80211_beacon_connection_loss_work [mac80211]
Call Trace:
dump_stack+0x78/0xb3
mark_lock+0x28a/0x2a0
__lock_acquire+0x568/0x1020
? iwl_mvm_set_tx_cmd+0x1c5/0x400 [iwlmvm]
lock_acquire+0xbd/0x1e0
? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
_raw_spin_lock+0x35/0x50
? iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
iwl_mvm_tx_mpdu+0xae/0x600 [iwlmvm]
? ieee80211_tx_h_select_key+0xf1/0x4a0 [mac80211]
iwl_mvm_tx_skb+0x1f8/0x460 [iwlmvm]
iwl_mvm_mac_itxq_xmit+0xcc/0x200 [iwlmvm]
? iwl_mvm_mac_itxq_xmit+0x55/0x200 [iwlmvm]
_ieee80211_wake_txqs+0x2cf/0x660 [mac80211]
? _ieee80211_wake_txqs+0x5/0x660 [mac80211]
? __ieee80211_wake_queue+0x219/0x340 [mac80211]
ieee80211_wake_queues_by_reason+0x64/0xa0 [mac80211]
ieee80211_set_disassoc+0x3b1/0x520 [mac80211]
__ieee80211_disconnect+0x81/0x110 [mac80211]
process_one_work+0x1f0/0x5b0
? process_one_work+0x16a/0x5b0
worker_thread+0x4c/0x3f0
kthread+0x103/0x140
? process_one_work+0x5b0/0x5b0
? kthread_bind+0x10/0x10
ret_from_fork+0x3a/0x50
Signed-off-by: Jiri Kosina <jkosina@...e.cz>
---
drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 6ac114a393cc..bcfb290beaaf 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1106,7 +1106,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
*/
info->flags &= ~IEEE80211_TX_STATUS_EOSP;
- spin_lock(&mvmsta->lock);
+ spin_lock_bh(&mvmsta->lock);
/* nullfunc frames should go to the MGMT queue regardless of QOS,
* the condition of !ieee80211_is_qos_nullfunc(fc) keeps the default
@@ -1145,7 +1145,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (WARN_ONCE(txq_id == IWL_MVM_INVALID_QUEUE, "Invalid TXQ id")) {
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
return 0;
}
@@ -1181,7 +1181,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
if (tid < IWL_MAX_TID_COUNT && !ieee80211_has_morefrags(fc))
mvmsta->tid_data[tid].seq_number = seq_number + 0x10;
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
if (iwl_mvm_tx_pkt_queued(mvm, mvmsta,
tid == IWL_MAX_TID_COUNT ? 0 : tid))
@@ -1191,7 +1191,7 @@ static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
drop_unlock_sta:
iwl_trans_free_tx_cmd(mvm->trans, dev_cmd);
- spin_unlock(&mvmsta->lock);
+ spin_unlock_bh(&mvmsta->lock);
drop:
IWL_DEBUG_TX(mvm, "TX to [%d|%d] dropped\n", mvmsta->sta_id, tid);
return -1;
--
Jiri Kosina
SUSE Labs
Powered by blists - more mailing lists