Hello, updated diff below adopts to recent changes to pf_find_trans() in diff 2/3 [1]
in case DIOCGETRULE does not find desired transaction the ioctl(2) call to /dev/pf returns ENXIO. thanks and regards sashan [1] https://marc.info/?l=openbsd-tech&m=168254555211083&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 ebe1b912766..0f78c5b12ac 100644 --- a/sys/net/pf_ioctl.c +++ b/sys/net/pf_ioctl.c @@ -122,6 +122,10 @@ struct pf_trans *pf_find_trans(uint32_t, 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 { @@ -548,7 +552,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; @@ -559,20 +563,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); @@ -851,7 +855,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; @@ -860,7 +864,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) @@ -875,7 +879,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); @@ -1214,7 +1218,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); @@ -1235,7 +1239,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(); @@ -1266,7 +1270,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(); @@ -1313,7 +1317,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(); @@ -1409,7 +1413,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(); @@ -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.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(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(minor(dev), pr->ticket); + if (t == NULL) + return (ENXIO); + 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.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; @@ -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; @@ -1583,7 +1598,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(); @@ -1633,7 +1648,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(); @@ -1730,7 +1745,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); @@ -2669,7 +2684,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(); @@ -3301,9 +3316,38 @@ pf_find_trans(uint32_t unit, 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 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 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;
