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@

diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index a7e6641..3766d74 100644 (file)
--- 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);
@@ -268,6 +267,21 @@ pfclose(dev_t dev, int flags, int fmt, struct proc *p)
        return (0);
 }
 
+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)
 {
@@ -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;