Hello,
there was still one more glitch catched by mikeb:
I have to sanitize pointer when copying rule to userland.
The other thing pointed out by mike is the Expired time should
be printed for expired rules only in debug mode output (pfctl -sr -g)
Incremental patch is as follows:
--------8<---------------8<---------------8<------------------8<--------
diff -r 7a93d568ede3 -r b366ba821dfb src/sbin/pfctl/pfctl.c
--- a/src/sbin/pfctl/pfctl.c Sat Sep 03 15:06:16 2016 +0200
+++ b/src/sbin/pfctl/pfctl.c Sat Sep 03 16:20:24 2016 +0200
@@ -702,14 +702,9 @@ 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 (rule->rule_flag & PFRULE_EXPIRED)
+ printf(" [ Expired: %lld secs ago ]\n",
+ (long long)(time(NULL) - rule->exptime));
}
if (opts & PF_OPT_VERBOSE) {
printf(" [ Evaluations: %-8llu Packets: %-8llu "
diff -r 7a93d568ede3 -r b366ba821dfb src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.c Sat Sep 03 15:06:16 2016 +0200
+++ b/src/sys/net/pf_ioctl.c Sat Sep 03 16:20:24 2016 +0200
@@ -1268,6 +1268,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
pr->rule.rcv_kif = NULL;
pr->rule.anchor = NULL;
pr->rule.overload_tbl = NULL;
+ bzero(&pr->rule.gcle, sizeof(pr->rule.gcle));
+ pr->rule.ruleset = NULL;
if (pf_anchor_copyout(ruleset, rule, pr)) {
error = EBUSY;
break;
--------8<---------------8<---------------8<------------------8<--------
complete patch I'd like to commit is further below.
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 16:26:08 2016 +0200
@@ -701,6 +701,10 @@ 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_EXPIRED)
+ printf(" [ Expired: %lld secs ago ]\n",
+ (long long)(time(NULL) - rule->exptime));
}
if (opts & PF_OPT_VERBOSE) {
printf(" [ Evaluations: %-8llu Packets: %-8llu "
@@ -848,7 +852,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 16:26:08 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 16:26:08 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 16:26:08 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;
}
@@ -1273,6 +1268,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
pr->rule.rcv_kif = NULL;
pr->rule.anchor = NULL;
pr->rule.overload_tbl = NULL;
+ bzero(&pr->rule.gcle, sizeof(pr->rule.gcle));
+ pr->rule.ruleset = NULL;
if (pf_anchor_copyout(ruleset, rule, pr)) {
error = EBUSY;
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 16:26:08 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 *,
