Hello Richard,
> > 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.
I think it's not realistic with current PF at OpenBSD. The pf_test() function
does not run concurrently, so there can be no such race.
FYI: PF@solaris uses mutex there to protect the insertion to gcl. Code goes as
follows at Solaris:
if (!(r->rule_flags & PFRULE_EXPIRED)) {
mutex_enter(&gcl_mutex);
/* retest under exclusive protection */
if (!(r->rule_flags & PFRULE_EXPIRED)) {
r->rule_flag |= PFRULE_EXPIRED;
SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
}
}
regards
sasha
