Re: removing expired once rules in pf_purge_thread()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

richard.n.procter

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. 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.

best,
Richard.

Tested via pf.conf

set skip on lo
pass
block in proto icmp

anchor selfdestruct {
        pass in proto icmp once no state
}

Index: net/pf.c
===================================================================
--- net.orig/pf.c
+++ net/pf.c
@@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key,
 RB_GENERATE(pf_state_tree_id, pf_state,
     entry_id, pf_state_compare_id);
 
+SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl =
+ SLIST_HEAD_INITIALIZER(pf_rule_gcl);
+
 __inline int
 pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af)
 {
@@ -1137,6 +1140,24 @@ pf_state_export(struct pfsync_state *sp,
 /* END state table stuff */
 
 void
+pf_purge_expired_rules(void)
+{
+ struct pf_rule *r;
+
+ if (SLIST_EMPTY(&pf_rule_gcl)) {
+ return;
+ }
+
+ rw_enter_write(&pf_consistency_lock);
+ while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) {
+ SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle);
+ KASSERT(r->rule_flag & PFRULE_EXPIRED);
+ pf_purge_rule(r);
+ }
+ rw_exit_write(&pf_consistency_lock);
+}
+
+void
 pf_purge_thread(void *v)
 {
  int nloops = 0, s;
@@ -1154,6 +1175,7 @@ pf_purge_thread(void *v)
  if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
  pf_purge_expired_fragments();
  pf_purge_expired_src_nodes(0);
+ pf_purge_expired_rules();
  nloops = 0;
  }
 
@@ -3135,6 +3157,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 +3459,18 @@ 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) {
+ int only_rule = (TAILQ_NEXT(r, entries) == NULL &&
+ TAILQ_PREV(r, pf_rulequeue, entries) == NULL);
+
+ if (only_rule && a) {
+ atomic_setbits_int(&a->rule_flag, PFRULE_EXPIRED);
+ SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle);
+ }
+
+ atomic_setbits_int(&r->rule_flag, PFRULE_EXPIRED);
+ SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
+ }
 
 #ifdef INET6
  if (rewrite && skw->af != sks->af)
Index: net/pf_ioctl.c
===================================================================
--- net.orig/pf_ioctl.c
+++ net/pf_ioctl.c
@@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu
 }
 
 void
-pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule,
-    struct pf_ruleset *aruleset, struct pf_rule *arule)
+pf_purge_rule(struct pf_rule *rule)
 {
  u_int32_t nr = 0;
+ struct pf_ruleset *ruleset;
 
- KASSERT(ruleset != NULL && rule != NULL);
+ KASSERT((rule != NULL) && (rule->myruleset != NULL));
+ ruleset = rule->myruleset;
 
  pf_rm_rule(ruleset->rules.active.ptr, rule);
  ruleset->rules.active.rcount--;
@@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset
  rule->nr = nr++;
  ruleset->rules.active.ticket++;
  pf_calc_skip_steps(ruleset->rules.active.ptr);
-
- /* remove the parent anchor rule */
- if (nr == 0 && arule && aruleset) {
- pf_rm_rule(aruleset->rules.active.ptr, arule);
- aruleset->rules.active.rcount--;
- TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries)
- rule->nr = nr++;
- aruleset->rules.active.ticket++;
- pf_calc_skip_steps(aruleset->rules.active.ptr);
- }
 }
 
 u_int16_t
@@ -1209,6 +1200,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  }
  TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
     rule, entries);
+ rule->myruleset = ruleset;
  ruleset->rules.inactive.rcount++;
  break;
  }
Index: net/pfvar.h
===================================================================
--- net.orig/pfvar.h
+++ net/pfvar.h
@@ -563,6 +563,9 @@ struct pf_rule {
  struct pf_addr addr;
  u_int16_t port;
  } divert, divert_packet;
+
+ SLIST_ENTRY(pf_rule) gcle;
+ struct pf_ruleset *myruleset;
 };
 
 /* rule flags */
