Skip to content

wifi: mt76: mt7925: fix rmmod hang and token idr race in unregister - #71

Open
Ashcal9669 wants to merge 1 commit into
morrownr:mainfrom
Ashcal9669:mt7925-fix-rmmod-napi-hang
Open

wifi: mt76: mt7925: fix rmmod hang and token idr race in unregister#71
Ashcal9669 wants to merge 1 commit into
morrownr:mainfrom
Ashcal9669:mt7925-fix-rmmod-napi-hang

Conversation

@Ashcal9669

Copy link
Copy Markdown
Contributor

Supersedes #70 (closed after review found a real race in that version).

@Lucid-Duck found that the napi_disable() loop removed in #70 wasn't
redundant: it was accidentally quiescing NAPI before
mt7925_tx_token_put()'s idr_destroy(). Without it, a still in-flight
RX NAPI poll can reach PKT_TYPE_TXRX_NOTIFY -> mt76_token_release() -> idr_remove() on the same idr concurrently with that idr_destroy().

This version:

  • Restores a single napi_disable()/napi_enable() pair around
    mt7925_tx_token_put()napi_disable() synchronously waits out any
    in-flight poll, closing the race before idr_destroy() runs.
  • Keeps the original double-napi_disable() hang fixed by adding
    MT76_REMOVED early-exits to mt792x_irq_tasklet(), mt792x_poll_tx(),
    mt792x_poll_rx() (mt792x_dma.c), matching the pattern already used by
    the non-PCI bus types in this driver family. napi_enable()'s re-open
    window is inert because of these guards, so mt792x_dma_cleanup()'s
    later napi_disable() is always a fresh, terminating one.
  • Adds a second __mt792x_mcu_drv_pmctrl() call right before
    mt792x_dma_cleanup()/mt792x_wfsys_reset(): those do raw WFDMA/WFSYS
    register I/O and need driver ownership, but mt76_unregister_device()'s
    vif teardown can queue pm->ps_work and hand ownership back to firmware
    before cancel_delayed_work_sync(&pm->ps_work) catches it. The
    pre-existing call before mt76_unregister_device() is kept since that
    teardown itself needs ownership too.

Full mechanism and the double-disable history (4ab8f2122dcb
f5f14a017454 revert → 332bbe9b2784 reintroduction) are in the commit
message.

