Hello,
</snip>
> >updated version is below.
> >
> >comments? O.K.?
>
> One comment, otherwise ok.
>
</snip>
> Could you assert the lock is held otherwise, this might save
> effort if/when this code is refactored:
>
> else
> rw_assert_wrlock(&pf_consistency_lock);
sure.
also mikeb came to me with one more suggestion. mikeb does not
like the change to print_rule():
@@ -1120,6 +1124,9 @@
printf(" ");
print_pool(&r->route, 0, 0, r->af, PF_POOL_ROUTE, verbose);
}
+
+ if (r->rule_flag & PFRULE_EXPIRED)
+ printf("[ rule expired ]");
}
he suggests to move this bit to pfctl_print_rules_counters().
Mike also proposed to record time in rule when it got expired
and print it along the rules counters (pfctl -sr -g).
I've also noticed pfctl -sr might print empty lines for expired
rules, hence there is a new check at pfctl_show_rules():
@@ -848,7 +857,13 @@ pfctl_show_rules(int dev, char *path, in
INDENT(depth, !(opts & PF_OPT_VERBOSE));
printf("}\n");
} else {
- printf("\n");
+ /*
+ * Do not print newline, when we have not
+ * printed expired rule.
+ */
+ if (!(pr.rule.rule_flag & PFRULE_EXPIRED) ||
+ (opts & (PF_OPT_VERBOSE2|PF_OPT_DEBUG)))
+ printf("\n");
pfctl_print_rule_counters(&pr.rule, opts);
}
is that OK?
thanks a lot
regards
sasha
--------8<---------------8<---------------8<------------------8<--------
diff -r 8006a1eca673 src/sbin/pfctl/pfctl.c
--- a/src/sbin/pfctl/pfctl.c Sat Sep 03 14:19:17 2016 +0200
+++ b/src/sbin/pfctl/pfctl.c Sat Sep 03 15:07:21 2016 +0200
@@ -701,6 +701,15 @@ pfctl_print_rule_counters(struct pf_rule
printf(" [ queue: qname=%s qid=%u pqname=%s pqid=%u ]\n",
rule->qname, rule->qid, rule->pqname, rule->pqid);
+
+ if (rule->rule_flag & PFRULE_ONCE)
+ if (rule->rule_flag & PFRULE_EXPIRED)
+ printf(" [ Expired: %lld secs ago ]\n",
+ (long long)(time(NULL) - rule->exptime));
+ else
+ printf(" [ Expired: not yet ]\n");
+ else
+ printf(" [ Expired: never ]\n");
}
if (opts & PF_OPT_VERBOSE) {
printf(" [ Evaluations: %-8llu Packets: %-8llu "
@@ -848,7 +857,13 @@ pfctl_show_rules(int dev, char *path, in
INDENT(depth, !(opts & PF_OPT_VERBOSE));
printf("}\n");
} else {
- printf("\n");
+ /*
+ * Do not print newline, when we have not
+ * printed expired rule.
+ */
+ if (!(pr.rule.rule_flag & PFRULE_EXPIRED) ||
+ (opts & (PF_OPT_VERBOSE2|PF_OPT_DEBUG)))
+ printf("\n");
pfctl_print_rule_counters(&pr.rule, opts);
}
break;
diff -r 8006a1eca673 src/sbin/pfctl/pfctl_parser.c
--- a/src/sbin/pfctl/pfctl_parser.c Sat Sep 03 14:19:17 2016 +0200
+++ b/src/sbin/pfctl/pfctl_parser.c Sat Sep 03 15:07:21 2016 +0200
@@ -701,8 +701,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]) {
diff -r 8006a1eca673 src/sys/net/pf.c
--- a/src/sys/net/pf.c Sat Sep 03 14:19:17 2016 +0200
+++ b/src/sys/net/pf.c Sat Sep 03 15:07:21 2016 +0200
@@ -311,6 +311,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)
{
@@ -1174,6 +1177,29 @@ 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);
+ else
+ rw_assert_wrlock(&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;
@@ -1191,6 +1217,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;
}
@@ -3491,6 +3518,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);
@@ -3796,8 +3827,17 @@ 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;
+ a->exptime = time_second;
+ SLIST_INSERT_HEAD(&pf_rule_gcl, a, gcle);
+ }
+
+ r->rule_flag |= PFRULE_EXPIRED;
+ r->exptime = time_second;
+ SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
+ }
return (action);
diff -r 8006a1eca673 src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.c Sat Sep 03 14:19:17 2016 +0200
+++ b/src/sys/net/pf_ioctl.c Sat Sep 03 15:07:21 2016 +0200
@@ -309,12 +309,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--;
@@ -322,16 +323,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
@@ -783,6 +774,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)
@@ -1217,6 +1211,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;
}
diff -r 8006a1eca673 src/sys/net/pfvar.h
--- a/src/sys/net/pfvar.h Sat Sep 03 14:19:17 2016 +0200
+++ b/src/sys/net/pfvar.h Sat Sep 03 15:07:21 2016 +0200
@@ -571,6 +571,10 @@ struct pf_rule {
struct pf_addr addr;
u_int16_t port;
} divert, divert_packet;
+
+ SLIST_ENTRY(pf_rule) gcle;
+ struct pf_ruleset *ruleset;
+ time_t exptime;
};
/* rule flags */
@@ -589,6 +593,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 */
@@ -1666,6 +1671,7 @@ extern void
pf_calc_skip_steps(struct
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 *);
@@ -1695,9 +1701,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 *,
