[PATCH 1/4] mt76: fix concurrent rx calls on A-MPDU release

[PATCH 1/4] mt76: fix concurrent rx calls on A-MPDU release

[Felix Fietkau]

Add a spinlock in mt76_rx_complete. Without this, multiple stats updates
could happen in parallel, which can lead to deadlocks. There are
probably more corner cases fixed by this change.

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/mac80211.c    | 2 ++
 drivers/net/wireless/mediatek/mt76/mt76.h        | 1 +
 drivers/net/wireless/mediatek/mt76/mt76x2_init.c | 1 +
 3 files changed, 4 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index eb49e0a6758c..915e61733131 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -561,6 +561,7 @@ void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
 	if (queue >= 0)
 	    napi = &dev->napi[queue];
 
+	spin_lock(&dev->rx_lock);
 	while ((skb = __skb_dequeue(frames)) != NULL) {
 		if (mt76_check_ccmp_pn(skb)) {
 			dev_kfree_skb(skb);
@@ -570,6 +571,7 @@ void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
 		sta = mt76_rx_convert(skb);
 		ieee80211_rx_napi(dev->hw, sta, skb, napi);
 	}
+	spin_unlock(&dev->rx_lock);
 }
 
 void mt76_rx_poll_complete(struct mt76_dev *dev, enum mt76_rxq_id q)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 065ff78059c3..a74e6eef51e9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -241,6 +241,7 @@ struct mt76_dev {
 	struct device *dev;
 
 	struct net_device napi_dev;
+	spinlock_t rx_lock;
 	struct napi_struct napi[__MT_RXQ_MAX];
 	struct sk_buff_head rx_skb[__MT_RXQ_MAX];
 
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2_init.c b/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
index 359b105235b3..d21e4a7c1bb9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2_init.c
@@ -645,6 +645,7 @@ struct mt76x2_dev *mt76x2_alloc_device(struct device *pdev)
 	dev->mt76.drv = &drv_ops;
 	mutex_init(&dev->mutex);
 	spin_lock_init(&dev->irq_lock);
+	spin_lock_init(&dev->mt76.rx_lock);
 
 	return dev;
 }
-- 
2.14.2

[PATCH 2/4] mt76: add rcu locking in tid reorder function

[Felix Fietkau]

Avoids having the tid or station entry disappear prematurely.
Also cancel the reorder work earlier to avoid further processing delayed
by waiting for the lock to be released

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/agg-rx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/agg-rx.c b/drivers/net/wireless/mediatek/mt76/agg-rx.c
index dbf4057d2d3e..b67acc6189bf 100644
--- a/drivers/net/wireless/mediatek/mt76/agg-rx.c
+++ b/drivers/net/wireless/mediatek/mt76/agg-rx.c
@@ -103,6 +103,7 @@ mt76_rx_aggr_reorder_work(struct work_struct *work)
 	__skb_queue_head_init(&frames);
 
 	local_bh_disable();
+	rcu_read_lock();
 
 	spin_lock(&tid->lock);
 	mt76_rx_aggr_check_release(tid, &frames);
@@ -114,6 +115,7 @@ mt76_rx_aggr_reorder_work(struct work_struct *work)
 					     REORDER_TIMEOUT);
 	mt76_rx_complete(dev, &frames, -1);
 
+	rcu_read_unlock();
 	local_bh_enable();
 }
 
@@ -266,6 +268,8 @@ static void mt76_rx_aggr_shutdown(struct mt76_dev *dev, struct mt76_rx_tid *tid)
 	u8 size = tid->size;
 	int i;
 
+	cancel_delayed_work(&tid->reorder_work);
+
 	spin_lock_bh(&tid->lock);
 
 	tid->stopped = true;
@@ -280,8 +284,6 @@ static void mt76_rx_aggr_shutdown(struct mt76_dev *dev, struct mt76_rx_tid *tid)
 	}
 
 	spin_unlock_bh(&tid->lock);
-
-	cancel_delayed_work(&tid->reorder_work);
 }
 
 void mt76_rx_aggr_stop(struct mt76_dev *dev, struct mt76_wcid *wcid, u8 tidno)
-- 
2.14.2

[PATCH 3/4] mt76: add rcu locking around tx scheduling

[Felix Fietkau]

Fixes a reported lockdep error in mac80211:

