[PATCH net] wifi: ath9k: Fix potential spin_lock() before spin_lock_init()

[PATCH net] wifi: ath9k: Fix potential spin_lock() before spin_lock_init()

[Thomas Fourier]

The function ath9k_init_wmi() initializes wmi->wmi_lock. It is called in
ath9k_htc_probe_device(), and the priv->initialized flag is set.
However, the ath9k_wmi_event_tasklet takes the lock before checking the
priv->initialized flag, so the lock may not be initialized before
being taken.  This could be the case, for example, if the spin_lock_init()
is reordered with tasklet_setup() in ath9k_init_wmi() by the compiler or
CPU.

There is a write memory barrier before setting the priv->initialized,
but no corresponding read memory barrier is used after checking the
flag.

Move priv->initialized at the start of ath9k_wmi_event_tasklet() and
add a corresponding read memory barrier.

Fixes: 24355fcb0d4c ("wifi: ath9k: delay all of ath9k_wmi_event_tasklet() until init is complete")
Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/net/wireless/ath/ath9k/wmi.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
index 284e8c13b043..df4a3a625536 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -146,6 +146,15 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
 	unsigned long flags;
 	u16 cmd_id;
 
+	/* Check if ath9k_htc_probe_device() completed. */
+	if (!data_race(priv->initialized))
+		return;
+	/*
+	 * Make sure ath9k_htc_probe_device() initialization is
+	 * committed to memory before processing skb.
+	 */
+	smp_rmb();
+
 	do {
 		spin_lock_irqsave(&wmi->wmi_lock, flags);
 		skb = __skb_dequeue(&wmi->wmi_event_queue);
@@ -155,12 +164,6 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
 		}
 		spin_unlock_irqrestore(&wmi->wmi_lock, flags);
 
-		/* Check if ath9k_htc_probe_device() completed. */
-		if (!data_race(priv->initialized)) {
-			kfree_skb(skb);
-			continue;
-		}
-
 		hdr = (struct wmi_cmd_hdr *) skb->data;
 		cmd_id = be16_to_cpu(hdr->command_id);
 		wmi_event = skb_pull(skb, sizeof(struct wmi_cmd_hdr));
-- 
2.43.0

Re: [PATCH net] wifi: ath9k: Fix potential spin_lock() before spin_lock_init()

[Toke Høiland-Jørgensen]

Thomas Fourier <fourier.thomas@gmail.com> writes:

> The function ath9k_init_wmi() initializes wmi->wmi_lock. It is called in
> ath9k_htc_probe_device(), and the priv->initialized flag is set.
> However, the ath9k_wmi_event_tasklet takes the lock before checking the
> priv->initialized flag, so the lock may not be initialized before
> being taken.  This could be the case, for example, if the spin_lock_init()
> is reordered with tasklet_setup() in ath9k_init_wmi() by the compiler or
> CPU.
>
> There is a write memory barrier before setting the priv->initialized,
> but no corresponding read memory barrier is used after checking the
> flag.
>
> Move priv->initialized at the start of ath9k_wmi_event_tasklet() and
> add a corresponding read memory barrier.
>
> Fixes: 24355fcb0d4c ("wifi: ath9k: delay all of ath9k_wmi_event_tasklet() until init is complete")
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
>  drivers/net/wireless/ath/ath9k/wmi.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
> index 284e8c13b043..df4a3a625536 100644
> --- a/drivers/net/wireless/ath/ath9k/wmi.c
> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
> @@ -146,6 +146,15 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
>  	unsigned long flags;
>  	u16 cmd_id;
>  
> +	/* Check if ath9k_htc_probe_device() completed. */
> +	if (!data_race(priv->initialized))
> +		return;

Moving this out of the loop changes behaviour: Before, the loop would
keep spinning waiting for initialisation, now we just exit. I don't see
any guarantee that we'll come back here, so this has the risk of
stalling things. We'll need to re-schedule the tasklet before returning
if we're moving the check here.

-Toke

