On Mon, Nov 07, 2022 at 06:53:47PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> updated diff. It buys suggestions from Klemens:
>
> pf.conf(5) section which covers once rules reads as follows:
>
> once Creates a one shot rule. The first matching packet marks rule as
> expired. The expired rule is never evaluated then. pfctl(8)
> does not report expired rules unless run in verbose mode ('-vv').
> In verbose mode pfctl(8) appends '# expired' to note the once
> rule which got hit by packet other already.
Wording and markup can be improved, but I don't want to block this
change with boring churn, so I'm happy to do that later.
>
> this how pfctl reports expired once rules now:
>
> pf# pfctl -a test/foo/bar -vv -sr
> @0 pass out proto tcp from any to any port = 80 flags S/SA once #
> expired
> [ Evaluations: 25 Packets: 5489 Bytes: 4936817
> States: 0 ]
> [ Inserted: uid 0 pid 2768 State Creations: 1 ]
>
> updated diff is below.
OK kn
>
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
> index 6f39ad72384..fbe19747fc6 100644
> --- a/sbin/pfctl/pfctl_parser.c
> +++ b/sbin/pfctl/pfctl_parser.c
> @@ -1148,6 +1148,9 @@ print_rule(struct pf_rule *r, const char *anchor_call,
> int opts)
> printf(" ");
> print_pool(&r->route, 0, 0, r->af, PF_POOL_ROUTE, verbose);
> }
> +
> + if (r->rule_flag & PFRULE_EXPIRED)
> + printf(" # expired");
> }
>
> void
> diff --git a/share/man/man5/pf.conf.5 b/share/man/man5/pf.conf.5
> index 3e5a17acb95..b98b9f2fd9c 100644
> --- a/share/man/man5/pf.conf.5
> +++ b/share/man/man5/pf.conf.5
> @@ -661,10 +661,14 @@ When the rate is exceeded, all ICMP is blocked until
> the rate falls below
> 100 per 10 seconds again.
> .Pp
> .It Cm once
> -Creates a one shot rule that will remove itself from an active ruleset after
> -the first match.
> -In case this is the only rule in the anchor, the anchor will be destroyed
> -automatically after the rule is matched.
> +Creates a one shot rule. The first matching packet marks rule as expired.
> +The expired rule is never evaluated then.
> +.Xr pfctl 8
> +does not report expired rules unless run in verbose mode ('-vv'). In verbose
> +mode
> +.Xr pfctl 8
> +appends '# expired' to note the once rule which got hit by packet other
> +already.
> .Pp
> .It Cm probability Ar number Ns %
> A probability attribute can be attached to a rule,
> 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;
> };
>
>
