pf: once for match rules?

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

pf: once for match rules?

Mike Belopuhov-5
Hi,

Before I send a diff for pfctl to disable "once" on "match" rules,
I've decided to try and see how much work is it to make it actually
work.  Turns out that I need to extend pf_rule_item by 3 pointers
to track the match rule ruleset, anchor rule and the ruleset it
belongs to.

Here's what this means in practice.  Consider a ruleset:

 block drop all
 match out log proto tcp to port 22 once
 anchor "foo" all {
   match out log proto tcp to port 22 once
   anchor "bar" all {
     match out log proto tcp to port 22 once
     pass out quick proto tcp to port 22 once
   }
 }

Once we send a packet to port 22 the ruleset collapses to just:

 block drop all

Thoughts?

diff --git sys/net/pf.c sys/net/pf.c
index 9f0e2d6..5679a40 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -3279,15 +3279,16 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
     PR_NOWAIT)) == NULL) {
  REASON_SET(&reason, PFRES_MEMORY);
  goto cleanup;
  }
  ri->r = r;
+ ri->ar = a;
+ ri->rs = ruleset;
+ ri->ars = aruleset;
  /* order is irrelevant */
  SLIST_INSERT_HEAD(&rules, ri, entry);
  pf_rule_to_actions(r, &act);
- if (r->rule_flag & PFRULE_AFTO)
- pd->naf = r->naf;
  if (pf_get_transaddr(r, pd, sns, &nr) == -1) {
  REASON_SET(&reason, PFRES_TRANSLATE);
  goto cleanup;
  }
  if (r->log || act.log & PF_LOG_MATCHES) {
@@ -3428,10 +3429,12 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
     virtual_type, icmp_dir);
  }
  } else {
  while ((ri = SLIST_FIRST(&rules))) {
  SLIST_REMOVE_HEAD(&rules, entry);
+ if (ri->r->rule_flag & PFRULE_ONCE)
+ pf_purge_rule(ri->rs, ri->r, ri->ars, ri->ar);
  pool_put(&pf_rule_item_pl, ri);
  }
  }
 
  /* copy back packet headers if needed */
@@ -3454,10 +3457,23 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
  }
 #endif
 
  if (r->rule_flag & PFRULE_ONCE)
  pf_purge_rule(ruleset, r, aruleset, a);
+ if (*sm) {
+ SLIST_FOREACH(ri, &(*sm)->match_rules, entry) {
+ if (ri->r->rule_flag & PFRULE_ONCE)
+ /*
+ * We can be sure that pf_purge_rule won't
+ * pool_put the rule because when *sm != NULL
+ * STATE_INC_COUNTERS has increased states_cur.
+ * pf_rule_item's and rules will be g/c'ed by
+ * pf_free_state.
+ */
+ pf_purge_rule(ri->rs, ri->r, ri->ars, ri->ar);
+ }
+ }
 
 #if INET && INET6
  if (rewrite && skw->af != sks->af)
  return (PF_AFRT);
 #endif /* INET && INET6 */
diff --git sys/net/pfvar.h sys/net/pfvar.h
index a0d94f7..49af7b4 100644
--- sys/net/pfvar.h
+++ sys/net/pfvar.h
@@ -691,10 +691,13 @@ struct pf_threshold {
 };
 
 struct pf_rule_item {
  SLIST_ENTRY(pf_rule_item) entry;
  struct pf_rule *r;
+ struct pf_rule *ar;
+ struct pf_ruleset *rs;
+ struct pf_ruleset *ars;
 };
 
 SLIST_HEAD(pf_rule_slist, pf_rule_item);
 
 enum pf_sn_types { PF_SN_NONE, PF_SN_NAT, PF_SN_RDR, PF_SN_ROUTE, PF_SN_MAX };

Reply | Threaded
Open this post in threaded view
|

Re: pf: once for match rules?

Mike Belopuhov-5
On Tue, Jul 22, 2014 at 19:03 +0200, Mike Belopuhov wrote:

> Hi,
>
> Before I send a diff for pfctl to disable "once" on "match" rules,
> I've decided to try and see how much work is it to make it actually
> work.  Turns out that I need to extend pf_rule_item by 3 pointers
> to track the match rule ruleset, anchor rule and the ruleset it
> belongs to.
>
> Here's what this means in practice.  Consider a ruleset:
>
>  block drop all
>  match out log proto tcp to port 22 once
>  anchor "foo" all {
>    match out log proto tcp to port 22 once
>    anchor "bar" all {
>      match out log proto tcp to port 22 once
>      pass out quick proto tcp to port 22 once
>    }
>  }
>
> Once we send a packet to port 22 the ruleset collapses to just:
>
>  block drop all
>
> Thoughts?

Henning thinks it's a bit of an overkill.  Any other opinions?