@@ -581,6 +584,7 @@ struct pf_rule {
 #define PFRULE_PFLOW 0x00040000
 #define PFRULE_ONCE 0x00100000 /* one shot rule */
 #define PFRULE_AFTO 0x00200000 /* af-to rule */
+#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */
 
 #define PFSTATE_HIWAT 10000 /* default state table size */
 #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */
@@ -1701,9 +1705,7 @@ extern void pf_addrcpy(struct pf_addr
     sa_family_t);
 void pf_rm_rule(struct pf_rulequeue *,
     struct pf_rule *);
-void pf_purge_rule(struct pf_ruleset *,
-    struct pf_rule *, struct pf_ruleset *,
-    struct pf_rule *);
+void pf_purge_rule(struct pf_rule *);
 struct pf_divert *pf_find_divert(struct mbuf *);
 int pf_setup_pdesc(struct pf_pdesc *, void *,
     sa_family_t, int, struct pfi_kif *,

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Mike Belopuhov-5
On 13 December 2015 at 18:56, Richard Procter
<[hidden email]> 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!

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
Hello,

> >
> >    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!
>

I'm just trying out patch improved by Richard. I like his idea to put anchor
rule to garbage collector list. I'd like to give it a try on Solaris. There
might be small changes in details.

IMO once rules are fine, and offloading hard task to service/garbage collector
thread works just fine.

regards
sasha

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Mike Belopuhov-5
On 15 December 2015 at 15:36, Alexandr Nedvedicky
<[hidden email]> wrote:

> Hello,
>
>> >
>> >    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!
>>
>
> I'm just trying out patch improved by Richard. I like his idea to put anchor
> rule to garbage collector list. I'd like to give it a try on Solaris. There
> might be small changes in details.
>
> IMO once rules are fine, and offloading hard task to service/garbage collector
> thread works just fine.
>
> regards
> sasha

It just occurred to me that another possibility would be a match-only
rule that matches one but doesn't involve any purging machinery.  Right
now we install ftp-proxy rules as having maximum number of states equal
to 1, however there's a time window between the moment the state is no
longer there, but anchor is not gone yet and it can potentially create
more states which is not a problem for an eavesdropper who can sniff
the plaintext protocol.

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
Hello,

> It just occurred to me that another possibility would be a match-only
> rule that matches one but doesn't involve any purging machinery.  Right
> now we install ftp-proxy rules as having maximum number of states equal
> to 1, however there's a time window between the moment the state is no
> longer there, but anchor is not gone yet and it can potentially create
> more states which is not a problem for an eavesdropper who can sniff
> the plaintext protocol.

I'm not sure I'm following you. IMO the expire flag should solve that problem.
As soon as rule is marked as expired it can not be matched by packet any more.

I'm attaching yet another version of patch. The list of changes to
patch sent by Richard is as follows:

        - atomic operations are gone as we don't need them
          right now. those will be introduced as soon as we will get
          there. pf_test()/pf_test_rule() is still processing a single
          packet at a time.

        - it's better to use TAILQ_EMPTY() instead of checking link
          members to determine if last rule is being removed from
          anchor `a`

        - I've also realized we have to call pf_purge_expired_rules()
          from pf_commit_rules(). We have to make sure expired rules are gone
          from active lists. I think this is the best approach for pf@Puffy
          currently. It will be changed as soon as ruleset locks and
          reference counting for rules will be merged in. I had to also
          reshuffle a consistency lock a bit in pf_purge_thread().

        - last but not least: silly name got renamed (s/myruleset/ruleset)

so what do you think? OK?

regards
sasha

---------8<----------------8<----------------8<---------------8<--------
Index: pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.960
diff -u -p -r1.960 pf.c
--- pf.c 6 Dec 2015 10:03:23 -0000 1.960
+++ pf.c 16 Dec 2015 00:52:22 -0000
@@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key,
 RB_GENERATE(pf_state_tree_id, pf_state,
     entry_id, pf_state_compare_id);
 
+SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl =
+ SLIST_HEAD_INITIALIZER(pf_rule_gcl);
+
 __inline int
 pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af)
 {
@@ -1137,6 +1140,18 @@ pf_state_export(struct pfsync_state *sp,
 /* END state table stuff */
 
 void
+pf_purge_expired_rules(void)
+{
+ struct pf_rule *r;
+
+ while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) {
+ SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle);
+ KASSERT(r->rule_flag & PFRULE_EXPIRED);
+ pf_purge_rule(r);
+ }
+}
+
+void
 pf_purge_thread(void *v)
 {
  int nloops = 0, s;
@@ -1154,6 +1169,11 @@ pf_purge_thread(void *v)
  if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
  pf_purge_expired_fragments();
  pf_purge_expired_src_nodes(0);
+ if (!SLIST_EMPTY(&pf_rule_gcl)) {
+ rw_enter_write(&pf_consistency_lock);
+ pf_purge_expired_rules();
+ rw_exit_write(&pf_consistency_lock);
+ }
  nloops = 0;
  }
 
@@ -3135,6 +3155,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 +3457,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)
Index: pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.297
diff -u -p -r1.297 pf_ioctl.c
--- pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297
+++ pf_ioctl.c 16 Dec 2015 00:52:25 -0000
@@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu
 }
 
 void
-pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule,
-    struct pf_ruleset *aruleset, struct pf_rule *arule)
+pf_purge_rule(struct pf_rule *rule)
 {
  u_int32_t nr = 0;
+ struct pf_ruleset *ruleset;
 
- KASSERT(ruleset != NULL && rule != NULL);
+ KASSERT((rule != NULL) && (rule->ruleset != NULL));
+ ruleset = rule->ruleset;
 
  pf_rm_rule(ruleset->rules.active.ptr, rule);
  ruleset->rules.active.rcount--;
@@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset
  rule->nr = nr++;
  ruleset->rules.active.ticket++;
  pf_calc_skip_steps(ruleset->rules.active.ptr);
-
- /* remove the parent anchor rule */
- if (nr == 0 && arule && aruleset) {
- pf_rm_rule(aruleset->rules.active.ptr, arule);
- aruleset->rules.active.rcount--;
- TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries)
- rule->nr = nr++;
- aruleset->rules.active.ticket++;
- pf_calc_skip_steps(aruleset->rules.active.ptr);
- }
 }
 
 u_int16_t
