move allocations in DIOCSADDRULE and DIOCHANGERULE outside of locks.

this diff lets pf_rule_copyin() to be called outside of PF_LOCK()/NET_LOCK().

OK bluhm@

1 files changed, 186 insertions, 114 deletions

diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index a7e6641ae91..3766d749491 100644
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pf_ioctl.c,v 1.369 2021/12/26 14:04:29 sashan Exp $ */
+/*	$OpenBSD: pf_ioctl.c,v 1.370 2022/01/11 09:00:17 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -103,13 +103,12 @@ void			 pf_hash_rule_addr(MD5_CTX *, struct pf_rule_addr *);
 int			 pf_commit_rules(u_int32_t, char *);
 int			 pf_addr_setup(struct pf_ruleset *,
 			    struct pf_addr_wrap *, sa_family_t);
-int			 pf_kif_setup(char *, struct pfi_kif **);
+struct pfi_kif		*pf_kif_setup(struct pfi_kif *);
 void			 pf_addr_copyout(struct pf_addr_wrap *);
 void			 pf_trans_set_commit(void);
 void			 pf_pool_copyin(struct pf_pool *, struct pf_pool *);
 int			 pf_validate_range(u_int8_t, u_int16_t[2]);
-int			 pf_rule_copyin(struct pf_rule *, struct pf_rule *,
-			    struct pf_ruleset *);
+int			 pf_rule_copyin(struct pf_rule *, struct pf_rule *);
 u_int16_t		 pf_qname2qid(char *, int);
 void			 pf_qid2qname(u_int16_t, char *);
 void			 pf_qid_unref(u_int16_t);
@@ -269,6 +268,21 @@ pfclose(dev_t dev, int flags, int fmt, struct proc *p)
 }
 
 void
+pf_rule_free(struct pf_rule *rule)
+{
+	if (rule == NULL)
+		return;
+
+	pfi_kif_free(rule->kif);
+	pfi_kif_free(rule->rcv_kif);
+	pfi_kif_free(rule->rdr.kif);
+	pfi_kif_free(rule->nat.kif);
+	pfi_kif_free(rule->route.kif);
+
+	pool_put(&pf_rule_pl, rule);
+}
+
+void
 pf_rm_rule(struct pf_rulequeue *rulequeue, struct pf_rule *rule)
 {
 	if (rulequeue != NULL) {
@@ -880,19 +894,22 @@ pf_addr_setup(struct pf_ruleset *ruleset, struct pf_addr_wrap *addr,
 	return (0);
 }
 
-int
-pf_kif_setup(char *ifname, struct pfi_kif **kif)
+struct pfi_kif *
+pf_kif_setup(struct pfi_kif *kif_buf)
 {
-	if (ifname[0]) {
-		*kif = pfi_kif_get(ifname, NULL);
-		if (*kif == NULL)
-			return (EINVAL);
+	struct pfi_kif *kif;
 
-		pfi_kif_ref(*kif, PFI_KIF_REF_RULE);
-	} else
-		*kif = NULL;
+	if (kif_buf == NULL)
+		return (NULL);
 
-	return (0);
+	KASSERT(kif_buf->pfik_name[0] != '\0');
+
+	kif = pfi_kif_get(kif_buf->pfik_name, &kif_buf);
+	if (kif_buf != NULL)
+		pfi_kif_free(kif_buf);
+	pfi_kif_ref(kif, PFI_KIF_REF_RULE);
+
+	return (kif);
 }
 
 void
@@ -1318,41 +1335,18 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			break;
 		}
 
-		NET_LOCK();
-		PF_LOCK();
-		pr->anchor[sizeof(pr->anchor) - 1] = '\0';
-		ruleset = pf_find_ruleset(pr->anchor);
-		if (ruleset == NULL) {
-			error = EINVAL;
-			PF_UNLOCK();
-			NET_UNLOCK();
-			pool_put(&pf_rule_pl, rule);
+		if ((error = pf_rule_copyin(&pr->rule, rule))) {
+			pf_rule_free(rule);
+			rule = NULL;
 			break;
 		}
+
 		if (pr->rule.return_icmp >> 8 > ICMP_MAXTYPE) {
 			error = EINVAL;
-			PF_UNLOCK();
-			NET_UNLOCK();
-			pool_put(&pf_rule_pl, rule);
-			break;
-		}
-		if (pr->ticket != ruleset->rules.inactive.ticket) {
-			error = EBUSY;
-			PF_UNLOCK();
-			NET_UNLOCK();
-			pool_put(&pf_rule_pl, rule);
-			break;
-		}
-		if ((error = pf_rule_copyin(&pr->rule, rule, ruleset))) {
-			pf_rm_rule(NULL, rule);
+			pf_rule_free(rule);
 			rule = NULL;
-			PF_UNLOCK();
-			NET_UNLOCK();
 			break;
 		}
-		rule->cuid = p->p_ucred->cr_ruid;
-		rule->cpid = p->p_p->ps_pid;
-
 		switch (rule->af) {
 		case 0:
 			break;
@@ -1363,13 +1357,57 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			break;
 #endif /* INET6 */
 		default:
