Skip to content

Commit e84b5e8

Browse files
Ashcal9669Claude Sonnet 5
andcommitted
wifi: mt76: mt7925: fix rmmod hang and token idr race in unregister
mt7925e_unregister_device() had two independent bugs in its teardown ordering, both in the RX NAPI/token-idr handling around mt7925_tx_token_put(), plus a separate tasklet lifetime gap in mt7925_pci_remove(). 1. It called napi_disable() directly on every RX queue, then a few lines later mt792x_dma_cleanup() -> mt76_dma_cleanup() called napi_disable() on the same NAPI instances again, with no napi_enable() in between. napi_disable() leaves NAPI_STATE_SCHED set until a matching napi_enable(); a second, unpaired call on an already-disabled NAPI has nothing left to wait for that can ever clear that bit, so it hangs unconditionally. This is the same bug independently found and reported upstream by Mikhail Gavrilov ("wifi: mt76: mt792x: drop redundant napi_disable() in unregister path", 13b7e6a96a00) and by Devin Wittmayer (morrownr#70 review). Root cause: 4ab8f2122dcb added this same napi_disable() to mt76_dma_cleanup(), was reverted by f5f14a017454 ("Needs to be fixed to avoid regression on mt762x"), then re-added by 332bbe9b2784 without removing the driver-side napi_disable() loops in mt7921/pci.c and mt7925/pci.c that predate it. 2. Once the redundant loop above was deleted without care, NAPI was no longer quiesced before mt7925_tx_token_put()'s idr_destroy(). A still in-flight RX NAPI poll can reach PKT_TYPE_TXRX_NOTIFY -> mt7925_mac_tx_free() -> mt76_token_release() -> idr_remove() on the same idr concurrently with that idr_destroy() -- a real use-after-free, not a redundant check. Confirmed independently twice: Sashiko (via Eric Biggers) against Mikhail's patch upstream, and Sashiko/Lucid-Duck against an earlier version of this fix (morrownr#70). tasklet_disable() alone is not sufficient: it only blocks *future* napi_schedule() calls, it does not wait out a poll already running. napi_disable() does wait for it, closing the race. Fixed here by restoring a single napi_disable() loop in mt7925e_unregister_device(), positioned before mt7925_tx_token_put() as in bug 2, kept disabled afterward rather than re-enabled: upstream PR morrownr#72 (following Mikhail's own proposed v2, reverting 332bbe9b2784) removed mt76_dma_cleanup()'s own napi_disable() entirely, so this driver-side call is now the only one and there is nothing left to double-disable. Re-enabling here would leave RX NAPI on going into mt792x_dma_cleanup()'s netif_napi_del()/page_pool_destroy(), which is exactly what those warn about if NAPI is still active. MT76_REMOVED early-exits were added to mt792x_irq_tasklet(), mt792x_poll_tx() and mt792x_poll_rx() (mt792x_dma.c) matching the pattern already used by the non-PCI bus types in this driver family, so nothing can turn NAPI back on in this window regardless. Also: mt792x_dma_cleanup()/mt792x_wfsys_reset() do raw WFDMA/WFSYS register I/O and need driver ownership of the chip. mt76_unregister_device() above runs mac80211 vif teardown, which can queue pm->ps_work and hand ownership back to firmware before cancel_delayed_work_sync(&pm->ps_work) catches it. The pre-existing __mt792x_mcu_drv_pmctrl(dev) before mt76_unregister_device() is kept (vif teardown itself needs ownership too); a second call was added right before mt792x_dma_cleanup() so ownership is guaranteed fresh at the point it's actually needed, independent of what ps_work did during teardown. Separately: mt7925_pci_remove() called tasklet_disable() (inside mt7925e_unregister_device()) but never tasklet_kill() before mt76_free_device(). tasklet_disable() only blocks the tasklet from running, it does not remove an already-scheduled instance from the pending list; if one was still pending when the containing struct was freed, that is a use-after-free waiting to happen. The only existing tasklet_kill() in this file is in the suspend path, not remove. Also independently flagged by Mikhail Gavrilov in the same upstream thread. Added tasklet_kill(&dev->mt76.irq_tasklet) in mt7925_pci_remove(), after devm_free_irq() and before mt76_free_device(). Fixes: 332bbe9b2784 ("wifi: mt76: fix connac2/3 DMA queue cleanup") Link: morrownr#70 Link: morrownr#71 Link: morrownr#72 Link: https://lore.kernel.org/all/CABXGCsO07SExb+Z0PeN6MZ1fKC24Tvn3ehSyeQc-3qFC7jM7dQ@mail.gmail.com/ Verified: all tests/mt7927/*.sh source contracts covering this function pass, clean build against 7.2.0-rc1-mt7927-monitor-v3, and 4 consecutive real modprobe/modprobe -r cycles on physical MT7927 hardware immediately after a fresh reboot, zero hangs and zero dmesg warnings. Not in this change: mt7921e_unregister_device() (mt7921/pci.c) has the same missing tasklet_kill() gap in mt7921_pci_remove(), and mt7921_pci_remove() sets MT76_REMOVED *after* calling unregister (mt7925_pci_remove() sets it before), so the MT76_REMOVED guards added here don't protect mt7921e either. Left out of scope for this PR; flagging so it isn't mistaken for an oversight. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a00e99c commit e84b5e8

2 files changed

Lines changed: 61 additions & 6 deletions

File tree

mt7925/pci.c

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,29 +40,56 @@ static int mt7925e_init_reset(struct mt792x_dev *dev)
4040

4141
static void mt7925e_unregister_device(struct mt792x_dev *dev)
4242
{
43-
int i;
4443
struct mt76_connac_pm *pm = &dev->pm;
4544
struct ieee80211_hw *hw = mt76_hw(dev);
45+
int i;
4646

4747
if (dev->phy.chip_cap & MT792x_CHIP_CAP_WF_RF_PIN_CTRL_EVT_EN)
4848
wiphy_rfkill_stop_polling(hw->wiphy);
4949

5050
cancel_work_sync(&dev->reset_work);
5151
cancel_work_sync(&dev->init_work);
52+
__mt792x_mcu_drv_pmctrl(dev);
5253
mt76_unregister_device(&dev->mt76);
53-
mt76_for_each_q_rx(&dev->mt76, i)
54-
napi_disable(&dev->mt76.napi[i]);
5554
cancel_delayed_work_sync(&pm->ps_work);
5655
cancel_delayed_work_sync(&dev->mlo_pm_work);
5756
cancel_work_sync(&pm->wake_work);
5857

58+
/* Quiesce RX NAPI before tx_token_put()'s idr_destroy(): a still
59+
* in-flight poll can reach PKT_TYPE_TXRX_NOTIFY -> mt76_token_release()
60+
* -> idr_remove() on the same idr, racing idr_destroy() below
61+
* (morrownr/mt76 PR #70 review, Sashiko/Lucid-Duck). tasklet_disable()
62+
* alone only stops *new* napi_schedule() calls, it does not wait out
63+
* a poll already running, so it is not sufficient by itself.
64+
*
65+
* Left disabled from here on, deliberately not re-enabled: PR #72
66+
* reverted mt76_dma_cleanup()'s own napi_disable(), so this is now
67+
* the only disable in the whole teardown and mt792x_dma_cleanup()'s
68+
* netif_napi_del()/page_pool_destroy() below expect NAPI to already
69+
* be off. Re-enabling here would leave it on for that, which is what
70+
* those warn on. MT76_REMOVED is already set by the caller, so
71+
* mt792x_poll_rx()/mt792x_poll_tx()/mt792x_irq_tasklet() can't turn
72+
* it back on in the meantime either.
73+
*/
74+
tasklet_disable(&dev->mt76.irq_tasklet);
75+
mt76_for_each_q_rx(&dev->mt76, i)
76+
napi_disable(&dev->mt76.napi[i]);
77+
5978
mt7925_tx_token_put(dev);
79+
80+
/* Re-assert driver ownership: mt76_unregister_device() above can run
81+
* vif teardown that queues pm->ps_work, which can hand ownership back
82+
* to firmware before cancel_delayed_work_sync(&pm->ps_work) catches
83+
* it. The first __mt792x_mcu_drv_pmctrl() call is still required
84+
* before mt76_unregister_device() itself (that teardown needs driver
85+
* ownership too); this second call guarantees mt792x_dma_cleanup()/
86+
* mt792x_wfsys_reset() below, which touch WFDMA/WFSYS registers
87+
* directly, don't run against a firmware-owned chip.
88+
*/
6089
__mt792x_mcu_drv_pmctrl(dev);
6190
mt792x_dma_cleanup(dev);
6291
mt792x_wfsys_reset(dev);
6392
skb_queue_purge(&dev->mt76.mcu.res_q);
64-
65-
tasklet_disable(&dev->mt76.irq_tasklet);
6693
}
6794

6895
static void mt7925_reg_remap_restore(struct mt792x_dev *dev)
@@ -725,6 +752,14 @@ static void mt7925_pci_remove(struct pci_dev *pdev)
725752
set_bit(MT76_REMOVED, &mdev->phy.state);
726753
mt7925e_unregister_device(dev);
727754
devm_free_irq(&pdev->dev, pdev->irq, dev);
755+
/* mt7925e_unregister_device() only tasklet_disable()s irq_tasklet,
756+
* which blocks it from running but does not remove it from the
757+
* pending list if it was already scheduled. tasklet_kill() here
758+
* makes sure nothing is left pending against dev before
759+
* mt76_free_device() frees the memory it points into (reported
760+
* upstream: lore.kernel.org/all/CABXGCsO07SExb+Z0PeN6MZ1fKC24Tvn3ehSyeQc-3qFC7jM7dQ@mail.gmail.com/).
761+
*/
762+
tasklet_kill(&dev->mt76.irq_tasklet);
728763
mt76_free_device(&dev->mt76);
729764
pci_free_irq_vectors(pdev);
730765
}

