git: cbd06dd2afd5 - main - pf: Fix error handling when pf_map_addr() fails

From: Kajetan Staszkiewicz <ks_at_FreeBSD.org>
Date: Sun, 13 Jul 2025 11:53:07 UTC
The branch main has been updated by ks:

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

commit cbd06dd2afd543fc3cb9aa95ab2219a9ec60619d
Author:     Kajetan Staszkiewicz <ks@FreeBSD.org>
AuthorDate: 2025-06-05 18:40:28 +0000
Commit:     Kajetan Staszkiewicz <ks@FreeBSD.org>
CommitDate: 2025-07-13 11:48:39 +0000

    pf: Fix error handling when pf_map_addr() fails
    
    When pf_map_addr() fails, for example for a NAT pool, we expect packet will
    not be forwarded. The error returned by pf_map_addr() has been ignored in
    pf_map_addr_sn(), though, causing packets being forwarded without NAT
    applied. Catch the error, return the error to caller, let the caller handle
    error counters for route-to pools just like it does for NAT pools. Add
    tests for NAT and route-to rules.
    
    Improve logging by not hardcoding function name and use __func__
    instead.
    
    Reviewed by:  kp
    Approved by:  kp
    Sponsored by: InnoGames GmbH
    Differential Revision:    https://reviews.freebsd.org/D50763
---
 sys/netpfil/pf/pf.c              |  9 +++++----
 sys/netpfil/pf/pf_lb.c           | 18 ++++++------------
 tests/sys/netpfil/pf/nat.sh      | 33 +++++++++++++++++++++++++++++++++
 tests/sys/netpfil/pf/route_to.sh | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index acdeebb85e30..a410fe570c39 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -5906,11 +5906,12 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm,
 		 * it is applied only from the last pass rule.
 		 */
 		pd->act.rt = r->rt;