-			pf_rm_rule(NULL, rule);
+			pf_rule_free(rule);
 			rule = NULL;
 			error = EAFNOSUPPORT;
+			goto fail;
+		}
+
+		if (rule->src.addr.type == PF_ADDR_NONE ||
+		    rule->dst.addr.type == PF_ADDR_NONE) {
+			error = EINVAL;
+			pf_rule_free(rule);
+			rule = NULL;
+			break;
+		}
+
+		if (rule->rt && !rule->direction) {
+			error = EINVAL;
+			pf_rule_free(rule);
+			rule = NULL;
+			break;
+		}
+
+		if (rule->scrub_flags & PFSTATE_SETPRIO &&
+		    (rule->set_prio[0] > IFQ_MAXPRIO ||
+		    rule->set_prio[1] > IFQ_MAXPRIO)) {
+			error = EINVAL;
+			pf_rule_free(rule);
+			rule = NULL;
+			break;
+		}
+
+		NET_LOCK();
+		PF_LOCK();
+		pr->anchor[sizeof(pr->anchor) - 1] = '\0';
+		ruleset = pf_find_ruleset(pr->anchor);
+		if (ruleset == NULL) {
+			error = EINVAL;
 			PF_UNLOCK();
 			NET_UNLOCK();
-			goto fail;
+			pf_rule_free(rule);
+			break;
+		}
+		if (pr->ticket != ruleset->rules.inactive.ticket) {
+			error = EBUSY;
+			PF_UNLOCK();
+			NET_UNLOCK();
+			pf_rule_free(rule);
+			break;
 		}
+		rule->cuid = p->p_ucred->cr_ruid;
+		rule->cpid = p->p_p->ps_pid;
+
 		tail = TAILQ_LAST(ruleset->rules.inactive.ptr,
 		    pf_rulequeue);
 		if (tail)
@@ -1377,9 +1415,19 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 		else
 			rule->nr = 0;
 
-		if (rule->src.addr.type == PF_ADDR_NONE ||
-		    rule->dst.addr.type == PF_ADDR_NONE)
-			error = EINVAL;
+		rule->kif = pf_kif_setup(rule->kif);
+		rule->rcv_kif = pf_kif_setup(rule->rcv_kif);
+		rule->rdr.kif = pf_kif_setup(rule->rdr.kif);
+		rule->nat.kif = pf_kif_setup(rule->nat.kif);
+		rule->route.kif = pf_kif_setup(rule->route.kif);
+
+		if (rule->overload_tblname[0]) {
+			if ((rule->overload_tbl = pfr_attach_table(ruleset,
+			    rule->overload_tblname, 0)) == NULL)
+				error = EINVAL;
+			else
+				rule->overload_tbl->pfrkt_flags |= PFR_TFLAG_ACTIVE;
+		}
 
 		if (pf_addr_setup(ruleset, &rule->src.addr, rule->af))
 			error = EINVAL;
@@ -1393,12 +1441,6 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			error = EINVAL;
 		if (pf_anchor_setup(rule, ruleset, pr->anchor_call))
 			error = EINVAL;
