On Thu, Jul 18, 2019 at 09:46:58AM +0200, Alexandr Nedvedicky wrote: > Hello, > > I've just realized my suggestion [1] to lteo@ was not complete. The single > atomic_cas() used as I've suggested is not sufficient measure. The code > should also do a non-atomic test to check, whether the rule is not expired > yet. > > the non-atomic check deals with scenario, where competing thread arrives > to atomic_cas() well before the current thread. > > OK?
Tested and makes sense to me. ok lteo@ > thanks and > regards > sashan > > [1] https://marc.info/?l=openbsd-bugs&m=156341942417148&w=2 > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf.c b/sys/net/pf.c > index b858c43963b..b001f185665 100644 > --- a/sys/net/pf.c > +++ b/sys/net/pf.c > @@ -3963,8 +3963,9 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, > struct pf_state **sm, > * 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) { > + if (((rule_flag & PFRULE_EXPIRED) == 0) && > + 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); > }
