From nobody Wed May 17 13:22:02 2023 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 4QLv0B5J8Yz4B7JM; Wed, 17 May 2023 13:22:02 +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 4QLv0B4pFZz3G0r; Wed, 17 May 2023 13:22:02 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1684329722; 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=Zed2XLp59ZZLgxJ8eMshQ/Y3NVqlft6c+aHXSpb6qEU=; b=F48yKSCwraFpUIfcfrYB3XnWOF6dsGeWaHfwJj449NLAbhAa7bE6VKqI8ZjzMHseaEOp1Z x38gYY8j4IoPXT34cx1K+/E50xv8n33OPExcNREOZhJJSvdRUDGIfK6OVjQXS/aqDVloCF h4kCJhu1w8cenG2bGiE+kUAg4LIddlyNMcM55kfS0/e2vuSSS55luPHQeCygD7vyIpL9RY 5UfbBgHvTrLkV/RBE9dLo9lu9njlZBk2si2iGlmprNvl5rn3Hs5GNOD+lNztAVx5cXLTTa 39jNJbqgaqWXgd9lioLKYuLm0SZagXF++C1hxwe5rqBynncGjJFrR4cs98EOBA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1684329722; 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=Zed2XLp59ZZLgxJ8eMshQ/Y3NVqlft6c+aHXSpb6qEU=; b=Csa17N4iose4Vxima5chsnsD0zSf15UvYqVBxFNa1XACIlwpbCUM29LZrkh4lm/MEujaNB ZrmOTC9YoaVf+eC3h/v+yYcvgfGaSUtaifWsyk3uEBTsqF3SSUcwv5qIanS84BNlRFHKth 1pw5AP5tufaF1GXOaDmba6SM+Mzu3KNi5FhJSIPqazvOCvtEMBlhZJ7FiLhOc9f3XDjNIG yY9h6qzvMEPA4c/Kwz+QGtyeBP49CwZ03nnMsg7vrAuFH/nX+NULj9LQ+B7fk4pV+94t+R U9fkMQHfKM+9U3ni8MFPGr7fOEBbXc8F1IFmpB+N05nLxCk928YYeU02zp9fRQ== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1684329722; a=rsa-sha256; cv=none; b=rvtzFD+H3b57vtJbhnfVwvTZ0XYLx/O+gcnkjV+TGlLKWobAL/EZTTe3XGMxzC1OM9oSN6 sPxPiUZBP3iCGM9iZw3Ue1c2AHcRrHDZGGSUQ1OvTyDeQVUZ0IDDh+lOPBWcqzC2Qpzrfv EaQkS4TB2jK18zb178hD/tb8PhmSLc3MCvzqS6wIWWjKjfC9vE6KF3cObGCR5gQCRgb3CN 3BrMlne+Ljotvf+/yh5BDv63jLGOSFn1Mfd8LuO5mIR5lR+suWVAppaLSFkS7QIovC+RGB Sub4wVG47jBL50+lS37xXlclDKQABQj7GX2UQAnMW/I0nbr9zuGOj6xltkxt6Q== 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 4QLv0B3Zqkz155x; Wed, 17 May 2023 13:22:02 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 34HDM2qk084089; Wed, 17 May 2023 13:22:02 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 34HDM2qS084088; Wed, 17 May 2023 13:22:02 GMT (envelope-from git) Date: Wed, 17 May 2023 13:22:02 GMT Message-Id: <202305171322.34HDM2qS084088@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Kristof Provost Subject: git: bdd47177528b - main - pf: release rules lock before passing the packet to dummynet 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: kp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: bdd47177528b5beacabb4837bfac0e9de92aae74 Auto-Submitted: auto-generated X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=bdd47177528b5beacabb4837bfac0e9de92aae74 commit bdd47177528b5beacabb4837bfac0e9de92aae74 Author: Kristof Provost AuthorDate: 2023-05-11 16:10:33 +0000 Commit: Kristof Provost CommitDate: 2023-05-17 13:20:18 +0000 pf: release rules lock before passing the packet to dummynet In the Ethernet rules we held the PF_RULES lock while we called ip_dn_io_ptr() (i.e. dummynet). That meant that we could end up back in pf while still holding the PF_RULES lock. That's not immediately fatal, because that lock is recursive, but still not ideal. There also appear to be scenarios where this can actually trigger deadlocks. We don't need to hold the PF_RULES lock, as long as we make a local copy of the data we need from the rule (in this case, the action and bridge_to target). It's safe to keep the struct ifnet pointer around, because we remain in NET_EPOCH. See also: https://redmine.pfsense.org/issues/14373 MFC after: 1 week Reviewed by: mjg Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D40067 --- sys/netpfil/pf/pf.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index 1424099b1e5a..06270d34da85 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -3875,10 +3875,8 @@ pf_match_eth_tag(struct mbuf *m, struct pf_keth_rule *r, int *tag, int mtag) } static void -pf_bridge_to(struct pfi_kkif *kif, struct mbuf *m) +pf_bridge_to(struct ifnet *ifp, struct mbuf *m) { - struct ifnet *ifp = kif->pfik_ifp; - /* If we don't have the interface drop the packet. */ if (ifp == NULL) { m_freem(m); @@ -3897,7 +3895,7 @@ pf_bridge_to(struct pfi_kkif *kif, struct mbuf *m) return; } - kif->pfik_ifp->if_transmit(kif->pfik_ifp, m); + ifp->if_transmit(ifp, m); } static int @@ -3916,6 +3914,7 @@ pf_test_eth_rule(int dir, struct pfi_kkif *kif, struct mbuf **m0) struct pf_mtag *mtag; struct pf_keth_ruleq *rules; struct pf_addr *src = NULL, *dst = NULL; + struct pfi_kkif *bridge_to; sa_family_t af = 0; uint16_t proto; int asd = 0, match = 0; @@ -4097,6 +4096,9 @@ pf_test_eth_rule(int dir, struct pfi_kkif *kif, struct mbuf **m0) mtag->qid = r->qid; } + action = r->action; + bridge_to = r->bridge_to; + /* Dummynet */ if (r->dnpipe) { struct ip_fw_args dnflow; @@ -4151,21 +4153,21 @@ pf_test_eth_rule(int dir, struct pfi_kkif *kif, struct mbuf **m0) break; } + PF_RULES_RUNLOCK(); + mtag->flags |= PF_TAG_DUMMYNET; ip_dn_io_ptr(m0, &dnflow); if (*m0 != NULL) mtag->flags &= ~PF_TAG_DUMMYNET; + } else { + PF_RULES_RUNLOCK(); } - action = r->action; - - if (action == PF_PASS && r->bridge_to) { - pf_bridge_to(r->bridge_to, *m0); + if (action == PF_PASS && bridge_to) { + pf_bridge_to(bridge_to->pfik_ifp, *m0); *m0 = NULL; /* We've eaten the packet. */ } - PF_RULES_RUNLOCK(); - return (action); }