@@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char *
  int s, error;
  u_int32_t old_rcount;
 
+ /* make sure any expired rules get removed from active rules first */
+ pf_purge_expired_rules();
+
  rs = pf_find_ruleset(anchor);
  if (rs == NULL || !rs->rules.inactive.open ||
     ticket != rs->rules.inactive.ticket)
@@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  }
  TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
     rule, entries);
+ rule->ruleset = ruleset;
  ruleset->rules.inactive.rcount++;
  break;
  }
Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.426
diff -u -p -r1.426 pfvar.h
--- pfvar.h 3 Dec 2015 14:05:28 -0000 1.426
+++ pfvar.h 16 Dec 2015 00:52:28 -0000
@@ -563,6 +563,9 @@ struct pf_rule {
  struct pf_addr addr;
  u_int16_t port;
  } divert, divert_packet;
+
+ SLIST_ENTRY(pf_rule) gcle;
+ struct pf_ruleset *ruleset;
 };
 
 /* rule flags */
@@ -581,6 +584,7 @@ struct pf_rule {
 #define PFRULE_PFLOW 0x00040000
 #define PFRULE_ONCE 0x00100000 /* one shot rule */
 #define PFRULE_AFTO 0x00200000 /* af-to rule */
+#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */
 
 #define PFSTATE_HIWAT 10000 /* default state table size */
 #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */
@@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl;
 extern void pf_purge_thread(void *);
 extern void pf_purge_expired_src_nodes(int);
 extern void pf_purge_expired_states(u_int32_t);
+extern void pf_purge_expired_rules(void);
 extern void pf_remove_state(struct pf_state *);
 extern void pf_remove_divert_state(struct pf_state_key *);
 extern void pf_free_state(struct pf_state *);
@@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr
     sa_family_t);
 void pf_rm_rule(struct pf_rulequeue *,
     struct pf_rule *);
-void pf_purge_rule(struct pf_ruleset *,
-    struct pf_rule *, struct pf_ruleset *,
-    struct pf_rule *);
+void pf_purge_rule(struct pf_rule *);
 struct pf_divert *pf_find_divert(struct mbuf *);
 int pf_setup_pdesc(struct pf_pdesc *, void *,
     sa_family_t, int, struct pfi_kif *,

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

richard.n.procter
In reply to this post by Mike Belopuhov-5

On Tue, 15 Dec 2015, Mike Belopuhov wrote:

> >    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!

I tend to agree! And I can't see a way to reimplement it for a
multithreaded pf without introducing downsides.

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.)

One approach serialises the candidacy of 'once' rules during rule
matching. This ensures an unambiguous first match but costs pf a latent
glass jaw. In the worst case, a 'once' rule heads the ruleset and matches
any packet. Its candidacy therefore covers the entire rule-matching
process and so fully serialises it.
 
The other approach serialises the packets matched by 'once' rules, after
rule processing is done. This doesn't create a glass jaw but does permit
the 'once' rule to match more than once.

To my taste neither alternative is especially palatable.

That said, a 'quick' 'once' rule has no candidacy window so it can be
serialised by atomically testing/setting a flag when evaluating it. This
also both avoids the glass jaw and provides at most one first match.

(Note that this issue of determining the first match is independent of
whether a 'once' rule, having matched, is purged.)

On Wed, 16 Dec 2015, Mike Belopuhov wrote:

> It just occurred to me that another possibility would be a match-only
> rule that matches one but doesn't involve any purging machinery.

That is an interesting idea. Like a 'quick' rule, a match rule has no
candidacy window. However a 'once' match rule can no longer influence
pass/block state --- I can't see how it is usefull?

best,
Richard.

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
Hello,


On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote:

>
> On Tue, 15 Dec 2015, Mike Belopuhov wrote:
>
> > >    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!
>
> I tend to agree! And I can't see a way to reimplement it for a
> multithreaded pf without introducing downsides.
>
> 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). What has to be granted is there is one 'winner'
only, which puts the rule to garbage collector list. The pragmatic approach
wins here.

regards
sasha

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Martin Pieuchot
On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote:

> On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote:
> >
> > On Tue, 15 Dec 2015, Mike Belopuhov wrote:
> >
> > > >    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!
> >
> > I tend to agree! And I can't see a way to reimplement it for a
> > multithreaded pf without introducing downsides.

Guys, if none of you can come with a valid reason to keep "once" rules
please kill them.

