follow up to 'once rule' expiration

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

follow up to 'once rule' expiration

Alexandr Nedvedicky
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?

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);
  }

Reply | Threaded
Open this post in threaded view
|

Re: follow up to 'once rule' expiration

Lawrence Teo-7
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);
>   }