git: a3173175193e - main - pf: Keep every chunk of a table that a ruleset defines
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Tue, 20 Oct 2026 12:52:41 UTC
The branch main has been updated by rcm:
URL: https://cgit.FreeBSD.org/src/commit/?id=a3173175193e111f2a92c999842bc3cfbcc2d945
commit a3173175193e111f2a92c999842bc3cfbcc2d945
Author: R. Christian McDonald <rcm@FreeBSD.org>
AuthorDate: 2026-10-20 12:45:35 +0000
Commit: R. Christian McDonald <rcm@FreeBSD.org>
CommitDate: 2026-10-20 12:52:00 +0000
pf: Keep every chunk of a table that a ruleset defines
libpfctl sends a table definition in chunks of 256 addresses, each as
its own PFNL_CMD_INA_DEFINE. The kernel treated every chunk as a new
definition and replaced the shadow table, so only the last chunk
survived.
Mark the chunks after the first with PF_ID_APPEND and have the kernel
add them to the shadow table in progress. A request without the
attribute still replaces the shadow; one with it and no definition in
progress fails with EINVAL. Old kernels ignore the attribute, so both
halves are needed; the ioctl sends all addresses at once and is not
affected. Mask the flags userland passes to PFR_FLAG_ALLMASK, as
PFR_FLAG_APPEND is kernel-internal.
Add a test that defines a table of 1,000 addresses, also in an anchor.
Reviewed by: kp
Fixes: 219ce3063840 ("pf: convert DIOCRINADEFINE to netlink")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60876
---
lib/libpfctl/libpfctl.c | 10 +++++--
sys/net/pfvar.h | 1 +
sys/netpfil/pf/pf_ioctl.c | 3 +-
sys/netpfil/pf/pf_nl.c | 5 ++++
sys/netpfil/pf/pf_nl.h | 1 +
sys/netpfil/pf/pf_table.c | 59 ++++++++++++++++++++++++++++++++++++
tests/sys/netpfil/pf/table.sh | 70 +++++++++++++++++++++++++++++++++++++++++++
7 files changed, 146 insertions(+), 3 deletions(-)
diff --git a/lib/libpfctl/libpfctl.c b/lib/libpfctl/libpfctl.c
index 6d7c45ff993f..f8ce51e2df18 100644
--- a/lib/libpfctl/libpfctl.c
+++ b/lib/libpfctl/libpfctl.c
@@ -4044,7 +4044,7 @@ SNL_DECLARE_PARSER(ina_define_parser, struct genlmsghdr, snl_f_p_empty, ap_ina_d
static int
_pfctl_ina_define(struct pfctl_handle *h, struct pfr_table *tbl,
struct pfr_addr *addr, int size, int *nadd, int *naddr,
- int ticket, int flags)
+ int ticket, int flags, bool append)
{
struct snl_writer nw;
struct snl_errmsg_data e = {};
@@ -4061,6 +4061,7 @@ _pfctl_ina_define(struct pfctl_handle *h, struct pfr_table *tbl,
snl_add_msg_attr_table(&nw, PF_ID_TABLE, tbl);
snl_add_msg_attr_u32(&nw, PF_ID_TICKET, ticket);
snl_add_msg_attr_u32(&nw, PF_ID_FLAGS, flags);
+ snl_add_msg_attr_bool(&nw, PF_ID_APPEND, append);
for (int i = 0; i < size; i++)
snl_add_msg_attr_pfr_addr(&nw, PF_ID_ADDR, &addr[i]);
@@ -4106,10 +4107,15 @@ pfctl_ina_define(struct pfctl_handle *h, struct pfr_table *tbl,
if (naddr)
*naddr = 0;
+ /*
+ * The addresses go in chunks. The first one defines the table, the
+ * others add to it; without that, each would define it anew and
+ * only the last would be left.
+ */
do {
chunk_size = MIN(size - off, 256);
ret = _pfctl_ina_define(h, tbl, &addr[off], chunk_size,
- &partial_add, &partial_addr, ticket, flags);
+ &partial_add, &partial_addr, ticket, flags, off > 0);
if (ret != 0)
break;
if (nadd)
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 533cb4f08283..6c7c7d364ba2 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -2442,6 +2442,7 @@ struct pfioc_trans {
#define PFR_FLAG_ALLMASK 0x000000FF
#ifdef _KERNEL
#define PFR_FLAG_USERIOCTL 0x10000000
+#define PFR_FLAG_APPEND 0x20000000
#endif
struct pfioc_table {
diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c
index 8685a877a00f..55a5c38e1301 100644
--- a/sys/netpfil/pf/pf_ioctl.c
+++ b/sys/netpfil/pf/pf_ioctl.c
@@ -6152,7 +6152,8 @@ DIOCCHANGEADDR_error:
PF_RULES_WLOCK();
error = pfr_ina_define(&io->pfrio_table, pfras,
io->pfrio_size, &io->pfrio_nadd, &io->pfrio_naddr,
- io->pfrio_ticket, io->pfrio_flags | PFR_FLAG_USERIOCTL);
+ io->pfrio_ticket,
+ (io->pfrio_flags & PFR_FLAG_ALLMASK) | PFR_FLAG_USERIOCTL);
PF_RULES_WUNLOCK();
free(pfras, M_PF);
break;
diff --git a/sys/netpfil/pf/pf_nl.c b/sys/netpfil/pf/pf_nl.c
index d35545bbfe32..de7994b314c4 100644
--- a/sys/netpfil/pf/pf_nl.c
+++ b/sys/netpfil/pf/pf_nl.c
@@ -2911,6 +2911,7 @@ struct nl_parsed_ina_define {
size_t addr_count;
uint32_t ticket;
uint32_t flags;
+ bool append;
};
#define _OUT(_field) offsetof(struct nl_parsed_ina_define, _field)
@@ -2919,6 +2920,7 @@ static const struct nlattr_parser nla_ina_define_parser[] = {
{ .type = PF_ID_TICKET, .off = _OUT(ticket), .cb = nlattr_get_uint32 },
{ .type = PF_ID_FLAGS, .off = _OUT(flags), .cb = nlattr_get_uint32 },
{ .type = PF_ID_ADDR, .off = _OUT(addrs), .cb = nlattr_get_pfr_addr },
+ { .type = PF_ID_APPEND, .off = _OUT(append), .cb = nlattr_get_bool },
};
#undef _OUT
NL_DECLARE_PARSER(ina_define_parser, struct genlmsghdr, nlf_p_empty, nla_ina_define_parser);
@@ -2935,6 +2937,9 @@ pf_handle_ina_define(struct nlmsghdr *hdr, struct nl_pstate *npt)
error = nl_parse_nlmsg(hdr, &ina_define_parser, npt, &attrs);
if (error != 0)
return (error);
+ attrs.flags &= PFR_FLAG_ALLMASK;
+ if (attrs.append)
+ attrs.flags |= PFR_FLAG_APPEND;
PF_RULES_WLOCK();
error = pfr_ina_define(&attrs.table, attrs.addrs.addrs,
diff --git a/sys/netpfil/pf/pf_nl.h b/sys/netpfil/pf/pf_nl.h
index d7adad2e8bd6..347265dcdefa 100644
--- a/sys/netpfil/pf/pf_nl.h
+++ b/sys/netpfil/pf/pf_nl.h
@@ -607,6 +607,7 @@ enum pf_ina_define_t {
PF_ID_ADDR = 4, /* pfr_addr */
PF_ID_NADD = 5, /* u32 */
PF_ID_NADDR = 6, /* u32 */
+ PF_ID_APPEND = 7, /* bool */
};
enum pf_osfp_t {
diff --git a/sys/netpfil/pf/pf_table.c b/sys/netpfil/pf/pf_table.c
index b1bb68935ab5..b0bbd3a9f67a 100644
--- a/sys/netpfil/pf/pf_table.c
+++ b/sys/netpfil/pf/pf_table.c
@@ -1544,6 +1544,53 @@ pfr_ina_begin(struct pfr_table *trs, u_int32_t *ticket, int *ndel, int flags)
return (0);
}
+/*
+ * Add addresses to the shadow table of a definition that is in progress.
+ * The shadow is left as it was unless all of them are valid and there is
+ * memory for them.
+ */
+static int
+pfr_ina_append(struct pfr_ktable *shadow, struct pfr_addr *addr, int size,
+ int *nadd, int *naddr)
+{
+ struct pfr_kentryworkq addrq;
+ struct pfr_kentry *p;
+ struct pfr_addr *ad;
+ int i, xaddr = 0;
+
+ PF_RULES_WASSERT();
+
+ if (shadow->pfrkt_cnt == NO_ADDRESSES)
+ return (EINVAL);
+ for (i = 0, ad = addr; i < size; i++, ad++)
+ if (pfr_validate_addr(ad))
+ return (EINVAL);
+ SLIST_INIT(&addrq);
+ for (i = 0, ad = addr; i < size; i++, ad++) {
+ if (pfr_lookup_addr(shadow, ad, 1) != NULL)
+ continue;
+ p = pfr_create_kentry(ad,
+ (shadow->pfrkt_flags & PFR_TFLAG_COUNTERS) != 0);
+ if (p == NULL) {
+ pfr_clean_node_mask(shadow, &addrq);
+ pfr_destroy_kentries(&addrq);
+ return (ENOMEM);
+ }
+ if (pfr_route_kentry(shadow, p)) {
+ pfr_destroy_kentry(p);
+ continue;
+ }
+ SLIST_INSERT_HEAD(&addrq, p, pfrke_workq);
+ xaddr++;
+ }
+ shadow->pfrkt_cnt += xaddr;
+ if (nadd != NULL)
+ *nadd = 0;
+ if (naddr != NULL)
+ *naddr = xaddr;
+ return (0);
+}
+
int
pfr_ina_define(struct pfr_table *tbl, struct pfr_addr *addr, int size,
int *nadd, int *naddr, u_int32_t ticket, int flags)
@@ -1570,6 +1617,18 @@ pfr_ina_define(struct pfr_table *tbl, struct pfr_addr *addr, int size,
tbl->pfrt_flags |= PFR_TFLAG_INACTIVE;
SLIST_INIT(&tableq);
kt = RB_FIND(pfr_ktablehead, &V_pfr_ktables, (struct pfr_ktable *)tbl);
+ /*
+ * The addresses of one definition may arrive in several requests.
+ * All but the first ask for theirs to be added to the shadow table
+ * that the first one made, instead of replacing it. A dry run makes
+ * no shadow table; it only checks the addresses, below.
+ */
+ if ((flags & (PFR_FLAG_APPEND | PFR_FLAG_DUMMY)) == PFR_FLAG_APPEND) {
+ if (kt == NULL || kt->pfrkt_shadow == NULL)
+ return (EINVAL);
+ return (pfr_ina_append(kt->pfrkt_shadow, addr, size, nadd,
+ naddr));
+ }
if (kt == NULL) {
kt = pfr_create_ktable(tbl, 0, 1);
if (kt == NULL)
diff --git a/tests/sys/netpfil/pf/table.sh b/tests/sys/netpfil/pf/table.sh
index e21d998b4ece..2b7c84032d41 100644
--- a/tests/sys/netpfil/pf/table.sh
+++ b/tests/sys/netpfil/pf/table.sh
@@ -949,6 +949,75 @@ load_cleanup()
pft_cleanup
}
+atf_test_case "define_large" "cleanup"
+define_large_head()
+{
+ atf_set descr 'Define a table of more addresses than fit in one request'
+ atf_set require.user root
+}
+
+define_large_body()
+{
+ pft_init
+ pwd=$(pwd)
+
+ vnet_mkjail alcatraz
+ jexec alcatraz pfctl -e
+
+ for i in `seq 1 4`; do
+ for j in `seq 1 250`; do
+ echo "192.0.${i}.${j}" >> ${pwd}/foo.lst
+ done
+ done
+ expected=$(wc -l < ${pwd}/foo.lst | awk '{ print $1; }')
+
+ # libpfctl passes the addresses of a definition to the kernel in
+ # chunks of 256. The kernel used to start the table over with each
+ # of them, so that only the last chunk was left. The second pass
+ # redefines a table that is in use, which the kernel commits by
+ # merging instead of swapping.
+ for pass in 1 2; do
+ pft_set_rules noflush alcatraz \
+ "table <foo> persist file \"${pwd}/foo.lst\"" \
+ "pass in from <foo>"
+ actual=$(jexec alcatraz pfctl -t foo -T show | wc -l | \
+ awk '{ print $1; }')
+ if [ ${actual} -ne ${expected} ]; then
+ atf_fail "pass ${pass}: ${actual} of ${expected} addresses"
+ fi
+ done
+ atf_check -s exit:0 -o match:"192.0.1.1" -o match:"192.0.4.250" \
+ jexec alcatraz pfctl -t foo -T show
+
+ # The tables of an anchor are defined after its rules were loaded,
+ # by another path through pfctl.
+ pft_set_rules alcatraz \
+ "anchor \"bar\" {" \
+ " table <baz> persist file \"${pwd}/foo.lst\"" \
+ " pass in from <baz>" \
+ "}"
+ actual=$(jexec alcatraz pfctl -a bar -t baz -T show | wc -l | \
+ awk '{ print $1; }')
+ if [ ${actual} -ne ${expected} ]; then
+ atf_fail "anchor: ${actual} of ${expected} addresses"
+ fi
+
+ # A table defined twice in one ruleset is its last definition: the
+ # first request of a definition replaces what is there, only the
+ # later chunks of the same definition add to it.
+ pft_set_rules alcatraz \
+ "table <foo> { 192.0.2.1 }" \
+ "table <foo> { 192.0.2.2 }" \
+ "pass in from <foo>"
+ atf_check -s exit:0 -o match:"192.0.2.2" -o not-match:"192.0.2.1" \
+ jexec alcatraz pfctl -t foo -T show
+}
+
+define_large_cleanup()
+{
+ pft_cleanup
+}
+
atf_test_case "test" "cleanup"
test_head()
{
@@ -1095,6 +1164,7 @@ atf_init_test_cases()
atf_add_test_case "replace_verbose"
atf_add_test_case "replace_create"
atf_add_test_case "load"
+ atf_add_test_case "define_large"
atf_add_test_case "test"
atf_add_test_case "test_verbose"
atf_add_test_case "show_no_counters"