Hello,
On Wed, Apr 26, 2023 at 01:25:45AM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> this is 3rd of three diffs to improve DIOCGETRULE ioctl operation.
> the summary of changes done here is as follows:
> - add new members to pf_trans structure so it can
> hold data for DIOCGETRULE operation
>
> - DIOCGETRULES opens transaction and returns ticket/transaction id
> to pfctl. pfctl uses ticket in DIOCGETRULE to identify transaction
> when it retrieves the rule
>
> - change introduces new reference counter to pf_anchor which is used
> by pf_trans to keep reference to pf_anchor object.
>
> - DIOCGETRULES opens transaction and stores a reference to anchor
> with current ruleset version and pointer to the first rule
>
> - DIOCGETRULE looks up transaction, verifies version number is
> same so it can safely access next rule.
>
> - pfctl when handling 'pfctl -sr -R n' must iterate to
> n-th rule one-by-one.
>
updating diff to accommodate changes in diff 2/3 [1] in the stack
of changes.
thanks and
regards
sashan
[1] https://marc.info/?l=openbsd-tech&m=168251129621089&w=2
--------8<---------------8<---------------8<------------------8<--------
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 8ce0af53a89..d294a8a3b3c 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -122,6 +122,10 @@ 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 {
@@ -1470,7 +1474,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();
@@ -1482,14 +1488,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.version;
+ 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(minor(dev));
+ pf_init_tgetrule(t, ruleset->anchor, ruleset_version, rule);
+ pr->ticket = t->pft_ticket;
+
break;
}
@@ -1497,29 +1510,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_unit == minor(dev));
+ 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.version) {
+ 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;
@@ -1558,6 +1571,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;
@@ -3298,9 +3313,38 @@ pf_find_trans(uint64_t ticket)
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));
}
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 cf1e34c36b4..529fee3d322 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -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 5af2027733a..011c8f083a0 100644
--- a/sys/net/pfvar_priv.h
+++ b/sys/net/pfvar_priv.h
@@ -324,6 +324,7 @@ extern struct cpumem *pf_anchor_stack;
enum pf_trans_type {
PF_TRANS_NONE,
+ PF_TRANS_GETRULE,
PF_TRANS_MAX
};
@@ -332,7 +333,19 @@ struct pf_trans {
uint32_t pft_unit; /* 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;
