The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=ae96ff302f8ae50903a96d3a1857f9acf243f3c4
commit ae96ff302f8ae50903a96d3a1857f9acf243f3c4 Author: Mark Johnston <[email protected]> AuthorDate: 2025-12-19 14:07:26 +0000 Commit: Mark Johnston <[email protected]> CommitDate: 2025-12-19 14:07:26 +0000 pf: Avoid taking the pf rules write lock in a couple of ioctls The DIOCGETRULES ioctl handlers has taken the write lock ever since fine-grained locking was merged to pf, but I believe it's unneeded. Use the read lock instead. DIOCGETRULENV takes the write lock as well but I believe this is only required when clearing rule counters. Acquire the read lock if that is not the case. Reviewed by: kp, allanjude MFC after: 2 weeks Sponsored by: OPNsense Sponsored by: Klara, Inc. Differential Revision: https://reviews.freebsd.org/D54292 --- sys/netpfil/pf/pf_ioctl.c | 91 ++++++++++++++++++++++------------------------- sys/netpfil/pf/pf_nv.c | 2 +- sys/netpfil/pf/pf_nv.h | 2 +- 3 files changed, 45 insertions(+), 50 deletions(-) diff --git a/sys/netpfil/pf/pf_ioctl.c b/sys/netpfil/pf/pf_ioctl.c index 9856842c72b2..ca1815984797 100644 --- a/sys/netpfil/pf/pf_ioctl.c +++ b/sys/netpfil/pf/pf_ioctl.c @@ -2087,19 +2087,20 @@ pf_rule_to_krule(const struct pf_rule *rule, struct pf_krule *krule) int pf_ioctl_getrules(struct pfioc_rule *pr) { + PF_RULES_RLOCK_TRACKER; struct pf_kruleset *ruleset; struct pf_krule *tail; int rs_num; - PF_RULES_WLOCK(); + PF_RULES_RLOCK(); ruleset = pf_find_kruleset(pr->anchor); if (ruleset == NULL) { - PF_RULES_WUNLOCK(); + PF_RULES_RUNLOCK(); return (EINVAL); } rs_num = pf_get_ruleset_number(pr->rule.action); if (rs_num >= PF_RULESET_MAX) { - PF_RULES_WUNLOCK(); + PF_RULES_RUNLOCK(); return (EINVAL); } tail = TAILQ_LAST(ruleset->rules[rs_num].active.ptr, @@ -2109,7 +2110,7 @@ pf_ioctl_getrules(struct pfioc_rule *pr) else pr->nr = 0; pr->ticket = ruleset->rules[rs_num].active.ticket; - PF_RULES_WUNLOCK(); + PF_RULES_RUNLOCK(); return (0); } @@ -3692,6 +3693,7 @@ DIOCADDRULENV_error: } case DIOCGETRULENV: { + PF_RULES_RLOCK_TRACKER; struct pfioc_nv *nv = (struct pfioc_nv *)addr; nvlist_t *nvrule = NULL; nvlist_t *nvl = NULL; @@ -3702,6 +3704,13 @@ DIOCADDRULENV_error: bool clear_counter = false; #define ERROUT(x) ERROUT_IOCTL(DIOCGETRULENV_error, x) +#define ERROUT_LOCKED(x) do { \ + if (clear_counter) \ + PF_RULES_WUNLOCK(); \ + else \ + PF_RULES_RUNLOCK(); \ + ERROUT(x); \ +} while (0) if (nv->len > pf_ioctl_maxcount) ERROUT(ENOMEM); @@ -3733,78 +3742,64 @@ DIOCADDRULENV_error: nr = nvlist_get_number(nvl, "nr"); - PF_RULES_WLOCK(); + if (clear_counter) + PF_RULES_WLOCK(); + else + PF_RULES_RLOCK(); ruleset = pf_find_kruleset(nvlist_get_string(nvl, "anchor")); - if (ruleset == NULL) { - PF_RULES_WUNLOCK(); - ERROUT(ENOENT); - } + if (ruleset == NULL) + ERROUT_LOCKED(ENOENT); rs_num = pf_get_ruleset_number(nvlist_get_number(nvl, "ruleset")); - if (rs_num >= PF_RULESET_MAX) { - PF_RULES_WUNLOCK(); - ERROUT(EINVAL); - } + if (rs_num >= PF_RULESET_MAX) + ERROUT_LOCKED(EINVAL); if (nvlist_get_number(nvl, "ticket") != - ruleset->rules[rs_num].active.ticket) { - PF_RULES_WUNLOCK(); - ERROUT(EBUSY); - } + ruleset->rules[rs_num].active.ticket) + ERROUT_LOCKED(EBUSY); - if ((error = nvlist_error(nvl))) { - PF_RULES_WUNLOCK(); - ERROUT(error); - } + if ((error = nvlist_error(nvl))) + ERROUT_LOCKED(error); rule = TAILQ_FIRST(ruleset->rules[rs_num].active.ptr); while ((rule != NULL) && (rule->nr != nr)) rule = TAILQ_NEXT(rule, entries); - if (rule == NULL) { - PF_RULES_WUNLOCK(); - ERROUT(EBUSY); - } + if (rule == NULL) + ERROUT_LOCKED(EBUSY); nvrule = pf_krule_to_nvrule(rule); nvlist_destroy(nvl); nvl = nvlist_create(0); - if (nvl == NULL) { - PF_RULES_WUNLOCK(); - ERROUT(ENOMEM); - } + if (nvl == NULL) + ERROUT_LOCKED(ENOMEM); nvlist_add_number(nvl, "nr", nr); nvlist_add_nvlist(nvl, "rule", nvrule); nvlist_destroy(nvrule); nvrule = NULL; - if (pf_kanchor_nvcopyout(ruleset, rule, nvl)) { - PF_RULES_WUNLOCK(); - ERROUT(EBUSY); - } + if (pf_kanchor_nvcopyout(ruleset, rule, nvl)) + ERROUT_LOCKED(EBUSY); free(nvlpacked, M_NVLIST); nvlpacked = nvlist_pack(nvl, &nv->len); - if (nvlpacked == NULL) { - PF_RULES_WUNLOCK(); - ERROUT(ENOMEM); - } + if (nvlpacked == NULL) + ERROUT_LOCKED(ENOMEM); - if (nv->size == 0) { - PF_RULES_WUNLOCK(); - ERROUT(0); - } - else if (nv->size < nv->len) { - PF_RULES_WUNLOCK(); - ERROUT(ENOSPC); - } + if (nv->size == 0) + ERROUT_LOCKED(0); + else if (nv->size < nv->len) + ERROUT_LOCKED(ENOSPC); - if (clear_counter) + if (clear_counter) { pf_krule_clear_counters(rule); - - PF_RULES_WUNLOCK(); + PF_RULES_WUNLOCK(); + } else { + PF_RULES_RUNLOCK(); + } error = copyout(nvlpacked, nv->data, nv->len); +#undef ERROUT_LOCKED #undef ERROUT DIOCGETRULENV_error: free(nvlpacked, M_NVLIST); diff --git a/sys/netpfil/pf/pf_nv.c b/sys/netpfil/pf/pf_nv.c index 2f484e2dabc6..3e741dd39974 100644 --- a/sys/netpfil/pf/pf_nv.c +++ b/sys/netpfil/pf/pf_nv.c @@ -684,7 +684,7 @@ error: } nvlist_t * -pf_krule_to_nvrule(struct pf_krule *rule) +pf_krule_to_nvrule(const struct pf_krule *rule) { nvlist_t *nvl, *tmp; u_int64_t src_nodes_total = 0; diff --git a/sys/netpfil/pf/pf_nv.h b/sys/netpfil/pf/pf_nv.h index cf9fbf8bcf5b..9e43ff1e642a 100644 --- a/sys/netpfil/pf/pf_nv.h +++ b/sys/netpfil/pf/pf_nv.h @@ -78,7 +78,7 @@ int pf_nvstring(const nvlist_t *, const char *, char *, size_t); int pf_check_rule_addr(const struct pf_rule_addr *); -nvlist_t *pf_krule_to_nvrule(struct pf_krule *); +nvlist_t *pf_krule_to_nvrule(const struct pf_krule *); int pf_nvrule_to_krule(const nvlist_t *, struct pf_krule *); int pf_nvstate_kill_to_kstate_kill(const nvlist_t *, struct pf_kstate_kill *);