Re: [PATCH net] wifi: ath9k: Fix potential spin_lock() before spin_lock_init()

[Thomas Fourier]

On 08/09/2026 11:25, Toke Høiland-Jørgensen wrote:
> Thomas Fourier <fourier.thomas@gmail.com> writes:
> 
>> The function ath9k_init_wmi() initializes wmi->wmi_lock. It is called in
>> ath9k_htc_probe_device(), and the priv->initialized flag is set.
>> However, the ath9k_wmi_event_tasklet takes the lock before checking the
>> priv->initialized flag, so the lock may not be initialized before
>> being taken.  This could be the case, for example, if the spin_lock_init()
>> is reordered with tasklet_setup() in ath9k_init_wmi() by the compiler or
>> CPU.
>>
>> There is a write memory barrier before setting the priv->initialized,
>> but no corresponding read memory barrier is used after checking the
>> flag.
>>
>> Move priv->initialized at the start of ath9k_wmi_event_tasklet() and
>> add a corresponding read memory barrier.
>>
>> Fixes: 24355fcb0d4c ("wifi: ath9k: delay all of ath9k_wmi_event_tasklet() until init is complete")
>> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
>> ---
>>   drivers/net/wireless/ath/ath9k/wmi.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/wmi.c b/drivers/net/wireless/ath/ath9k/wmi.c
>> index 284e8c13b043..df4a3a625536 100644
>> --- a/drivers/net/wireless/ath/ath9k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath9k/wmi.c
>> @@ -146,6 +146,15 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
>>   	unsigned long flags;
>>   	u16 cmd_id;
>>   
>> +	/* Check if ath9k_htc_probe_device() completed. */
>> +	if (!data_race(priv->initialized))
>> +		return;
> 
> Moving this out of the loop changes behaviour: Before, the loop would
> keep spinning waiting for initialisation, now we just exit. I don't see
> any guarantee that we'll come back here, so this has the risk of
> stalling things. We'll need to re-schedule the tasklet before returning
> if we're moving the check here.
Thank you for your comment.

I'm not sure that I agree that the tasklet needs to be rescheduled. 
Yes, the behavior is changed, as you described, but when a packet is 
dequeued, in normal operations, the function ends (either with a return 
or break statement). This means that the function is scheduled regularly.

To not change the behavior while still fixing the potential lock on 
uninitialized lock (and the missing memory barrier), we could move the 
initialization check at the very start of the loop like so:

diff --git a/drivers/net/wireless/ath/ath9k/wmi.c 
b/drivers/net/wireless/ath/ath9k/wmi.c
index 284e8c13b043..a21c438e5b61 100644
--- a/drivers/net/wireless/ath/ath9k/wmi.c
+++ b/drivers/net/wireless/ath/ath9k/wmi.c
@@ -147,6 +147,17 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
         u16 cmd_id;

         do {
+               /* Check if ath9k_htc_probe_device() completed. */
+               if (!data_race(priv->initialized)) {
+                       kfree_skb(skb);
+                       continue;
+               }
+               /*
+                * Make sure ath9k_htc_probe_device() initialization is
+                * committed to memory before processing skb.
+                */
+               smp_rmb();
+
                 spin_lock_irqsave(&wmi->wmi_lock, flags);
                 skb = __skb_dequeue(&wmi->wmi_event_queue);
                 if (!skb) {
@@ -155,12 +166,6 @@ void ath9k_wmi_event_tasklet(struct tasklet_struct *t)
                 }
                 spin_unlock_irqrestore(&wmi->wmi_lock, flags);

-               /* Check if ath9k_htc_probe_device() completed. */
-               if (!data_race(priv->initialized)) {
-                       kfree_skb(skb);
-                       continue;
-               }
-
                 hdr = (struct wmi_cmd_hdr *) skb->data;
                 cmd_id = be16_to_cpu(hdr->command_id);
                 wmi_event = skb_pull(skb, sizeof(struct wmi_cmd_hdr));

---

Maybe that would be better?

Best,
Thomas>
> -Toke