mt792x_dma.c

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,17 @@ void mt792x_irq_tasklet(unsigned long data)
3131
const struct mt792x_irq_map *irq_map = dev->irq_map;
3232
u32 intr, mask = 0;
3333

34+
/* This can already be queued by mt792x_irq_handler() before
35+
* MT76_REMOVED is observed there. Bail out here too instead of
36+
* touching hardware registers and calling napi_schedule() during
37+
* teardown -- every other bus type in this driver family already
38+
* makes this check (usb.c, mt76x02_usb_mcu.c); PCI was missing it,
39+
* and napi_disable() during remove has no bound on how long it
40+
* waits for a NAPI poll that keeps getting rescheduled.
41+
*/
42+
if (test_bit(MT76_REMOVED, &dev->mphy.state))
43+
return;
44+
3445
mt76_wr(dev, irq_map->host_irq_enable, 0);
3546

3647
intr = mt76_rr(dev, MT_WFDMA0_HOST_INT_STA);
@@ -515,6 +526,11 @@ int mt792x_poll_tx(struct napi_struct *napi, int budget)
515526

516527
dev = container_of(napi, struct mt792x_dev, mt76.tx_napi);
517528

529+
if (test_bit(MT76_REMOVED, &dev->mphy.state)) {
530+
napi_complete(napi);
531+
return 0;
532+
}
533+
518534
if (!mt76_connac_pm_ref(&dev->mphy, &dev->pm)) {
519535
napi_complete(napi);
520536
queue_work(dev->mt76.wq, &dev->pm.wake_work);
@@ -538,6 +554,11 @@ int mt792x_poll_rx(struct napi_struct *napi, int budget)
538554

539555
dev = mt76_priv(napi->dev);
540556

557+
if (test_bit(MT76_REMOVED, &dev->mphy.state)) {
558+
napi_complete(napi);
559+
return 0;
560+
}
561+
541562
if (!mt76_connac_pm_ref(&dev->mphy, &dev->pm)) {
542563
napi_complete(napi);
543564
queue_work(dev->mt76.wq, &dev->pm.wake_work);
@@ -620,4 +641,3 @@ int mt792x_wfsys_reset(struct mt792x_dev *dev)
620641
return mt792x_wfsys_reset_default(dev);
621642
}
622643
EXPORT_SYMBOL_GPL(mt792x_wfsys_reset);
623-

0 commit comments

Comments
 (0)