There's so much work to do to make pf(4) runnable on multiple CPUs in
parallel that bikescheding/turd-polishing bits that are not used are
IMHO not the way to go.

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
On Wed, Dec 16, 2015 at 12:35:41PM +0100, Martin Pieuchot wrote:

> On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote:
> > On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote:
> > >
> > > On Tue, 15 Dec 2015, Mike Belopuhov wrote:
> > >
> > > > >    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!
> > >
> > > I tend to agree! And I can't see a way to reimplement it for a
> > > multithreaded pf without introducing downsides.
>
> Guys, if none of you can come with a valid reason to keep "once" rules
> please kill them.
>
> There's so much work to do to make pf(4) runnable on multiple CPUs in
> parallel that bikescheding/turd-polishing bits that are not used are
> IMHO not the way to go.

The patch I've sent recently works just fine for Solaris. So ONCE rules are no
issue. I like the idea of once rules as they make life potentially easier for
application proxies.

regards
sasha

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Martin Pieuchot
On 16/12/15(Wed) 13:30, Alexandr Nedvedicky wrote:

> On Wed, Dec 16, 2015 at 12:35:41PM +0100, Martin Pieuchot wrote:
> > On 16/12/15(Wed) 10:19, Alexandr Nedvedicky wrote:
> > > On Wed, Dec 16, 2015 at 02:48:49PM +1300, Richard Procter wrote:
> > > >
> > > > On Tue, 15 Dec 2015, Mike Belopuhov wrote:
> > > >
> > > > > >    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!
> > > >
> > > > I tend to agree! And I can't see a way to reimplement it for a
> > > > multithreaded pf without introducing downsides.
> >
> > Guys, if none of you can come with a valid reason to keep "once" rules
> > please kill them.
> >
> > There's so much work to do to make pf(4) runnable on multiple CPUs in
> > parallel that bikescheding/turd-polishing bits that are not used are
> > IMHO not the way to go.
>
> The patch I've sent recently works just fine for Solaris. So ONCE rules are no
> issue. I like the idea of once rules as they make life potentially easier for
> application proxies.

If "potentially easier" is good enough for you then please move forward
:)

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Mike Belopuhov-5
In reply to this post by Alexandr Nedvedicky
On Wed, Dec 16, 2015 at 02:31 +0100, Alexandr Nedvedicky wrote:

> Hello,
>
> > It just occurred to me that another possibility would be a match-only
> > rule that matches one but doesn't involve any purging machinery.  Right
> > now we install ftp-proxy rules as having maximum number of states equal
> > to 1, however there's a time window between the moment the state is no
> > longer there, but anchor is not gone yet and it can potentially create
> > more states which is not a problem for an eavesdropper who can sniff
> > the plaintext protocol.
>
> I'm not sure I'm following you. IMO the expire flag should solve
> that problem. As soon as rule is marked as expired it can not be
> matched by packet any more.
>

Sure.  I'm just saying that we could change the logic and remove
the purging part, while keeping only the "match once".

> I'm attaching yet another version of patch. The list of changes to
> patch sent by Richard is as follows:
>
> - atomic operations are gone as we don't need them
>  right now. those will be introduced as soon as we will get
>  there. pf_test()/pf_test_rule() is still processing a single
>  packet at a time.
>
> - it's better to use TAILQ_EMPTY() instead of checking link
>  members to determine if last rule is being removed from
>  anchor `a`
>
> - I've also realized we have to call pf_purge_expired_rules()
>  from pf_commit_rules(). We have to make sure expired rules are gone
>  from active lists. I think this is the best approach for pf@Puffy
>  currently. It will be changed as soon as ruleset locks and
>  reference counting for rules will be merged in. I had to also
>  reshuffle a consistency lock a bit in pf_purge_thread().
>
> - last but not least: silly name got renamed (s/myruleset/ruleset)
>
> so what do you think? OK?
>

I think it's almost OK, some comments inline.

> regards
> sasha
>
> ---------8<----------------8<----------------8<---------------8<--------
> Index: pf.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.960
> diff -u -p -r1.960 pf.c
> --- pf.c 6 Dec 2015 10:03:23 -0000 1.960
> +++ pf.c 16 Dec 2015 00:52:22 -0000
> @@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key,
>  RB_GENERATE(pf_state_tree_id, pf_state,
>      entry_id, pf_state_compare_id);
>  
> +SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl =
> + SLIST_HEAD_INITIALIZER(pf_rule_gcl);
> +
>  __inline int
>  pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af)
>  {
> @@ -1137,6 +1140,18 @@ pf_state_export(struct pfsync_state *sp,
>  /* END state table stuff */
>  
>  void
> +pf_purge_expired_rules(void)
> +{
> + struct pf_rule *r;
> +
> + while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) {
> + SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle);
> + KASSERT(r->rule_flag & PFRULE_EXPIRED);
> + pf_purge_rule(r);
> + }
> +}
> +
> +void
>  pf_purge_thread(void *v)
>  {
>   int nloops = 0, s;
> @@ -1154,6 +1169,11 @@ pf_purge_thread(void *v)
>   if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
>   pf_purge_expired_fragments();
>   pf_purge_expired_src_nodes(0);
> + if (!SLIST_EMPTY(&pf_rule_gcl)) {
> + rw_enter_write(&pf_consistency_lock);
> + pf_purge_expired_rules();
> + rw_exit_write(&pf_consistency_lock);
> + }
>   nloops = 0;
>   }
>  

