From 7d85dfeac481c5346ed9378c75e5c3644ad5c3ba Mon Sep 17 00:00:00 2001 From: Lawrence Teo Date: Thu, 18 Jul 2019 02:03:47 +0000 Subject: 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@ --- sys/net/pf.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) (limited to 'sys') 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); -- cgit v1.2.3