[PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

From: Jiri Kosina <jkosina@suse.cz>

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.

 IPv6: ADDRCONF(NETDEV_CHANGE): wlan0: link becomes ready

 ========================================================
 WARNING: possible irq lock inversion dependency detected
 5.1.0-rc5 #3 Not tainted
 --------------------------------------------------------
 swapper/1/0 just changed the state of lock:
 00000000a18af450 (_xmit_ETHER#2){+.-.}, at: __dev_queue_xmit+0x8da/0xbe0
 but this lock took another, SOFTIRQ-unsafe lock in the past:
  (&(&mvm_sta->lock)->rlock){+.+.}

 and interrupts could create inverse lock ordering between them.

 other info that might help us debug this:
  Possible interrupt unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&(&mvm_sta->lock)->rlock);
                                local_irq_disable();
                                lock(_xmit_ETHER#2);
                                lock(&(&mvm_sta->lock)->rlock);
   <Interrupt>
     lock(_xmit_ETHER#2);

  *** DEADLOCK ***

 4 locks held by swapper/1/0:
  #0: 000000008e12d112 ((&idev->mc_ifc_timer)){+.-.}, at: call_timer_fn+0x5/0x310
  #1: 0000000003eca5ef (rcu_read_lock){....}, at: mld_sendpack+0x5/0x3b0
  #2: 000000007eac744b (rcu_read_lock_bh){....}, at: ip6_finish_output2+0x5f/0x960
  #3: 000000007eac744b (rcu_read_lock_bh){....}, at: __dev_queue_xmit+0x4e/0xbe0

 the shortest dependencies between 2nd lock and 1st lock:
  -> (&(&mvm_sta->lock)->rlock){+.+.} {
     HARDIRQ-ON-W at:
                       _raw_spin_lock_bh+0x3d/0x50
                       iwl_mvm_add_new_dqa_stream_wk+0x102/0x910 [iwlmvm]
                       process_one_work+0x1f0/0x5b0
                       worker_thread+0x4c/0x3f0
                       kthread+0x103/0x140
                       ret_from_fork+0x3a/0x50
     SOFTIRQ-ON-W at:
                       _raw_spin_lock+0x35/0x50
                       iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
                       iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
                       iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
                       iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
                       process_one_work+0x1f0/0x5b0
                       worker_thread+0x4c/0x3f0
                       kthread+0x103/0x140
                       ret_from_fork+0x3a/0x50
     INITIAL USE at:
                      _raw_spin_lock_bh+0x3d/0x50
                      iwl_mvm_add_new_dqa_stream_wk+0x102/0x910 [iwlmvm]
                      process_one_work+0x1f0/0x5b0
                      worker_thread+0x4c/0x3f0
                      kthread+0x103/0x140
                      ret_from_fork+0x3a/0x50
   }
   ... key      at: [<ffffffffc0e80d80>] __key.87928+0x0/0xfffffffffffd9280 [iwlmvm]
   ... acquired at:
    _raw_spin_lock+0x35/0x50
    iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
    iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
    iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
    ieee80211_queue_skb+0x23a/0x470 [mac80211]
    ieee80211_tx+0xe1/0x140 [mac80211]
    __ieee80211_subif_start_xmit+0x257/0x310 [mac80211]
    ieee80211_subif_start_xmit+0x47/0x300 [mac80211]
    dev_hard_start_xmit+0xb7/0x340
    __dev_queue_xmit+0x88d/0xbe0
    packet_sendmsg+0x1025/0x1920 [af_packet]
    sock_sendmsg+0x36/0x50
    __sys_sendto+0xdc/0x160
    __x64_sys_sendto+0x24/0x30
    do_syscall_64+0x5d/0x190
    entry_SYSCALL_64_after_hwframe+0x49/0xbe

 -> (_xmit_ETHER#2){+.-.} {
    HARDIRQ-ON-W at:
                     _raw_spin_lock+0x35/0x50
                     dev_deactivate_many+0xec/0x3b0
                     dev_deactivate+0x4f/0x80
                     linkwatch_do_dev+0x34/0x50
                     __linkwatch_run_queue+0xf4/0x190
                     linkwatch_event+0x21/0x30
                     process_one_work+0x1f0/0x5b0
                     worker_thread+0x4c/0x3f0
                     kthread+0x103/0x140
                     ret_from_fork+0x3a/0x50
    IN-SOFTIRQ-W at:
                     _raw_spin_lock+0x35/0x50
                     __dev_queue_xmit+0x8da/0xbe0
                     ip6_finish_output2+0x22b/0x960
                     ip6_output+0x1a6/0x2b0
                     mld_sendpack+0x369/0x3b0
                     mld_ifc_timer_expire+0x19f/0x2e0
                     call_timer_fn+0x92/0x310
                     run_timer_softirq+0x216/0x560
                     __do_softirq+0x12c/0x447
                     irq_exit+0xac/0xc0
                     smp_apic_timer_interrupt+0xac/0x2a0
                     apic_timer_interrupt+0xf/0x20
                     cpuidle_enter_state+0xbf/0x450
                     do_idle+0x1cc/0x290
                     cpu_startup_entry+0x19/0x20
                     start_secondary+0x179/0x1c0
                     secondary_startup_64+0xa4/0xb0
    INITIAL USE at:
                    _raw_spin_lock+0x35/0x50
                    dev_deactivate_many+0xec/0x3b0
                    dev_deactivate+0x4f/0x80
                    linkwatch_do_dev+0x34/0x50
                    __linkwatch_run_queue+0xf4/0x190
                    linkwatch_event+0x21/0x30
                    process_one_work+0x1f0/0x5b0
                    worker_thread+0x4c/0x3f0
                    kthread+0x103/0x140
                    ret_from_fork+0x3a/0x50
  }
  ... key      at: [<ffffffffa1679a70>] netdev_xmit_lock_key+0x10/0x390
  ... acquired at:
    __lock_acquire+0x1ae/0xfe0
    lock_acquire+0xbd/0x1e0
    _raw_spin_lock+0x35/0x50
    __dev_queue_xmit+0x8da/0xbe0
    ip6_finish_output2+0x22b/0x960
    ip6_output+0x1a6/0x2b0
    mld_sendpack+0x369/0x3b0
    mld_ifc_timer_expire+0x19f/0x2e0
    call_timer_fn+0x92/0x310
    run_timer_softirq+0x216/0x560
    __do_softirq+0x12c/0x447
    irq_exit+0xac/0xc0
    smp_apic_timer_interrupt+0xac/0x2a0
    apic_timer_interrupt+0xf/0x20
    cpuidle_enter_state+0xbf/0x450
    do_idle+0x1cc/0x290
    cpu_startup_entry+0x19/0x20
    start_secondary+0x179/0x1c0
    secondary_startup_64+0xa4/0xb0

 stack backtrace:
 CPU: 1 PID: 0 Comm: swapper/1 Not tainted 5.1.0-rc5 #3
 Hardware name: LENOVO 20K5S22R00/20K5S22R00, BIOS R0IET38W (1.16 ) 05/31/2017
 Call Trace:
  <IRQ>
  dump_stack+0x78/0xb3
  print_irq_inversion_bug.part.40+0x19f/0x1aa
  check_usage_forwards+0x11e/0x120
  ? secondary_startup_64+0xa4/0xb0
  ? check_usage_backwards+0x120/0x120
  mark_lock+0x17c/0x280
  __lock_acquire+0x1ae/0xfe0
  ? find_held_lock+0x31/0xa0
  ? ieee80211_select_queue+0x124/0x2d0 [mac80211]
  lock_acquire+0xbd/0x1e0
  ? __dev_queue_xmit+0x8da/0xbe0
  _raw_spin_lock+0x35/0x50
  ? __dev_queue_xmit+0x8da/0xbe0
  __dev_queue_xmit+0x8da/0xbe0
  ? __dev_queue_xmit+0x4e/0xbe0
  ? neigh_resolve_output+0x15a/0x230
  ? ip6_finish_output2+0x22b/0x960
  ip6_finish_output2+0x22b/0x960
  ? ip6_finish_output2+0x5f/0x960
  ? ip6_mtu+0x12d/0x1f0
  ? ip6_mtu+0x71/0x1f0
  ip6_output+0x1a6/0x2b0
  ? ip6_output+0xb8/0x2b0
  ? ip6_fragment+0xb70/0xb70
  mld_sendpack+0x369/0x3b0
  ? mld_sendpack+0x5/0x3b0
  ? mld_dad_timer_expire+0x90/0x90
  mld_ifc_timer_expire+0x19f/0x2e0
  ? mld_dad_timer_expire+0x90/0x90
  ? mld_dad_timer_expire+0x90/0x90
  call_timer_fn+0x92/0x310
  ? call_timer_fn+0x5/0x310
  ? mld_dad_timer_expire+0x90/0x90
  run_timer_softirq+0x216/0x560
  __do_softirq+0x12c/0x447
  irq_exit+0xac/0xc0
  smp_apic_timer_interrupt+0xac/0x2a0
  apic_timer_interrupt+0xf/0x20
  </IRQ>
 RIP: 0010:cpuidle_enter_state+0xbf/0x450
 Code: 44 8b 63 04 49 89 c6 0f 1f 44 00 00 31 ff e8 08 3a 9b ff 80 7c 24 0b 00 0f 85 de 02 00 00 e8 e8 a4 a9 ff fb 66 0f 1f 44 00 00 <45> 85 ed 0f 88 b0 02 00 00 49 63 f5 4c 2b 34 24 48 ba cf f7 53 e3
 RSP: 0018:ffffa60b00d27ea0 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
 RAX: ffff8a5686a30340 RBX: ffffc60affc90f50 RCX: 0000000000000000
 RDX: ffff8a5686a30340 RSI: 0000000000000006 RDI: ffff8a5686a30340
 RBP: ffffffffa02fb360 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000001 R11: 071c71c71c71c71c R12: 0000000000000001
 R13: 0000000000000003 R14: 0000000542fed0f3 R15: 0000000000000001
  do_idle+0x1cc/0x290
  cpu_startup_entry+0x19/0x20
  start_secondary+0x179/0x1c0
  secondary_startup_64+0xa4/0xb0

Signed-off-by: Jiri Kosina <jkosina@suse.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 0c2aabc842f9..8c8ebb5e12bc 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1107,7 +1107,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
@@ -1146,7 +1146,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;
 	}
 
@@ -1182,7 +1182,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))
@@ -1192,7 +1192,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

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Johannes Berg]

