From nobody Wed Feb 14 19:49:28 2024 X-Original-To: dev-commits-src-main@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 4TZpgD6Rmtz52gvq; Wed, 14 Feb 2024 19:49:28 +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 4TZpgD4jB6z4jbn; Wed, 14 Feb 2024 19:49:28 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1707940168; 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=uGeFvfimV9pKPADbRaBH/XqTNP3x2a3lxzHvYxXy/Nc=; b=QthcpguPVK6WUzQW9ceFGofNX3mP7yKqWfLrkHcxuH5gsBLYrgDx0eAixDZjRi8tt0dCvS 7cwVXIJlp6mv1Gjx5JB1L25YU3cdqDmQNmkA6l9r2pZcz3K1Jw28p9myY0tM0Zpxf/XynY g5HcH0fdyh7yZtg8Ku4c61FDy3sz8XQERr9129vlrJ/3fd0tkJScE9Y9Rmo18UziCaX+ut tCp7kH95UQXd/JpTHEpP4b8Egp2wwJeaph4zwBI5SzMN/Julingc3Zqwbc3+L23fLiU/9W EKFvrYLaYbvd01Mb6YcC14uhWHxuhKHbUexyjcRtFu3rm3qGoipOmUWUB2483w== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1707940168; a=rsa-sha256; cv=none; b=DEpeEG7Pk8+tYqxkC3t3lcueWU6IpWLLG3BeLu/S5FQ/sA0TRtA2NCbmo3WnberH64eloM 4glsWc1d5REHu5a47SdN9HoO05zWXtPt6N9uSLetOKg+r1ETpWYtINzbo947dnMZXptz3r m0D4BCu+4OjoEbLYw2OoRBa5bkR05uNqbWSxXdsM/MYZ8ErGJnftOZpoSGUym+/mkDZ9JL /mwqlLz+Q8MZdNEBnwBpH6v1q8zHTo1AiVOUYpAS1Uwl2G5lRrpfi6Glttqs0d18cQAcZ+ o+WFp9pcwaT+xpI9aAcFiHPJQNAYXZzMYappfFObU1t+UVdwVPZxoem/KIKirQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1707940168; 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=uGeFvfimV9pKPADbRaBH/XqTNP3x2a3lxzHvYxXy/Nc=; b=soo41CVwSzVHcpmccOzddbzRxL51p05nvOrPxQca5P6UVbxI65XonhWYuhvVISW4VV/vOM TSbHyelBnpDWxyiCGA5k6IdFqrSaAGfGyWM2XyeHtPSb6A9LaayMPbEzlm7YEdwkHxAfOA AcprzBtjTkJDAXhVZjZPOQfypWab7TLtVtb83hoR+LrH7wXGpB9aiauwChIKoRiY7ymE5C c28VTNtxV237FTy6SodDyOYzw+ZP8ZKEKlVYV1gqEA3T+l98KbnfDeriIfnKMrrx2HlQ2V +c8LmTYr25F/SnJrGtelN3PgUfa9dElOa3lGrUiwHw2zkOJIezO8Pggloqe+Lg== 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 4TZpgD3h60z19kH; Wed, 14 Feb 2024 19:49:28 +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 41EJnS18068672; Wed, 14 Feb 2024 19:49:28 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.17.1/8.17.1/Submit) id 41EJnShA068669; Wed, 14 Feb 2024 19:49:28 GMT (envelope-from git) Date: Wed, 14 Feb 2024 19:49:28 GMT Message-Id: <202402141949.41EJnShA068669@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: "Bjoern A. Zeeb" Subject: git: 0936c648ad0e - main - LinuxKPI: 802.11: update the ni/lsta reference cycle List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@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/main X-Git-Reftype: branch X-Git-Commit: 0936c648ad0ee5152dc19f261e77fe9c1833fe05 Auto-Submitted: auto-generated The branch main has been updated by bz: URL: https://cgit.FreeBSD.org/src/commit/?id=0936c648ad0ee5152dc19f261e77fe9c1833fe05 commit 0936c648ad0ee5152dc19f261e77fe9c1833fe05 Author: Bjoern A. Zeeb AuthorDate: 2024-02-05 14:51:08 +0000 Commit: Bjoern A. Zeeb CommitDate: 2024-02-14 19:48:04 +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 MFC after: 3 days Reviewed by: cc Differential Revision: https://reviews.freebsd.org/D43753 --- 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 ea742371f797..99984569f35e 100644 --- a/sys/compat/linuxkpi/common/src/linux_80211.c +++ b/sys/compat/linuxkpi/common/src/linux_80211.c @@ -246,25 +246,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 * @@ -286,13 +275,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; @@ -377,6 +369,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); @@ -392,6 +385,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) { @@ -1051,11 +1092,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); } @@ -1064,7 +1111,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); } @@ -1072,6 +1119,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); @@ -1199,32 +1258,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__, @@ -1232,8 +1276,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; @@ -1286,6 +1335,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); @@ -1380,9 +1433,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 */ @@ -1718,9 +1775,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 */ @@ -2259,9 +2320,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 */ @@ -3408,7 +3473,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; @@ -3420,11 +3484,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(); @@ -3457,30 +3516,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); } @@ -3492,6 +3536,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? */