Hello,
below is a complete diff which makes DIOCGETRULE bit more sane.
This is the complete change I'd like to commit. It is also more
convenient for people who want to test the diff.
when running 'pfctl -sr' to show rules pfctl performs several
ioctl(2) calls. The first call is DIOCGETRULES to find out
the number of rules in ruleset. Then it performs 'n' DIOCGETRULE
commands to retrieve all rules from ruleset one-by-one. Let's
look at DIOCGETRULE code to see how bad things are for extremely
large rulesets:
1489 if (pr->ticket != ruleset->rules.active.ticket) {
1490 error = EBUSY;
1491 PF_UNLOCK();
1492 NET_UNLOCK();
1493 goto fail;
1494 }
1495 rule = TAILQ_FIRST(ruleset->rules.active.ptr);
1496 while ((rule != NULL) && (rule->nr != pr->nr))
1497 rule = TAILQ_NEXT(rule, entries);
1498 if (rule == NULL) {
1499 error = EBUSY;
1500 PF_UNLOCK();
1501 NET_UNLOCK();
1502 goto fail;
1503 }
to retrieve n-th rule DIOCGETRULE ioctl iterates over a ruleset
from beginning. This is OK for conventional rulesets. For large
rulesets the delay to retrieve all rules becomes noticable.
to measure it I've decided to create extreme ruleset with 26200 rules.
It took ~10 seconds on current:
current:
pf# time pfctl -sr |wc -l
26200
0m10.80s real 0m00.38s user 0m10.44s system
when I apply diff below I'm getting significantly better result:
fixed:
pf# time pfctl.test -sr |wc -l
26200
0m00.49s real 0m00.15s user 0m00.31s system
The proposed fix introduces transaction (pf_trans) to pf(4)
ioctl(2) subsystem. I use transactions in my tree to update
pf(4) configuration, but this part is not ready for review yet.
Process which opens /dev/pf may create one ore more transactions.
Transaction is either closed when process is done with change
or when closes /dev/pf. This should work, because /dev/pf
is cleanable since last November [1].
DIOCGETRULE operation uses transaction to keep a reference
to ruleset, ruleset version and pointer to next rule to retrieve,
so we no longer need to iterate from beginning to retrieve n-th rule.
when 'pfctl -sr' retrieves the ruleset it first issues DIOCGETRULES
ioctl(2). It receives ticket (transaction id) which must be passed
as parameter to subsequent DIOCGETRULE requests. It continues
to read rules one-by-one using DIOCGETRULE until ENOENT is seen
which indicates all rules got retrieved.
diff below is complete change. I'm going to send partial
diffs for review asking for OKs.
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-cvs&m=166773945126422&w=2
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 8cbd9d77b4f..3787e97a6b1 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -837,7 +837,7 @@ pfctl_show_rules(int dev, char *path, int opts, enum
pfctl_show format,
char *anchorname, int depth, int wildcard, long shownr)
{
struct pfioc_rule pr;
- u_int32_t nr, mnr, header = 0;
+ u_int32_t header = 0;
int len = strlen(path), ret = 0;
char *npath, *p;
@@ -893,24 +893,9 @@ pfctl_show_rules(int dev, char *path, int opts, enum
pfctl_show format,
goto error;
}
- if (shownr < 0) {
- mnr = pr.nr;
- nr = 0;
- } else if (shownr < pr.nr) {
- nr = shownr;
- mnr = shownr + 1;
- } else {
- warnx("rule %ld not found", shownr);
- ret = -1;
- goto error;
- }
- for (; nr < mnr; ++nr) {
- pr.nr = nr;
- if (ioctl(dev, DIOCGETRULE, &pr) == -1) {
- warn("DIOCGETRULE");
- ret = -1;
- goto error;
- }
+ while (ioctl(dev, DIOCGETRULE, &pr) != -1) {
+ if ((shownr != -1) && (shownr != pr.nr))
+ continue;
/* anchor is the same for all rules in it */
if (pr.rule.anchor_wildcard == 0)
@@ -968,6 +953,13 @@ pfctl_show_rules(int dev, char *path, int opts, enum
pfctl_show format,
case PFCTL_SHOW_NOTHING:
break;
}
+ errno = 0;
+ }
+
+ if ((errno != 0) && (errno != ENOENT)) {
+ warn("DIOCGETRULE");
+ ret = -1;
+ goto error;
}
/*
diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index 1141069dcf6..50286dbcb96 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -117,6 +117,15 @@ void pf_qid_unref(u_int16_t);
int pf_states_clr(struct pfioc_state_kill *);
int pf_states_get(struct pfioc_states *);
+struct pf_trans *pf_open_trans(pid_t);
+struct pf_trans *pf_find_trans(uint64_t);
+void pf_free_trans(struct pf_trans *);
+void pf_rollback_trans(struct pf_trans *);
+
+void pf_init_tgetrule(struct pf_trans *,
+ struct pf_anchor *, uint32_t, struct pf_rule *);
+void pf_cleanup_tgetrule(struct pf_trans *t);
+
struct pf_rule pf_default_rule, pf_default_rule_new;
struct {
@@ -168,6 +177,8 @@ int pf_rtlabel_add(struct pf_addr_wrap *);
void pf_rtlabel_remove(struct pf_addr_wrap *);
void pf_rtlabel_copyout(struct pf_addr_wrap *);
+uint64_t trans_ticket = 1;
+LIST_HEAD(, pf_trans) pf_ioctl_trans = LIST_HEAD_INITIALIZER(pf_trans);
void
pfattach(int num)
@@ -293,6 +304,29 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p)
int
pfclose(dev_t dev, int flags, int fmt, struct proc *p)
{
+ struct pf_trans *w, *s;
+ LIST_HEAD(, pf_trans) tmp_list;
+
+ if (minor(dev) >= 1)
+ return (ENXIO);
+
+ if (flags & FWRITE) {
+ LIST_INIT(&tmp_list);
+ rw_enter_write(&pfioctl_rw);
+ LIST_FOREACH_SAFE(w, &pf_ioctl_trans, pft_entry, s) {
+ if (w->pft_pid == p->p_p->ps_pid) {
+ LIST_REMOVE(w, pft_entry);
+ LIST_INSERT_HEAD(&tmp_list, w, pft_entry);
+ }
+ }
+ rw_exit_write(&pfioctl_rw);
+
+ while ((w = LIST_FIRST(&tmp_list)) != NULL) {
+ LIST_REMOVE(w, pft_entry);
+ pf_free_trans(w);
+ }
+ }
+
return (0);
}
@@ -522,7 +556,7 @@ pf_qid_unref(u_int16_t qid)
}
int
-pf_begin_rules(u_int32_t *ticket, const char *anchor)
+pf_begin_rules(u_int32_t *version, const char *anchor)
{
struct pf_ruleset *rs;
struct pf_rule *rule;
@@ -533,20 +567,20 @@ pf_begin_rules(u_int32_t *ticket, const char *anchor)
pf_rm_rule(rs->rules.inactive.ptr, rule);
rs->rules.inactive.rcount--;
}
- *ticket = ++rs->rules.inactive.ticket;
+ *version = ++rs->rules.inactive.version;
rs->rules.inactive.open = 1;
return (0);
}
void
-pf_rollback_rules(u_int32_t ticket, char *anchor)
+pf_rollback_rules(u_int32_t version, char *anchor)
{
struct pf_ruleset *rs;
struct pf_rule *rule;
rs = pf_find_ruleset(anchor);
if (rs == NULL || !rs->rules.inactive.open ||
- rs->rules.inactive.ticket != ticket)
+ rs->rules.inactive.version != version)
return;
while ((rule = TAILQ_FIRST(rs->rules.inactive.ptr)) != NULL) {
pf_rm_rule(rs->rules.inactive.ptr, rule);
@@ -825,7 +859,7 @@ pf_hash_rule(MD5_CTX *ctx, struct pf_rule *rule)
}
int
-pf_commit_rules(u_int32_t ticket, char *anchor)
+pf_commit_rules(u_int32_t version, char *anchor)
{
struct pf_ruleset *rs;
struct pf_rule *rule;
@@ -834,7 +868,7 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
rs = pf_find_ruleset(anchor);
if (rs == NULL || !rs->rules.inactive.open ||
- ticket != rs->rules.inactive.ticket)
+ version != rs->rules.inactive.version)
return (EBUSY);
if (rs == &pf_main_ruleset)
@@ -849,7 +883,7 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
rs->rules.inactive.ptr = old_rules;
rs->rules.inactive.rcount = old_rcount;
- rs->rules.active.ticket = rs->rules.inactive.ticket;
+ rs->rules.active.version = rs->rules.inactive.version;
pf_calc_skip_steps(rs->rules.active.ptr);
@@ -1142,10 +1176,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
return (EACCES);
}
- if (flags & FWRITE)
- rw_enter_write(&pfioctl_rw);
- else
- rw_enter_read(&pfioctl_rw);
+ rw_enter_write(&pfioctl_rw);
switch (cmd) {
@@ -1191,7 +1222,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
NET_LOCK();
PF_LOCK();
- pq->ticket = pf_main_ruleset.rules.active.ticket;
+ pq->ticket = pf_main_ruleset.rules.active.version;
/* save state to not run over them all each time? */
qs = TAILQ_FIRST(pf_queues_active);
@@ -1212,7 +1243,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
NET_LOCK();
PF_LOCK();
- if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
+ if (pq->ticket != pf_main_ruleset.rules.active.version) {
error = EBUSY;
PF_UNLOCK();
NET_UNLOCK();
@@ -1243,7 +1274,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
NET_LOCK();
PF_LOCK();
- if (pq->ticket != pf_main_ruleset.rules.active.ticket) {
+ if (pq->ticket != pf_main_ruleset.rules.active.version) {
error = EBUSY;
PF_UNLOCK();
NET_UNLOCK();
@@ -1290,7 +1321,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
NET_LOCK();
PF_LOCK();
- if (q->ticket != pf_main_ruleset.rules.inactive.ticket) {
+ if (q->ticket != pf_main_ruleset.rules.inactive.version) {
error = EBUSY;
PF_UNLOCK();
NET_UNLOCK();
@@ -1386,7 +1417,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
pf_rule_free(rule);
goto fail;
}
- if (pr->ticket != ruleset->rules.inactive.ticket) {
+ if (pr->ticket != ruleset->rules.inactive.version) {
error = EBUSY;
PF_UNLOCK();
NET_UNLOCK();
@@ -1447,7 +1478,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
case DIOCGETRULES: {
struct pfioc_rule *pr = (struct pfioc_rule *)addr;
struct pf_ruleset *ruleset;
- struct pf_rule *tail;
+ struct pf_rule *rule;
+ struct pf_trans *t;
+ u_int32_t ruleset_version;
NET_LOCK();
PF_LOCK();
@@ -1459,14 +1492,21 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
NET_UNLOCK();
goto fail;
}
- tail = TAILQ_LAST(ruleset->rules.active.ptr, pf_rulequeue);
- if (tail)
- pr->nr = tail->nr + 1;
+ rule = TAILQ_LAST(ruleset->rules.active.ptr, pf_rulequeue);
+ if (rule)
+ pr->nr = rule->nr + 1;
else
pr->nr = 0;
- pr->ticket = ruleset->rules.active.ticket;
+ ruleset_version = ruleset->rules.active.version;
+ pf_anchor_take(ruleset->anchor);
+ rule = TAILQ_FIRST(ruleset->rules.active.ptr);
PF_UNLOCK();
NET_UNLOCK();
+
+ t = pf_open_trans(p->p_p->ps_pid);
+ pf_init_tgetrule(t, ruleset->anchor, ruleset_version, rule);
+ pr->ticket = t->pft_ticket;
+
break;
}
@@ -1474,29 +1514,29 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
struct pfioc_rule *pr = (struct pfioc_rule *)addr;
struct pf_ruleset *ruleset;
struct pf_rule *rule;
+ struct pf_trans *t;
int i;
+ t = pf_find_trans(pr->ticket);
+ if (t == NULL)
+ return (EINVAL);
+ KASSERT(t->pft_pid == p->p_p->ps_pid);
+ if (t->pft_type != PF_TRANS_GETRULE)
+ return (EINVAL);
+
NET_LOCK();
PF_LOCK();
- pr->anchor[sizeof(pr->anchor) - 1] = '\0';
- ruleset = pf_find_ruleset(pr->anchor);
- if (ruleset == NULL) {
- error = EINVAL;
- PF_UNLOCK();
- NET_UNLOCK();
- goto fail;
- }
- if (pr->ticket != ruleset->rules.active.ticket) {
+ KASSERT(t->pftgr_anchor != NULL);
+ ruleset = &t->pftgr_anchor->ruleset;
+ if (t->pftgr_version != ruleset->rules.active.version) {
error = EBUSY;
PF_UNLOCK();
NET_UNLOCK();
goto fail;
}
- rule = TAILQ_FIRST(ruleset->rules.active.ptr);
- while ((rule != NULL) && (rule->nr != pr->nr))
- rule = TAILQ_NEXT(rule, entries);
+ rule = t->pftgr_rule;
if (rule == NULL) {
- error = EBUSY;
+ error = ENOENT;
PF_UNLOCK();
NET_UNLOCK();
goto fail;
@@ -1535,6 +1575,8 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
rule->bytes[0] = rule->bytes[1] = 0;
rule->states_tot = 0;
}
+ pr->nr = rule->nr;
+ t->pftgr_rule = TAILQ_NEXT(rule, entries);
PF_UNLOCK();
NET_UNLOCK();
break;
@@ -1560,7 +1602,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
if (ruleset == NULL)
error = EINVAL;
else
- pcr->ticket = ++ruleset->rules.active.ticket;
+ pcr->ticket = ++ruleset->rules.active.version;
PF_UNLOCK();
NET_UNLOCK();
@@ -1610,7 +1652,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
goto fail;
}
- if (pcr->ticket != ruleset->rules.active.ticket) {
+ if (pcr->ticket != ruleset->rules.active.version) {
error = EINVAL;
PF_UNLOCK();
NET_UNLOCK();
@@ -1707,7 +1749,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
TAILQ_FOREACH(oldrule, ruleset->rules.active.ptr, entries)
oldrule->nr = nr++;
- ruleset->rules.active.ticket++;
+ ruleset->rules.active.version++;
pf_calc_skip_steps(ruleset->rules.active.ptr);
pf_remove_if_empty_ruleset(ruleset);
@@ -2646,7 +2688,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
rs = pf_find_ruleset(ioe->anchor);
if (rs == NULL ||
!rs->rules.inactive.open ||
- rs->rules.inactive.ticket !=
+ rs->rules.inactive.version !=
ioe->ticket) {
PF_UNLOCK();
NET_UNLOCK();
@@ -3022,10 +3064,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
break;
}
fail:
- if (flags & FWRITE)
- rw_exit_write(&pfioctl_rw);
- else
- rw_exit_read(&pfioctl_rw);
+ rw_exit_write(&pfioctl_rw);
return (error);
}
@@ -3244,3 +3283,81 @@ pf_sysctl(void *oldp, size_t *oldlenp, void *newp,
size_t newlen)
return sysctl_rdstruct(oldp, oldlenp, newp, &pfs, sizeof(pfs));
}
+
+struct pf_trans *
+pf_open_trans(pid_t pid)
+{
+ static uint64_t ticket = 1;
+ struct pf_trans *t;
+
+ rw_assert_wrlock(&pfioctl_rw);
+
+ t = malloc(sizeof(*t), M_TEMP, M_WAITOK);
+ memset(t, 0, sizeof(struct pf_trans));
+ t->pft_pid = pid;
+ t->pft_ticket = ticket++;
+
+ LIST_INSERT_HEAD(&pf_ioctl_trans, t, pft_entry);
+
+ return (t);
+}
+
+struct pf_trans *
+pf_find_trans(uint64_t ticket)
+{
+ struct pf_trans *t;
+
+ rw_assert_anylock(&pfioctl_rw);
+
+ LIST_FOREACH(t, &pf_ioctl_trans, pft_entry) {
+ if (t->pft_ticket == ticket)
+ break;
+ }
+
+ return (t);
+}
+
+void
+pf_init_tgetrule(struct pf_trans *t, struct pf_anchor *a,
+ uint32_t rs_version, struct pf_rule *r)
+{
+ t->pft_type = PF_TRANS_GETRULE;
+ if (a == NULL)
+ t->pftgr_anchor = &pf_main_anchor;
+ else
+ t->pftgr_anchor = a;
+
+ t->pftgr_version = rs_version;
+ t->pftgr_rule = r;
+}
+
+void
+pf_cleanup_tgetrule(struct pf_trans *t)
+{
+ KASSERT(t->pft_type == PF_TRANS_GETRULE);
+ pf_anchor_rele(t->pftgr_anchor);
+}
+
+void
+pf_free_trans(struct pf_trans *t)
+{
+ switch (t->pft_type) {
+ case PF_TRANS_GETRULE:
+ pf_cleanup_tgetrule(t);
+ break;
+ default:
+ log(LOG_ERR, "%s unknown transaction type: %d\n",
+ __func__, t->pft_type);
+ }
+ free(t, M_TEMP, sizeof(*t));
+}
+
+void
+pf_rollback_trans(struct pf_trans *t)
+{
+ if (t != NULL) {
+ rw_assert_wrlock(&pfioctl_rw);
+ LIST_REMOVE(t, pft_entry);
+ pf_free_trans(t);
+ }
+}
diff --git a/sys/net/pf_ruleset.c b/sys/net/pf_ruleset.c
index 6131895751e..9d17d71e124 100644
--- a/sys/net/pf_ruleset.c
+++ b/sys/net/pf_ruleset.c
@@ -233,6 +233,9 @@ pf_create_anchor(struct pf_anchor *parent, const char
*aname)
pf_init_ruleset(&anchor->ruleset);
anchor->ruleset.anchor = anchor;
+#ifdef _KERNEL
+ refcnt_init(&anchor->ref);
+#endif
return (anchor);
}
@@ -308,7 +311,7 @@ pf_remove_if_empty_ruleset(struct pf_ruleset *ruleset)
if ((parent = ruleset->anchor->parent) != NULL)
RB_REMOVE(pf_anchor_node, &parent->children,
ruleset->anchor);
- rs_pool_put_anchor(ruleset->anchor);
+ pf_anchor_rele(ruleset->anchor);
if (parent == NULL)
return;
ruleset = &parent->ruleset;
@@ -431,3 +434,27 @@ pf_remove_anchor(struct pf_rule *r)
pf_remove_if_empty_ruleset(&r->anchor->ruleset);
r->anchor = NULL;
}
+
+void
+pf_anchor_rele(struct pf_anchor *anchor)
+{
+ if ((anchor == NULL) || (anchor == &pf_main_anchor))
+ return;
+
+#ifdef _KERNEL
+ if (refcnt_rele(&anchor->ref))
+ rs_pool_put_anchor(anchor);
+#else
+ rs_pool_put_anchor(anchor);
+#endif
+}
+
+struct pf_anchor *
+pf_anchor_take(struct pf_anchor *anchor)
+{
+#ifdef _KERNEL
+ if ((anchor != NULL) && (anchor != &pf_main_anchor))
+ refcnt_take(&anchor->ref);
+#endif
+ return (anchor);
+}
diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 3a7ff6b295c..529fee3d322 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -822,7 +822,7 @@ struct pf_ruleset {
struct {
struct pf_rulequeue *ptr;
u_int32_t rcount;
- u_int32_t ticket;
+ u_int32_t version;
int open;
} active, inactive;
} rules;
@@ -844,6 +844,7 @@ struct pf_anchor {
struct pf_ruleset ruleset;
int refcnt; /* anchor rules */
int match;
+ struct refcnt ref; /* for transactions */
};
RB_PROTOTYPE(pf_anchor_global, pf_anchor, entry_global, pf_anchor_compare)
RB_PROTOTYPE(pf_anchor_node, pf_anchor, entry_node, pf_anchor_compare)
@@ -1823,6 +1824,8 @@ struct pf_ruleset *pf_get_leaf_ruleset(char *,
char **);
struct pf_anchor *pf_create_anchor(struct pf_anchor *, const char *);
struct pf_ruleset *pf_find_or_create_ruleset(const char *);
void pf_rs_initialize(void);
+void pf_anchor_rele(struct pf_anchor *);
+struct pf_anchor *pf_anchor_take(struct pf_anchor *);
/* The fingerprint functions can be linked into userland programs (tcpdump) */
int pf_osfp_add(struct pf_osfp_ioctl *);
diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h
index 38fff6473aa..51d68610907 100644
--- a/sys/net/pfvar_priv.h
+++ b/sys/net/pfvar_priv.h
@@ -40,6 +40,8 @@
#include <sys/rwlock.h>
#include <sys/mutex.h>
#include <sys/percpu.h>
+#include <sys/queue.h>
+#include <net/pfvar.h>
struct pf_state_item {
TAILQ_ENTRY(pf_state_item)
@@ -322,6 +324,30 @@ enum {
extern struct cpumem *pf_anchor_stack;
+enum pf_trans_type {
+ PF_TRANS_NONE,
+ PF_TRANS_GETRULE,
+ PF_TRANS_MAX
+};
+
+struct pf_trans {
+ LIST_ENTRY(pf_trans) pft_entry;
+ pid_t pft_pid; /* process id */
+ uint64_t pft_ticket;
+ enum pf_trans_type pft_type;
+ union {
+ struct {
+ u_int32_t gr_version;
+ struct pf_anchor *gr_anchor;
+ struct pf_rule *gr_rule;
+ } u_getrule;
+ } u;
+};
+
+#define pftgr_version u.u_getrule.gr_version
+#define pftgr_anchor u.u_getrule.gr_anchor
+#define pftgr_rule u.u_getrule.gr_rule
+
extern struct task pf_purge_task;
extern struct timeout pf_purge_to;
