git: 6bc57500b8b7 - main - pf: Change source limiter tables from a task
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Mon, 12 Oct 2026 16:49:17 UTC
The branch main has been updated by rcm:
URL: https://cgit.FreeBSD.org/src/commit/?id=6bc57500b8b700a3d9998fd417fdee653f93366d
commit 6bc57500b8b700a3d9998fd417fdee653f93366d
Author: R. Christian McDonald <rcm@FreeBSD.org>
AuthorDate: 2026-10-12 16:47:21 +0000
Commit: R. Christian McDonald <rcm@FreeBSD.org>
CommitDate: 2026-10-12 16:47:21 +0000
pf: Change source limiter tables from a task
pf_source_used() puts a source in the table of its source limiter when
the source has more states than the high-water mark, and
pf_source_rele() takes it out again below the low-water mark. Both
changed the table in place: pf_source_used() from pf_create_state(),
with the rules lock held for reading, and pf_source_rele() from
pf_remove_state(), with no rules lock at all. A table may only be
changed with the rules lock held for writing, as packets look addresses
up in its radix tree at the same time. With INVARIANTS the first
insertion panics ("Lock pf rulesets not exclusively locked" in
pfr_route_kentry()); without, the tree can be corrupted.
Queue the changes instead, as pf_overload_task() does for overload
tables, and make them from a task with the rules lock held for writing.
An entry names the limiter by its id and remembers its table: the
limiter, or its table, may be gone or replaced by the time the task
runs, and then the entry is dropped. pfsr_intable changes only once
the change is queued, so a failed allocation is retried the next time
the source crosses a mark; a removal is also retried when the source is
purged. Drain the task when a vnet goes away, after its last state.
Add a regression test.
Reviewed by: kp
Fixes: 461648121230 ("pf: introduce source and state limiters")
Sponsored by: Rubicon Communications, LLC ("Netgate")
Differential Revision: https://reviews.freebsd.org/D60526
---
sys/netpfil/pf/pf.c | 134 ++++++++++++++++++++++++++++++++++-----
tests/sys/netpfil/pf/limiters.sh | 60 ++++++++++++++++++
2 files changed, 177 insertions(+), 17 deletions(-)
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index dffaabf77434..817790ba0ef0 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -286,6 +286,31 @@ MTX_SYSINIT(pf_overloadqueue_mtx, &pf_overloadqueue_mtx,
#define PF_OVERLOADQ_LOCK() mtx_lock(&pf_overloadqueue_mtx)
#define PF_OVERLOADQ_UNLOCK() mtx_unlock(&pf_overloadqueue_mtx)
+/*
+ * Source limiter overload table changes, applied by pf_source_table_task().
+ * Tables can only be modified with the rules lock held exclusively, which
+ * packet processing does not hold.
+ */
+struct pf_source_table_entry {
+ STAILQ_ENTRY(pf_source_table_entry) next;
+ struct pfr_addr addr;
+ struct pfr_ktable *table; /* compared only */
+ uint32_t id;
+ bool insert;
+};
+
+STAILQ_HEAD(pf_source_table_head, pf_source_table_entry);
+VNET_DEFINE_STATIC(struct pf_source_table_head, pf_source_tablequeue);
+#define V_pf_source_tablequeue VNET(pf_source_tablequeue)
+VNET_DEFINE_STATIC(struct task, pf_source_tabletask);
+#define V_pf_source_tabletask VNET(pf_source_tabletask)
+
+static struct mtx_padalign pf_source_tablequeue_mtx;
+MTX_SYSINIT(pf_source_tablequeue_mtx, &pf_source_tablequeue_mtx,
+ "pf source limiter table queue", MTX_DEF);
+#define PF_SOURCE_TABLEQ_LOCK() mtx_lock(&pf_source_tablequeue_mtx)
+#define PF_SOURCE_TABLEQ_UNLOCK() mtx_unlock(&pf_source_tablequeue_mtx)
+
VNET_DEFINE(struct pf_krulequeue, pf_unlinked_rules);
struct mtx_padalign pf_unlnkdrules_mtx;
MTX_SYSINIT(pf_unlnkdrules_mtx, &pf_unlnkdrules_mtx, "pf unlinked rules",
@@ -402,6 +427,9 @@ static void pf_log_matches(struct pf_pdesc *, struct pf_krule *,
struct pf_krule *, struct pf_kruleset *,
struct pf_krule_slist *);
static void pf_overload_task(void *v, int pending);
+static void pf_source_table_task(void *v, int pending);
+static bool pf_source_table_enqueue(struct pf_source *sr,
+ bool insert);
static u_short pf_insert_src_node(struct pf_ksrc_node *[PF_SN_MAX],
struct pf_srchash *[PF_SN_MAX], struct pf_krule *,
struct pf_addr *, sa_family_t, struct pf_addr *,
@@ -571,6 +599,15 @@ pf_source_purge(void)
srlim->pfsrlim_rate.seconds + 1)
continue;
+ /* Retry a removal pf_source_rele() failed to queue. */
+ if (sr->pfsr_intable &&
+ srlim->pfsrlim_overload.table != NULL &&
+ srlim->pfsrlim_overload.lwm > 0) {
+ if (!pf_source_table_enqueue(sr, false))
+ continue;
+ sr->pfsr_intable = 0;
+ }
+
TAILQ_REMOVE(&pf_source_gc, sr, pfsr_empty_gc);
RB_REMOVE(pf_source_tree, &srlim->pfsrlim_sources, sr);
@@ -603,11 +640,79 @@ pf_source_pfr_addr(struct pfr_addr *p, const struct pf_source *sr)
}
}
+/*
+ * Queue an overload table change for pf_source_table_task(). The entry
+ * identifies the limiter by id and remembers its table: either may be gone
+ * or replaced by the time the task runs, in which case the change is
+ * dropped.
+ */
+static bool
+pf_source_table_enqueue(struct pf_source *sr, bool insert)
+{
+ struct pf_source_table_entry *pfste;
+
+ pfste = malloc(sizeof(*pfste), M_PFTEMP, M_NOWAIT);
+ if (pfste == NULL)
+ return (false);
+
+ pf_source_pfr_addr(&pfste->addr, sr);
+ pfste->table = sr->pfsr_parent->pfsrlim_overload.table;
+ pfste->id = sr->pfsr_parent->pfsrlim_id;
+ pfste->insert = insert;
+
+ PF_SOURCE_TABLEQ_LOCK();
+ STAILQ_INSERT_TAIL(&V_pf_source_tablequeue, pfste, next);
+ PF_SOURCE_TABLEQ_UNLOCK();
+ taskqueue_enqueue(taskqueue_swi, &V_pf_source_tabletask);
+
+ return (true);
+}
+
+static void
+pf_source_table_task(void *v, int pending __unused)
+{
+ struct pf_source_table_head queue = STAILQ_HEAD_INITIALIZER(queue);
+ struct pf_source_table_entry *pfste, *npfste;
+ struct pf_sourcelim *srlim;
+ struct pfr_ktable *t;
+
+ CURVNET_SET((struct vnet *)v);
+
+ PF_SOURCE_TABLEQ_LOCK();
+ STAILQ_CONCAT(&queue, &V_pf_source_tablequeue);
+ PF_SOURCE_TABLEQ_UNLOCK();
+
+ /* A previous run may already have processed the queue. */
+ if (STAILQ_EMPTY(&queue)) {
+ CURVNET_RESTORE();
+ return;
+ }
+
+ PF_RULES_WLOCK();
+ STAILQ_FOREACH(pfste, &queue, next) {
+ srlim = pf_sourcelim_find(pfste->id);
+ if (srlim == NULL ||
+ (t = srlim->pfsrlim_overload.table) == NULL ||
+ t != pfste->table)
+ continue;
+
+ if (pfste->insert)
+ pfr_insert_kentry(t, &pfste->addr, time_second);
+ else
+ pfr_remove_kentry(t, &pfste->addr);
+ }
+ PF_RULES_WUNLOCK();
+
+ STAILQ_FOREACH_SAFE(pfste, &queue, next, npfste)
+ free(pfste, M_PFTEMP);
+
+ CURVNET_RESTORE();
+}
+
static void
pf_source_used(struct pf_source *sr)
{
struct pf_sourcelim *srlim = sr->pfsr_parent;
- struct pfr_ktable *t;
unsigned int used;
used = sr->pfsr_inuse++;
@@ -615,14 +720,10 @@ pf_source_used(struct pf_source *sr)
if (used == 0)
TAILQ_REMOVE(&pf_source_gc, sr, pfsr_empty_gc);
- else if ((t = srlim->pfsrlim_overload.table) != NULL &&
+ else if (srlim->pfsrlim_overload.table != NULL &&
used >= srlim->pfsrlim_overload.hwm && !sr->pfsr_intable) {
- struct pfr_addr p;
-
- pf_source_pfr_addr(&p, sr);
-
- pfr_insert_kentry(t, &p, time_second);
- sr->pfsr_intable = 1;
+ if (pf_source_table_enqueue(sr, true))
+ sr->pfsr_intable = 1;
}
}
@@ -630,20 +731,14 @@ static void
pf_source_rele(struct pf_source *sr)
{
struct pf_sourcelim *srlim = sr->pfsr_parent;
- struct pfr_ktable *t;
unsigned int used;
used = --sr->pfsr_inuse;
- t = srlim->pfsrlim_overload.table;
- if (t != NULL && sr->pfsr_intable &&
+ if (srlim->pfsrlim_overload.table != NULL && sr->pfsr_intable &&
used < srlim->pfsrlim_overload.lwm) {
- struct pfr_addr p;
-
- pf_source_pfr_addr(&p, sr);
-
- pfr_remove_kentry(t, &p);
- sr->pfsr_intable = 0;
+ if (pf_source_table_enqueue(sr, false))
+ sr->pfsr_intable = 0;
}
if (used == 0) {
@@ -1543,6 +1638,8 @@ retry_waitok3:
STAILQ_INIT(&V_pf_sendqueue);
SLIST_INIT(&V_pf_overloadqueue);
TASK_INIT(&V_pf_overloadtask, 0, pf_overload_task, curvnet);
+ STAILQ_INIT(&V_pf_source_tablequeue);
+ TASK_INIT(&V_pf_source_tabletask, 0, pf_source_table_task, curvnet);
/* Unlinked, but may be referenced rules. */
TAILQ_INIT(&V_pf_unlinked_rules);
@@ -2980,6 +3077,9 @@ pf_unload_vnet_purge(void)
pf_purge_expired_src_nodes();
pf_source_purge();
+ /* All states are gone, so no more table changes can be queued. */
+ taskqueue_drain(taskqueue_swi, &V_pf_source_tabletask);
+
/*
* Now all kifs & rules should be unreferenced,
* thus should be successfully freed.
diff --git a/tests/sys/netpfil/pf/limiters.sh b/tests/sys/netpfil/pf/limiters.sh
index c8ecc517e26c..d109fa424d22 100644
--- a/tests/sys/netpfil/pf/limiters.sh
+++ b/tests/sys/netpfil/pf/limiters.sh
@@ -306,6 +306,65 @@ source_basic_cleanup()
pft_cleanup
}
+atf_test_case "source_table" "cleanup"
+source_table_head()
+{
+ atf_set descr 'Test the table of a source limiter'
+ atf_set require.user root
+}
+
+source_table_body()
+{
+ pft_init
+
+ epair=$(vnet_mkepair)
+
+ ifconfig ${epair}a 192.0.2.2/24 up
+
+ vnet_mkjail alcatraz ${epair}b
+ jexec alcatraz ifconfig ${epair}b 192.0.2.1/24 up
+
+ # Sanity check
+ atf_check -s exit:0 -o ignore \
+ ping -c 1 192.0.2.1
+
+ jexec alcatraz pfctl -e
+
+ # This used to panic a kernel with INVARIANTS: the table was changed
+ # while the packet was handled, with the rules lock held for reading.
+ pft_set_rules alcatraz \
+ "set timeout icmp.error 120" \
+ "table <many> persist" \
+ "source limiter \"server\" id 1 entries 128 limit 100 table <many> above 2 below 2" \
+ "pass in proto icmp source limiter \"server\""
+
+ # Each ping creates a state. Two states do not exceed the mark.
+ atf_check -s exit:0 -o ignore ping -c 1 192.0.2.1
+ atf_check -s exit:0 -o ignore ping -c 1 192.0.2.1
+ sleep 1
+ atf_check -s exit:0 -o empty \
+ jexec alcatraz pfctl -t many -T show
+
+ atf_check -s exit:0 -o ignore ping -c 1 192.0.2.1
+ atf_check -s exit:0 -o ignore ping -c 1 192.0.2.1
+ # The address is added to the table by a task.
+ sleep 1
+ atf_check -s exit:0 -o "match:192.0.2.2" \
+ jexec alcatraz pfctl -t many -T show
+
+ # Once the states are gone the source is below the mark and is removed.
+ atf_check -s exit:0 -e ignore \
+ jexec alcatraz pfctl -F states
+ sleep 1
+ atf_check -s exit:0 -o empty \
+ jexec alcatraz pfctl -t many -T show
+}
+
+source_table_cleanup()
+{
+ pft_cleanup
+}
+
atf_test_case "out" "cleanup"
out_head()
{
@@ -358,5 +417,6 @@ atf_init_test_cases()
atf_add_test_case "state_block"
atf_add_test_case "state_multiple"
atf_add_test_case "source_basic"
+ atf_add_test_case "source_table"
atf_add_test_case "out"
}