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.
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?
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;
}
@@ -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);
+ }
#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();
+
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 *,