Please follow the example of pf_purge_expired_src_nodes and fold
all of this including the rwlocks inside pf_purge_expired_rules.

> @@ -3135,6 +3155,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 +3457,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);
> + }
>

I like the generalised approach to a/r purging.  I have a few
questions however:

Since you're not removing the rule from the ruleset at this point
what does "pfctl -sr" display in this case?  What happens if you
list the anchor?  And what should the user see?

Should we omit exporting those rules into userland via ioctl?

What happens with pfsync in this case?

>  #ifdef INET6
>   if (rewrite && skw->af != sks->af)
> Index: pf_ioctl.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.297
> diff -u -p -r1.297 pf_ioctl.c
> --- pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297
> +++ pf_ioctl.c 16 Dec 2015 00:52:25 -0000
> @@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu
>  }
>  
>  void
> -pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule,
> -    struct pf_ruleset *aruleset, struct pf_rule *arule)
> +pf_purge_rule(struct pf_rule *rule)
>  {
>   u_int32_t nr = 0;
> + struct pf_ruleset *ruleset;
>  
> - KASSERT(ruleset != NULL && rule != NULL);
> + KASSERT((rule != NULL) && (rule->ruleset != NULL));
> + ruleset = rule->ruleset;
>  
>   pf_rm_rule(ruleset->rules.active.ptr, rule);
>   ruleset->rules.active.rcount--;
> @@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset
>   rule->nr = nr++;
>   ruleset->rules.active.ticket++;
>   pf_calc_skip_steps(ruleset->rules.active.ptr);
> -
> - /* remove the parent anchor rule */
> - if (nr == 0 && arule && aruleset) {
> - pf_rm_rule(aruleset->rules.active.ptr, arule);
> - aruleset->rules.active.rcount--;
> - TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries)
> - rule->nr = nr++;
> - aruleset->rules.active.ticket++;
> - pf_calc_skip_steps(aruleset->rules.active.ptr);
> - }
>  }
>  
>  u_int16_t
> @@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char *
>   int s, error;
>   u_int32_t old_rcount;
>  
> + /* make sure any expired rules get removed from active rules first */
> + pf_purge_expired_rules();
> +

After folding rwlocks inside pf_purge_expired_rules make sure to
pass a flag to it to indicate that the rwlock is already taken.

