The branch main has been updated by ks:
URL:
https://cgit.FreeBSD.org/src/commit/?id=cbd06dd2afd543fc3cb9aa95ab2219a9ec60619d
commit cbd06dd2afd543fc3cb9aa95ab2219a9ec60619d
Author: Kajetan Staszkiewicz <k...@freebsd.org>
AuthorDate: 2025-06-05 18:40:28 +0000
Commit: Kajetan Staszkiewicz <k...@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"
}