-		/* Don't use REASON_SET, pf_map_addr increases the reason counters */
-		ctx.reason = pf_map_addr_sn(pd->af, r, pd->src, &pd->act.rt_addr,
-		    &pd->act.rt_kif, NULL, &(r->route), PF_SN_ROUTE);
-		if (ctx.reason != 0)
+		if ((transerror = pf_map_addr_sn(pd->af, r, pd->src,
+		    &pd->act.rt_addr, &pd->act.rt_kif, NULL, &(r->route),
+		    PF_SN_ROUTE)) != PFRES_MATCH) {
+			REASON_SET(&ctx.reason, transerror);
 			goto cleanup;
+		}
 	}
 
 	if (pd->virtual_proto != PF_VPROTO_FRAGMENT &&
diff --git a/sys/netpfil/pf/pf_lb.c b/sys/netpfil/pf/pf_lb.c
index d4728f61dce8..9db1d88ce52e 100644
--- a/sys/netpfil/pf/pf_lb.c
+++ b/sys/netpfil/pf/pf_lb.c
@@ -755,10 +755,6 @@ pf_map_addr(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr,
 done_pool_mtx:
 	mtx_unlock(&rpool->mtx);
 
-	if (reason) {
-		counter_u64_add(V_pf_status.counters[reason], 1);
-	}
-
 	return (reason);
 }
 
@@ -793,7 +789,7 @@ pf_map_addr_sn(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr,
 		if (nkif)
 			*nkif = sn->rkif;
 		if (V_pf_status.debug >= PF_DEBUG_NOISY) {
-			printf("pf_map_addr: src tracking maps ");
+			printf("%s: src tracking maps ", __func__);
 			pf_print_host(saddr, 0, af);
 			printf(" to ");
 			pf_print_host(naddr, 0, af);
@@ -808,14 +804,16 @@ pf_map_addr_sn(sa_family_t af, struct pf_krule *r, struct pf_addr *saddr,
 	 * Source node has not been found. Find a new address and store it
 	 * in variables given by the caller.
 	 */
-	if (pf_map_addr(af, r, saddr, naddr, nkif, init_addr, rpool) != 0) {
-		/* pf_map_addr() sets reason counters on its own */
+	if ((reason = pf_map_addr(af, r, saddr, naddr, nkif, init_addr,
+	    rpool)) != 0) {
+		if (V_pf_status.debug >= PF_DEBUG_MISC)
+			printf("%s: pf_map_addr has failed\n", __func__);
 		goto done;
 	}
 
 	if (V_pf_status.debug >= PF_DEBUG_NOISY &&
 	    (rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_NONE) {
-		printf("pf_map_addr: selected address ");
+		printf("%s: selected address ", __func__);
 		pf_print_host(naddr, 0, af);
 		if (nkif)
 			printf("@%s", (*nkif)->pfik_name);
@@ -826,10 +824,6 @@ done:
 	if (sn != NULL)
 		PF_SRC_NODE_UNLOCK(sn);
 
-	if (reason) {
-		counter_u64_add(V_pf_status.counters[reason], 1);
-	}
-
 	return (reason);
 }
 
diff --git a/tests/sys/netpfil/pf/nat.sh b/tests/sys/netpfil/pf/nat.sh
index f1fdf6405d97..16c981f97399 100644
--- a/tests/sys/netpfil/pf/nat.sh
+++ b/tests/sys/netpfil/pf/nat.sh
@@ -777,6 +777,38 @@ binat_match_cleanup()
 	kill $(cat ${PWD}/inetd_tester.pid)
 }
 
+atf_test_case "empty_pool" "cleanup"
+empty_pool_head()
+{
+	atf_set descr 'NAT with empty pool'
+	atf_set require.user root
+}
+
+empty_pool_body()
+{
+	pft_init
+	setup_router_server_ipv6
+
+
+	pft_set_rules router \
+		"block" \
+		"pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv }" \
+		"pass in  on ${epair_tester}b" \
+		"pass out on ${epair_server}a inet6 from any to ${net_server_host_server} nat-to <nonexistent>" \
+
+	# pf_map_addr_sn() won't be able to pick a target address, because
+	# the table used in redireciton pool is empty. Packet will not be
+	# forwarded, error counter will be increased.
+	ping_server_check_reply exit:1
+	# Ignore warnings about not-loaded ALTQ
+	atf_check -o "match:map-failed +1 +" -x "jexec router pfctl -qvvsi 2> /dev/null"
+}
+
+empty_pool_cleanup()
+{
+	pft_cleanup
+}
+
 atf_init_test_cases()
 {
 	atf_add_test_case "exhaust"
@@ -794,4 +826,5 @@ atf_init_test_cases()
 	atf_add_test_case "nat_match"
 	atf_add_test_case "binat_compat"
 	atf_add_test_case "binat_match"
+	atf_add_test_case "empty_pool"
 }
diff --git a/tests/sys/netpfil/pf/route_to.sh b/tests/sys/netpfil/pf/route_to.sh
index 5c0d355b8ea1..91c12c8ce2ae 100644
--- a/tests/sys/netpfil/pf/route_to.sh
+++ b/tests/sys/netpfil/pf/route_to.sh
@@ -859,6 +859,40 @@ ttl_cleanup()
 	pft_cleanup
 }
 
+
+atf_test_case "empty_pool" "cleanup"
+empty_pool_head()
+{
+	atf_set descr 'Route-to with empty pool'
+	atf_set require.user root
+}
+
+empty_pool_body()
+{
+	pft_init
+	setup_router_server_ipv6
+
+
+	pft_set_rules router \
+		"block" \
+		"pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv }" \
+		"pass in  on ${epair_tester}b route-to (${epair_server}a <nonexistent>) inet6 from any to ${net_server_host_server}" \
+		"pass out on ${epair_server}a"
+
+	# pf_map_addr_sn() won't be able to pick a target address, because
+	# the table used in redireciton pool is empty. Packet will not be
+	# forwarded, error counter will be increased.
+	ping_server_check_reply exit:1
+	# Ignore warnings about not-loaded ALTQ
+	atf_check -o "match:map-failed +1 +" -x "jexec router pfctl -qvvsi 2> /dev/null"
+}
+
+empty_pool_cleanup()
+{
+	pft_cleanup
+}
+
+
 atf_init_test_cases()
 {
 	atf_add_test_case "v4"
@@ -877,4 +911,5 @@ atf_init_test_cases()
 	atf_add_test_case "dummynet_double"
 	atf_add_test_case "sticky"
 	atf_add_test_case "ttl"
+	atf_add_test_case "empty_pool"
 }