>   rs = pf_find_ruleset(anchor);
>   if (rs == NULL || !rs->rules.inactive.open ||
>      ticket != rs->rules.inactive.ticket)
> @@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
>   }
>   TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
>      rule, entries);
> + rule->ruleset = ruleset;
>   ruleset->rules.inactive.rcount++;
>   break;
>   }
> Index: pfvar.h
> ===================================================================
> RCS file: /cvs/src/sys/net/pfvar.h,v
> retrieving revision 1.426
> diff -u -p -r1.426 pfvar.h
> --- pfvar.h 3 Dec 2015 14:05:28 -0000 1.426
> +++ pfvar.h 16 Dec 2015 00:52:28 -0000
> @@ -563,6 +563,9 @@ struct pf_rule {
>   struct pf_addr addr;
>   u_int16_t port;
>   } divert, divert_packet;
> +
> + SLIST_ENTRY(pf_rule) gcle;
> + struct pf_ruleset *ruleset;
>  };
>  
>  /* rule flags */
> @@ -581,6 +584,7 @@ struct pf_rule {
>  #define PFRULE_PFLOW 0x00040000
>  #define PFRULE_ONCE 0x00100000 /* one shot rule */
>  #define PFRULE_AFTO 0x00200000 /* af-to rule */
> +#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */
>  
>  #define PFSTATE_HIWAT 10000 /* default state table size */
>  #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */
> @@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl;
>  extern void pf_purge_thread(void *);
>  extern void pf_purge_expired_src_nodes(int);
>  extern void pf_purge_expired_states(u_int32_t);
> +extern void pf_purge_expired_rules(void);
>  extern void pf_remove_state(struct pf_state *);
>  extern void pf_remove_divert_state(struct pf_state_key *);
>  extern void pf_free_state(struct pf_state *);
> @@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr
>      sa_family_t);
>  void pf_rm_rule(struct pf_rulequeue *,
>      struct pf_rule *);
> -void pf_purge_rule(struct pf_ruleset *,
> -    struct pf_rule *, struct pf_ruleset *,
> -    struct pf_rule *);
> +void pf_purge_rule(struct pf_rule *);
>  struct pf_divert *pf_find_divert(struct mbuf *);
>  int pf_setup_pdesc(struct pf_pdesc *, void *,
>      sa_family_t, int, struct pfi_kif *,

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
hello,


> > > It just occurred to me that another possibility would be a match-only
> > > rule that matches one but doesn't involve any purging machinery.  Right
> > > now we install ftp-proxy rules as having maximum number of states equal
> > > to 1, however there's a time window between the moment the state is no
> > > longer there, but anchor is not gone yet and it can potentially create
> > > more states which is not a problem for an eavesdropper who can sniff
> > > the plaintext protocol.
> >
> > I'm not sure I'm following you. IMO the expire flag should solve
> > that problem. As soon as rule is marked as expired it can not be
> > matched by packet any more.
> >
>
> Sure.  I'm just saying that we could change the logic and remove
> the purging part, while keeping only the "match once".
>
that might be other possibility. However I would give a try to
garbage collection.

>
> Please follow the example of pf_purge_expired_src_nodes and fold
> all of this including the rwlocks inside pf_purge_expired_rules.
>

fixed

>
> I like the generalised approach to a/r purging.  I have a few
> questions however:
>
> Since you're not removing the rule from the ruleset at this point
> what does "pfctl -sr" display in this case?

new version of the patch makes pfctl -sr aware of PFRULE_EXPIRED
flag. The expired rule will be shown in verbose mode only.

> What happens if you
> list the anchor?  And what should the user see?
>
> Should we omit exporting those rules into userland via ioctl?

I think we should export those rules to userland and let userland
to interpret what it gets from kernel. Changing DIOCGETRULE might
not be that easy.

>
> What happens with pfsync in this case?

I keep forgetting about pfsync...

After looking at current pf_purge_rule() implementation in CVS it
seems to me it just does not care about pfsync at all.

The patch I'd like to commit keeps things same with respect to pfsync.

from my very limited understanding of pfsync, only main rulesets are synced up
between nodes. When pf_rules_commit() loads new rules to active list it calls
pf_setup_pfsync_matching(). the function calculates a checksum on ruleset.
It's still not clear to me how/when (if ever) are rules synced up.

thanks and
regards
sasha
 
--------8<---------------8<---------------8<------------------8<--------

Index: sys/net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.960
diff -u -p -r1.960 pf.c
--- sys/net/pf.c 6 Dec 2015 10:03:23 -0000 1.960
+++ sys/net/pf.c 16 Dec 2015 16:28:28 -0000
@@ -295,6 +295,9 @@ RB_GENERATE(pf_state_tree, pf_state_key,
 RB_GENERATE(pf_state_tree_id, pf_state,
     entry_id, pf_state_compare_id);
 
+SLIST_HEAD(pf_rule_gcl, pf_rule) pf_rule_gcl =
+ SLIST_HEAD_INITIALIZER(pf_rule_gcl);
+
 __inline int
 pf_addr_compare(struct pf_addr *a, struct pf_addr *b, sa_family_t af)
 {
@@ -1137,6 +1140,27 @@ pf_state_export(struct pfsync_state *sp,
 /* END state table stuff */
 
 void
+pf_purge_expired_rules(int locked)
+{
+ struct pf_rule *r;
+
+ if (SLIST_EMPTY(&pf_rule_gcl))
+ return;
+
+ if (!locked)
+ rw_enter_write(&pf_consistency_lock);
+
+ while ((r = SLIST_FIRST(&pf_rule_gcl)) != NULL) {
+ SLIST_REMOVE(&pf_rule_gcl, r, pf_rule, gcle);
+ KASSERT(r->rule_flag & PFRULE_EXPIRED);
+ pf_purge_rule(r);
+ }
+
+ if (!locked)
+ rw_exit_write(&pf_consistency_lock);
+}
+
+void
 pf_purge_thread(void *v)
 {
  int nloops = 0, s;
@@ -1154,6 +1178,7 @@ pf_purge_thread(void *v)
  if (++nloops >= pf_default_rule.timeout[PFTM_INTERVAL]) {
  pf_purge_expired_fragments();
  pf_purge_expired_src_nodes(0);
+ pf_purge_expired_rules(0);
  nloops = 0;
  }
 
@@ -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)
Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.297
diff -u -p -r1.297 pf_ioctl.c
--- sys/net/pf_ioctl.c 3 Dec 2015 13:30:18 -0000 1.297
+++ sys/net/pf_ioctl.c 16 Dec 2015 16:28:28 -0000
@@ -301,12 +301,13 @@ pf_rm_rule(struct pf_rulequeue *rulequeu
 }
 
 void
-pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule,
-    struct pf_ruleset *aruleset, struct pf_rule *arule)
+pf_purge_rule(struct pf_rule *rule)
 {
  u_int32_t nr = 0;
+ struct pf_ruleset *ruleset;
 
- KASSERT(ruleset != NULL && rule != NULL);
+ KASSERT((rule != NULL) && (rule->ruleset != NULL));
+ ruleset = rule->ruleset;
 
  pf_rm_rule(ruleset->rules.active.ptr, rule);
  ruleset->rules.active.rcount--;
@@ -314,16 +315,6 @@ pf_purge_rule(struct pf_ruleset *ruleset
  rule->nr = nr++;
  ruleset->rules.active.ticket++;
  pf_calc_skip_steps(ruleset->rules.active.ptr);
-
- /* remove the parent anchor rule */
- if (nr == 0 && arule && aruleset) {
- pf_rm_rule(aruleset->rules.active.ptr, arule);
- aruleset->rules.active.rcount--;
- TAILQ_FOREACH(rule, aruleset->rules.active.ptr, entries)
- rule->nr = nr++;
- aruleset->rules.active.ticket++;
- pf_calc_skip_steps(aruleset->rules.active.ptr);
- }
 }
 
 u_int16_t
@@ -775,6 +766,9 @@ pf_commit_rules(u_int32_t ticket, char *
  int s, error;
  u_int32_t old_rcount;
 
+ /* Make sure any expired rules get removed from active rules first. */
+ pf_purge_expired_rules(1);
+
  rs = pf_find_ruleset(anchor);
  if (rs == NULL || !rs->rules.inactive.open ||
     ticket != rs->rules.inactive.ticket)
@@ -1209,6 +1203,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
  }
  TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
     rule, entries);
+ rule->ruleset = ruleset;
  ruleset->rules.inactive.rcount++;
  break;
  }
Index: sys/net/pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.426
diff -u -p -r1.426 pfvar.h
--- sys/net/pfvar.h 3 Dec 2015 14:05:28 -0000 1.426
+++ sys/net/pfvar.h 16 Dec 2015 16:28:28 -0000
@@ -563,6 +563,9 @@ struct pf_rule {
  struct pf_addr addr;
  u_int16_t port;
  } divert, divert_packet;
+
+ SLIST_ENTRY(pf_rule) gcle;
+ struct pf_ruleset *ruleset;
 };
 
 /* rule flags */
@@ -581,6 +584,7 @@ struct pf_rule {
 #define PFRULE_PFLOW 0x00040000
 #define PFRULE_ONCE 0x00100000 /* one shot rule */
 #define PFRULE_AFTO 0x00200000 /* af-to rule */
+#define PFRULE_EXPIRED 0x00400000 /* one shot rule hit by pkt */
 
 #define PFSTATE_HIWAT 10000 /* default state table size */
 #define PFSTATE_ADAPT_START 6000 /* default adaptive timeout start */
@@ -1669,6 +1673,7 @@ extern struct pool pf_state_scrub_pl;
 extern void pf_purge_thread(void *);
 extern void pf_purge_expired_src_nodes(int);
 extern void pf_purge_expired_states(u_int32_t);
+extern void pf_purge_expired_rules(int);
 extern void pf_remove_state(struct pf_state *);
 extern void pf_remove_divert_state(struct pf_state_key *);
 extern void pf_free_state(struct pf_state *);
@@ -1701,9 +1706,7 @@ extern void pf_addrcpy(struct pf_addr
     sa_family_t);
 void pf_rm_rule(struct pf_rulequeue *,
     struct pf_rule *);
-void pf_purge_rule(struct pf_ruleset *,
-    struct pf_rule *, struct pf_ruleset *,
-    struct pf_rule *);
+void pf_purge_rule(struct pf_rule *);
 struct pf_divert *pf_find_divert(struct mbuf *);
 int pf_setup_pdesc(struct pf_pdesc *, void *,
     sa_family_t, int, struct pfi_kif *,
Index: sbin/pfctl/pfctl_parser.c
===================================================================
RCS file: /cvs/src/sbin/pfctl/pfctl_parser.c,v
retrieving revision 1.306
diff -u -p -r1.306 pfctl_parser.c
--- sbin/pfctl/pfctl_parser.c 3 Sep 2015 12:46:47 -0000 1.306
+++ sbin/pfctl/pfctl_parser.c 16 Dec 2015 16:28:28 -0000
@@ -700,8 +700,12 @@ print_rule(struct pf_rule *r, const char
  int verbose = opts & (PF_OPT_VERBOSE2 | PF_OPT_DEBUG);
  char *p;
 
+ if ((r->rule_flag & PFRULE_EXPIRED) && (!verbose))
+ return;
+
  if (verbose)
  printf("@%d ", r->nr);
+
  if (r->action > PF_MATCH)
  printf("action(%d)", r->action);
  else if (anchor_call[0]) {
@@ -1119,6 +1123,9 @@ print_rule(struct pf_rule *r, const char
  printf(" ");
  print_pool(&r->route, 0, 0, r->af, PF_POOL_ROUTE, verbose);
  }
+
+ if (r->rule_flag & PFRULE_EXPIRED)
+ printf("[ rule expired ]");
 }
 
 void

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

richard.n.procter
In reply to this post by Alexandr Nedvedicky


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)

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

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

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

richard.n.procter

Hi Sasha,

On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote:

> > Right. I'll just note though that the patch as it stands allows
> > multiple winners [...] 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.

Fair enough :) I presumed that an upcoming concurrent pf_test() would rely
on this patch.

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

I was wondering about that. For this patch presumably it's thought
unnecessary to mutex the SLIST ops (or for that matter to preserve rule
lifetime) for one-shot rules as the purge thread runs so infrequently.

BTW, the purge queue exists to pass a result between threads; an
alternative is to recompute it in the purge thread by searching for
expired rules: no need for the queue, its mutexes, etc. Easier to preserve
rule lifetime, too. May need another anchor stack though. I might have a
go next year when the code is more settled.

Ok, enough from me on this.

best,
Richard.

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

David Gwynne-5

> On 17 Dec 2015, at 13:30, Richard Procter <[hidden email]> wrote:
>
>
> Hi Sasha,
>
> On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote:
>
>>> Right. I'll just note though that the patch as it stands allows
>>> multiple winners [...] 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.
>
> Fair enough :) I presumed that an upcoming concurrent pf_test() would rely
> on this patch.
>
>> 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);
>> }
>> }
>
> I was wondering about that. For this patch presumably it's thought
> unnecessary to mutex the SLIST ops (or for that matter to preserve rule
> lifetime) for one-shot rules as the purge thread runs so infrequently.

