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)