[PATCH ath-next 1/2] wifi: ath11k: guard copy engine access against a cleared srng

[PATCH ath-next 1/2] wifi: ath11k: guard copy engine access against a cleared srng

[Julius Bairaktaris]

ath11k_core_reconfigure_on_crash() zeroes the ring list with
ath11k_hal_srng_clear() and only rebuilds the copy engine rings later,
inside ath11k_core_qmi_firmware_ready() -> ath11k_ce_init_pipes().

ath11k_ce_send() is gated on ATH11K_FLAG_CRASH_FLUSH, but that flag is
cleared before ath11k_core_qmi_firmware_ready() is called - it has to be,
since the firmware bring-up itself sends WMI commands through the copy
engine. For the whole of that bring-up the gate is open while the ring is
still zeroed, so any WMI command issued in that window dereferences the
NULL u.src_ring.tp_addr in ath11k_hal_srng_access_begin():

  Unable to handle kernel read from unreadable memory at virtual address 0
  CPU: 0 UID: 101 PID: 1846 Comm: hostapd
  pc : ath11k_hal_srng_access_begin+0xc/0x60 [ath11k]
  lr : ath11k_ce_send+0x114/0x350 [ath11k]
  Call trace:
   ath11k_hal_srng_access_begin+0xc/0x60 [ath11k] (P)
   ath11k_htc_send+0x188/0x3c8 [ath11k]
   ath11k_wmi_cmd_send+0xc8/0x2f4 [ath11k]
   ath11k_wmi_send_peer_create_cmd+0x78/0xe0 [ath11k]
   ath11k_peer_create+0x138/0x500 [ath11k]

The caller matters: this is a command issued fresh during the recovery,
not a task that was already blocked in ath11k_wmi_cmd_send(). A blocked
one sleeps through the whole window and never re-enters ath11k_ce_send(),
which is why hammering beacon updates does not reproduce it and a station
associating during recovery does.

