Re: small set of qwx bug fixes

On Mon, Jun 22, 2026 at 12:57:06AM +0200, Stefan Sperling wrote:
> i. Dealloc resources via qwx_stop() if we fail to get RUNNING in qwx_init().
>    Cleanup via qwx_stop() was only performed in case of complete success,
>    i.e. once IFF_RUNNING is set.

The above change reintroduced an issue where firmware is not getting
stopped properly and uses freed buffers during DMA, triggering mbuf
cluster use-after-free checks.
Below is a revised patch which aims to address this problem, with all
my pending changes rolled in.

Please test this diff instead of previous versions.

I have this queued as 7 separate commits with a rationale for each change
in log messages, to make change history easier to follow.

M  sys/dev/ic/qwx.c          |  64+  48-
M  sys/dev/pci/if_qwx_pci.c  |   2+   1-

2 files changed, 66 insertions(+), 49 deletions(-)

commit - 510a3c8701b503ed3d6bbf6e2c2c1d66315cb14c
commit + d7ec0f51335bd2d1bafe3b34722ae728b12fba4b
blob - 4c744b205a09dec658faed98adb3e4e7a7f557be
blob + bd67bf6b3fb868108c968c294e3196c01cc7aa60
--- sys/dev/ic/qwx.c
+++ sys/dev/ic/qwx.c
@@ -271,13 +271,16 @@ qwx_init(struct ifnet *ifp)
 	sc->scan.state = ATH11K_SCAN_IDLE;
 	sc->vdev_id_11d_scan = QWX_11D_INVALID_VDEV_ID;
 
-	error = qwx_core_init(sc);
-	if (error)
-		return error;
-
 	memset(&sc->qrtr_server, 0, sizeof(sc->qrtr_server));
 	sc->qrtr_server.node = QRTR_NODE_BCAST;
 