>
> diff --git sys/net/pf.c sys/net/pf.c
> index 9f0e2d6..5679a40 100644
> --- sys/net/pf.c
> +++ sys/net/pf.c
> @@ -3279,15 +3279,16 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
>      PR_NOWAIT)) == NULL) {
>   REASON_SET(&reason, PFRES_MEMORY);
>   goto cleanup;
>   }
>   ri->r = r;
> + ri->ar = a;
> + ri->rs = ruleset;
> + ri->ars = aruleset;
>   /* order is irrelevant */
>   SLIST_INSERT_HEAD(&rules, ri, entry);
>   pf_rule_to_actions(r, &act);
> - if (r->rule_flag & PFRULE_AFTO)
> - pd->naf = r->naf;
>   if (pf_get_transaddr(r, pd, sns, &nr) == -1) {
>   REASON_SET(&reason, PFRES_TRANSLATE);
>   goto cleanup;
>   }
>   if (r->log || act.log & PF_LOG_MATCHES) {
> @@ -3428,10 +3429,12 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
>      virtual_type, icmp_dir);
>   }
>   } else {
>   while ((ri = SLIST_FIRST(&rules))) {
>   SLIST_REMOVE_HEAD(&rules, entry);
> + if (ri->r->rule_flag & PFRULE_ONCE)
> + pf_purge_rule(ri->rs, ri->r, ri->ars, ri->ar);
>   pool_put(&pf_rule_item_pl, ri);
>   }
>   }
>  
>   /* copy back packet headers if needed */
> @@ -3454,10 +3457,23 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm, struct pf_state **sm,
>   }
>  #endif
>  
>   if (r->rule_flag & PFRULE_ONCE)
>   pf_purge_rule(ruleset, r, aruleset, a);
> + if (*sm) {
> + SLIST_FOREACH(ri, &(*sm)->match_rules, entry) {
> + if (ri->r->rule_flag & PFRULE_ONCE)
> + /*
> + * We can be sure that pf_purge_rule won't
> + * pool_put the rule because when *sm != NULL
> + * STATE_INC_COUNTERS has increased states_cur.
> + * pf_rule_item's and rules will be g/c'ed by
> + * pf_free_state.
> + */
> + pf_purge_rule(ri->rs, ri->r, ri->ars, ri->ar);
> + }
> + }
>  
>  #if INET && INET6
>   if (rewrite && skw->af != sks->af)
>   return (PF_AFRT);
>  #endif /* INET && INET6 */
> diff --git sys/net/pfvar.h sys/net/pfvar.h
> index a0d94f7..49af7b4 100644
> --- sys/net/pfvar.h
> +++ sys/net/pfvar.h
> @@ -691,10 +691,13 @@ struct pf_threshold {
>  };
>  
>  struct pf_rule_item {
>   SLIST_ENTRY(pf_rule_item) entry;
>   struct pf_rule *r;
> + struct pf_rule *ar;
> + struct pf_ruleset *rs;
> + struct pf_ruleset *ars;
>  };
>  
>  SLIST_HEAD(pf_rule_slist, pf_rule_item);
>  
>  enum pf_sn_types { PF_SN_NONE, PF_SN_NAT, PF_SN_RDR, PF_SN_ROUTE, PF_SN_MAX };

Reply | Threaded
Open this post in threaded view
|

Re: pf: once for match rules?

Mike Belopuhov-5
On Tue, Aug 12, 2014 at 18:26 +0200, Mike Belopuhov wrote:

> On Tue, Jul 22, 2014 at 19:03 +0200, Mike Belopuhov wrote:
> > Hi,
> >
> > Before I send a diff for pfctl to disable "once" on "match" rules,
> > I've decided to try and see how much work is it to make it actually
> > work.  Turns out that I need to extend pf_rule_item by 3 pointers
> > to track the match rule ruleset, anchor rule and the ruleset it
> > belongs to.
> >
> > Here's what this means in practice.  Consider a ruleset:
> >
> >  block drop all
> >  match out log proto tcp to port 22 once
> >  anchor "foo" all {
> >    match out log proto tcp to port 22 once
> >    anchor "bar" all {
> >      match out log proto tcp to port 22 once
> >      pass out quick proto tcp to port 22 once
> >    }
> >  }
> >
> > Once we send a packet to port 22 the ruleset collapses to just:
> >
> >  block drop all
> >
> > Thoughts?
>
> Henning thinks it's a bit of an overkill.  Any other opinions?
>

here we go then.  OK?

diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y
index c277b8d..61c2646 100644
--- sbin/pfctl/parse.y
+++ sbin/pfctl/parse.y
@@ -1488,12 +1488,18 @@ pfrule : action dir logquick interface af proto fromto
  if ($8.marker & FOM_SETPRIO) {
  r.set_prio[0] = $8.set_prio[0];
  r.set_prio[1] = $8.set_prio[1];
  r.scrub_flags |= PFSTATE_SETPRIO;
  }
- if ($8.marker & FOM_ONCE)
+ if ($8.marker & FOM_ONCE) {
+ if (r.action == PF_MATCH) {
+ yyerror("can't specify once for "
+    "match rules");
+ YYERROR;
+ }
  r.rule_flag |= PFRULE_ONCE;
+ }
  if ($8.marker & FOM_AFTO)
  r.rule_flag |= PFRULE_AFTO;
  r.af = $5;
 
  if ($8.tag)