git: 266331cb8bbc - stable/15 - pf: Re-optimize state key handling

The branch stable/15 has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=266331cb8bbcdaab45f183ddba8775cb0c4f79d0

commit 266331cb8bbcdaab45f183ddba8775cb0c4f79d0
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2026-08-25 18:09:37 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2026-09-09 08:34:33 +0000

    pf: Re-optimize state key handling
    
    pf states may be looked up using one of two keys: the stack key or the
    wire key.  For states involving address translation, these will be
    distinct; the stack key describes the addresses seen by the local
    network stack, and the wire key has the translated addresses.
    
    Historically, pf would avoid allocating separate keys if both are
    identical.  This changed in commit fcdb520c1b4e ("pf: nat64") to always
    allocate separate state key structures.  Incidentally, OpenBSD seems to
    maintain the optimization, but also has an explicit reference count
    embedded in state keys.
    
    The change breaks another optimization: pf_state_key_attach() still uses
    state key pointer equality to check whether the stack and wire keys are
    equal, so those checks are always false after the aforementioned commit.
    Thus we never skip the second key lookup, even when that's possible
    (i.e., no address translation is involved).
    
    So, for some rulesets we're consuming more memory than needed and
    performing more state key lookups than needed.  The behaviour of always
    looking up the stack key also happens to break some existing rulesets
    involving RDR and divert-to, which is how I noticed the problem.  I
    think those rulesets effectively worked by accident before, but it seems
    worth restoring the optimization regardless.
    
    Reviewed by:    kp
    MFC after:      2 weeks
    Fixes:          fcdb520c1b4e ("pf: nat64")
    Sponsored by:   OPNsense
    Sponsored by:   Klara, Inc.
    Differential Revision:  https://reviews.freebsd.org/D58922
    
    (cherry picked from commit 918fbc947356c1434760b1bc0deb8558283ce8c5)
---
 sys/netpfil/pf/pf.c    | 25 +++++++++++++++----------
 sys/netpfil/pf/pf_lb.c | 11 ++++++++++-
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index d8010ef7c224..8b17abb81b09 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -1742,14 +1742,14 @@ pf_state_key_setup(struct pf_pdesc *pd, u_int16_t sport, u_int16_t dport,
 	(*sk)->proto = pd->proto;
 	(*sk)->af = pd->af;
 
-	*nk = pf_state_key_clone(*sk);
-	if (*nk == NULL) {
-		uma_zfree(V_pf_state_key_z, *sk);
-		*sk = NULL;
-		return (ENOMEM);
-	}
-
 	if (pd->af != pd->naf) {
+		*nk = pf_state_key_clone(*sk);
+		if (*nk == NULL) {
+			uma_zfree(V_pf_state_key_z, *sk);
+			*sk = NULL;
+			return (ENOMEM);
+		}
+
 		(*sk)->port[pd->sidx] = pd->osport;
 		(*sk)->port[pd->didx] = pd->odport;
 
@@ -1787,6 +1787,8 @@ pf_state_key_setup(struct pf_pdesc *pd, u_int16_t sport, u_int16_t dport,
 		default:
 			(*nk)->proto = pd->proto;
 		}
+	} else {
+		*nk = *sk;
 	}
 
 	return (0);
@@ -6090,7 +6092,8 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
 		}
 	} else {
 		uma_zfree(V_pf_state_key_z, ctx.sk);
-		uma_zfree(V_pf_state_key_z, ctx.nk);
+		if (ctx.sk != ctx.nk)
+			uma_zfree(V_pf_state_key_z, ctx.nk);
 		ctx.sk = ctx.nk = NULL;
 		pf_udp_mapping_release(ctx.udp_mapping);
 	}
@@ -6117,7 +6120,8 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
 
 cleanup:
 	uma_zfree(V_pf_state_key_z, ctx.sk);
-	uma_zfree(V_pf_state_key_z, ctx.nk);
+	if (ctx.sk != ctx.nk)
+		uma_zfree(V_pf_state_key_z, ctx.nk);
 	pf_udp_mapping_release(ctx.udp_mapping);
 	*reason = ctx.reason;
 
@@ -6356,7 +6360,8 @@ pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx,
 
 csfailed:
 	uma_zfree(V_pf_state_key_z, ctx->sk);
-	uma_zfree(V_pf_state_key_z, ctx->nk);
+	if (ctx->sk != ctx->nk)
+		uma_zfree(V_pf_state_key_z, ctx->nk);
 
 	for (pf_sn_types_t sn_type=0; sn_type<PF_SN_MAX; sn_type++) {
 		if (pf_src_node_exists(&sns[sn_type], snhs[sn_type])) {
diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c
index beffdcd017b4..cb99038a07fa 100644
--- a/sys/netpfil/pf/pf_lb.c
+++ b/sys/netpfil/pf/pf_lb.c
@@ -1010,6 +1010,14 @@ pf_get_transaddr(struct pf_test_ctx *ctx, struct pf_krule *r,
 		if (pf_state_key_setup(pd, pd->nsport, pd->ndport, &ctx->sk,
 		    &ctx->nk))
 			return (PFRES_MEMORY);
+		if (ctx->sk == ctx->nk) {
+			ctx->nk = pf_state_key_clone(ctx->sk);
+			if (ctx->nk == NULL) {
+				uma_zfree(V_pf_state_key_z, ctx->sk);
+				ctx->sk = NULL;
+				return (PFRES_MEMORY);
+			}
+		}
 	}
 
 	switch (nat_action) {
@@ -1242,7 +1250,8 @@ out:
 	reason = PFRES_MAX;
 notrans:
 	uma_zfree(V_pf_state_key_z, ctx->nk);
-	uma_zfree(V_pf_state_key_z, ctx->sk);
+	if (ctx->nk != ctx->sk)
+		uma_zfree(V_pf_state_key_z, ctx->sk);
 	ctx->sk = ctx->nk = NULL;
 
 	return (reason);