+	/* This flag will be cleared if firmware starts up successfully. */
+	set_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags);
+
+	error = qwx_core_init(sc);
+	if (error)
+		return error;
+
 	/* wait for QRTR init to be done */
 	while (sc->qrtr_server.node == QRTR_NODE_BCAST) {
 		error = tsleep_nsec(&sc->qrtr_server, 0, "qwxqrtr",
@@ -309,19 +312,18 @@ qwx_init(struct ifnet *ifp)
 			    sc->sc_dev.dv_xname, ether_sprintf(ic->ic_myaddr),
 			    error);
 
-		ieee80211_media_init(ifp, qwx_media_change,
+		ieee80211_media_init(ifp, ieee80211_media_change,
 		    ieee80211_media_status);
 	}
 
 	if (ifp->if_flags & IFF_UP) {
-		refcnt_init(&sc->task_refs);
-
-		ifq_clr_oactive(&ifp->if_snd);
-
 		error = qwx_mac_start(sc);
 		if (error)
 			return error;
 
+		refcnt_init(&sc->task_refs);
+		ifq_clr_oactive(&ifp->if_snd);
+
 		ifp->if_flags |= IFF_RUNNING;
 		sc->ops.irq_enable(sc);
 		ieee80211_begin_scan(ifp);
@@ -414,10 +416,12 @@ qwx_stop(struct ifnet *ifp)
 	struct qwx_softc *sc = ifp->if_softc;
 	struct ieee80211com *ic = &sc->sc_ic;
 	int s = splnet();
+	int was_running;
 
 	rw_assert_wrlock(&sc->ioctl_rwl);
 
-	if (ic->ic_opmode == IEEE80211_M_STA &&
+	was_running = (ifp->if_flags & IFF_RUNNING) != 0;
+	if (was_running && ic->ic_opmode == IEEE80211_M_STA &&
 	    ic->ic_state == IEEE80211_S_RUN &&
 	    (ic->ic_bss->ni_flags & IEEE80211_NODE_MFP) &&
 	    ic->ic_bss->ni_port_valid)
@@ -433,7 +437,8 @@ qwx_stop(struct ifnet *ifp)
 	/* Cancel scheduled tasks and let any stale tasks finish up. */
 	task_del(systq, &sc->init_task);
 	qwx_del_task_all(sc);
-	refcnt_finalize(&sc->task_refs, "qwxstop");
+	if (was_running)
+		refcnt_finalize(&sc->task_refs, "qwxstop");
 
 	ifp->if_timer = sc->sc_tx_timer = 0;
 
@@ -444,20 +449,21 @@ qwx_stop(struct ifnet *ifp)
 	sc->bgscan_unref_arg = NULL;
 	sc->bgscan_unref_arg_size = 0;
 
-	clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
-
 	/*
 	 * Manually run the newstate task's code for switching to INIT state.
 	 * This reconfigures firmware state to stop scanning, or disassociate
 	 * from our current AP, and/or stop the VIF, etc.
 	 */
-	if (ic->ic_state != IEEE80211_S_INIT) {
+	if (was_running && ic->ic_state != IEEE80211_S_INIT) {
 		sc->ns_nstate = IEEE80211_S_INIT;
 		sc->ns_arg = -1; /* do not send management frames */
 		refcnt_init(&sc->task_refs);
 		refcnt_take(&sc->task_refs);
+		clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
 		qwx_newstate_task(sc);
-		if (ic->ic_state != IEEE80211_S_INIT) { /* task code failed */
+		set_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
+		if (ic->ic_state != IEEE80211_S_INIT) {
+			/* task code failed */
 			task_del(systq, &sc->init_task);
 			sc->sc_newstate(ic, IEEE80211_S_INIT, -1);
 		}
@@ -472,7 +478,14 @@ qwx_stop(struct ifnet *ifp)
 	sc->vdev_id_11d_scan = QWX_11D_INVALID_VDEV_ID;
 	sc->pdevs_active = 0;
 
-	/* power off hardware */
+	/*
+	 * If we were running then allow commands to be sent to
+	 * firmware during core_deinit().
+	 */
+	if (was_running)
+		clear_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags);
+
+	/* free some DMA allocations and power off hardware */
 	qwx_core_deinit(sc);
 
 	qwx_vif_free_all(sc);
@@ -517,6 +530,10 @@ qwx_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
 				/* Force reload of firmware image from disk. */
 				qwx_free_firmware(sc);
 				err = qwx_init(ifp);
+				if (err) {
+					KASSERT(!(ifp->if_flags & IFF_RUNNING));
+					qwx_stop(ifp);
+				}
 			}
 		} else {
 			if (ifp->if_flags & IFF_RUNNING)
@@ -681,24 +698,6 @@ qwx_watchdog(struct ifnet *ifp)
 }
 
 int
-qwx_media_change(struct ifnet *ifp)
-{
-	int err;
-
-	err = ieee80211_media_change(ifp);
-	if (err != ENETRESET)
-		return err;
-
-	if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) ==
-	    (IFF_UP | IFF_RUNNING)) {
-		qwx_stop(ifp);
-		err = qwx_init(ifp);
-	}
-
-	return err;
-}
-
-int
 qwx_queue_setkey_cmd(struct ieee80211com *ic, struct ieee80211_node *ni,
     struct ieee80211_key *k, int cmd)
 {
@@ -15353,6 +15352,9 @@ qwx_dp_rxdma_ring_buf_setup(struct qwx_softc *sc,
 	if (rx_ring->rx_data == NULL)
 		return ENOMEM;
 
+	rx_ring->bufs_max = num_entries;
+	memset(rx_ring->freemap, 0xff, sizeof(rx_ring->freemap));
+
 	for (i = 0; i < num_entries; i++) {
 		struct qwx_rx_data *rx_data = &rx_ring->rx_data[i];
 
@@ -15361,9 +15363,6 @@ qwx_dp_rxdma_ring_buf_setup(struct qwx_softc *sc,
 			return ENOMEM;
 	}
 
-	rx_ring->bufs_max = num_entries;
-	memset(rx_ring->freemap, 0xff, sizeof(rx_ring->freemap));
-
 	return qwx_dp_rxbufs_replenish(sc, dp->mac_id, rx_ring, num_entries,
 	    sc->hw_params.hal_params->rx_buf_rbm);
 }
@@ -20786,10 +20785,11 @@ qwx_flush_rx_rings(struct qwx_softc *sc)
 void
 qwx_core_stop(struct qwx_softc *sc)
 {
-	if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags))
+	if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, sc->sc_flags) &&
+	    !test_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags)) {
 		qwx_qmi_firmware_stop(sc);
-	
-	qwx_flush_rx_rings(sc);
+		qwx_flush_rx_rings(sc);
+	}
 
 	sc->ops.stop(sc);
 	qwx_wmi_detach(sc);