Verified: all tests/mt7927/*.sh source contracts covering this
function pass, clean build against 7.2.0-rc1-mt7927-monitor-v3, and 8
consecutive real modprobe/modprobe -r cycles on physical MT7927
hardware with zero hangs and zero dmesg warnings.

Out of scope, flagged not missed: mt7921e_unregister_device()
(mt7921/pci.c) has the identical double-napi_disable() pattern, and
mt7921_pci_remove() sets MT76_REMOVED after calling unregister
(mt7925_pci_remove() sets it before), so the shared guards added here
don't protect mt7921e. Happy to follow up there if wanted.

@Lucid-Duck

Copy link
Copy Markdown
Collaborator

@Ashcal9669 I've opened #72, a revert of the commit that added the second disable, which changes what this one should be.

Once that lands, the napi_enable here leaves receive NAPI on when the queues get deleted, and that brings back the warnings the original commit was written to stop. Enabling clears the bit that both netif_napi_del and the page pool teardown warn on.

The MT76_REMOVED checks are a different story and worth keeping either way. They close a real window and they hold with or without the revert.

If you narrow this to just those, I'll merge it.

@Ashcal9669

Copy link
Copy Markdown
Contributor Author

@Lucid-Duck on it. Thank you for reviewing it. I'll report back asap.

@Ashcal9669

Copy link
Copy Markdown
Contributor Author

Narrowed to just the MT76_REMOVED guards + napi_disable() (no re-enable), as requested — force-pushed. Also folded in a separate, related bug: tasklet_kill() was missing from mt7925_pci_remove() (only present in the suspend path), which Mikhail Gavrilov flagged in the same upstream thread as a potential use-after-free if a tasklet is still pending when mt76_free_device() frees the memory it points into. Added tasklet_kill(&dev->mt76.irq_tasklet) there.

Verified: contracts pass, clean build, 4 consecutive modprobe/modprobe -r cycles on physical MT7927 hardware immediately after a fresh reboot, zero hangs/warnings.

Ashcal9669 added a commit to Ashcal9669/mt76 that referenced this pull request Aug 1, 2026
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>
@Ashcal9669
Ashcal9669 force-pushed the mt7925-fix-rmmod-napi-hang branch from 5fa05b7 to e84b5e8 Compare August 1, 2026 02:51
@Lucid-Duck

Copy link
Copy Markdown
Collaborator

@Ashcal9669 Good catch folding in the tasklet_kill, that gap is real.

One thing about tasklet_kill that caught me out when I went looking: it waits on TASKLET_STATE_SCHED, and a disabled tasklet gets requeued without ever clearing that bit, so a kill on a disabled one spins instead of returning. unregister_device disables irq_tasklet and nothing re-enables it, so remove arrives with the count still up. The suspend path sidesteps it by doing synchronize_irq then kill with nothing disabled. Might be simplest to drop the tasklet_disable and let kill cover both jobs, since your MT76_REMOVED guard already keeps it off the hardware.

One I could not follow: the message calls the pmctrl before mt76_unregister_device the pre-existing one, but on main the only one is after tx_token_put. Which did you mean?

Worth knowing an idle unload will not show this either way. It needs a tasklet actually pending, so traffic in flight when remove runs. I can put that through here if it would help.

Ashcal9669 added a commit to Ashcal9669/mt76 that referenced this pull request Aug 1, 2026
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. A new
__mt792x_mcu_drv_pmctrl(dev) call was added right before
mt76_unregister_device(), since that teardown itself needs ownership
too; the existing call right before mt792x_dma_cleanup() is unchanged
from before this fix and guarantees ownership is fresh again at the
point it's actually needed, independent of what ps_work did in
between.

Separately: mt7925_pci_remove() never called tasklet_kill() on
irq_tasklet before mt76_free_device(); the only existing
tasklet_kill() in this file is in the suspend path. tasklet_disable()
alone does not remove an already-scheduled instance from the pending
list, so a tasklet still pending when the containing struct is freed
is a use-after-free waiting to happen. Independently flagged upstream
by Mikhail Gavrilov in the same thread as bug 1. Added
tasklet_kill(&dev->mt76.irq_tasklet) in mt7925_pci_remove(), after
devm_free_irq() and before mt76_free_device().

That fix's first version also added a tasklet_disable() in
mt7925e_unregister_device(), reasoning it would help close bug 2 the
same way napi_disable() does. It doesn't, and it actively conflicts
with tasklet_kill(): tasklet_kill() waits for TASKLET_STATE_SCHED to
clear, but a *disabled* tasklet that gets scheduled never clears that
bit (the softirq handler sees it's disabled and just re-queues it), so
tasklet_kill() on a disabled-and-pending tasklet spins instead of
returning (morrownr#71 review, Lucid-Duck). mt792x_irq_handler()
already checks MT76_REMOVED before calling tasklet_schedule() at all,
so no new scheduling can happen once it's set (before this function
runs); mt792x_irq_tasklet()'s own MT76_REMOVED guard makes the tasklet
body itself inert if it does run once more from an IRQ that raced the
flag. Between those two, tasklet_disable() here was not preventing
anything tasklet_kill() doesn't already handle correctly on its own,
so it was dropped.

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, 4
consecutive real modprobe/modprobe -r cycles on physical MT7927
hardware idle, and 5 more with active RX traffic in flight during each
unload, zero hangs and zero dmesg warnings across all of them.

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>
@Ashcal9669
Ashcal9669 force-pushed the mt7925-fix-rmmod-napi-hang branch from e84b5e8 to ee118b2 Compare August 1, 2026 07:45
Ashcal9669 added a commit to Ashcal9669/mt76 that referenced this pull request Aug 1, 2026
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. A new
__mt792x_mcu_drv_pmctrl(dev) call was added right before
mt76_unregister_device(), since that teardown itself needs ownership
too; the existing call right before mt792x_dma_cleanup() is unchanged
from before this fix and guarantees ownership is fresh again at the
point it's actually needed, independent of what ps_work did in
between.

Separately: mt7925_pci_remove() never called tasklet_kill() on
irq_tasklet before mt76_free_device(); the only existing
tasklet_kill() in this file is in the suspend path. tasklet_disable()
alone does not remove an already-scheduled instance from the pending
list, so a tasklet still pending when the containing struct is freed
is a use-after-free waiting to happen. Independently flagged upstream
by Mikhail Gavrilov in the same thread as bug 1. Added
tasklet_kill(&dev->mt76.irq_tasklet) in mt7925_pci_remove(), after
devm_free_irq() and before mt76_free_device().

That fix's first version also added a tasklet_disable() in
mt7925e_unregister_device(), reasoning it would help close bug 2 the
same way napi_disable() does. It doesn't, and it actively conflicts
with tasklet_kill(): tasklet_kill() waits for TASKLET_STATE_SCHED to
clear, but a *disabled* tasklet that gets scheduled never clears that
bit (the softirq handler sees it's disabled and just re-queues it), so
tasklet_kill() on a disabled-and-pending tasklet spins instead of
returning (morrownr#71 review, Lucid-Duck). mt792x_irq_handler()
already checks MT76_REMOVED before calling tasklet_schedule() at all,
so no new scheduling can happen once it's set (before this function
runs); mt792x_irq_tasklet()'s own MT76_REMOVED guard makes the tasklet
body itself inert if it does run once more from an IRQ that raced the
flag. Between those two, tasklet_disable() here was not preventing
anything tasklet_kill() doesn't already handle correctly on its own,
so it was dropped.

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, 4
consecutive real modprobe/modprobe -r cycles on physical MT7927
hardware idle, and 5 more with active RX traffic in flight during each
unload, zero hangs and zero dmesg warnings across all of them.

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>
@Ashcal9669
Ashcal9669 force-pushed the mt7925-fix-rmmod-napi-hang branch from ee118b2 to 5747653 Compare August 1, 2026 07:46
@Ashcal9669

Copy link
Copy Markdown
Contributor Author

@Lucid-Duck Both real, force-pushed a fix for both.

tasklet_kill spin: you're right, and I found the mechanism you're describing confirmed in mt792x_irq_handler() — it already checks MT76_REMOVED before tasklet_schedule(), so no new scheduling happens once that's set regardless of tasklet_disable(). Dropped the tasklet_disable() call in mt7925e_unregister_device() entirely; tasklet_kill() in mt7925_pci_remove() now does the whole job on its own, relying on mt792x_irq_tasklet()'s own MT76_REMOVED guard to stay inert if it does run once more. Tested idle (4x clean) and with active RX traffic in flight during unload (5x clean, tcpdump running concurrently) — didn't hit the spin, but I'll take the offer if you're able to put real traffic through it on your end, the window's inherently narrow and I'd rather have your confirmation than just mine.

pmctrl wording: my mistake, not yours — checked the actual pre-PR base (c1455af3) and there was never a call before mt76_unregister_device(), only the one after tx_token_put() that's still there unchanged. I misdescribed the first one as "pre-existing" when it's new. Commit message corrected.

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. A new
__mt792x_mcu_drv_pmctrl(dev) call was added right before
mt76_unregister_device(), since that teardown itself needs ownership
too; the existing call right before mt792x_dma_cleanup() is unchanged
from before this fix and guarantees ownership is fresh again at the
point it's actually needed, independent of what ps_work did in
between.

Separately: mt7925_pci_remove() never called tasklet_kill() on
irq_tasklet before mt76_free_device(); the only existing
tasklet_kill() in this file is in the suspend path. tasklet_disable()
alone does not remove an already-scheduled instance from the pending
list, so a tasklet still pending when the containing struct is freed
is a use-after-free waiting to happen. Independently flagged upstream
by Mikhail Gavrilov in the same thread as bug 1. Added
tasklet_kill(&dev->mt76.irq_tasklet) in mt7925_pci_remove(), after
devm_free_irq() and before mt76_free_device().

That fix's first version also added a tasklet_disable() in
mt7925e_unregister_device(), reasoning it would help close bug 2 the
same way napi_disable() does. It doesn't, and it actively conflicts
with tasklet_kill(): tasklet_kill() waits for TASKLET_STATE_SCHED to
clear, but a *disabled* tasklet that gets scheduled never clears that
bit (the softirq handler sees it's disabled and just re-queues it), so
tasklet_kill() on a disabled-and-pending tasklet spins instead of
returning (morrownr#71 review, Lucid-Duck). mt792x_irq_handler()
already checks MT76_REMOVED before calling tasklet_schedule() at all,
so no new scheduling can happen once it's set (before this function
runs); mt792x_irq_tasklet()'s own MT76_REMOVED guard makes the tasklet
body itself inert if it does run once more from an IRQ that raced the
flag. Between those two, tasklet_disable() here was not preventing
anything tasklet_kill() doesn't already handle correctly on its own,
so it was dropped.

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, 4
consecutive real modprobe/modprobe -r cycles on physical MT7927
hardware idle, and 5 more with active RX traffic in flight during each
unload, zero hangs and zero dmesg warnings across all of them.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants