Hello,
</snip>
>
> To fix both bugs, I wrote this diff that adds 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 further
> connections from creating state if they match the expired once rule.
>
> I have sent this diff to abieber@ who confirmed that it fixes the crash
> for him.
>
> Thoughts? ok?
>
thanks for detailed analysis. it is correct.
>
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1084
> diff -u -p -r1.1084 pf.c
> --- pf.c 9 Jul 2019 11:30:19 -0000 1.1084
> +++ pf.c 12 Jul 2019 02:54:33 -0000
> @@ -3862,6 +3862,13 @@ pf_test_rule(struct pf_pdesc *pd, struct
> if (r->action == PF_DROP)
> goto cleanup;
>
> + /*
> + * If an expired "once" rule has not been purged, drop any new matching
> + * packets to prevent the rule from being added to pf_rule_gcl again.
> + */
> + 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)
your change looks good and makes sense. However I would ask for one more
tweak to your fix:
--------8<---------------8<---------------8<------------------8<--------
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 9e454e5c941..df75d7e4acf 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -3860,6 +3860,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)
@@ -3945,9 +3952,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);
--------8<---------------8<---------------8<------------------8<--------
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.
you have my OK with/without the suggested tweak.
thanks and
regards
sashan