On Mon, 2019-04-15 at 13:03 +0200, Jiri Kosina wrote:

(FWIW, would be good to cc linux-wireless, but no need for linux-kernel
I guess :-) )

> 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.

I don't think this is the right fix.

The only problem is this path:

>      SOFTIRQ-ON-W at:
>                        _raw_spin_lock+0x35/0x50
>                        iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
>                        iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
>                        iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
>                        iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
>                        process_one_work+0x1f0/0x5b0

which I believe we just made use local_bh_disable() since there are
other reasons as well for that, IIRC.

Yes, here's the fix:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a

It's on the way upstream.

Thanks for the patch/report though!

johannes

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

On Mon, 15 Apr 2019, Johannes Berg wrote:

> (FWIW, would be good to cc linux-wireless, but no need for linux-kernel
> I guess :-) )

Hm, no idea where I lost that ML from the get_maintainer output, sorry for 
that.

> > 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.
> 
> I don't think this is the right fix.
> 
> The only problem is this path:

Agreed, that seems to be the only culprit.

> >      SOFTIRQ-ON-W at:
> >                        _raw_spin_lock+0x35/0x50
> >                        iwl_mvm_tx_mpdu+0xae/0x5f0 [iwlmvm]
> >                        iwl_mvm_tx_skb+0x200/0x470 [iwlmvm]
> >                        iwl_mvm_mac_itxq_xmit+0xd5/0x1f0 [iwlmvm]
> >                        iwl_mvm_add_new_dqa_stream_wk+0x34c/0x910 [iwlmvm]
> >                        process_one_work+0x1f0/0x5b0
> 
> which I believe we just made use local_bh_disable() since there are
> other reasons as well for that, IIRC.
> 
> Yes, here's the fix:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a
> 
> It's on the way upstream.