right now pf doesnt run concurrently so extra locking is unnecessary. when it does run concurrently then it will need a mutex, despite how infrequently the gc runs.

>
> BTW, the purge queue exists to pass a result between threads; an
> alternative is to recompute it in the purge thread by searching for
> expired rules: no need for the queue, its mutexes, etc. Easier to preserve
> rule lifetime, too. May need another anchor stack though. I might have a
> go next year when the code is more settled.

i believe the purge thread exists because once upon a time pool_free from interrupt context wasnt the best. i might be misremembering that though.

currently that thread only looks at states. traversing the ruleset is possible i guess. if the semantic of the once rules is that they only match once, and if pf will run on multiple cpus, then coordinating between the cpus will require atomicity or serialisation of some sort.

dlg

>
> Ok, enough from me on this.
>
> best,
> Richard.
>

Reply | Threaded
Open this post in threaded view
|

Re: removing expired once rules in pf_purge_thread()

Alexandr Nedvedicky
Hello,

On Tue, Aug 30, 2016 at 10:53:56AM +1000, David Gwynne wrote:

>
> > On 17 Dec 2015, at 13:30, Richard Procter <[hidden email]> wrote:
> >
> >
> > Hi Sasha,
> >
> > On Fri, 18 Dec 2015, Alexandr Nedvedicky wrote:
> >
> >>> Right. I'll just note though that the patch as it stands allows
> >>> multiple winners [...] 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.
> >
> > Fair enough :) I presumed that an upcoming concurrent pf_test() would rely
> > on this patch.
> >
> >> 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);
> >> }
> >> }
> >
> > I was wondering about that. For this patch presumably it's thought
> > unnecessary to mutex the SLIST ops (or for that matter to preserve rule
> > lifetime) for one-shot rules as the purge thread runs so infrequently.
>
> right now pf doesnt run concurrently so extra locking is unnecessary. when it
> does run concurrently then it will need a mutex, despite how infrequently the
> gc runs.

    I see. I'll remove the mutex and resend the patch.

