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

@@ -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 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
 	}
 
 	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 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
 		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 @@ pfioctl(struct cdev *dev, u_long cmd, caddr_t addr, int flags, struct thread *td
 
 		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);

sys/netpfil/pf/pf_nv.c

@@ -684,7 +684,7 @@ pf_divert_to_nvdivert(const struct pf_krule *rule)
 }
 
 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;

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 *);