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