>
> >
> > BTW, the purge queue exists to pass a result between threads; an
> > alternative is to recompute it in the purge thread by searching for
> > expired rules: no need for the queue, its mutexes, etc. Easier to preserve
> > rule lifetime, too. May need another anchor stack though. I might have a
> > go next year when the code is more settled.
>
> i believe the purge thread exists because once upon a time pool_free from
> interrupt context wasnt the best. i might be misremembering that though.

    PF at Solaris still keeps purge thread around, although it's entirely
    possible to let packet free memory. PF on Solaris does not have to deal with
    such constrains.

>
> currently that thread only looks at states. traversing the ruleset is
> possible i guess.

    yes, ruleset traversal is possible, however gcl (garbage collector list) is
    meant as a shortcut. If packet hits ONCE rule it sets the flag to indicate
    rule has matched and inserts the rule to gcl.

    the purge thread then does not need to walk traverse all rulesets to collect
    'dead' once rules. Instead it just walks the gcl to find out, which ONCE
    rules are dead to pull them out.

    There is also yet another option:

        remove gcl completely and just mark the ONCE rule got hit.  we can
        leave dead ONCE rule to remain in ruleset until new rules will be
        loaded or system will be rebooted.

> if the semantic of the once rules is that they only match
> once, and if pf will run on multiple cpus, then coordinating between the cpus
> will require atomicity or serialisation of some sort.

    PF on Solaris is willing to pay price of not having ONCE rules 100% atomic,
    there is currently no atomic op, which would set the flag to indicate rule
    matched packet.


regards
sasha