-		if (rule->rt && !rule->direction)
-			error = EINVAL;
-		if (rule->scrub_flags & PFSTATE_SETPRIO &&
-		    (rule->set_prio[0] > IFQ_MAXPRIO ||
-		    rule->set_prio[1] > IFQ_MAXPRIO))
-			error = EINVAL;
 
 		if (error) {
 			pf_rm_rule(NULL, rule);
@@ -1525,51 +1567,40 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 			break;
 		}
 
-		newrule = pool_get(&pf_rule_pl, PR_WAITOK|PR_LIMITFAIL|PR_ZERO);
-		if (newrule == NULL) {
-			error = ENOMEM;
-			break;
-		}
+		if (pcr->action == PF_CHANGE_GET_TICKET) {
+			NET_LOCK();
+			PF_LOCK();
+
+			ruleset = pf_find_ruleset(pcr->anchor);
+			if (ruleset == NULL)
+				error = EINVAL;
+			else
+				pcr->ticket = ++ruleset->rules.active.ticket;
 
-		NET_LOCK();
-		PF_LOCK();
-		ruleset = pf_find_ruleset(pcr->anchor);
-		if (ruleset == NULL) {
-			error = EINVAL;
 			PF_UNLOCK();
 			NET_UNLOCK();
-			pool_put(&pf_rule_pl, newrule);
 			break;
 		}
 
-		if (pcr->action == PF_CHANGE_GET_TICKET) {
-			pcr->ticket = ++ruleset->rules.active.ticket;
-			PF_UNLOCK();
-			NET_UNLOCK();
-			pool_put(&pf_rule_pl, newrule);
-			break;
-		} else {
-			if (pcr->ticket !=
-			    ruleset->rules.active.ticket) {
-				error = EINVAL;
-				PF_UNLOCK();
-				NET_UNLOCK();
-				pool_put(&pf_rule_pl, newrule);
+		if (pcr->action != PF_CHANGE_REMOVE) {
+			newrule = pool_get(&pf_rule_pl,
+			    PR_WAITOK|PR_LIMITFAIL|PR_ZERO);
+			if (newrule == NULL) {
+				error = ENOMEM;
 				break;
 			}
+
 			if (pcr->rule.return_icmp >> 8 > ICMP_MAXTYPE) {
 				error = EINVAL;
-				PF_UNLOCK();
-				NET_UNLOCK();
 				pool_put(&pf_rule_pl, newrule);
 				break;
 			}
-		}
-
-		if (pcr->action != PF_CHANGE_REMOVE) {
-			pf_rule_copyin(&pcr->rule, newrule, ruleset);
-			newrule->cuid = p->p_ucred->cr_ruid;
-			newrule->cpid = p->p_p->ps_pid;
+			error = pf_rule_copyin(&pcr->rule, newrule);
+			if (error != 0) {
+				pf_rule_free(newrule);
+				newrule = NULL;
+				break;
+			}
 
 			switch (newrule->af) {
 			case 0:
@@ -1581,24 +1612,74 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
 				break;
 #endif /* INET6 */
 			default:
-				pf_rm_rule(NULL, newrule);
 				error = EAFNOSUPPORT;
-				PF_UNLOCK();
-				NET_UNLOCK();
+				pf_rule_free(newrule);
+				newrule = NULL;
 				goto fail;
 			}
 
-			if (newrule->rt && !newrule->direction)
+			if (newrule->rt && !newrule->direction) {
+				pf_rule_free(newrule);
 				error = EINVAL;
-			if (pf_addr_setup(ruleset, &newrule->src.addr, newrule->af))
+				newrule = NULL;
+				break;
+			}
+		}
+
+		NET_LOCK();
+		PF_LOCK();
+		ruleset = pf_find_ruleset(pcr->anchor);
+		if (ruleset == NULL) {
+			error = EINVAL;
+			PF_UNLOCK();
+			NET_UNLOCK();
+			pf_rule_free(newrule);
+			break;
+		}
+
+		if (pcr->ticket != ruleset->rules.active.ticket) {
+			error = EINVAL;
+			PF_UNLOCK();
+			NET_UNLOCK();
+			pf_rule_free(newrule);
+			break;
+		}
+
+		if (pcr->action != PF_CHANGE_REMOVE) {
+			KASSERT(newrule != NULL);
+			newrule->cuid = p->p_ucred->cr_ruid;
+			newrule->cpid = p->p_p->ps_pid;
+
+			newrule->kif = pf_kif_setup(newrule->kif);
+			newrule->rcv_kif = pf_kif_setup(newrule->rcv_kif);
+			newrule->rdr.kif = pf_kif_setup(newrule->rdr.kif);
+			newrule->nat.kif = pf_kif_setup(newrule->nat.kif);
+			newrule->route.kif = pf_kif_setup(newrule->route.kif);
+
+			if (newrule->overload_tblname[0]) {
+				newrule->overload_tbl = pfr_attach_table(
+				    ruleset, newrule->overload_tblname, 0);
+				if (newrule->overload_tbl == NULL)
+					error = EINVAL;
+				else
+					newrule->overload_tbl->pfrkt_flags |=
+					    PFR_TFLAG_ACTIVE;
+			}
+
+			if (pf_addr_setup(ruleset, &newrule->src.addr,
+			    newrule->af))
 				error = EINVAL;
-			if (pf_addr_setup(ruleset, &newrule->dst.addr, newrule->af))
+			if (pf_addr_setup(ruleset, &newrule->dst.addr,
+			    newrule->af))
 				error = EINVAL;
-			if (pf_addr_setup(ruleset, &newrule->rdr.addr, newrule->af))
+			if (pf_addr_setup(ruleset, &newrule->rdr.addr,
+			    newrule->af))
 				error = EINVAL;
-			if (pf_addr_setup(ruleset, &newrule->nat.addr, newrule->af))
+			if (pf_addr_setup(ruleset, &newrule->nat.addr,
+			    newrule->af))
 				error = EINVAL;
-			if (pf_addr_setup(ruleset, &newrule->route.addr, newrule->af))
+			if (pf_addr_setup(ruleset, &newrule->route.addr,
+			    newrule->af))
 				error = EINVAL;
 			if (pf_anchor_setup(newrule, ruleset, pcr->anchor_call))
 				error = EINVAL;
@@ -3006,8 +3087,7 @@ pf_validate_range(u_int8_t op, u_int16_t port[2])
 }
 
 int
-pf_rule_copyin(struct pf_rule *from, struct pf_rule *to,
-    struct pf_ruleset *ruleset)
+pf_rule_copyin(struct pf_rule *from, struct pf_rule *to)
 {
 	int i;
 
@@ -3041,24 +3121,16 @@ pf_rule_copyin(struct pf_rule *from, struct pf_rule *to,
 	if (pf_validate_range(to->rdr.port_op, to->rdr.proxy_port))
 		return (EINVAL);
 
-	if (pf_kif_setup(to->ifname, &to->kif))
-		return (EINVAL);
-	if (pf_kif_setup(to->rcv_ifname, &to->rcv_kif))
-		return (EINVAL);
-	if (to->overload_tblname[0]) {
-		if ((to->overload_tbl = pfr_attach_table(ruleset,
-		    to->overload_tblname, 0)) == NULL)
-			return (EINVAL);
-		else
-			to->overload_tbl->pfrkt_flags |= PFR_TFLAG_ACTIVE;
-	}
-
-	if (pf_kif_setup(to->rdr.ifname, &to->rdr.kif))
-		return (EINVAL);
-	if (pf_kif_setup(to->nat.ifname, &to->nat.kif))
-		return (EINVAL);
-	if (pf_kif_setup(to->route.ifname, &to->route.kif))
-		return (EINVAL);
+	to->kif = (to->ifname[0]) ?
+	    pfi_kif_alloc(to->ifname, M_WAITOK) : NULL;
+	to->rcv_kif = (to->rcv_ifname[0]) ?
+	    pfi_kif_alloc(to->rcv_ifname, M_WAITOK) : NULL;
+	to->rdr.kif = (to->rdr.ifname[0]) ?
+	    pfi_kif_alloc(to->rdr.ifname, M_WAITOK) : NULL;
+	to->nat.kif = (to->nat.ifname[0]) ?
+	    pfi_kif_alloc(to->nat.ifname, M_WAITOK) : NULL;
+	to->route.kif = (to->route.ifname[0]) ?
+	    pfi_kif_alloc(to->route.ifname, M_WAITOK) : NULL;
 
 	to->os_fingerprint = from->os_fingerprint;