If there are other reasons why disable BH for the whole function (are 
there?), then this bigger hammer works as well of course.

	Tested-by: Jiri Kosina <jkosina@suse.cz>

> Thanks for the patch/report though!

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Johannes Berg]

On Mon, 2019-04-15 at 13:33 +0200, Jiri Kosina wrote:

> > Yes, here's the fix:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=f5ae2f932e2f8f4f79796f44832ae8fca26f188a
> > 
> > It's on the way upstream.
> 
> 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.

> 	Tested-by: Jiri Kosina <jkosina@suse.cz>

Too late now to add this to the git tree, but thanks for checking! :-)

johannes

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

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.

Given that we don't have a primitive in lockdep to assert this invariant, 
would you consider queuing the below on top of f5ae2f932e2f in 
wireless-drivers.git?

Thanks.




From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] iwlwifi: iwl_mvm_tx_mpdu() must be called with BH disabled

As iwl_mvm_tx_mpdu() is not disabling BH while obtaining iwl_mvm_sta->lock 
(which is being taken from BH context as well), it has to be always 
invoked with BH disabled. Make that clear in a comment.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/net/wireless/intel/iwlwifi/mvm/tx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index 0c2aabc842f9..ad4760307726 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1063,7 +1063,9 @@ static int iwl_mvm_tx_pkt_queued(struct iwl_mvm *mvm,
 }
 
 /*
- * Sets the fields in the Tx cmd that are crypto related
+ * Sets the fields in the Tx cmd that are crypto related.
+ *
+ * This function must be called with BHs disabled.
  */
 static int iwl_mvm_tx_mpdu(struct iwl_mvm *mvm, struct sk_buff *skb,
 			   struct ieee80211_tx_info *info,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Johannes Berg]

