From nobody Mon Feb 19 08:08:03 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TdZsc1lkpz5B2vY; Mon, 19 Feb 2024 08:08:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TdZsc0sl0z4KtY; Mon, 19 Feb 2024 08:08:04 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1708330084; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=oa8xxx+zJxA7gNk7e9Qz+8M4iKR367csCLdYOqPr7J4=; b=eeDpSMhtcw4EhHAMZKc6P5C2uWAt/NZ0Lo0rNyW/a4jt49tJ+tdTrC08KP6kpVtPyUNAVT rym40+dpioZDFMmacxlT/UHbv0JY0vp/gVC31SA/zQzvJwCrcYUD6vGyrKtLDIS1Y46AFy G7+2w4Twh4CZAqqCpvhp1vwYFqvMpIVpj1hb2JLUHXgA+ncy5EREpbzXRJ1cFTRqsBSln6 3Yf/pjIIAefYg38viH8RVMgZuzk23ckP8YJSD+S+RaoNZSgQ8UOZN4vS5uKGaDOjSBKqGD W3kHpjjFFPdUXvJeUQhQspf9NnuyT0IR/bFYVoWDq9oik3WN1Ma5SBdBK/L8pQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1708330084; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=oa8xxx+zJxA7gNk7e9Qz+8M4iKR367csCLdYOqPr7J4=; b=VplPcWMLRxJcl1nydRMtA7+p5kQmeeamNBQXryJDLYchEhustZcTc3DUA4+IXzR6QQu7ND xncSaG2k4ubDR8S2aWx5qVnv6CBS1svLgWOz44yZ0MKgm1ThLJKRNvFhjYhtMx+yXv7sio e/t3FKlRRvyXdNDGLc+900yADj/X6QGPs9/cx2z/TfNx+3G0XJ3X+5+wbrpqJYOCxSHWFP XJpr4UaV8VAKn8UfHPP5KFYt+KW3S0lMElnU03XHH4fNsqUJf5ZPthkR5BkGxmM4voceG7 Fm2F3+T7rrF7108zF+FaVANcRkLpP6IvWbt05zc+6D2CgzVdZNnASj2b2yEbdA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1708330084; a=rsa-sha256; cv=none; b=nb/D95tqDcPWihutGqHxSoOHihRhefrUmZ2bGpS+KUlxtSdpITT3ldTqt6Mzo+nCz9neyd ASh5arjg0BolfInjGV0PUb52c3VmE5rUXhs/Azo5l1P/+TNykPkq6bgnlGBZxjDRMI1AUa kmcDcMVizcbhfPIdOtW9znnjydtJmdfg8ZpkV/65JPOjRSERC/8q10VkMQVniCa+zeEGsl +5ckqvT6ryEdGYn2wJsUA+1CRC1/A+QjNIMA6yW0rOKt11Os8FzD1DDdJ8MlyGrv/QefLh oYv/q32FyBMFpqFRd1DPuDtBpqHeQlFl9K9jTdMLGjb3OPR0qI8HQE2VFnGgYw== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4TdZsb747zzRGk; Mon, 19 Feb 2024 08:08:03 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.17.1/8.17.1) with ESMTP id 41J8830h016510; Mon, 19 Feb 2024 08:08:03 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 41J883Xw016507; Mon, 19 Feb 2024 08:08:03 GMT (envelope-from git) Date: Mon, 19 Feb 2024 08:08:03 GMT Message-Id: <202402190808.41J883Xw016507@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: "Bjoern A. Zeeb" Subject: git: 223edc1a3c2f - stable/13 - LinuxKPI: 802.11: update the ni/lsta reference cycle List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: bz X-Git-Repository: src X-Git-Refname: refs/heads/stable/13 X-Git-Reftype: branch X-Git-Commit: 223edc1a3c2fc86dbc7fa0ecd00f26a85d7c7b43 Auto-Submitted: auto-generated The branch stable/13 has been updated by bz: URL: https://cgit.FreeBSD.org/src/commit/?id=223edc1a3c2fc86dbc7fa0ecd00f26a85d7c7b43 commit 223edc1a3c2fc86dbc7fa0ecd00f26a85d7c7b43 Author: Bjoern A. Zeeb AuthorDate: 2024-02-05 14:51:08 +0000 Commit: Bjoern A. Zeeb CommitDate: 2024-02-19 08:02:02 +0000 LinuxKPI: 802.11: update the ni/lsta reference cycle Update the ni/lsta reference cycle, add extra checks and assertions. This is to accomodate problems we were seeing based on net80211 behaviour (join1() and (*iv_update_bss)() as well as state changes for new iv_bss nodes during an active session). This should hopefully help to stabilise behaviour until the underlying problems gets properly addressed (for this and all other device drivers). PR: 272607, 273985, 274003 Reviewed by: cc Differential Revision: https://reviews.freebsd.org/D43753 (cherry picked from commit 0936c648ad0ee5152dc19f261e77fe9c1833fe05) --- sys/compat/linuxkpi/common/src/linux_80211.c | 209 +++++++++++++++++---------- sys/compat/linuxkpi/common/src/linux_80211.h | 1 + 2 files changed, 130 insertions(+), 80 deletions(-) diff --git a/sys/compat/linuxkpi/common/src/linux_80211.c b/sys/compat/linuxkpi/common/src/linux_80211.c index b1c3b5c32ded..35eef4cf0640 100644 --- a/sys/compat/linuxkpi/common/src/linux_80211.c +++ b/sys/compat/linuxkpi/common/src/linux_80211.c @@ -247,25 +247,14 @@ lkpi_lsta_dump(struct lkpi_sta *lsta, struct ieee80211_node *ni, static void lkpi_lsta_remove(struct lkpi_sta *lsta, struct lkpi_vif *lvif) { - struct ieee80211_node *ni; - - IMPROVE("XXX-BZ remove tqe_prev check once ni-sta-state-sync is fixed"); - ni = lsta->ni; LKPI_80211_LVIF_LOCK(lvif); - if (lsta->lsta_entry.tqe_prev != NULL) - TAILQ_REMOVE(&lvif->lsta_head, lsta, lsta_entry); + KASSERT(lsta->lsta_entry.tqe_prev != NULL, + ("%s: lsta %p lsta_entry.tqe_prev %p ni %p\n", __func__, + lsta, lsta->lsta_entry.tqe_prev, lsta->ni)); + TAILQ_REMOVE(&lvif->lsta_head, lsta, lsta_entry); LKPI_80211_LVIF_UNLOCK(lvif); - - lsta->ni = NULL; - ni->ni_drv_data = NULL; - if (ni != NULL) - ieee80211_free_node(ni); - - IMPROVE("more from lkpi_ic_node_free() should happen here."); - - free(lsta, M_LKPI80211); } static struct lkpi_sta * @@ -287,13 +276,16 @@ lkpi_lsta_alloc(struct ieee80211vap *vap, const uint8_t mac[IEEE80211_ADDR_LEN], lsta->added_to_drv = false; lsta->state = IEEE80211_STA_NOTEXIST; -#if 0 /* - * This needs to be done in node_init() as ieee80211_alloc_node() - * will initialise the refcount after us. + * Link the ni to the lsta here without taking a reference. + * For one we would have to take the reference in node_init() + * as ieee80211_alloc_node() will initialise the refcount after us. + * For the other a ni and an lsta are 1:1 mapped and always together + * from [ic_]node_alloc() to [ic_]node_free() so we are essentally + * using the ni references for the lsta as well despite it being + * two separate allocations. */ - lsta->ni = ieee80211_ref_node(ni); -#endif + lsta->ni = ni; /* The back-pointer "drv_data" to net80211_node let's us get lsta. */ ni->ni_drv_data = lsta; @@ -378,6 +370,7 @@ lkpi_lsta_alloc(struct ieee80211vap *vap, const uint8_t mac[IEEE80211_ADDR_LEN], mtx_init(&lsta->txq_mtx, "lsta_txq", NULL, MTX_DEF); TASK_INIT(&lsta->txq_task, 0, lkpi_80211_txq_task, lsta); mbufq_init(&lsta->txq, IFQ_MAXLEN); + lsta->txq_ready = true; return (lsta); @@ -393,6 +386,54 @@ cleanup: return (NULL); } +static void +lkpi_lsta_free(struct lkpi_sta *lsta, struct ieee80211_node *ni) +{ + struct mbuf *m; + + if (lsta->added_to_drv) + panic("%s: Trying to free an lsta still known to firmware: " + "lsta %p ni %p added_to_drv %d\n", + __func__, lsta, ni, lsta->added_to_drv); + + /* XXX-BZ free resources, ... */ + IMPROVE(); + + /* XXX locking */ + lsta->txq_ready = false; + + /* Drain taskq, won't be restarted until added_to_drv is set again. */ + while (taskqueue_cancel(taskqueue_thread, &lsta->txq_task, NULL) != 0) + taskqueue_drain(taskqueue_thread, &lsta->txq_task); + + /* Flush mbufq (make sure to release ni refs!). */ + m = mbufq_dequeue(&lsta->txq); + while (m != NULL) { + struct ieee80211_node *nim; + + nim = (struct ieee80211_node *)m->m_pkthdr.rcvif; + if (nim != NULL) + ieee80211_free_node(nim); + m_freem(m); + m = mbufq_dequeue(&lsta->txq); + } + KASSERT(mbufq_empty(&lsta->txq), ("%s: lsta %p has txq len %d != 0\n", + __func__, lsta, mbufq_len(&lsta->txq))); + + /* Drain sta->txq[] */ + mtx_destroy(&lsta->txq_mtx); + + /* Remove lsta from vif; that is done by the state machine. Should assert it? */ + + IMPROVE("Make sure everything is cleaned up."); + + /* Free lsta. */ + lsta->ni = NULL; + ni->ni_drv_data = NULL; + free(lsta, M_LKPI80211); +} + + static enum nl80211_band lkpi_net80211_chan_to_nl80211_band(struct ieee80211_channel *c) { @@ -1052,11 +1093,17 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int ic_printf(vap->iv_ic, "%s: no iv_bss for vap %p\n", __func__, vap); return (EINVAL); } + /* + * Keep the ni alive locally. In theory (and practice) iv_bss can change + * once we unlock here. This is due to net80211 allowing state changes + * and new join1() despite having an active node as well as due to + * the fact that the iv_bss can be swapped under the hood in (*iv_update_bss). + */ ni = ieee80211_ref_node(vap->iv_bss); if (ni->ni_chan == NULL || ni->ni_chan == IEEE80211_CHAN_ANYC) { ic_printf(vap->iv_ic, "%s: no channel set for iv_bss ni %p " "on vap %p\n", __func__, ni, vap); - ieee80211_free_node(ni); + ieee80211_free_node(ni); /* Error handling for the local ni. */ return (EINVAL); } @@ -1065,7 +1112,7 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int if (chan == NULL) { ic_printf(vap->iv_ic, "%s: failed to get LKPI channel from " "iv_bss ni %p on vap %p\n", __func__, ni, vap); - ieee80211_free_node(ni); + ieee80211_free_node(ni); /* Error handling for the local ni. */ return (ESRCH); } @@ -1073,6 +1120,18 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int lvif = VAP_TO_LVIF(vap); vif = LVIF_TO_VIF(lvif); + LKPI_80211_LVIF_LOCK(lvif); + /* XXX-BZ KASSERT later? */ + if (lvif->lvif_bss_synched || lvif->lvif_bss != NULL) { + ic_printf(vap->iv_ic, "%s:%d: lvif %p vap %p iv_bss %p lvif_bss %p " + "lvif_bss->ni %p synched %d\n", __func__, __LINE__, + lvif, vap, vap->iv_bss, lvif->lvif_bss, + (lvif->lvif_bss != NULL) ? lvif->lvif_bss->ni : NULL, + lvif->lvif_bss_synched); + return (EBUSY); + } + LKPI_80211_LVIF_UNLOCK(lvif); + IEEE80211_UNLOCK(vap->iv_ic); LKPI_80211_LHW_LOCK(lhw); @@ -1200,32 +1259,17 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int lkpi_80211_mo_bss_info_changed(hw, vif, &vif->bss_conf, bss_changed); /* - * This is a bandaid for now. If we went through (*iv_update_bss)() - * and then removed the lsta we end up here without a lsta and have - * to manually allocate and link it in as lkpi_ic_node_alloc()/init() - * would normally do. - * XXX-BZ I do not like this but currently we have no good way of - * intercepting the bss swap and state changes and packets going out - * workflow so live with this. It is a compat layer after all. + * Given ni and lsta are 1:1 from alloc to free we can assert that + * ni always has lsta data attach despite net80211 node swapping + * under the hoods. */ - if (ni->ni_drv_data == NULL) { - ic_printf(vap->iv_ic, "%s:%d: lkpi_lsta_alloc to be called: " - "ni %p lsta %p\n", __func__, __LINE__, ni, ni->ni_drv_data); - lsta = lkpi_lsta_alloc(vap, ni->ni_macaddr, hw, ni); - if (lsta == NULL) { - error = ENOMEM; - ic_printf(vap->iv_ic, "%s:%d: lkpi_lsta_alloc " - "failed: %d\n", __func__, __LINE__, error); - goto out; - } - lsta->ni = ieee80211_ref_node(ni); - } else { - lsta = ni->ni_drv_data; - } + KASSERT(ni->ni_drv_data != NULL, ("%s: ni %p ni_drv_data %p\n", + __func__, ni, ni->ni_drv_data)); + lsta = ni->ni_drv_data; LKPI_80211_LVIF_LOCK(lvif); - /* XXX-BZ KASSERT later? */ - /* XXX-BZ this should have caught the upper lkpi_lsta_alloc() too! */ + /* Re-check given (*iv_update_bss) could have happened. */ + /* XXX-BZ KASSERT later? or deal as error? */ if (lvif->lvif_bss_synched || lvif->lvif_bss != NULL) ic_printf(vap->iv_ic, "%s:%d: lvif %p vap %p iv_bss %p lvif_bss %p " "lvif_bss->ni %p synched %d, ni %p lsta %p\n", __func__, __LINE__, @@ -1233,8 +1277,13 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int (lvif->lvif_bss != NULL) ? lvif->lvif_bss->ni : NULL, lvif->lvif_bss_synched, ni, lsta); - /* Reference the ni for this cache of lsta. */ - ieee80211_ref_node(vap->iv_bss); + /* + * Reference the ni for this cache of lsta/ni on lvif->lvif_bss + * essentially out lsta version of the iv_bss. + * Do NOT use iv_bss here anymore as that may have diverged from our + * function local ni already and would lead to inconsistencies. + */ + ieee80211_ref_node(ni); lvif->lvif_bss = lsta; lvif->lvif_bss_synched = true; @@ -1287,6 +1336,10 @@ lkpi_sta_scan_to_auth(struct ieee80211vap *vap, enum ieee80211_state nstate, int out: LKPI_80211_LHW_UNLOCK(lhw); IEEE80211_LOCK(vap->iv_ic); + /* + * Release the reference that keop the ni stable locally + * during the work of this function. + */ if (ni != NULL) ieee80211_free_node(ni); return (error); @@ -1381,9 +1434,13 @@ lkpi_sta_auth_to_scan(struct ieee80211vap *vap, enum ieee80211_state nstate, int lvif->lvif_bss = NULL; lvif->lvif_bss_synched = false; LKPI_80211_LVIF_UNLOCK(lvif); - ieee80211_free_node(ni); /* was lvif->lvif_bss->ni */ - lkpi_lsta_remove(lsta, lvif); + /* + * The very last release the reference on the ni for the ni/lsta on + * lvif->lvif_bss. Upon return from this both ni and lsta are invalid + * and potentially freed. + */ + ieee80211_free_node(ni); /* conf_tx */ @@ -1719,9 +1776,13 @@ _lkpi_sta_assoc_to_down(struct ieee80211vap *vap, enum ieee80211_state nstate, i lvif->lvif_bss = NULL; lvif->lvif_bss_synched = false; LKPI_80211_LVIF_UNLOCK(lvif); - ieee80211_free_node(ni); /* was lvif->lvif_bss->ni */ - lkpi_lsta_remove(lsta, lvif); + /* + * The very last release the reference on the ni for the ni/lsta on + * lvif->lvif_bss. Upon return from this both ni and lsta are invalid + * and potentially freed. + */ + ieee80211_free_node(ni); /* conf_tx */ @@ -2260,9 +2321,13 @@ lkpi_sta_run_to_init(struct ieee80211vap *vap, enum ieee80211_state nstate, int lvif->lvif_bss = NULL; lvif->lvif_bss_synched = false; LKPI_80211_LVIF_UNLOCK(lvif); - ieee80211_free_node(ni); /* was lvif->lvif_bss->ni */ - lkpi_lsta_remove(lsta, lvif); + /* + * The very last release the reference on the ni for the ni/lsta on + * lvif->lvif_bss. Upon return from this both ni and lsta are invalid + * and potentially freed. + */ + ieee80211_free_node(ni); /* conf_tx */ @@ -3409,7 +3474,6 @@ lkpi_ic_node_init(struct ieee80211_node *ni) { struct ieee80211com *ic; struct lkpi_hw *lhw; - struct lkpi_sta *lsta; int error; ic = ni->ni_ic; @@ -3421,11 +3485,6 @@ lkpi_ic_node_init(struct ieee80211_node *ni) return (error); } - lsta = ni->ni_drv_data; - - /* Now take the reference before linking it to the table. */ - lsta->ni = ieee80211_ref_node(ni); - /* XXX-BZ Sync other state over. */ IMPROVE(); @@ -3458,30 +3517,15 @@ lkpi_ic_node_free(struct ieee80211_node *ni) ic = ni->ni_ic; lhw = ic->ic_softc; lsta = ni->ni_drv_data; - if (lsta == NULL) - goto out; - /* XXX-BZ free resources, ... */ - IMPROVE(); + /* KASSERT lsta is not NULL here. Print ni/ni__refcnt. */ - /* Flush mbufq (make sure to release ni refs!). */ -#ifdef __notyet__ - KASSERT(mbufq_empty(&lsta->txq), ("%s: lsta %p has txq len %d != 0\n", - __func__, lsta, mbufq_len(&lsta->txq))); -#endif - /* Drain taskq. */ - - /* Drain sta->txq[] */ - mtx_destroy(&lsta->txq_mtx); - - /* Remove lsta if added_to_drv. */ - - /* Remove lsta from vif */ - /* Remove ref from lsta node... */ - /* Free lsta. */ - lkpi_lsta_remove(lsta, VAP_TO_LVIF(ni->ni_vap)); + /* + * Pass in the original ni just in case of error we could check that + * it is the same as lsta->ni. + */ + lkpi_lsta_free(lsta, ni); -out: if (lhw->ic_node_free != NULL) lhw->ic_node_free(ni); } @@ -3493,6 +3537,11 @@ lkpi_ic_raw_xmit(struct ieee80211_node *ni, struct mbuf *m, struct lkpi_sta *lsta; lsta = ni->ni_drv_data; + /* XXX locking */ + if (!lsta->txq_ready) { + m_free(m); + return (ENETDOWN); + } /* Queue the packet and enqueue the task to handle it. */ LKPI_80211_LSTA_LOCK(lsta); diff --git a/sys/compat/linuxkpi/common/src/linux_80211.h b/sys/compat/linuxkpi/common/src/linux_80211.h index 4aeca414973c..b36b27168566 100644 --- a/sys/compat/linuxkpi/common/src/linux_80211.h +++ b/sys/compat/linuxkpi/common/src/linux_80211.h @@ -134,6 +134,7 @@ struct lkpi_sta { struct ieee80211_key_conf *kc; enum ieee80211_sta_state state; + bool txq_ready; /* Can we run the taskq? */ bool added_to_drv; /* Driver knows; i.e. we called ...(). */ bool in_mgd; /* XXX-BZ should this be per-vif? */