The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=a0e4c65f1814a7a677364dc29bb703f84323d175
commit a0e4c65f1814a7a677364dc29bb703f84323d175 Author: Kristof Provost <[email protected]> AuthorDate: 2026-04-29 15:04:44 +0000 Commit: Kristof Provost <[email protected]> CommitDate: 2026-05-05 20:20:42 +0000 pf: do not reject rules with colliding hashes We insert rules in pf_krule_global solely for the benefit of the 'keepcounters' feature. Failing to insert (beause the rule hash collides, or an identical rule already exists) would be worse than restoring counts to the wrong rule (or failing to restore them at all). PR: 282863, 294860, 294859, 294858 MFC after: 3 days Sponsored by: Rubicon Communications, LLC ("Netgate") Differential Revision: https://reviews.freebsd.org/D56745 --- sys/netpfil/pf/pf_ioctl.c | 24 ++++++++---------------- tests/sys/netpfil/pf/match.sh | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index d3e60b137c1a..ab2140a60ce7 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -3226,14 +3226,12 @@ pf_ioctl_addrule(struct pf_krule *rule, uint32_t ticket, PF_RULES_WUNLOCK(); pf_hash_rule(rule); - if (RB_INSERT(pf_krule_global, ruleset->rules[rs_num].inactive.tree, rule) != NULL) { - PF_RULES_WLOCK(); - TAILQ_REMOVE(ruleset->rules[rs_num].inactive.ptr, rule, entries); - ruleset->rules[rs_num].inactive.rcount--; - pf_free_rule(rule); - rule = NULL; - ERROUT(EEXIST); - } + /** + * Note: rule hashes may collide. Accept this, because the worst that can + * happen is that we get counter preservation wrong. + * Failing to insert here would be worse. + **/ + RB_INSERT(pf_krule_global, ruleset->rules[rs_num].inactive.tree, rule); PF_CONFIG_UNLOCK(); return (0); @@ -4895,14 +4893,8 @@ DIOCGETRULENV_error: ruleset->rules[rs_num].active.rcount--; } else { pf_hash_rule(newrule); - if (RB_INSERT(pf_krule_global, - ruleset->rules[rs_num].active.tree, newrule) != NULL) { - pf_free_rule(newrule); - PF_RULES_WUNLOCK(); - PF_CONFIG_UNLOCK(); - error = EEXIST; - goto fail; - } + RB_INSERT(pf_krule_global, + ruleset->rules[rs_num].active.tree, newrule); if (oldrule == NULL) TAILQ_INSERT_TAIL( diff --git a/tests/sys/netpfil/pf/match.sh b/tests/sys/netpfil/pf/match.sh index 992e32d9f887..c732ec7c5c17 100644 --- a/tests/sys/netpfil/pf/match.sh +++ b/tests/sys/netpfil/pf/match.sh @@ -234,10 +234,46 @@ double_match_cleanup() pft_cleanup } +atf_test_case "duplicate_rules" "cleanup" +duplicate_rules_head() +{ + atf_set descr 'Test identical rules' + atf_set require.user root +} + +duplicate_rules_body() +{ + pft_init + + epair=$(vnet_mkepair) + vnet_mkjail alcatraz ${epair}b + + ifconfig ${epair}a 192.0.2.1/24 up + jexec alcatraz ifconfig ${epair}b 192.0.2.2/24 up + + # Sanity check + atf_check -s exit:0 -o ignore ping -c 1 192.0.2.2 + + jexec alcatraz pfctl -e + pft_set_rules alcatraz \ + "block" \ + "pass tagged FOO" \ + "match tag FOO" \ + "pass tagged FOO" + + atf_check -s exit:0 -o ignore ping -c 3 192.0.2.2 +} + +duplicate_rules_cleanup() +{ + pft_cleanup +} + atf_init_test_cases() { atf_add_test_case "dummynet" atf_add_test_case "quick" atf_add_test_case "allow_opts" atf_add_test_case "double_match" + atf_add_test_case "duplicate_rules" }
