Commit e84b5e8
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
(#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
(#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 #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: #70
Link: #71
Link: #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
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
40 | 40 | | |
41 | 41 | | |
42 | 42 | | |
43 | | - | |
44 | 43 | | |
45 | 44 | | |
| 45 | + | |
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
52 | 53 | | |
53 | | - | |
54 | | - | |
55 | 54 | | |
56 | 55 | | |
57 | 56 | | |
58 | 57 | | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
59 | 78 | | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
60 | 89 | | |
61 | 90 | | |
62 | 91 | | |
63 | 92 | | |
64 | | - | |
65 | | - | |
66 | 93 | | |
67 | 94 | | |
68 | 95 | | |
| |||
725 | 752 | | |
726 | 753 | | |
727 | 754 | | |
| 755 | + | |
| 756 | + | |
| 757 | + | |
| 758 | + | |
| 759 | + | |
| 760 | + | |
| 761 | + | |
| 762 | + | |
728 | 763 | | |
729 | 764 | | |
730 | 765 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
31 | 31 | | |
32 | 32 | | |
33 | 33 | | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
34 | 45 | | |
35 | 46 | | |
36 | 47 | | |
| |||
515 | 526 | | |
516 | 527 | | |
517 | 528 | | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
518 | 534 | | |
519 | 535 | | |
520 | 536 | | |
| |||
538 | 554 | | |
539 | 555 | | |
540 | 556 | | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
541 | 562 | | |
542 | 563 | | |
543 | 564 | | |
| |||
620 | 641 | | |
621 | 642 | | |
622 | 643 | | |
623 | | - | |
| |||
0 commit comments