On Mon, Nov 07, 2022 at 04:09:44PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> resending the same diff, just updated to current.
> (pointed out by dlg@)
I like trading the rule garbage collector for a simple flag.
OK kn, but...
pfctl(8) already skips printing PFRULE_EXPIRED rules (unless -v),
so that is good, but previously this meant the rule would eventually
disappear.
No they remain and keep being printed in verbose mode without any
indication as to whether they're permanent or expired once rules.
So I think this should go in together with something in pfctl that
clearly shows expired rules as such.
>
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 2c6124e74f2..6295b4eb9d7 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -313,9 +313,6 @@ RB_GENERATE(pf_state_tree, pf_state_key, entry,
> pf_state_compare_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)
> {
> @@ -1478,23 +1475,6 @@ pf_state_import(const struct pfsync_state *sp, int
> flags)
>
> /* END state table stuff */
>
> -void
> -pf_purge_expired_rules(void)
> -{
> - struct pf_rule *r;
> -
> - PF_ASSERT_LOCKED();
> -
> - if (SLIST_EMPTY(&pf_rule_gcl))
> - return;
> -
> - 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_states(void *);
> struct task pf_purge_states_task =
> TASK_INITIALIZER(pf_purge_states, NULL);
> @@ -1588,7 +1568,6 @@ pf_purge(void *null)
> PF_LOCK();
>
> pf_purge_expired_src_nodes();
> - pf_purge_expired_rules();
>
> PF_UNLOCK();
>
> @@ -3916,8 +3895,11 @@ pf_match_rule(struct pf_test_ctx *ctx, struct
> pf_ruleset *ruleset)
> struct pf_rule *save_a;
> struct pf_ruleset *save_aruleset;
>
> +retry:
> r = TAILQ_FIRST(ruleset->rules.active.ptr);
> while (r != NULL) {
> + PF_TEST_ATTRIB(r->rule_flag & PFRULE_EXPIRED,
> + TAILQ_NEXT(r, entries));
> r->evaluations++;
> PF_TEST_ATTRIB(
> (pfi_kif_match(r->kif, ctx->pd->kif) == r->ifnot),
> @@ -4042,6 +4024,19 @@ pf_match_rule(struct pf_test_ctx *ctx, struct
> pf_ruleset *ruleset)
> if (r->tag)
> ctx->tag = r->tag;
> if (r->anchor == NULL) {
> +
> + if (r->rule_flag & PFRULE_ONCE) {
> + u_int32_t rule_flag;
> +
> + rule_flag = r->rule_flag;
> + if (((rule_flag & PFRULE_EXPIRED) == 0) &&
> + atomic_cas_uint(&r->rule_flag, rule_flag,
> + rule_flag | PFRULE_EXPIRED) == rule_flag)
> + r->exptime = gettime();
> + else
> + goto retry;
> + }
> +
> if (r->action == PF_MATCH) {
> if ((ctx->ri = pool_get(&pf_rule_item_pl,
> PR_NOWAIT)) == NULL) {
> @@ -4253,13 +4248,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
> struct pf_state **sm,
> if (r->action == PF_DROP)
> goto cleanup;
>
> - /*
> - * If an expired "once" rule has not been purged, drop any new matching
> - * packets.
> - */
> - if (r->rule_flag & PFRULE_EXPIRED)
> - goto cleanup;
> -
> pf_tag_packet(pd->m, ctx.tag, ctx.act.rtableid);
> if (ctx.act.rtableid >= 0 &&
> rtable_l2(ctx.act.rtableid) != pd->rdomain)
> @@ -4330,22 +4318,6 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
> struct pf_state **sm,
> m_copyback(pd->m, pd->off, pd->hdrlen, &pd->hdr, M_NOWAIT);
> }
>
> - if (r->rule_flag & PFRULE_ONCE) {
> - u_int32_t rule_flag;
> -
> - /*
> - * Use atomic_cas() to determine a clear winner, which will
> - * insert an expired rule to gcl.
> - */
> - rule_flag = r->rule_flag;
> - if (((rule_flag & PFRULE_EXPIRED) == 0) &&
> - atomic_cas_uint(&r->rule_flag, rule_flag,
> - rule_flag | PFRULE_EXPIRED) == rule_flag) {
> - r->exptime = gettime();
> - SLIST_INSERT_HEAD(&pf_rule_gcl, r, gcle);
> - }
> - }
> -
> #if NPFSYNC > 0
> if (*sm != NULL && !ISSET((*sm)->state_flags, PFSTATE_NOSYNC) &&
> pd->dir == PF_OUT && pfsync_up()) {
> diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> index b5d3261f8d1..6744fc022fb 100644
> --- a/sys/net/pf_ioctl.c
> +++ b/sys/net/pf_ioctl.c
> @@ -348,27 +348,6 @@ pf_rm_rule(struct pf_rulequeue *rulequeue, struct
> pf_rule *rule)
> pool_put(&pf_rule_pl, rule);
> }
>
> -void
> -pf_purge_rule(struct pf_rule *rule)
> -{
> - u_int32_t nr = 0;
> - struct pf_ruleset *ruleset;
> -
> - KASSERT((rule != NULL) && (rule->ruleset != NULL));
> - ruleset = rule->ruleset;
> -
> - pf_rm_rule(ruleset->rules.active.ptr, rule);
> - ruleset->rules.active.rcount--;
> - TAILQ_FOREACH(rule, ruleset->rules.active.ptr, entries)
> - rule->nr = nr++;
> - ruleset->rules.active.ticket++;
> - pf_calc_skip_steps(ruleset->rules.active.ptr);
> - pf_remove_if_empty_ruleset(ruleset);
> -
> - if (ruleset == &pf_main_ruleset)
> - pf_calc_chksum(ruleset);
> -}
> -
> u_int16_t
> tagname2tag(struct pf_tags *head, char *tagname, int create)
> {
> @@ -837,9 +816,6 @@ pf_commit_rules(u_int32_t ticket, char *anchor)
> struct pf_rulequeue *old_rules;
> 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)
> @@ -1447,7 +1423,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> }
> TAILQ_INSERT_TAIL(ruleset->rules.inactive.ptr,
> rule, entries);
> - rule->ruleset = ruleset;
> ruleset->rules.inactive.rcount++;
> PF_UNLOCK();
> NET_UNLOCK();
> @@ -1521,8 +1496,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> pr->rule.anchor = NULL;
> pr->rule.overload_tbl = NULL;
> pr->rule.pktrate.limit /= PF_THRESHOLD_MULT;
> - memset(&pr->rule.gcle, 0, sizeof(pr->rule.gcle));
> - pr->rule.ruleset = NULL;
> if (pf_anchor_copyout(ruleset, rule, pr)) {
> error = EBUSY;
> PF_UNLOCK();
> @@ -1713,7 +1686,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
> struct proc *p)
> ruleset->rules.active.ptr,
> oldrule, newrule, entries);
> ruleset->rules.active.rcount++;
> - newrule->ruleset = ruleset;
> }
>
> nr = 0;
> diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
> index 91b309ab729..bf6a91131a8 100644
> --- a/sys/net/pfvar.h
> +++ b/sys/net/pfvar.h
> @@ -599,8 +599,6 @@ struct pf_rule {
> u_int8_t type;
> } divert;
>
> - SLIST_ENTRY(pf_rule) gcle;
> - struct pf_ruleset *ruleset;
> time_t exptime;
> };
>
>