@@ -20921,7 +20921,8 @@ qwx_core_qmi_firmware_ready(struct qwx_softc *sc)
 	default:
 		printf("%s: invalid crypto_mode: %d\n",
 		    sc->sc_dev.dv_xname, sc->crypto_mode);
-		return EINVAL;
+		ret = EINVAL;
+		goto err_dp_free;
 	}
 
 	if (sc->frame_mode == ATH11K_HW_TXRX_RAW)
@@ -20970,7 +20971,7 @@ err_firmware_stop:
 	return ret;
 }
 
-void
+int
 qwx_qmi_fw_init_done(struct qwx_softc *sc)
 {
 	int ret = 0;
@@ -20984,10 +20985,15 @@ qwx_qmi_fw_init_done(struct qwx_softc *sc)
 		clear_bit(ATH11K_FLAG_RECOVERY, sc->sc_flags);
 		ret = qwx_core_qmi_firmware_ready(sc);
 		if (ret) {
+			/*
+			 * This flags tells qwx_stop() that core is stopped
+			 * and ATH11K_FIRMWARE_MODE_OFF was already sent.
+			 */
 			set_bit(ATH11K_FLAG_QMI_FAIL, sc->sc_flags);
-			return;
 		}
 	}
+
+	return ret;
 }
 
 int
@@ -21047,8 +21053,7 @@ qwx_qmi_event_server_arrive(struct qwx_softc *sc)
 		}
 	}
 
-	qwx_qmi_fw_init_done(sc);
-	return 0;
+	return qwx_qmi_fw_init_done(sc);
 }
 
 int
@@ -24323,8 +24328,17 @@ qwx_init_task(void *arg)
 	struct qwx_softc *sc = arg;
 	struct ifnet *ifp = &sc->sc_ic.ic_if;
 	int s = splnet();
-	rw_enter_write(&sc->ioctl_rwl);
 
+	/*
+	 * Do not sleep for this lock. The init task is a one-shot
+	 * recovery mechanism. If the ioctl handler is busy then
+	 * we are being reconfigured or reset already.
+	 */
+	if (rw_enter(&sc->ioctl_rwl, RW_WRITE | RW_NOSLEEP) != 0) {
+		splx(s);
+		return;
+	}
+
 	if (ifp->if_flags & IFF_RUNNING)
 		qwx_stop(ifp);
 
@@ -27444,11 +27458,11 @@ qwx_activate(struct device *self, int act)
 
 	switch (act) {
 	case DVACT_QUIESCE:
+		rw_enter_write(&sc->ioctl_rwl);
 		if (ifp->if_flags & IFF_RUNNING) {
-			rw_enter_write(&sc->ioctl_rwl);
 			qwx_stop(ifp);
-			rw_exit(&sc->ioctl_rwl);
 		}
+		rw_exit(&sc->ioctl_rwl);
 		break;
 	case DVACT_RESUME:
 		err = qwx_hal_srng_init(sc);
@@ -27457,12 +27471,14 @@ qwx_activate(struct device *self, int act)
 			    sc->sc_dev.dv_xname);
 		break;
 	case DVACT_WAKEUP:
+		rw_enter_write(&sc->ioctl_rwl);
 		if ((ifp->if_flags & (IFF_UP | IFF_RUNNING)) == IFF_UP) {
 			err = qwx_init(ifp);
 			if (err)
 				printf("%s: could not initialize hardware\n",
 				    sc->sc_dev.dv_xname);
 		}
+		rw_exit(&sc->ioctl_rwl);
 		break;
 	}
 
blob - a0e416a3cafea6c49646852c11c9fa9045b71ce2
blob + 00da7a38928ecf5dee26f3b73a8eeb10ca0fd04c
--- sys/dev/pci/if_qwx_pci.c
+++ sys/dev/pci/if_qwx_pci.c
@@ -1113,7 +1113,8 @@ unsupported_wcn6855_soc:
 	memcpy(ifp->if_xname, sc->sc_dev.dv_xname, IFNAMSIZ);
 	if_attach(ifp);
 	ieee80211_ifattach(ifp);
-	ieee80211_media_init(ifp, qwx_media_change, ieee80211_media_status);
+	ieee80211_media_init(ifp, ieee80211_media_change,
+	    ieee80211_media_status);
 
 	ic->ic_node_alloc = qwx_node_alloc;