[  179.867321] =============================
[  179.871510] WARNING: suspicious RCU usage
[  179.875528] 4.14.32 #0 Not tainted
[  179.878924] -----------------------------
[  179.882981] backports-2017-11-01/net/mac80211/tx.c:594 suspicious rcu_dereference_check() usage!
[  179.891785]
[  179.891785] other info that might help us debug this:
[  179.891785]
[  179.899824]
[  179.899824] rcu_scheduler_active = 2, debug_locks = 1
[  179.906343] 2 locks held by ksoftirqd/0/7:
[  179.910479]  #0:  (&(&q->lock)->rlock){+.-.}, at: [<86b207a4>] mt76_dma_tx_cleanup+0x64/0x354 [mt76]
[  179.919734]  #1:  (&(&fq->lock)->rlock){+.-.}, at: [<87238410>] ieee80211_tx_dequeue+0x54/0xc3c [mac80211]
[  179.929890]
[  179.929890] stack backtrace:
[  179.934257] CPU: 0 PID: 7 Comm: ksoftirqd/0 Not tainted 4.14.32 #0
[  179.940421] Stack : 00000000 00000000 00000000 00000000 80e0fce2 00000036 00000000 00000000
[  179.948864]         87c3d24c 80696377 8061039c 00000000 00000007 00000001 87c5db78 6534689d
[  179.957306]         00000000 00000000 80e10000 87c5da74 00000001 0000015a 00000007 00000000
[  179.965748]         00000000 806a0000 000e4171 00000000 00000000 00000000 ffffffff 00000001
[  179.974189]         806c0000 8692b240 86b000d0 87316fe4 00000001 802c9a68 00000000 80700000
[  179.982632]         ...
[  179.985104] Call Trace:
[  179.987582] [<80010a48>] show_stack+0x58/0x100
[  179.992040] [<804c2c58>] dump_stack+0xe8/0x170
[  179.996868] [<87234a04>] ieee80211_tx_h_select_key+0xa8/0x5b8 [mac80211]
[  180.004299] [<87238d44>] ieee80211_tx_dequeue+0x988/0xc3c [mac80211]
[  180.011048] [<86b230dc>] mt76_txq_schedule+0x110/0x3a4 [mt76]
[  180.016821] [<86b209d0>] mt76_dma_tx_cleanup+0x290/0x354 [mt76]
[  180.022777] [<86be2e60>] mt7603_tx_tasklet+0x40/0x6c [mt7603e]
[  180.028637] [<80037058>] tasklet_action+0x110/0x1ec
[  180.033532] [<804e1dac>] __do_softirq+0x164/0x35c
[  180.038235] [<80037174>] run_ksoftirqd+0x40/0x84
[  180.042870] [<800580c8>] smpboot_thread_fn+0x1a8/0x1d8
[  180.048023] [<800542e8>] kthread+0x130/0x144
[  180.052297] [<8000b1f8>] ret_from_kernel_thread+0x14/0x1c

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/tx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 4eef69bd8a9e..6aca794cf998 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -422,12 +422,14 @@ void mt76_txq_schedule(struct mt76_dev *dev, struct mt76_queue *hwq)
 {
 	int len;
 
+	rcu_read_lock();
 	do {
 		if (hwq->swq_queued >= 4 || list_empty(&hwq->swq))
 			break;
 
 		len = mt76_txq_schedule_list(dev, hwq);
 	} while (len > 0);
+	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(mt76_txq_schedule);
 
-- 
2.14.2

[PATCH 4/4] mt76: check for pending reset before attempting to schedule tx

[Felix Fietkau]

The check within mt76_txq_send_burst is not enough, as it happens after
a first frame has already been queued up

Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/mediatek/mt76/tx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 6aca794cf998..7ecd2d7c5db4 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -385,6 +385,10 @@ mt76_txq_schedule_list(struct mt76_dev *dev, struct mt76_queue *hwq)
 		bool empty = false;
 		int cur;
 
+		if (test_bit(MT76_SCANNING, &dev->state) ||
+		    test_bit(MT76_RESET, &dev->state))
+			return -EBUSY;
+
 		mtxq = list_first_entry(&hwq->swq, struct mt76_txq, list);
 		if (mtxq->send_bar && mtxq->aggr) {
 			struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
-- 
2.14.2

Re: [1/4] mt76: fix concurrent rx calls on A-MPDU release

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> wrote:

> Add a spinlock in mt76_rx_complete. Without this, multiple stats updates
> could happen in parallel, which can lead to deadlocks. There are
> probably more corner cases fixed by this change.
> 
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

4 patches applied to wireless-drivers-next.git, thanks.

c3d7c82a8bb0 mt76: fix concurrent rx calls on A-MPDU release
9febfa67ca15 mt76: add rcu locking in tid reorder function
1d868b70e06a mt76: add rcu locking around tx scheduling
95135e8c0b4a mt76: check for pending reset before attempting to schedule tx

-- 
https://patchwork.kernel.org/patch/10361963/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches