This commit fixes two bugs involving PF once rules:

1. If a packet happens to match an expired once rule before the rule is removed
   by the purge thread, the rule will be added to the pf_rule_gcl list again,
   eventually causing a kernel crash when the purge thread tries to remove the
   expired rule multiple times; and

2. A packet that matches an expired once rule will still cause a state to be
   created, so a once rule is not truly a one shot rule while it is in that
   expired-but-not-purged time window.

To fix both bugs, add a check in pf_test_rule() to prevent expired once rules
from being added to pf_rule_gcl.  The check is added "early" in pf_test_rule()
to prevent any new connections from creating state if they match the expired
once rule.

This commit also includes a tweak by sashan@ to ensure that only one PF task
will mark a once rule as expired.  Here is sashan@'s commentary:

"As soon as there will be more PF tasks running in parallel, we would be
able to hit similar crash you are fixing now. The rules are covered by
read lock, so with more PF tasks there might be two packets racing
to expire at once rule at the same time. Using atomic_cas() is sufficient
measure to serialize competing packets."

tested by abieber@ who reported the kernel crash on bugs@
ok sashan@

1 files changed, 20 insertions, 4 deletions

diff --git a/sys/net/pf.c b/sys/net/pf.c
index b0e6a4b5352..b858c43963b 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf.c,v 1.1085 2019/07/11 09:39:52 sashan Exp $ */
+/*	$OpenBSD: pf.c,v 1.1086 2019/07/18 02:03:46 lteo Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -3864,6 +3864,13 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
 	if (r->action == PF_DROP)
 		goto cleanup;
 
+	/*
+	 * If an expired "once" rule has not been purged, drop any new matching
+	 * packets.
+	 */
+	if (r->rule_flag & PFRULE_EXPIRED)
+		goto cleanup;
+
 	pf_tag_packet(pd->m, ctx.tag, ctx.act.rtableid);
 	if (ctx.act.rtableid >= 0 &&
 	    rtable_l2(ctx.act.rtableid) != pd->rdomain)
@@ -3949,9 +3956,18 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
 #endif	/* NPFSYNC > 0 */
 
 	if (r->rule_flag & PFRULE_ONCE) {
-		r->rule_flag |= PFRULE_EXPIRED;
-		r->exptime = time_second;
-		SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
+		u_int32_t	rule_flag;
+
+		/*
+		 * Use atomic_cas() to determine a clear winner, which will
+		 * insert an expired rule to gcl.
+		 */
+		rule_flag = r->rule_flag;
+		if (atomic_cas_uint(&r->rule_flag, rule_flag,
+		    rule_flag | PFRULE_EXPIRED) == rule_flag) {
+			r->exptime = time_second;
+			SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
+		}
 	}
 
 	return (action);