Date: Sun, 28 Sep 2025 17:25:44 GMT From: Kajetan Staszkiewicz <ks@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 6353f5d9a5c6 - main - pf: Fix rule and state counters Message-ID: <202509281725.58SHPiKL027396@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by ks: URL: https://cgit.FreeBSD.org/src/commit/?id=6353f5d9a5c6f194bb014b8785a57f5314e8c652 commit 6353f5d9a5c6f194bb014b8785a57f5314e8c652 Author: Kajetan Staszkiewicz <ks@FreeBSD.org> AuthorDate: 2025-09-03 18:27:11 +0000 Commit: Kajetan Staszkiewicz <ks@FreeBSD.org> CommitDate: 2025-09-28 17:23:02 +0000 pf: Fix rule and state counters Increasing counters on "match" rules causes the 1st packet making a connection to be double-counted, but only for rule counters, not rules' tables, because those are not increased at all during rule parsing. Remove "match" rule counter handling during rule parsing, do it only in pf_counters_inc(). NAT can be performed either by "nat" rules in the NAT ruleset or by "match" rules. Rules before the NAT rule, and the NAT rule itself match on pre-NAT addresses, and later rules match on post-NAT addresses. When increasing counters go over rules in the same order as a packet would and use source and destination addresses for updating table counters from appropriate state key, taking into consideration on which rule NAT happens. Use AF from state key, so that table counters can be properly updated for af-to rules. Synchronize match rule updating behaviour to that of OpenBSD: if rules match, but state is not created, don't update counters. Reviewed by: kp Sponsored by: InnoGames GmbH Differential Revision: https://reviews.freebsd.org/D52447 --- sys/net/pfvar.h | 7 +- sys/netpfil/pf/pf.c | 339 +++++++++------- tests/sys/netpfil/pf/Makefile | 1 + tests/sys/netpfil/pf/counters.sh | 817 +++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/nat64.sh | 26 +- tests/sys/netpfil/pf/utils.subr | 3 + 6 files changed, 1039 insertions(+), 154 deletions(-) diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h index c6a3448584ac..8aefe514946e 100644 --- a/sys/net/pfvar.h +++ b/sys/net/pfvar.h @@ -1166,7 +1166,6 @@ struct pf_test_ctx { int rewrite; u_short reason; struct pf_src_node *sns[PF_SN_MAX]; - struct pf_krule_slist rules; struct pf_krule *nr; struct pf_krule *tr; struct pf_krule **rm; @@ -2724,8 +2723,10 @@ int pf_osfp_match(struct pf_osfp_enlist *, pf_osfp_t); #ifdef _KERNEL void pf_print_host(struct pf_addr *, u_int16_t, sa_family_t); -enum pf_test_status pf_step_into_anchor(struct pf_test_ctx *, struct pf_krule *); -enum pf_test_status pf_match_rule(struct pf_test_ctx *, struct pf_kruleset *); +enum pf_test_status pf_step_into_anchor(struct pf_test_ctx *, struct pf_krule *, + struct pf_krule_slist *match_rules); +enum pf_test_status pf_match_rule(struct pf_test_ctx *, struct pf_kruleset *, + struct pf_krule_slist *); void pf_step_into_keth_anchor(struct pf_keth_anchor_stackframe *, int *, struct pf_keth_ruleset **, struct pf_keth_rule **, struct pf_keth_rule **, diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index f50599627255..9f250476cb75 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -344,10 +344,12 @@ static int pf_test_eth_rule(int, struct pfi_kkif *, struct mbuf **); static int pf_test_rule(struct pf_krule **, struct pf_kstate **, struct pf_pdesc *, struct pf_krule **, - struct pf_kruleset **, u_short *, struct inpcb *); + struct pf_kruleset **, u_short *, struct inpcb *, + struct pf_krule_slist *); static int pf_create_state(struct pf_krule *, struct pf_test_ctx *, - struct pf_kstate **, u_int16_t, u_int16_t); + struct pf_kstate **, u_int16_t, u_int16_t, + struct pf_krule_slist *match_rules); static int pf_state_key_addr_setup(struct pf_pdesc *, struct pf_state_key_cmp *, int); static int pf_tcp_track_full(struct pf_kstate *, @@ -393,7 +395,7 @@ static bool pf_src_connlimit(struct pf_kstate *); static int pf_match_rcvif(struct mbuf *, struct pf_krule *); static void pf_counters_inc(int, struct pf_pdesc *, struct pf_kstate *, struct pf_krule *, - struct pf_krule *); + struct pf_krule *, struct pf_krule_slist *); static void pf_log_matches(struct pf_pdesc *, struct pf_krule *, struct pf_krule *, struct pf_kruleset *, struct pf_krule_slist *); @@ -489,26 +491,30 @@ BOUND_IFACE(struct pf_kstate *st, struct pf_pdesc *pd) counter_u64_add(s->anchor->states_cur, 1); \ counter_u64_add(s->anchor->states_tot, 1); \ } \ - if (s->nat_rule != NULL) { \ - counter_u64_add(s->nat_rule->states_cur, 1);\ - counter_u64_add(s->nat_rule->states_tot, 1);\ + if (s->nat_rule != NULL && s->nat_rule != s->rule) { \ + counter_u64_add(s->nat_rule->states_cur, 1); \ + counter_u64_add(s->nat_rule->states_tot, 1); \ } \ SLIST_FOREACH(mrm, &s->match_rules, entry) { \ - counter_u64_add(mrm->r->states_cur, 1); \ - counter_u64_add(mrm->r->states_tot, 1); \ + if (s->nat_rule != mrm->r) { \ + counter_u64_add(mrm->r->states_cur, 1); \ + counter_u64_add(mrm->r->states_tot, 1); \ + } \ } \ } while (0) #define STATE_DEC_COUNTERS(s) \ do { \ struct pf_krule_item *mrm; \ - if (s->nat_rule != NULL) \ - counter_u64_add(s->nat_rule->states_cur, -1);\ - if (s->anchor != NULL) \ - counter_u64_add(s->anchor->states_cur, -1); \ counter_u64_add(s->rule->states_cur, -1); \ + if (s->anchor != NULL) \ + counter_u64_add(s->anchor->states_cur, -1); \ + if (s->nat_rule != NULL && s->nat_rule != s->rule) \ + counter_u64_add(s->nat_rule->states_cur, -1); \ SLIST_FOREACH(mrm, &s->match_rules, entry) \ - counter_u64_add(mrm->r->states_cur, -1); \ + if (s->nat_rule != mrm->r) { \ + counter_u64_add(mrm->r->states_cur, -1);\ + } \ } while (0) MALLOC_DEFINE(M_PFHASH, "pf_hash", "pf(4) hash header structures"); @@ -2869,20 +2875,24 @@ pf_alloc_state(int flags) return (uma_zalloc(V_pf_state_z, flags | M_ZERO)); } +static __inline void +pf_free_match_rules(struct pf_krule_slist *match_rules) { + struct pf_krule_item *ri; + + while ((ri = SLIST_FIRST(match_rules))) { + SLIST_REMOVE_HEAD(match_rules, entry); + free(ri, M_PF_RULE_ITEM); + } +} + void pf_free_state(struct pf_kstate *cur) { - struct pf_krule_item *ri; - KASSERT(cur->refs == 0, ("%s: %p has refs", __func__, cur)); KASSERT(cur->timeout == PFTM_UNLINKED, ("%s: timeout %u", __func__, cur->timeout)); - while ((ri = SLIST_FIRST(&cur->match_rules))) { - SLIST_REMOVE_HEAD(&cur->match_rules, entry); - free(ri, M_PF_RULE_ITEM); - } - + pf_free_match_rules(&(cur->match_rules)); pf_normalize_tcp_cleanup(cur); uma_zfree(V_pf_state_z, cur); pf_counter_u64_add(&V_pf_status.fcounters[FCNT_STATE_REMOVALS], 1); @@ -4740,7 +4750,8 @@ pf_tag_packet(struct pf_pdesc *pd, int tag) } while (0) enum pf_test_status -pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r) +pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r, + struct pf_krule_slist *match_rules) { enum pf_test_status rv; @@ -4758,7 +4769,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r) struct pf_kanchor *child; rv = PF_TEST_OK; RB_FOREACH(child, pf_kanchor_node, &r->anchor->children) { - rv = pf_match_rule(ctx, &child->ruleset); + rv = pf_match_rule(ctx, &child->ruleset, match_rules); if ((rv == PF_TEST_QUICK) || (rv == PF_TEST_FAIL)) { /* * we either hit a rule with quick action @@ -4769,7 +4780,7 @@ pf_step_into_anchor(struct pf_test_ctx *ctx, struct pf_krule *r) } } } else { - rv = pf_match_rule(ctx, &r->anchor->ruleset); + rv = pf_match_rule(ctx, &r->anchor->ruleset, match_rules); /* * Unless errors occured, stop iff any rule matched * within quick anchors. @@ -5618,9 +5629,10 @@ pf_rule_apply_nat(struct pf_test_ctx *ctx, struct pf_krule *r) } enum pf_test_status -pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset) +pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset, + struct pf_krule_slist *match_rules) { - struct pf_krule_item *ri; + struct pf_krule_item *ri, *rt; struct pf_krule *r; struct pf_krule *save_a; struct pf_kruleset *save_aruleset; @@ -5777,11 +5789,14 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset) return (PF_TEST_FAIL); } ri->r = r; - SLIST_INSERT_HEAD(&ctx->rules, ri, entry); - pf_counter_u64_critical_enter(); - pf_counter_u64_add_protected(&r->packets[pd->dir == PF_OUT], 1); - pf_counter_u64_add_protected(&r->bytes[pd->dir == PF_OUT], pd->tot_len); - pf_counter_u64_critical_exit(); + + if (SLIST_EMPTY(match_rules)) { + SLIST_INSERT_HEAD(match_rules, ri, entry); + } else { + SLIST_INSERT_AFTER(rt, ri, entry); + } + rt = ri; + pf_rule_to_actions(r, &pd->act); if (r->log) PFLOG_PACKET(r->action, PFRES_MATCH, r, @@ -5805,7 +5820,7 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset) ctx->arsm = ctx->aruleset; } if (pd->act.log & PF_LOG_MATCHES) - pf_log_matches(pd, r, ctx->a, ruleset, &ctx->rules); + pf_log_matches(pd, r, ctx->a, ruleset, match_rules); if (r->quick) { ctx->test_status = PF_TEST_QUICK; break; @@ -5822,7 +5837,7 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset) * Note: we don't need to restore if we are not going * to continue with ruleset evaluation. */ - if (pf_step_into_anchor(ctx, r) != PF_TEST_OK) { + if (pf_step_into_anchor(ctx, r, match_rules) != PF_TEST_OK) { break; } ctx->a = save_a; @@ -5838,11 +5853,11 @@ pf_match_rule(struct pf_test_ctx *ctx, struct pf_kruleset *ruleset) static int pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, struct pf_pdesc *pd, struct pf_krule **am, - struct pf_kruleset **rsm, u_short *reason, struct inpcb *inp) + struct pf_kruleset **rsm, u_short *reason, struct inpcb *inp, + struct pf_krule_slist *match_rules) { struct pf_krule *r = NULL; struct pf_kruleset *ruleset = NULL; - struct pf_krule_item *ri; struct pf_test_ctx ctx; u_short transerror; int action = PF_PASS; @@ -5859,7 +5874,6 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, ctx.rsm = rsm; ctx.th = &pd->hdr.tcp; ctx.reason = *reason; - SLIST_INIT(&ctx.rules); pf_addrcpy(&pd->nsaddr, pd->src, pd->af); pf_addrcpy(&pd->ndaddr, pd->dst, pd->af); @@ -5952,7 +5966,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, } ruleset = &pf_main_ruleset; - rv = pf_match_rule(&ctx, ruleset); + rv = pf_match_rule(&ctx, ruleset, match_rules); if (rv == PF_TEST_FAIL) { /* * Reason has been set in pf_match_rule() already. @@ -5988,7 +6002,7 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, PFLOG_PACKET(r->action, ctx.reason, r, ctx.a, ruleset, pd, 1, NULL); } if (pd->act.log & PF_LOG_MATCHES) - pf_log_matches(pd, r, ctx.a, ruleset, &ctx.rules); + pf_log_matches(pd, r, ctx.a, ruleset, match_rules); if (pd->virtual_proto != PF_VPROTO_FRAGMENT && (r->action == PF_DROP) && ((r->rule_flag & PFRULE_RETURNRST) || @@ -6033,7 +6047,8 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, (pd->flags & PFDESC_TCP_NORM)))) { bool nat64; - action = pf_create_state(r, &ctx, sm, bproto_sum, bip_sum); + action = pf_create_state(r, &ctx, sm, bproto_sum, bip_sum, + match_rules); ctx.sk = ctx.nk = NULL; if (action != PF_PASS) { pf_udp_mapping_release(ctx.udp_mapping); @@ -6079,11 +6094,6 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, action = PF_AFRT; } } else { - while ((ri = SLIST_FIRST(&ctx.rules))) { - SLIST_REMOVE_HEAD(&ctx.rules, entry); - free(ri, M_PF_RULE_ITEM); - } - uma_zfree(V_pf_state_key_z, ctx.sk); uma_zfree(V_pf_state_key_z, ctx.nk); ctx.sk = ctx.nk = NULL; @@ -6111,11 +6121,6 @@ pf_test_rule(struct pf_krule **rm, struct pf_kstate **sm, return (action); cleanup: - while ((ri = SLIST_FIRST(&ctx.rules))) { - SLIST_REMOVE_HEAD(&ctx.rules, entry); - free(ri, M_PF_RULE_ITEM); - } - uma_zfree(V_pf_state_key_z, ctx.sk); uma_zfree(V_pf_state_key_z, ctx.nk); pf_udp_mapping_release(ctx.udp_mapping); @@ -6126,7 +6131,8 @@ cleanup: static int pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx, - struct pf_kstate **sm, u_int16_t bproto_sum, u_int16_t bip_sum) + struct pf_kstate **sm, u_int16_t bproto_sum, u_int16_t bip_sum, + struct pf_krule_slist *match_rules) { struct pf_pdesc *pd = ctx->pd; struct pf_kstate *s = NULL; @@ -6140,7 +6146,6 @@ pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx, struct tcphdr *th = &pd->hdr.tcp; u_int16_t mss = V_tcp_mssdflt; u_short sn_reason; - struct pf_krule_item *ri; /* check maximums */ if (r->max_states && @@ -6192,7 +6197,7 @@ pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx, s->rule = r; s->nat_rule = ctx->nr; s->anchor = ctx->a; - memcpy(&s->match_rules, &ctx->rules, sizeof(s->match_rules)); + s->match_rules = *match_rules; memcpy(&s->act, &pd->act, sizeof(struct pf_rule_actions)); if (pd->act.allow_opts) @@ -6356,11 +6361,6 @@ pf_create_state(struct pf_krule *r, struct pf_test_ctx *ctx, return (PF_PASS); csfailed: - while ((ri = SLIST_FIRST(&ctx->rules))) { - SLIST_REMOVE_HEAD(&ctx->rules, entry); - free(ri, M_PF_RULE_ITEM); - } - uma_zfree(V_pf_state_key_z, ctx->sk); uma_zfree(V_pf_state_key_z, ctx->nk); @@ -7518,6 +7518,7 @@ static void pf_sctp_multihome_delayed(struct pf_pdesc *pd, struct pfi_kkif *kif, struct pf_kstate *s, int action) { + struct pf_krule_slist match_rules; struct pf_sctp_multihome_job *j, *tmp; struct pf_sctp_source *i; int ret; @@ -7565,8 +7566,14 @@ again: if (s->rule->rule_flag & PFRULE_ALLOW_RELATED) { j->pd.related_rule = s->rule; } + SLIST_INIT(&match_rules); ret = pf_test_rule(&r, &sm, - &j->pd, &ra, &rs, &reason, NULL); + &j->pd, &ra, &rs, &reason, NULL, &match_rules); + /* + * Nothing to do about match rules, the processed + * packet has already increased the counters. + */ + pf_free_match_rules(&match_rules); PF_RULES_RUNLOCK(); SDT_PROBE4(pf, sctp, multihome, test, kif, r, j->pd.m, ret); if (ret != PF_DROP && sm != NULL) { @@ -10685,108 +10692,149 @@ pf_setup_pdesc(sa_family_t af, int dir, struct pf_pdesc *pd, struct mbuf **m0, return (0); } +static __inline void +pf_rule_counters_inc(struct pf_pdesc *pd, struct pf_krule *r, int dir_out, + int op_pass, sa_family_t af, struct pf_addr *src_host, + struct pf_addr *dst_host) +{ + pf_counter_u64_add_protected(&(r->packets[dir_out]), 1); + pf_counter_u64_add_protected(&(r->bytes[dir_out]), pd->tot_len); + pf_update_timestamp(r); + + if (r->src.addr.type == PF_ADDR_TABLE) + pfr_update_stats(r->src.addr.p.tbl, src_host, af, + pd->tot_len, dir_out, op_pass, r->src.neg); + if (r->dst.addr.type == PF_ADDR_TABLE) + pfr_update_stats(r->dst.addr.p.tbl, dst_host, af, + pd->tot_len, dir_out, op_pass, r->dst.neg); +} + static void -pf_counters_inc(int action, struct pf_pdesc *pd, - struct pf_kstate *s, struct pf_krule *r, struct pf_krule *a) +pf_counters_inc(int action, struct pf_pdesc *pd, struct pf_kstate *s, + struct pf_krule *r, struct pf_krule *a, struct pf_krule_slist *match_rules) { - struct pf_krule *tr; - int dir = pd->dir; - int dirndx; + struct pf_krule_slist *mr = match_rules; + struct pf_krule_item *ri; + struct pf_krule *nr = NULL; + struct pf_addr *src_host = pd->src; + struct pf_addr *dst_host = pd->dst; + struct pf_state_key *key; + int dir_out = (pd->dir == PF_OUT); + int op_pass = (r->action == PF_PASS); + sa_family_t af = pd->af; + int s_dir_in, s_dir_out, s_dir_rev; pf_counter_u64_critical_enter(); + pf_counter_u64_add_protected( - &pd->kif->pfik_bytes[pd->af == AF_INET6][dir == PF_OUT][action != PF_PASS], + &pd->kif->pfik_bytes[pd->af == AF_INET6][dir_out][action != PF_PASS], pd->tot_len); pf_counter_u64_add_protected( - &pd->kif->pfik_packets[pd->af == AF_INET6][dir == PF_OUT][action != PF_PASS], + &pd->kif->pfik_packets[pd->af == AF_INET6][dir_out][action != PF_PASS], 1); - if (action == PF_PASS || action == PF_AFRT || r->action == PF_DROP) { - dirndx = (dir == PF_OUT); - pf_counter_u64_add_protected(&r->packets[dirndx], 1); - pf_counter_u64_add_protected(&r->bytes[dirndx], pd->tot_len); - pf_update_timestamp(r); + /* If the rule has failed to apply, don't increase its counters */ + if (!(action == PF_PASS || action == PF_AFRT || r->action == PF_DROP)) { + pf_counter_u64_critical_exit(); + return; + } - if (a != NULL) { - pf_counter_u64_add_protected(&a->packets[dirndx], 1); - pf_counter_u64_add_protected(&a->bytes[dirndx], pd->tot_len); + if (s != NULL) { + PF_STATE_LOCK_ASSERT(s); + mr = &(s->match_rules); + + /* + * For af-to on the inbound direction we can determine + * the direction only by checking direction of AF translation, + * since the state is always "in" and so is packet's direction. + */ + if (pd->af != pd->naf && s->direction == PF_IN) { + dir_out = (pd->naf == s->rule->naf); + s_dir_in = 1; + s_dir_out = 0; + s_dir_rev = (pd->naf != s->rule->naf); + } + else { + dir_out = (pd->dir == PF_OUT); + s_dir_in = (s->direction == PF_IN); + s_dir_out = (s->direction == PF_OUT); + s_dir_rev = (pd->dir != s->direction); } - if (s != NULL) { - struct pf_krule_item *ri; - if (s->nat_rule != NULL) { - pf_counter_u64_add_protected(&s->nat_rule->packets[dirndx], + s->packets[s_dir_rev]++; + s->bytes[s_dir_rev] += pd->tot_len; + + /* + * Source nodes are accessed unlocked here. But since we are + * operating with stateful tracking and the state is locked, + * those SNs could not have been freed. + */ + for (pf_sn_types_t sn_type=0; sn_type<PF_SN_MAX; sn_type++) { + if (s->sns[sn_type] != NULL) { + counter_u64_add( + s->sns[sn_type]->packets[dir_out], 1); - pf_counter_u64_add_protected(&s->nat_rule->bytes[dirndx], + counter_u64_add( + s->sns[sn_type]->bytes[dir_out], pd->tot_len); } - /* - * Source nodes are accessed unlocked here. - * But since we are operating with stateful tracking - * and the state is locked, those SNs could not have - * been freed. - */ - for (pf_sn_types_t sn_type=0; sn_type<PF_SN_MAX; sn_type++) { - if (s->sns[sn_type] != NULL) { - counter_u64_add( - s->sns[sn_type]->packets[dirndx], - 1); - counter_u64_add( - s->sns[sn_type]->bytes[dirndx], - pd->tot_len); - } - } - dirndx = (dir == s->direction) ? 0 : 1; - s->packets[dirndx]++; - s->bytes[dirndx] += pd->tot_len; - - SLIST_FOREACH(ri, &s->match_rules, entry) { - pf_counter_u64_add_protected(&ri->r->packets[dirndx], 1); - pf_counter_u64_add_protected(&ri->r->bytes[dirndx], pd->tot_len); + } - if (ri->r->src.addr.type == PF_ADDR_TABLE) - pfr_update_stats(ri->r->src.addr.p.tbl, - (s == NULL) ? pd->src : - &s->key[(s->direction == PF_IN)]-> - addr[(s->direction == PF_OUT)], - pd->af, pd->tot_len, dir == PF_OUT, - r->action == PF_PASS, ri->r->src.neg); - if (ri->r->dst.addr.type == PF_ADDR_TABLE) - pfr_update_stats(ri->r->dst.addr.p.tbl, - (s == NULL) ? pd->dst : - &s->key[(s->direction == PF_IN)]-> - addr[(s->direction == PF_IN)], - pd->af, pd->tot_len, dir == PF_OUT, - r->action == PF_PASS, ri->r->dst.neg); + /* Start with pre-NAT addresses */ + key = s->key[(s->direction == PF_OUT)]; + src_host = &(key->addr[s_dir_out]); + dst_host = &(key->addr[s_dir_in]); + af = key->af; + if (s->nat_rule) { + /* Old-style NAT rules */ + if (s->nat_rule->action == PF_NAT || + s->nat_rule->action == PF_RDR || + s->nat_rule->action == PF_BINAT) { + nr = s->nat_rule; + pf_rule_counters_inc(pd, s->nat_rule, dir_out, + op_pass, af, src_host, dst_host); + /* Use post-NAT addresses from now on */ + key = s->key[s_dir_in]; + src_host = &(key->addr[s_dir_out]); + dst_host = &(key->addr[s_dir_in]); + af = key->af; } } + } - tr = r; - if (s != NULL && s->nat_rule != NULL && - r == &V_pf_default_rule) - tr = s->nat_rule; - - if (tr->src.addr.type == PF_ADDR_TABLE) - pfr_update_stats(tr->src.addr.p.tbl, - (s == NULL) ? pd->src : - &s->key[(s->direction == PF_IN)]-> - addr[(s->direction == PF_OUT)], - pd->af, pd->tot_len, dir == PF_OUT, - r->action == PF_PASS, tr->src.neg); - if (tr->dst.addr.type == PF_ADDR_TABLE) - pfr_update_stats(tr->dst.addr.p.tbl, - (s == NULL) ? pd->dst : - &s->key[(s->direction == PF_IN)]-> - addr[(s->direction == PF_IN)], - pd->af, pd->tot_len, dir == PF_OUT, - r->action == PF_PASS, tr->dst.neg); + SLIST_FOREACH(ri, mr, entry) { + pf_rule_counters_inc(pd, ri->r, dir_out, op_pass, af, + src_host, dst_host); + if (s && s->nat_rule == ri->r) { + /* Use post-NAT addresses after a match NAT rule */ + key = s->key[s_dir_in]; + src_host = &(key->addr[s_dir_out]); + dst_host = &(key->addr[s_dir_in]); + af = key->af; + } + } + + if (s == NULL) { + pf_free_match_rules(mr); } + + if (a != NULL) { + pf_rule_counters_inc(pd, a, dir_out, op_pass, af, + src_host, dst_host); + } + + if (r != nr) { + pf_rule_counters_inc(pd, r, dir_out, op_pass, af, + src_host, dst_host); + } + pf_counter_u64_critical_exit(); } + static void pf_log_matches(struct pf_pdesc *pd, struct pf_krule *rm, struct pf_krule *am, struct pf_kruleset *ruleset, - struct pf_krule_slist *matchrules) + struct pf_krule_slist *match_rules) { struct pf_krule_item *ri; @@ -10794,7 +10842,7 @@ pf_log_matches(struct pf_pdesc *pd, struct pf_krule *rm, if (rm->log & PF_LOG_MATCHES) return; - SLIST_FOREACH(ri, matchrules, entry) + SLIST_FOREACH(ri, match_rules, entry) if (ri->r->log & PF_LOG_MATCHES) PFLOG_PACKET(rm->action, PFRES_MATCH, rm, am, ruleset, pd, 1, ri->r); @@ -10811,6 +10859,8 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 struct pf_krule *a = NULL, *r = &V_pf_default_rule; struct pf_kstate *s = NULL; struct pf_kruleset *ruleset = NULL; + struct pf_krule_item *ri; + struct pf_krule_slist match_rules; struct pf_pdesc pd; int use_2nd_queue = 0; uint16_t tag; @@ -10847,6 +10897,7 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 } pf_init_pdesc(&pd, *m0); + SLIST_INIT(&match_rules); if (pd.pf_mtag != NULL && (pd.pf_mtag->flags & PF_MTAG_FLAG_ROUTE_TO)) { pd.pf_mtag->flags &= ~PF_MTAG_FLAG_ROUTE_TO; @@ -10943,7 +10994,7 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 action = PF_DROP; else action = pf_test_rule(&r, &s, &pd, &a, - &ruleset, &reason, inp); + &ruleset, &reason, inp, &match_rules); if (action != PF_PASS) REASON_SET(&reason, PFRES_FRAG); break; @@ -11001,7 +11052,7 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 break; } else { action = pf_test_rule(&r, &s, &pd, - &a, &ruleset, &reason, inp); + &a, &ruleset, &reason, inp, &match_rules); } } break; @@ -11022,7 +11073,7 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 a = s->anchor; } else if (s == NULL) { action = pf_test_rule(&r, &s, - &pd, &a, &ruleset, &reason, inp); + &pd, &a, &ruleset, &reason, inp, &match_rules); } break; @@ -11050,7 +11101,7 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 a = s->anchor; } else if (s == NULL) action = pf_test_rule(&r, &s, &pd, - &a, &ruleset, &reason, inp); + &a, &ruleset, &reason, inp, &match_rules); break; } @@ -11059,8 +11110,11 @@ pf_test(sa_family_t af, int dir, int pflags, struct ifnet *ifp, struct mbuf **m0 done: PF_RULES_RUNLOCK(); - if (pd.m == NULL) + /* if packet sits in reassembly queue, return without error */ + if (pd.m == NULL) { + pf_free_match_rules(&match_rules); goto eat_pkt; + } if (s) memcpy(&pd.act, &s->act, sizeof(s->act)); @@ -11157,6 +11211,8 @@ done: (dir == PF_IN) ? PF_DIVERT_MTAG_DIR_IN : PF_DIVERT_MTAG_DIR_OUT; + pf_counters_inc(action, &pd, s, r, a, &match_rules); + if (s) PF_STATE_UNLOCK(s); @@ -11198,7 +11254,6 @@ done: if (pd.act.log) { struct pf_krule *lr; - struct pf_krule_item *ri; if (s != NULL && s->nat_rule != NULL && s->nat_rule->log & PF_LOG_ALL) @@ -11217,7 +11272,7 @@ done: } } - pf_counters_inc(action, &pd, s, r, a); + pf_counters_inc(action, &pd, s, r, a, &match_rules); switch (action) { case PF_SYNPROXY_DROP: diff --git a/tests/sys/netpfil/pf/Makefile b/tests/sys/netpfil/pf/Makefile index 7ddeb5369f47..99500fc90806 100644 --- a/tests/sys/netpfil/pf/Makefile +++ b/tests/sys/netpfil/pf/Makefile @@ -5,6 +5,7 @@ TESTS_SUBDIRS+= ioctl ATF_TESTS_SH+= altq \ anchor \ + counters \ debug \ divert-to \ dup \ diff --git a/tests/sys/netpfil/pf/counters.sh b/tests/sys/netpfil/pf/counters.sh new file mode 100644 index 000000000000..a0119b4710c1 --- /dev/null +++ b/tests/sys/netpfil/pf/counters.sh @@ -0,0 +1,817 @@ +# +# SPDX-License-Identifier: BSD-2-Clause +# +# Copyright (c) 2025 Kajetan Staszkiewicz +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +. $(atf_get_srcdir)/utils.subr + +get_counters() +{ + echo " === rules ===" + rules=$(mktemp) || exit + (jexec router pfctl -qvvsn ; jexec router pfctl -qvvsr) | normalize_pfctl_s > $rules + cat $rules + + echo " === tables ===" + tables=$(mktemp) || exit 1 + jexec router pfctl -qvvsT > $tables + cat $tables + + echo " === states ===" + states=$(mktemp) || exit 1 + jexec router pfctl -qvvss | normalize_pfctl_s > $states + cat $states + + echo " === nodes ===" + nodes=$(mktemp) || exit 1 + jexec router pfctl -qvvsS | normalize_pfctl_s > $nodes + cat $nodes +} + +atf_test_case "match_pass_state" "cleanup" +match_pass_state_head() +{ + atf_set descr 'Counters on match and pass rules' + atf_set require.user root +} + +match_pass_state_body() +{ + setup_router_server_ipv6 + + # Thest counters for a statefull firewall. Expose the behaviour of + # increasing table counters if a table is used multiple times. + # The table "tbl_in" is used both in match and pass rule. It's counters + # are incremented twice. The tables "tbl_out_match" and "tbl_out_pass" + # are used only once and have their countes increased only once. + # Test source node counters for this simple scenario too. + pft_set_rules router \ + "set state-policy if-bound" \ + "table <tbl_in> { ${net_tester_host_tester} }" \ + "table <tbl_out_pass> { ${net_server_host_server} }" \ + "table <tbl_out_match> { ${net_server_host_server} }" \ + "block" \ + "pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv }" \ + "match in on ${epair_tester}b inet6 proto tcp from <tbl_in> scrub (random-id)" \ + "pass in on ${epair_tester}b inet6 proto tcp from <tbl_in> keep state (max-src-states 3 source-track rule)" \ + "match out on ${epair_server}a inet6 proto tcp to <tbl_out_match> scrub (random-id)" \ + "pass out on ${epair_server}a inet6 proto tcp to <tbl_out_pass> keep state" + + # Use a real TCP connection so that it will be properly closed, guaranteeing the amount of packets. + atf_check -s exit:0 -o match:"This is a test" -x \ + "echo 'This is a test' | nc -w3 ${net_server_host_server} echo" + # Let FINs pass through. + sleep 1 + get_counters + + for rule_regexp in \ + "@3 match in on ${epair_tester}b .* Packets: 10 Bytes: 766 States: 1 " \ + "@4 pass in on ${epair_tester}b .* Packets: 10 Bytes: 766 States: 1 " \ + "@5 match out on ${epair_server}a .* Packets: 10 Bytes: 766 States: 1 " \ + "@6 pass out on ${epair_server}a .* Packets: 10 Bytes: 766 States: 1 " \ + ; do + grep -qE "${rule_regexp}" $rules || atf_fail "Rule regexp not found for '${rule_regexp}'" + done + + table_counters_single="Evaluations: NoMatch: 0 Match: 1 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 4 Bytes: 311 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 6 Bytes: 455 Out/XPass: Packets: 0 Bytes: 0" + table_counters_double="Evaluations: NoMatch: 0 Match: 2 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 12 Bytes: 910 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 8 Bytes: 622 Out/XPass: Packets: 0 Bytes: 0" + for table_test in \ + "tbl_in___${table_counters_double}" \ + "tbl_out_match___${table_counters_single}" \ + "tbl_out_pass___${table_counters_single}" \ + ; do + table_name=${table_test%%___*} + table_regexp=${table_test##*___} + table=$(mktemp) || exit 1 + cat $tables | grep -A10 $table_name | tr '\n' ' ' | awk '{gsub("[\\[\\]]", " ", $0); gsub("[[:blank:]]+"," ",$0); print $0}' > ${table} + grep -qE "${table_regexp}" ${table} || atf_fail "Bad counters for table ${table_name}" + done; + + for state_regexp in \ + "${epair_tester}b tcp ${net_server_host_server}.* <- ${net_tester_host_tester}.* 6:4 pkts, 455:311 bytes, rule 4," \ + "${epair_server}a tcp ${net_server_host_tester}.* -> ${net_server_host_server}.* 6:4 pkts, 455:311 bytes, rule 6," \ + ; do + grep -qE "${state_regexp}" $states || atf_fail "State not found for '${state_regexp}'" + done + + for node_regexp in \ + "${net_tester_host_tester} -> :: .* 10 pkts, 766 bytes, filter rule 4, limit source-track"\ + ; do + grep -qE "${node_regexp}" $nodes || atf_fail "Source node not found for '${node_regexp}'" + done +} + +match_pass_state_cleanup() +{ + pft_cleanup +} + +atf_test_case "match_pass_no_state" "cleanup" +match_pass_no_state_head() +{ + atf_set descr 'Counters on match and pass rules without keep state' + atf_set require.user root +} + +match_pass_no_state_body() +{ + setup_router_server_ipv6 + + # Test counters for a stateless firewall. + # The table "tbl_in" is used both in match and pass rule in the inbound + # direction. The "In/Pass" counter is incremented twice. The table + # "tbl_inout" matches the same host on inbound and outbound direction. + # It will also be incremented twice. The tables "tbl_out_match" and + # "tbl_out_pass" will have their counters increased only once. + pft_set_rules router \ + "table <tbl_in> { ${net_tester_host_tester} }" \ + "table <tbl_inout> { ${net_tester_host_tester} }" \ + "table <tbl_out_match> { ${net_server_host_server} }" \ + "table <tbl_out_pass> { ${net_server_host_server} }" \ + "block" \ + "pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv }" \ + "match in on ${epair_tester}b inet6 proto tcp from <tbl_inout>" \ + "match in on ${epair_tester}b inet6 proto tcp from <tbl_in>" \ + "pass in on ${epair_tester}b inet6 proto tcp from <tbl_in> no state" \ + "pass out on ${epair_tester}b inet6 proto tcp to <tbl_in> no state" \ + "match in on ${epair_server}a inet6 proto tcp from <tbl_out_match>" \ + "pass in on ${epair_server}a inet6 proto tcp from <tbl_out_pass> no state" \ + "match out on ${epair_server}a inet6 proto tcp from <tbl_inout> no state" \ + "pass out on ${epair_server}a inet6 proto tcp to <tbl_out_pass> no state" + + # Use a real TCP connection so that it will be properly closed, guaranteeing the amount of packets. + atf_check -s exit:0 -o match:"This is a test" -x \ + "echo 'This is a test' | nc -w3 ${net_server_host_server} echo" + sleep 1 + get_counters + + for rule_regexp in \ + "@3 match in on ${epair_tester}b .* Packets: 6 Bytes: 455 " \ + "@4 match in on ${epair_tester}b .* Packets: 6 Bytes: 455 " \ + "@5 pass in on ${epair_tester}b .* Packets: 6 Bytes: 455 " \ + "@6 pass out on ${epair_tester}b .* Packets: 4 Bytes: 311 " \ + "@7 match in on ${epair_server}a .* Packets: 4 Bytes: 311 " \ + "@8 pass in on ${epair_server}a .* Packets: 4 Bytes: 311 " \ + "@10 pass out on ${epair_server}a .* Packets: 6 Bytes: 455 " \ + ; do + grep -qE "${rule_regexp}" $rules || atf_fail "Rule regexp not found for '${rule_regexp}'" + done + + for table_test in \ + "tbl_in___Evaluations: NoMatch: 0 Match: 16 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 12 Bytes: 910 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 4 Bytes: 311 Out/XPass: Packets: 0 Bytes: 0" \ + "tbl_out_match___Evaluations: NoMatch: 0 Match: 4 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 4 Bytes: 311 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 0 Bytes: 0 Out/XPass: Packets: 0 Bytes: 0" \ + "tbl_out_pass___Evaluations: NoMatch: 0 Match: 10 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 4 Bytes: 311 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 6 Bytes: 455 Out/XPass: Packets: 0 Bytes: 0" \ + "tbl_inout___Evaluations: NoMatch: 0 Match: 12 In/Block: Packets: 0 Bytes: 0 In/Pass: Packets: 6 Bytes: 455 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 6 Bytes: 455 Out/XPass: Packets: 0 Bytes: 0" \ + ; do + table_name=${table_test%%___*} + table_regexp=${table_test##*___} + table=$(mktemp) || exit 1 + cat $tables | grep -A10 $table_name | tr '\n' ' ' | awk '{gsub("[\\[\\]]", " ", $0); gsub("[[:blank:]]+"," ",$0); print $0}' > ${table} + grep -qE "${table_regexp}" ${table} || atf_fail "Bad counters for table ${table_name}" + done; +} + +match_pass_no_state_cleanup() +{ + pft_cleanup +} + +atf_test_case "match_block" "cleanup" +match_block_head() +{ + atf_set descr 'Counters on match and block rules' + atf_set require.user root +} + +match_block_body() +{ + setup_router_server_ipv6 + + # Stateful firewall with a blocking rule. The rule will have its + # counters increased because it matches and applies correctly. + # The "match" rule before the "pass" rule will have its counters + # increased for blocked traffic too. + pft_set_rules router \ + "set state-policy if-bound" \ + "table <tbl_in_match> { ${net_server_host_server} }" \ + "table <tbl_in_block> { ${net_server_host_server} }" \ + "block" \ + "pass inet6 proto icmp6 icmp6-type { neighbrsol, neighbradv }" \ + "match in on ${epair_tester}b inet6 proto tcp to <tbl_in_match> scrub (random-id)" \ + "block in on ${epair_tester}b inet6 proto tcp to <tbl_in_block>" \ + "pass out on ${epair_server}a inet6 proto tcp keep state" + + # Wait 3 seconds, that will cause 2 SYNs to be sent out. + echo 'This is a test' | nc -w3 ${net_server_host_server} echo + sleep 1 + get_counters + + for rule_regexp in \ + "@3 match in on ${epair_tester}b .* Packets: 2 Bytes: 160 States: 0 " \ + "@4 block drop in on ${epair_tester}b .* Packets: 2 Bytes: 160 States: 0 " \ + ; do + grep -qE "${rule_regexp}" $rules || atf_fail "Rule regexp not found for '${rule_regexp}'" + done + + # OpenBSD has (In|Out)/Match. We don't (yet) have it in FreeBSD + # so we follow the action of the "pass" rule ("block" for this test) + # in "match" rules. + for table_test in \ + "tbl_in_match___Evaluations: NoMatch: 0 Match: 2 In/Block: Packets: 2 Bytes: 160 In/Pass: Packets: 0 Bytes: 0 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 0 Bytes: 0 Out/XPass: Packets: 0 Bytes: 0" \ + "tbl_in_block___Evaluations: NoMatch: 0 Match: 2 In/Block: Packets: 2 Bytes: 160 In/Pass: Packets: 0 Bytes: 0 In/XPass: Packets: 0 Bytes: 0 Out/Block: Packets: 0 Bytes: 0 Out/Pass: Packets: 0 Bytes: 0 Out/XPass: Packets: 0 Bytes: 0" \ + ; do + table_name=${table_test%%___*} + table_regexp=${table_test##*___} + table=$(mktemp) || exit 1 + cat $tables | grep -A10 $table_name | tr '\n' ' ' | awk '{gsub("[\\[\\]]", " ", $0); gsub("[[:blank:]]+"," ",$0); print $0}' > ${table} + grep -qE "${table_regexp}" ${table} || atf_fail "Bad counters for table ${table_name}" + done; +} + +match_block_cleanup() +{ + pft_cleanup +} *** 693 LINES SKIPPED ***
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202509281725.58SHPiKL027396>