On Wed, 16 Dec 2015, Alexandr Nedvedicky wrote:
> On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote:
> > Quoting pf.conf(5) "once - Creates a one shot rule that will remove itself
> > from an active ruleset after the first match."
> >
> > A 'first' match presupposes a total ordering. This comes for free when pf
> > is single-threaded but concurrent rule matching must take the trouble to
> > re-impose it somewhere. (I assume that pf aims to do concurrent matching
> > of packets against the ruleset.)
>
> pf@solaris allows the race here.
> The price for correct behavior, which is to allow one and only one
> packet to hit the rule is too high (given the once rules are kind of
> special case).
I agree, at least, I do for 'once' rules that aren't 'quick'.
> What has to be granted is there is one 'winner' only, which puts the
> rule to garbage collector list. The pragmatic approach wins here.
Right. I'll just note though that the patch as it stands allows multiple
winners: consider the window between testing PFRULE_EXPIRED and setting it
(quoted below). Multiple threads may execute in that window before one of
them closes it by setting PFRULE_EXPIRED. Each of them will delete the
same rule in turn, causing a probable crash. At least,
SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
crashes my machine.
Whether that's a realistic issue, I don't know. I have though been bitten
by enough edge cases like this to be very wary of them.
best,
Richard.
On Wed, 16 Dec 2015, Alexandr Nedvedicky wrote:
@@ -3135,6 +3160,10 @@ pf_test_rule(struct pf_pdesc *pd, struct
ruleset = &pf_main_ruleset;
r = TAILQ_FIRST(pf_main_ruleset.rules.active.ptr);
while (r != NULL) {
+ if (r->rule_flag & PFRULE_EXPIRED) {
+ r = TAILQ_NEXT(r, entries);
+ goto nextrule;
+ }
r->evaluations++;
PF_TEST_ATTRIB((pfi_kif_match(r->kif, pd->kif) == r->ifnot),
r->skip[PF_SKIP_IFP].ptr);
> @@ -3433,8 +3462,15 @@ pf_test_rule(struct pf_pdesc *pd, struct
> }
> #endif /* NPFSYNC > 0 */
>
> - if (r->rule_flag & PFRULE_ONCE)
> - pf_purge_rule(ruleset, r, aruleset, a);
> + if (r->rule_flag & PFRULE_ONCE) {
> + if ((a != NULL) && TAILQ_EMPTY(a->ruleset->rules.active.ptr)) {
> + a->rule_flag |= PFRULE_EXPIRED;
> + SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle);
> + }
> +
> + r->rule_flag |= PFRULE_EXPIRED;
> + SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
> + }
>
> #ifdef INET6
> if (rewrite && skw->af != sks->af)
