From nobody Fri Feb 04 14:23:21 2022 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 DCF6C19A80AD; Fri, 4 Feb 2022 14:23:21 +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 4JqyST4tRRz4RyK; Fri, 4 Feb 2022 14:23:21 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1643984601; 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=FA3vVaW8ET6D7XsZF93J8T4LUio/cR5l7HIQ9s+zvpc=; b=hkb5FFOX9WN5PHrbxwKtjLMnAtmR0B+fSiVFOvZqxcB2mUeal885tlN1F9rH/dMmcSnRvF suRwD8ZKyTlr8OlHvQ7s6nxXlsv+QhoX6ZR9TfGhY9FwTN0acUa92zmylwrBXUxBHejLc3 IDGKa5M2TOWmgZo5woH3En//czH+vGvmyHNjvhfwih4vME0VNEikfJRbzuyY0rJqVZ2VT1 lYHCCBiGRDkdocgKvCa1OV5fE0BW1qIuKyDSW7CCmEw1R1Ikm0wV+1dXuEiSsYlU7U1cW4 Mh6JanJ9O9QeueSTBo+jCpnli958PCQk6P606d34OvvS1j9c2rjkVM1feU4S3Q== 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 7C9855342; Fri, 4 Feb 2022 14:23:21 +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 214ENLjD050988; Fri, 4 Feb 2022 14:23:21 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 214ENLvp050987; Fri, 4 Feb 2022 14:23:21 GMT (envelope-from git) Date: Fri, 4 Feb 2022 14:23:21 GMT Message-Id: <202202041423.214ENLvp050987@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Kristof Provost Subject: git: a9f1b6ccfa82 - stable/12 - pf: protect the rpool from races 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/stable/12 X-Git-Reftype: branch X-Git-Commit: a9f1b6ccfa8279f30a705482cd780523eac9032e Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1643984601; 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=FA3vVaW8ET6D7XsZF93J8T4LUio/cR5l7HIQ9s+zvpc=; b=DSbc/XbIe8ISLQDCtgOpQTa8x+ttJa6fdyaL2p+YtMcId/ei0FyHCCsHOV1GfGFv33Nptz Ro6gMf4v4zRYkE01zYJkY1z57yO8gAlWCL4Gn4ZxGJpvv6Cl8DMYj9ry1olKm9chgMz/oA P4mXuyyZV/HDYYYGhOMo4bowMBhl6A7aE3lnJWmo91cEXUOHs9aTi2XYLbPlqp4zX53D55 to52QppH7BJRQAUx0wdw8oLtpwrNjS4j/jTlygtc1GquuDz+VrH19S605cc170COP4RSFl xH1TV6eqKG/8trR0DS2toTCCCXgN9CWr3+aTF1rWKoAGIvIg9MDqcAnU5Pnaqg== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1643984601; a=rsa-sha256; cv=none; b=GzG2Ekv24osWx7COHnb5S/vmswBDPSE+zwPlMmF7n8JR+9XRIqpu81PzvSjZDbxDz4WXLa MgwlxmPuWNrmcsW+V+R2BNUhYiQkQD9cLPJYKBCC6SBBVmmRxo9T0s7GbNEdrvOKitAtPW gpnAmY/3hXUAjr8xMlKnPbPqpazJHfkitICVz0gC90Eo/Q/RtLyvK/NfErdj+a1GgQAZlX uJLn99zs4Hqnj2Q/8SF1uD7j13a+zWsXa8q687Etgx4QIn9Y7liVLDr6WVYCC6ClcbHW2P 72tL0/g617zxMLoh1hVzRfw5AuqyPFNYe69SvXibG3mWajjq2UAb1vQgAbEfdg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch stable/12 has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=a9f1b6ccfa8279f30a705482cd780523eac9032e commit a9f1b6ccfa8279f30a705482cd780523eac9032e Author: Kristof Provost AuthorDate: 2022-01-10 16:49:26 +0000 Commit: Kristof Provost CommitDate: 2022-02-04 10:37:44 +0000 pf: protect the rpool from races The roundrobin pool stores its state in the rule, which could potentially lead to invalid addresses being returned. For example, thread A just executed PF_AINC(&rpool->counter) and immediately afterwards thread B executes PF_ACPY(naddr, &rpool->counter) (i.e. after the pf_match_addr() check of rpool->counter). Lock the rpool with its own mutex to prevent these races. The performance impact of this is expected to be low, as each rule has its own lock, and the lock is also only relevant when state is being created (so only for the initial packets of a connection, not for all traffic). See also: https://redmine.pfsense.org/issues/12660 Reviewed by: glebius MFC after: 3 weeks Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D33874 (cherry picked from commit 5f5e32f1b3945087a687c5962071d3f46e34e1ff) --- sys/net/pfvar.h | 1 + sys/netpfil/pf/pf_ioctl.c | 4 ++++ sys/netpfil/pf/pf_lb.c | 46 +++++++++++++++++++--------------------------- 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index 4c4fc7c65015..469b7250b896 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -547,6 +547,7 @@ struct pf_kpooladdr { TAILQ_HEAD(pf_kpalist, pf_kpooladdr); struct pf_kpool { + struct mtx mtx; struct pf_kpalist list; struct pf_kpooladdr *cur; struct pf_poolhashkey key; diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index f5d7626c6754..48bbfe53b948 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -1538,6 +1538,8 @@ pf_krule_free(struct pf_krule *rule) counter_u64_free(rule->states_cur); counter_u64_free(rule->states_tot); counter_u64_free(rule->src_nodes); + + mtx_destroy(&rule->rpool.mtx); free(rule, M_PFRULE); } @@ -2107,6 +2109,8 @@ pf_ioctl_addrule(struct pf_krule *rule, uint32_t ticket, TAILQ_INSERT_TAIL(ruleset->rules[rs_num].inactive.ptr, rule, entries); ruleset->rules[rs_num].inactive.rcount++; + + mtx_init(&rule->rpool.mtx, "pf_krule_pool", NULL, MTX_DEF); PF_RULES_WUNLOCK(); return (0); diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c index 24ff907cdc61..bf9afc8723e0 100644 --- a/sys/netpfil/pf/pf_lb.c +++ b/sys/netpfil/pf/pf_lb.c @@ -370,36 +370,45 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, return (0); } + mtx_lock(&rpool->mtx); /* Find the route using chosen algorithm. Store the found route in src_node if it was given or found. */ - if (rpool->cur->addr.type == PF_ADDR_NOROUTE) + if (rpool->cur->addr.type == PF_ADDR_NOROUTE) { + mtx_unlock(&rpool->mtx); return (1); + } if (rpool->cur->addr.type == PF_ADDR_DYNIFTL) { switch (af) { #ifdef INET case AF_INET: if (rpool->cur->addr.p.dyn->pfid_acnt4 < 1 && (rpool->opts & PF_POOL_TYPEMASK) != - PF_POOL_ROUNDROBIN) + PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); - raddr = &rpool->cur->addr.p.dyn->pfid_addr4; - rmask = &rpool->cur->addr.p.dyn->pfid_mask4; + } + raddr = &rpool->cur->addr.p.dyn->pfid_addr4; + rmask = &rpool->cur->addr.p.dyn->pfid_mask4; break; #endif /* INET */ #ifdef INET6 case AF_INET6: if (rpool->cur->addr.p.dyn->pfid_acnt6 < 1 && (rpool->opts & PF_POOL_TYPEMASK) != - PF_POOL_ROUNDROBIN) + PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); + } raddr = &rpool->cur->addr.p.dyn->pfid_addr6; rmask = &rpool->cur->addr.p.dyn->pfid_mask6; break; #endif /* INET6 */ } } else if (rpool->cur->addr.type == PF_ADDR_TABLE) { - if ((rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_ROUNDROBIN) + if ((rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_ROUNDROBIN) { + mtx_unlock(&rpool->mtx); return (1); /* unsupported */ + } } else { raddr = &rpool->cur->addr.v.a.addr; rmask = &rpool->cur->addr.v.a.mask; @@ -463,27 +472,6 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, { struct pf_kpooladdr *acur = rpool->cur; - /* - * XXXGL: in the round-robin case we need to store - * the round-robin machine state in the rule, thus - * forwarding thread needs to modify rule. - * - * This is done w/o locking, because performance is assumed - * more important than round-robin precision. - * - * In the simpliest case we just update the "rpool->cur" - * pointer. However, if pool contains tables or dynamic - * addresses, then "tblidx" is also used to store machine - * state. Since "tblidx" is int, concurrent access to it can't - * lead to inconsistence, only to lost of precision. - * - * Things get worse, if table contains not hosts, but - * prefixes. In this case counter also stores machine state, - * and for IPv6 address, counter can't be updated atomically. - * Probably, using round-robin on a table containing IPv6 - * prefixes (or even IPv4) would cause a panic. - */ - if (rpool->cur->addr.type == PF_ADDR_TABLE) { if (!pfr_pool_get(rpool->cur->addr.p.tbl, &rpool->tblidx, &rpool->counter, af)) @@ -507,6 +495,7 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, /* table contains no address of type 'af' */ if (rpool->cur != acur) goto try_next; + mtx_unlock(&rpool->mtx); return (1); } } else if (rpool->cur->addr.type == PF_ADDR_DYNIFTL) { @@ -516,6 +505,7 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, /* table contains no address of type 'af' */ if (rpool->cur != acur) goto try_next; + mtx_unlock(&rpool->mtx); return (1); } } else { @@ -535,6 +525,8 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr, if (*sn != NULL) PF_ACPY(&(*sn)->raddr, naddr, af); + mtx_unlock(&rpool->mtx); + if (V_pf_status.debug >= PF_DEBUG_MISC && (rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) { printf("pf_map_addr: selected address ");