Re: [PATCH] wifi: ath9k: stop WMI event tasklet from rescheduling itself on probe failure

[Jeff Johnson <jeff.johnson@oss.qualcomm.com>]

On 10/2/2026 7:11 PM, Deepanshu Kartikey wrote:
> Since the init guards were consolidated, ath9k_wmi_event_tasklet()
> reschedules itself until priv->initialized is set. If
> ath9k_init_device() fails, initialized is never set, so the tasklet
> keeps rescheduling itself while the probe error path calls
> ath9k_destroy_wmi() and frees struct wmi. tasklet_action_common() then
> touches the freed tasklet, causing a slab-use-after-free.
> 
> Make the tasklet drop its queued events and stop rescheduling once
> wmi->stopped is set, and call tasklet_kill() before freeing wmi in
> ath9k_destroy_wmi(). The probe error path already sets stopped and
> kills the RX URBs first, so the tasklet cannot be re-armed afterwards.
> 
> Fixes: 1af8dc9ca726 ("wifi: ath9k: Clean up device initialisation guards")
> Reported-by: syzbot+e91dc13b1b37759b3d9f@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e91dc13b1b37759b3d9f
> Tested-by: syzbot+e91dc13b1b37759b3d9f@syzkaller.appspotmail.com
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  drivers/net/wireless/ath/ath9k/wmi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
> index 4a08babb6d9d..f8b174b0f209 100644
> --- a/drivers/net/wireless/ath/ath9k/wmi.c
> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
> @@ -122,6 +122,10 @@ void ath9k_stop_wmi(struct ath9k_htc_priv *priv)
>  
>  void ath9k_destroy_wmi(struct ath9k_htc_priv *priv)
>  {
> +	struct wmi *wmi = priv->wmi;
> +
> +	tasklet_kill(&wmi->wmi_event_tasklet);
> +	skb_queue_purge(&wmi->wmi_event_queue);
>  	kfree(priv->wmi);
>  }
>  
> @@ -148,6 +152,10 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
>  
>  	/* Check if ath9k_init_device() completed. */
>  	if (!smp_load_acquire(&priv->initialized)) {
> +		if (READ_ONCE(wmi->stopped)) {
> +			skb_queue_purge(&wmi->wmi_event_queue);

Can you check my review agent's analysis:

Locking inconsistency in the new tasklet early-exit path** (`wmi.c:156`)
`wmi_event_queue` has two lockguards in this file:
- **producer** (`ath9k_wmi_ctrl_rx`, line 249): holds `wmi->wmi_lock`, uses
`__skb_queue_tail()` (lockless variant)
- **consumer** (tasklet normal path, line 164): holds `wmi->wmi_lock`, uses
`__skb_dequeue()`
- **`ath9k_wmi_event_drain()`** (line 137–139): holds `wmi->wmi_lock`, uses
`__skb_queue_purge()`

The new `skb_queue_purge()` (locking variant) uses `queue->lock`, which is a
**different lock** from `wmi->wmi_lock`. The window between
`ath9k_stop_wmi()` setting `stopped=true` and `ath9k_hif_usb_dealloc_urbs()`
finishing means `ath9k_wmi_ctrl_rx()` could still be enqueuing under
`wmi_lock` while the tasklet purges under `queue->lock` — two different
locks protecting the same queue simultaneously.

It works in practice because `ath9k_wmi_ctrl_rx()` also checks
`unlikely(wmi->stopped)` at entry and bails early, but the locking is
technically unsound. The correct pattern (as used in
`ath9k_wmi_event_drain()`) would be:

```c
if (READ_ONCE(wmi->stopped)) {
    spin_lock_irqsave(&wmi->wmi_lock, flags);
    __skb_queue_purge(&wmi->wmi_event_queue);
    spin_unlock_irqrestore(&wmi->wmi_lock, flags);
    return;
}
```

The `skb_queue_purge()` in `ath9k_destroy_wmi()` (line 128) is called after
`tasklet_kill()` and after `ath9k_hif_usb_dealloc_urbs()`, so no concurrent
producer can run there — that usage is safe.

I always have another agent verify these automated code reviews, and my
verification agent concurred:

Issue #1 — Locking inconsistency at wmi.c:156 — Valid

The review is correct. Confirmed from the live source:

All code that touches wmi_event_queue uses wmi->wmi_lock with the lockless
__skb_* variants:
- producer ath9k_wmi_ctrl_rx (lines 249–252):
spin_lock_irqsave(&wmi->wmi_lock) + __skb_queue_tail()
- consumer tasklet normal path (lines 164–169):
spin_lock_irqsave(&wmi->wmi_lock) + __skb_dequeue()
- drain ath9k_wmi_event_drain (lines 137–139):
spin_lock_irqsave(&wmi->wmi_lock) + __skb_queue_purge()

The new early-exit path (lines 155–157) breaks this pattern by using
skb_queue_purge() (the locking variant, which takes queue->lock — a completely
different lock). The race is real: ath9k_wmi_ctrl_rx's guard at line 238 is a
plain read of wmi->stopped (not even READ_ONCE), so it can miss the
stopped=true write and proceed to enqueue under wmi->wmi_lock while the
tasklet purges under queue->lock. Two unrelated locks protecting the same
queue simultaneously is unsound.

The skb_queue_purge() in the new ath9k_destroy_wmi() (line 128) is fine — it
runs after tasklet_kill() and after URB dealloc, so no concurrent producer
exists there.

Fix: mirror ath9k_wmi_event_drain's pattern in the tasklet early-exit:
if (READ_ONCE(wmi->stopped)) {
    spin_lock_irqsave(&wmi->wmi_lock, flags);
    __skb_queue_purge(&wmi->wmi_event_queue);
    spin_unlock_irqrestore(&wmi->wmi_lock, flags);
    return;
}
Note flags is already declared in the tasklet function (line 150), so no new
variable is needed.

> +			return;
> +		}
>  		tasklet_schedule(t);
>  		return;
>  	}