On Mon, Jul 15, 2019 at 11:06:50AM +0200, Alexandr Nedvedicky wrote:
> 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.
Thank you! I have committed the fix with your tweak.
Lawrence