ath11k_ce_completed_send_next() has no gate at all, and
ath11k_core_reconfigure_on_crash() calls ath11k_ce_cleanup_pipes() before
anything else, so a crash arriving after a previous recovery already ran
ath11k_hal_srng_clear() reaps rings whose tp_addr is NULL. This is a
different entry into the same dereference from the one fixed by
commit e8d85672dd7e ("wifi: ath11k: fix NULL pointer dereference in
ath11k_hal_srng_access_begin"), which stops a successful bring-up from
being repeated; it does not close this window.

Test u.src_ring.tp_addr rather than srng->initialized in both places:
ath11k_hal_srng_setup() publishes ->initialized before ->tp_addr with no
barrier between them, so an ->initialized check can be true while the
pointer that is about to be dereferenced is still NULL.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1

Fixes: 32be3ca4cf78 ("wifi: ath11k: HAL SRNG: don't deinitialize and re-initialize again")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/ce.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
--- a/drivers/net/wireless/ath/ath11k/ce.c
+++ b/drivers/net/wireless/ath/ath11k/ce.c
@@ -472,6 +472,17 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe

 	spin_lock_bh(&srng->lock);

+	/*
+	 * ath11k_ce_cleanup_pipes() can reach this on a ring that
+	 * ath11k_hal_srng_clear() has zeroed and that recovery has not
+	 * rebuilt. Bail out as for an empty ring rather than dereference
+	 * the NULL tp_addr in ath11k_hal_srng_access_begin().
+	 */
+	if (unlikely(!srng->u.src_ring.tp_addr)) {
+		skb = ERR_PTR(-EIO);
+		goto err_unlock;
+	}
+
 	ath11k_hal_srng_access_begin(ab, srng);

 	desc = ath11k_hal_srng_src_reap_next(ab, srng);
@@ -750,6 +761,19 @@ int ath11k_ce_send(struct ath11k_base *ab, struct sk_buff *skb, u8 pipe_id,

 	srng = &ab->hal.srng_list[pipe->src_ring->hal_ring_id];

+	/*
+	 * ATH11K_FLAG_CRASH_FLUSH is cleared before the copy engine rings
+	 * are rebuilt, so the check above lets a send through while the
+	 * ring is still zeroed. Test the pointer that would be
+	 * dereferenced: ath11k_hal_srng_setup() publishes ->initialized
+	 * before ->tp_addr without a barrier, so ->initialized would still
+	 * race the rebuild.
+	 */
+	if (unlikely(!srng->u.src_ring.tp_addr)) {
+		spin_unlock_bh(&ab->ce.ce_lock);
+		return -ESHUTDOWN;
+	}
+
 	spin_lock_bh(&srng->lock);

 	ath11k_hal_srng_access_begin(ab, srng);

[PATCH ath-next 2/2] wifi: ath11k: initialise the CE descriptor waitqueue once per device

[Julius Bairaktaris]

ath11k_wmi_pdev_attach() runs init_waitqueue_head() on the pdev's
tx_ce_desc_wq. It is called from ath11k_wmi_attach() ->
ath11k_core_start(), which firmware crash recovery re-runs through
ath11k_core_qmi_firmware_ready(), so the head is reinitialised on every
recovery - including while a task is still queued on it.

Parts with hw_params.credit_flow clear - ipq8074, ipq6018, ipq5018,
wcn6750 - take the else branch of ath11k_wmi_cmd_send() and sleep there
in wait_event_timeout() for up to WMI_SEND_TIMEOUT_HZ. That is longer
than a recovery takes, so an ordinary WMI command issued from userspace
just before the crash is still on the queue when the head is cleared
under it, and finish_wait() then deletes the entry from a list head that
no longer refers to it:

  WARNING: CPU: 1 PID: 1861 at lib/list_debug.c:62 __list_del_entry_valid_or_report+0x110/0x154
  CPU: 0 UID: 101 Comm: hostapd
  Call trace:
   __list_del_entry_valid_or_report+0x110/0x154 (P)
   finish_wait+0xcc/0xd0
   ath11k_wmi_cmd_send+0x150/0x2f4 [ath11k]
   ath11k_wmi_vdev_set_param_cmd+0x64/0xd8 [ath11k]
   ath11k_mac_op_bss_info_changed+0xe8/0xf60 [ath11k]
   drv_link_info_changed+0x118/0x160 [mac80211]
   ieee80211_link_info_change_notify+0xe0/0x100 [mac80211]
   ieee80211_change_bss+0x1c8/0x250 [mac80211]
   nl80211_set_bss+0x1a0/0x330 [cfg80211]

Initialise it where its credit-flow sibling ab->wmi_ab.tx_credits_wq is
already initialised, in ath11k_core_alloc(), which runs once per device.
The array is fixed size, so all of it can be set up before any pdev is
attached, and nothing on the recovery path touches the head again.

Reproduced on a Xiaomi AX3600 (IPQ8074 hw2.0) with a userspace loop
pushing beacon updates while the firmware is asserted through the debugfs
simulate_fw_crash 'assert' trigger, on a stock ath11k with no out-of-tree
modules loaded: one warning per recovery, on all eleven recoveries
observed across two builds, and none at all over six recoveries with this
applied.

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1

Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
---
 drivers/net/wireless/ath/ath11k/core.c | 5 +++++
 drivers/net/wireless/ath/ath11k/wmi.c  | 1 -
 2 files changed, 5 insertions(+), 1 deletion(-)

--- a/drivers/net/wireless/ath/ath11k/core.c
+++ b/drivers/net/wireless/ath/ath11k/core.c
@@ -2746,6 +2746,7 @@
 				      enum ath11k_bus bus)
 {
 	struct ath11k_base *ab;
+	int i;
 
 	ab = kzalloc(sizeof(*ab) + priv_size, GFP_KERNEL);
 	if (!ab)
@@ -2772,6 +2773,10 @@
 	INIT_LIST_HEAD(&ab->peers);
 	init_waitqueue_head(&ab->peer_mapping_wq);
 	init_waitqueue_head(&ab->wmi_ab.tx_credits_wq);
+
+	for (i = 0; i < ARRAY_SIZE(ab->wmi_ab.wmi); i++)
+		init_waitqueue_head(&ab->wmi_ab.wmi[i].tx_ce_desc_wq);
+
 	init_waitqueue_head(&ab->qmi.cold_boot_waitq);
 	INIT_WORK(&ab->restart_work, ath11k_core_restart);
 	INIT_WORK(&ab->update_11d_work, ath11k_update_11d);
--- a/drivers/net/wireless/ath/ath11k/wmi.c
+++ b/drivers/net/wireless/ath/ath11k/wmi.c
@@ -9064,7 +9064,6 @@
 	ab->wmi_ab.wmi_endpoint_id[pdev_idx] = conn_resp.eid;
 	ab->wmi_ab.wmi[pdev_idx].eid = conn_resp.eid;
 	ab->wmi_ab.max_msg_len[pdev_idx] = conn_resp.max_msg_len;
-	init_waitqueue_head(&ab->wmi_ab.wmi[pdev_idx].tx_ce_desc_wq);
 
 	return 0;
 }

Re: [PATCH ath-next 1/2] wifi: ath11k: guard copy engine access against a cleared srng

[Baochen Qiang]

On 8/6/2026 3:21 AM, Julius Bairaktaris wrote:
> ath11k_core_reconfigure_on_crash() zeroes the ring list with
> ath11k_hal_srng_clear() and only rebuilds the copy engine rings later,
> inside ath11k_core_qmi_firmware_ready() -> ath11k_ce_init_pipes().
> 
> ath11k_ce_send() is gated on ATH11K_FLAG_CRASH_FLUSH, but that flag is
> cleared before ath11k_core_qmi_firmware_ready() is called - it has to be,
> since the firmware bring-up itself sends WMI commands through the copy
> engine. For the whole of that bring-up the gate is open while the ring is
> still zeroed, so any WMI command issued in that window dereferences the
> NULL u.src_ring.tp_addr in ath11k_hal_srng_access_begin():
> 
>   Unable to handle kernel read from unreadable memory at virtual address 0
>   CPU: 0 UID: 101 PID: 1846 Comm: hostapd
>   pc : ath11k_hal_srng_access_begin+0xc/0x60 [ath11k]
>   lr : ath11k_ce_send+0x114/0x350 [ath11k]
>   Call trace:
>    ath11k_hal_srng_access_begin+0xc/0x60 [ath11k] (P)
>    ath11k_htc_send+0x188/0x3c8 [ath11k]
>    ath11k_wmi_cmd_send+0xc8/0x2f4 [ath11k]
>    ath11k_wmi_send_peer_create_cmd+0x78/0xe0 [ath11k]
>    ath11k_peer_create+0x138/0x500 [ath11k]
> 
> The caller matters: this is a command issued fresh during the recovery,
> not a task that was already blocked in ath11k_wmi_cmd_send(). A blocked
> one sleeps through the whole window and never re-enters ath11k_ce_send(),
> which is why hammering beacon updates does not reproduce it and a station
> associating during recovery does.
> 
> ath11k_ce_completed_send_next() has no gate at all, and
> ath11k_core_reconfigure_on_crash() calls ath11k_ce_cleanup_pipes() before
> anything else, so a crash arriving after a previous recovery already ran
> ath11k_hal_srng_clear() reaps rings whose tp_addr is NULL. This is a
> different entry into the same dereference from the one fixed by
> commit e8d85672dd7e ("wifi: ath11k: fix NULL pointer dereference in
> ath11k_hal_srng_access_begin"), which stops a successful bring-up from
> being repeated; it does not close this window.
> 
> Test u.src_ring.tp_addr rather than srng->initialized in both places:
> ath11k_hal_srng_setup() publishes ->initialized before ->tp_addr with no
> barrier between them, so an ->initialized check can be true while the
> pointer that is about to be dereferenced is still NULL.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1
> 
> Fixes: 32be3ca4cf78 ("wifi: ath11k: HAL SRNG: don't deinitialize and re-initialize again")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
> ---
>  drivers/net/wireless/ath/ath11k/ce.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/ce.c b/drivers/net/wireless/ath/ath11k/ce.c
> --- a/drivers/net/wireless/ath/ath11k/ce.c
> +++ b/drivers/net/wireless/ath/ath11k/ce.c
> @@ -472,6 +472,17 @@ static struct sk_buff *ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe
> 
>  	spin_lock_bh(&srng->lock);
> 
> +	/*
> +	 * ath11k_ce_cleanup_pipes() can reach this on a ring that
> +	 * ath11k_hal_srng_clear() has zeroed and that recovery has not
> +	 * rebuilt. Bail out as for an empty ring rather than dereference
> +	 * the NULL tp_addr in ath11k_hal_srng_access_begin().
> +	 */
> +	if (unlikely(!srng->u.src_ring.tp_addr)) {
> +		skb = ERR_PTR(-EIO);
> +		goto err_unlock;
> +	}
> +
>  	ath11k_hal_srng_access_begin(ab, srng);
> 
>  	desc = ath11k_hal_srng_src_reap_next(ab, srng);
> @@ -750,6 +761,19 @@ int ath11k_ce_send(struct ath11k_base *ab, struct sk_buff *skb, u8 pipe_id,
> 
>  	srng = &ab->hal.srng_list[pipe->src_ring->hal_ring_id];
> 
> +	/*
> +	 * ATH11K_FLAG_CRASH_FLUSH is cleared before the copy engine rings
> +	 * are rebuilt, so the check above lets a send through while the
> +	 * ring is still zeroed. Test the pointer that would be
> +	 * dereferenced: ath11k_hal_srng_setup() publishes ->initialized
> +	 * before ->tp_addr without a barrier, so ->initialized would still
> +	 * race the rebuild.
> +	 */
> +	if (unlikely(!srng->u.src_ring.tp_addr)) {
> +		spin_unlock_bh(&ab->ce.ce_lock);
> +		return -ESHUTDOWN;
> +	}
> +

I understand this is a real issue but both changes look not a proper fix to me. at least
we should not test hal srng fields at CE layer since that is a layer violation.

>  	spin_lock_bh(&srng->lock);
> 
>  	ath11k_hal_srng_access_begin(ab, srng);

Re: [PATCH ath-next 2/2] wifi: ath11k: initialise the CE descriptor waitqueue once per device

[Baochen Qiang]

On 8/6/2026 3:21 AM, Julius Bairaktaris wrote:
> ath11k_wmi_pdev_attach() runs init_waitqueue_head() on the pdev's
> tx_ce_desc_wq. It is called from ath11k_wmi_attach() ->
> ath11k_core_start(), which firmware crash recovery re-runs through
> ath11k_core_qmi_firmware_ready(), so the head is reinitialised on every
> recovery - including while a task is still queued on it.
> 
> Parts with hw_params.credit_flow clear - ipq8074, ipq6018, ipq5018,
> wcn6750 - take the else branch of ath11k_wmi_cmd_send() and sleep there
> in wait_event_timeout() for up to WMI_SEND_TIMEOUT_HZ. That is longer
> than a recovery takes, so an ordinary WMI command issued from userspace
> just before the crash is still on the queue when the head is cleared
> under it, and finish_wait() then deletes the entry from a list head that
> no longer refers to it:
> 
>   WARNING: CPU: 1 PID: 1861 at lib/list_debug.c:62 __list_del_entry_valid_or_report+0x110/0x154
>   CPU: 0 UID: 101 Comm: hostapd
>   Call trace:
>    __list_del_entry_valid_or_report+0x110/0x154 (P)
>    finish_wait+0xcc/0xd0
>    ath11k_wmi_cmd_send+0x150/0x2f4 [ath11k]
>    ath11k_wmi_vdev_set_param_cmd+0x64/0xd8 [ath11k]
>    ath11k_mac_op_bss_info_changed+0xe8/0xf60 [ath11k]
>    drv_link_info_changed+0x118/0x160 [mac80211]
>    ieee80211_link_info_change_notify+0xe0/0x100 [mac80211]
>    ieee80211_change_bss+0x1c8/0x250 [mac80211]
>    nl80211_set_bss+0x1a0/0x330 [cfg80211]
> 
> Initialise it where its credit-flow sibling ab->wmi_ab.tx_credits_wq is
> already initialised, in ath11k_core_alloc(), which runs once per device.
> The array is fixed size, so all of it can be set up before any pdev is
> attached, and nothing on the recovery path touches the head again.
> 
> Reproduced on a Xiaomi AX3600 (IPQ8074 hw2.0) with a userspace loop
> pushing beacon updates while the firmware is asserted through the debugfs
> simulate_fw_crash 'assert' trigger, on a stock ath11k with no out-of-tree
> modules loaded: one warning per recovery, on all eleven recoveries
> observed across two builds, and none at all over six recoveries with this
> applied.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>

Reviewed-by: Baochen Qiang <baochen.qiang@oss.qualcomm.com>

Re: [PATCH ath-next 2/2] wifi: ath11k: initialise the CE descriptor waitqueue once per device

[Baochen Qiang]

On 8/6/2026 3:21 AM, Julius Bairaktaris wrote:
> ath11k_wmi_pdev_attach() runs init_waitqueue_head() on the pdev's

did you mean ath11k_connect_pdev_htc_service() ?

> tx_ce_desc_wq. It is called from ath11k_wmi_attach() ->
> ath11k_core_start(), which firmware crash recovery re-runs through
> ath11k_core_qmi_firmware_ready(), so the head is reinitialised on every
> recovery - including while a task is still queued on it.
> 
> Parts with hw_params.credit_flow clear - ipq8074, ipq6018, ipq5018,
> wcn6750 - take the else branch of ath11k_wmi_cmd_send() and sleep there
> in wait_event_timeout() for up to WMI_SEND_TIMEOUT_HZ. That is longer
> than a recovery takes, so an ordinary WMI command issued from userspace
> just before the crash is still on the queue when the head is cleared
> under it, and finish_wait() then deletes the entry from a list head that
> no longer refers to it:
> 
>   WARNING: CPU: 1 PID: 1861 at lib/list_debug.c:62 __list_del_entry_valid_or_report+0x110/0x154
>   CPU: 0 UID: 101 Comm: hostapd
>   Call trace:
>    __list_del_entry_valid_or_report+0x110/0x154 (P)
>    finish_wait+0xcc/0xd0
>    ath11k_wmi_cmd_send+0x150/0x2f4 [ath11k]
>    ath11k_wmi_vdev_set_param_cmd+0x64/0xd8 [ath11k]
>    ath11k_mac_op_bss_info_changed+0xe8/0xf60 [ath11k]
>    drv_link_info_changed+0x118/0x160 [mac80211]
>    ieee80211_link_info_change_notify+0xe0/0x100 [mac80211]
>    ieee80211_change_bss+0x1c8/0x250 [mac80211]
>    nl80211_set_bss+0x1a0/0x330 [cfg80211]
> 
> Initialise it where its credit-flow sibling ab->wmi_ab.tx_credits_wq is
> already initialised, in ath11k_core_alloc(), which runs once per device.
> The array is fixed size, so all of it can be set up before any pdev is
> attached, and nothing on the recovery path touches the head again.
> 
> Reproduced on a Xiaomi AX3600 (IPQ8074 hw2.0) with a userspace loop
> pushing beacon updates while the firmware is asserted through the debugfs
> simulate_fw_crash 'assert' trigger, on a stock ath11k with no out-of-tree
> modules loaded: one warning per recovery, on all eleven recoveries
> observed across two builds, and none at all over six recoveries with this
> applied.
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.12-01460-QCAHKSWPL_SILICONZ-1
> 
> Fixes: f951380a6022 ("ath11k: Disabling credit flow for WMI path")
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Julius Bairaktaris <julius@bairaktaris.de>
> ---
>  drivers/net/wireless/ath/ath11k/core.c | 5 +++++
>  drivers/net/wireless/ath/ath11k/wmi.c  | 1 -
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> --- a/drivers/net/wireless/ath/ath11k/core.c
> +++ b/drivers/net/wireless/ath/ath11k/core.c
> @@ -2746,6 +2746,7 @@
>  				      enum ath11k_bus bus)
>  {
>  	struct ath11k_base *ab;
> +	int i;
>  
>  	ab = kzalloc(sizeof(*ab) + priv_size, GFP_KERNEL);
>  	if (!ab)
> @@ -2772,6 +2773,10 @@
>  	INIT_LIST_HEAD(&ab->peers);
>  	init_waitqueue_head(&ab->peer_mapping_wq);
>  	init_waitqueue_head(&ab->wmi_ab.tx_credits_wq);
> +
> +	for (i = 0; i < ARRAY_SIZE(ab->wmi_ab.wmi); i++)
> +		init_waitqueue_head(&ab->wmi_ab.wmi[i].tx_ce_desc_wq);
> +
>  	init_waitqueue_head(&ab->qmi.cold_boot_waitq);
>  	INIT_WORK(&ab->restart_work, ath11k_core_restart);
>  	INIT_WORK(&ab->update_11d_work, ath11k_update_11d);
> --- a/drivers/net/wireless/ath/ath11k/wmi.c
> +++ b/drivers/net/wireless/ath/ath11k/wmi.c
> @@ -9064,7 +9064,6 @@
>  	ab->wmi_ab.wmi_endpoint_id[pdev_idx] = conn_resp.eid;
>  	ab->wmi_ab.wmi[pdev_idx].eid = conn_resp.eid;
>  	ab->wmi_ab.max_msg_len[pdev_idx] = conn_resp.max_msg_len;
> -	init_waitqueue_head(&ab->wmi_ab.wmi[pdev_idx].tx_ce_desc_wq);
>  
>  	return 0;
>  }