On 13 December 2015 at 18:56, Richard Procter <richard.n.proc...@gmail.com> wrote: > > If I understand this patch: > > Rule removal requires the consistency lock but to acquire it one must be > able to sleep. pf_test_rule() cannot sleep as it executes in a (soft) > interrupt handler, so it passes the expired rule to a thread which can. > > However, the code to remove a 'once' rule may wish to also remove its > parent rule, now computed in the other thread.
This is right: "once" rules remove it's parent anchor if it's empty. > The updated patch below > avoids passing that state between threads in struct pf_rule by explictly > passing the parent rule to be purged when necessary. > > There also seem to me a couple of issues, one hairier than the other: > > - r->rule_flag should be updated atomically with respect to other > (potential) updates (included in the attached patch). > > - Multiple rule-matching threads may match the same 'once' rule before > one of them reaches the code to set PFRULE_EXPIRED. A thread may even > match a 'once' rule after PFRULE_EXPIRED is set, if that occurs during > the 'once' rule's candidacy as a potential match. (This issue is not > addressed by the attached patch.) > > This creates two problems: > > a) rules may be purged more than once > b) what to do with packets matched by expired 'once' rules > > In general, we do not know whether or not a 'once' rule candidate is > the match until a new candidate rule replaces it or the rule matching > process completes. Serialising this window of candicacy would prevent a > 'once' rule becoming a candidate, and possibly the match, for multiple > packets at once. That's one approach. > > A weaker approach instead permits multiple matches of one 'once' rule > but processes these serially in a critical section afterwards. The > first thread to reach it is deemed the first match (note this isn't > necessarily the first matching packet received at the interface!). It > then atomically marks it PFRULE_EXPIRED and places it on the purge > queue. Packets for matches of expired 'once' rule, it seems to me, can > be dropped as if we never saw them. An alternative might be to retry > them against the updated rules. > > Another possibility would be to require 'once' rules to be 'quick'. > This closes the candidacy window and makes its serialisation, to > preclude multiple matches, more feasible. > > Yet another possibility is to drop 'once' rules as too complex to > implement for multiprocessor but I have no idea if this is viable. > It is. And I have said that before with an authority of the implementor of "once" rules: since we don't have any userland applications that would use this yet, we can ditch them for now and possibly devise a better approach later. Don't make your lives harder than they have to be!
