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 *,
