On 08/30/16 02:32, Alexandr Nedvedicky wrote:
Hello,
mikeb has just pointed out the patch fell under the desk asking me to resend
it.
henning@ and mikeb@ showed some interest to change handling of once rules to
the same way as PF has it on Solaris. Just to refresh the audience on once
option offered by PF:
once Creates a one shot rule that will remove itself from an active
ruleset after the first match. In case this is the only rule in
the anchor, the anchor will be destroyed automatically after the
rule is matched.
-- pf.conf(5)
Currently the once rules are removed by matching packet. Patch makes life for
packets, which match once rules bit easier. Packets instead of removing rule
from ruleset just mark rule as expired and put it to garbage colloector list.
The list is processed by pf_purge_thread(), which just removes and deletes
those expired rules. To get there we need to simplify pf_purge_rule() image,
which currently looks as follows:
void
pf_purge_rule(struct pf_ruleset *ruleset, struct pf_rule *rule,
struct pf_ruleset *aruleset, struct pf_rule *arule)
- ruleset is the ruleset, where once rule is being removed from
- rule is a once rule to remove
- aruleset holds an anchor rule with once-rule we remove
- arule an anchor which holds a once rule
To make pf_purge_rule() suitable for pf_purge_thread() it has to be changed to:
void
pf_purge_rule(struct pf_rule *once_rule)
To get there the ruleset and arule has to be carried by once_rule itself.
Therefore patch adds those members to pf_rule:
struct pf_ruleset *myruleset
struct pf_rule *myarule
SLIST_ENTRY(pf_rule) gcle
(the gcle is garbage colleter list link).
Patch sets myruleset as soon as rule gets inserted to ruleset in SIOCADDRULE
ioctl. The myarule is set in pf_test_rule(), when once rule is marked as
expired.
Don't forget to recompile all user-land bits (pfctl, proxies et. al.) when
you'll be testing the patch, since pf_rule structure gets changed.
regards
sasha
updated version is below.
comments? O.K.?
One comment, otherwise ok.
--------8<---------------8<---------------8<------------------8<--------
diff -r ca96e396772c src/sbin/pfctl/pfctl_parser.c
--- a/src/sbin/pfctl/pfctl_parser.c Mon Aug 29 23:23:38 2016 +0200
+++ b/src/sbin/pfctl/pfctl_parser.c Tue Aug 30 00:41:03 2016 +0200
@@ -701,8 +701,12 @@
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]) {
@@ -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 ]");
}
void
diff -r ca96e396772c src/sys/net/pf.c
--- a/src/sys/net/pf.c Mon Aug 29 23:23:38 2016 +0200
+++ b/src/sys/net/pf.c Tue Aug 30 00:41:03 2016 +0200
@@ -311,6 +311,9 @@
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,27 @@
/* 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);
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);
+
+ 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 +1215,7 @@
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 +3516,10 @@
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 +3825,15 @@
}
#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);
+ }
return (action);
diff -r ca96e396772c src/sys/net/pf_ioctl.c
--- a/src/sys/net/pf_ioctl.c Mon Aug 29 23:23:38 2016 +0200
+++ b/src/sys/net/pf_ioctl.c Tue Aug 30 00:41:03 2016 +0200
@@ -301,12 +301,13 @@
}
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 @@
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 @@
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 @@
}
TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
rule, entries);
+ rule->ruleset = ruleset;
ruleset->rules.inactive.rcount++;
break;
}
diff -r ca96e396772c src/sys/net/pfvar.h
--- a/src/sys/net/pfvar.h Mon Aug 29 23:23:38 2016 +0200
+++ b/src/sys/net/pfvar.h Tue Aug 30 00:41:03 2016 +0200
@@ -571,6 +571,9 @@
struct pf_addr addr;
u_int16_t port;
} divert, divert_packet;
+
+ SLIST_ENTRY(pf_rule) gcle;
+ struct pf_ruleset *ruleset;
};
/* rule flags */
@@ -589,6 +592,7 @@
#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 +1670,7 @@
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 +1700,7 @@
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 *,
