From nobody Fri Jan 14 09:30:58 2022 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 72265195A698; Fri, 14 Jan 2022 09:30:58 +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 4JZwyp2mbVz3Q9S; Fri, 14 Jan 2022 09:30:58 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642152658; 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=V5+Cp/IGUo89Vr7L3SEw4q3fCO91P1ocVoYH97FuD3M=; b=Vv/7gbfjD+FlTDJ83lEDp2F1CVsUTYRyzNpGr9zdMxl89xymStstWtSrBUX/oA5AyXziqE aZqS9D44OeYGTMCYMQFIuJV6wFTDK3L6Q/Cw19iTuTthdnvv3rk+3EYfoh0JwXh8K7RBYL bYYUST6HX72MvBqk1bY/d09jZstFXSgqtdu5dG1BZqOGS1c7SfErMvfUBvpPt52EcDUfDS fznQh0CwuPwBd0E943qiW2wPOK5PGgrsoVdKnEIWOn83U9HPp3NUhf/bdlHPqe17/btS1+ 9hhZP10hH3oCo0Z7ZTKvfZOQ4gvbLBlNrxbe29OsPRhaYO84F/lLJ4RTMSoOaw== 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 3EB421174A; Fri, 14 Jan 2022 09:30:58 +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 20E9UwFP099181; Fri, 14 Jan 2022 09:30:58 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 20E9UwTD099180; Fri, 14 Jan 2022 09:30:58 GMT (envelope-from git) Date: Fri, 14 Jan 2022 09:30:58 GMT Message-Id: <202201140930.20E9UwTD099180@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: 5f5e32f1b394 - main - pf: protect the rpool from races 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: kp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 5f5e32f1b3945087a687c5962071d3f46e34e1ff Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642152658; 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=V5+Cp/IGUo89Vr7L3SEw4q3fCO91P1ocVoYH97FuD3M=; b=o7k1sgwsTEfD9KSFkJ4IVhA/oZ9Nan9YHAxgR/D7db4rhhSqB3BCxGzl8B5A8JMambjDw2 oU6UKtHRLIFxHbRJlfWN/N4171ktORipoA7hmZ58SoASoKdiPsje63nFJqQHBFKf+Eo2gR 7q3UHHNJP2RzamhB+NxsjkoUIDHLBKcn0mZn+O2HYOGGCXmx4iPuT1XyHwK3b8aZcOpdM7 YrsQlYdTb7BEXNHQ1Ju58JOr1oIlZHKxEiWy0dtVBQSj5Yw0PJEKJwxjUxtt5U3+lnQHHb CaUArmfQELP+TNoYLAiQaR7c94EJsYt1DZy5hpEjKjn422qUthpfwcdq84dTOQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1642152658; a=rsa-sha256; cv=none; b=axDn3DxwfND5lq/B3cDFlifRbjfoxH22waCM2kZPTpWcUppGXZOaNjk7aKBXmPGQlq/t9A aXIWzvP1x/YSncyGkaDaIA8T1JM/YUa74mAMD71PBg47k3D5Hglic82iVnSpe3fKAVcvex VIJpc7gJMBIwlCU6MQtM9L5KDV8hKNPOc8a2Bj2uhCA2pVU/fpraarNt4o9XwFERYnOlgn qBvWwN2nUt5Jdz+M2eX1iZb961zmzcjY9G5hkbMzn6F5dYeLe/2+02vxMa6wrU4+uVyiPi aXjwoCbiGeiJJIa1K+MjkC3h9VUlKdHmhhmCbXeA2uFRzb2T/R0mm5k0S91Ang== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=5f5e32f1b3945087a687c5962071d3f46e34e1ff commit 5f5e32f1b3945087a687c5962071d3f46e34e1ff Author: Kristof Provost AuthorDate: 2022-01-10 16:49:26 +0000 Commit: Kristof Provost CommitDate: 2022-01-14 09:30:33 +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 --- 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 115ff5cd6d36..7794b8f849fe 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -551,6 +551,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 f5612f6d0084..20bf8943e774 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -1542,6 +1542,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); } @@ -1999,6 +2001,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 26fe17a9c3b4..3803ff27a111 100644 --- a/sys/netpfil/pf/pf_lb.c +++ b/sys/netpfil/pf/pf_lb.c @@ -374,36 +374,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; @@ -467,27 +476,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)) @@ -511,6 +499,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) { @@ -520,6 +509,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 { @@ -539,6 +529,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_NOISY && (rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) { printf("pf_map_addr: selected address ");