> Given that we don't have a primitive in lockdep to assert this invariant, 
> would you consider queuing the below on top of f5ae2f932e2f in 
> wireless-drivers.git?

I think this would probably go via -next, but sure, I've applied it
internally and we'll let it percolate through the trees as usual.

johannes

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

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@suse.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@suse.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

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

On Wed, 11 Sep 2019, Jiri Kosina wrote:

> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

Hm, scratch that, that might actually spuriously enable BHs if called from 
contexts that already did disabled BHs.

So what solution would you prefer here? Just stick another par of 
bh_disable() / bh_enable() somewhere to the wake_txs() -> 
iwl_mvm_mac_itxq_xmit() -> iwl_mvm_tx_skb() -> iwl_mvm_tx_mpdu() path?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Jiri Kosina]

On Wed, 11 Sep 2019, Jiri Kosina wrote:

> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe
> 
> Hm, scratch that, that might actually spuriously enable BHs if called from 
> contexts that already did disabled BHs.
> 
> So what solution would you prefer here? Just stick another par of 
> bh_disable() / bh_enable() somewhere to the wake_txs() -> 
> iwl_mvm_mac_itxq_xmit() -> iwl_mvm_tx_skb() -> iwl_mvm_tx_mpdu() path?

Ping? This seems to be still the case.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Johannes Berg]

Hi Jiri,

Sorry for the long delay.

>  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]
> 

I'm a bit confused by this.

ieee80211_wake_queues_by_reason() does
spin_lock_irqsave()/spin_unlock_irqrestore() - why is that "{SOFTIRQ-ON-
W} usage"?

Or what did you snip?

johannes

Re: [PATCH v5.1-rc] iwlwifi: make locking in iwl_mvm_tx_mpdu() BH-safe

[Johannes Berg]

On Tue, 2019-10-01 at 11:46 +0200, Johannes Berg wrote:
> 
> ieee80211_wake_queues_by_reason() does
> spin_lock_irqsave()/spin_unlock_irqrestore() - why is that "{SOFTIRQ-ON-
> W} usage"?

scratch that - _ieee80211_wake_txqs() unlocks that again...

It does hold RCU critical section, but that's not the same as disabling
BHs.

I think we should do this perhaps - I think it'd be better to ensure
that the drivers' wake_tx_queue op is always called with softirqs
disabled, since that happens in almost all cases already ...


diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 051a02ddcb85..ad1e88958da2 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -273,9 +273,9 @@ static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac)
 						&txqi->flags))
 				continue;
 
-			spin_unlock_bh(&fq->lock);
+			spin_unlock(&fq->lock);
 			drv_wake_tx_queue(local, txqi);
-			spin_lock_bh(&fq->lock);
+			spin_lock(&fq->lock);
 		}
 	}
 
Perhaps we could add some validation into drv_wake_tx_queue(), but I
didn't find the right thing to call right now ...


Toke, what do you think?

johannes