move memory allocations in pfr_add_addrs() outside of NET_LOCK()/PF_LOCK()

scope.

feedback by bluhm@

OK bluhm@

diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
index e960f77..d61face 100644 (file)
--- a/sys/net/pf_ioctl.c
+++ b/sys/net/pf_ioctl.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf_ioctl.c,v 1.366 2021/11/11 12:35:01 sashan Exp $ */
+/*     $OpenBSD: pf_ioctl.c,v 1.367 2021/11/16 20:51:30 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -2270,13 +2270,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p)
                        error = ENODEV;
                        break;
                }
-               NET_LOCK();
-               PF_LOCK();
                error = pfr_add_addrs(&io->pfrio_table, io->pfrio_buffer,
                    io->pfrio_size, &io->pfrio_nadd, io->pfrio_flags |
                    PFR_FLAG_USERIOCTL);
-               PF_UNLOCK();
-               NET_UNLOCK();
                break;
        }
 

diff --git a/sys/net/pf_table.c b/sys/net/pf_table.c
index 3a136a8..3f895b4 100644 (file)
--- a/sys/net/pf_table.c
+++ b/sys/net/pf_table.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pf_table.c,v 1.137 2021/11/11 12:35:01 sashan Exp $   */
+/*     $OpenBSD: pf_table.c,v 1.138 2021/11/16 20:51:31 sashan Exp $   */
 
 /*
  * Copyright (c) 2002 Cedric Berger
@@ -155,8 +155,13 @@ void                        pfr_enqueue_addrs(struct pfr_ktable *,
 void                    pfr_mark_addrs(struct pfr_ktable *);
 struct pfr_kentry      *pfr_lookup_addr(struct pfr_ktable *,
                            struct pfr_addr *, int);
+struct pfr_kentry      *pfr_lookup_kentry(struct pfr_ktable *,
+                           struct pfr_kentry *, int);
 struct pfr_kentry      *pfr_create_kentry(struct pfr_addr *);
+struct pfr_kentry      *pfr_create_kentry_unlocked(struct pfr_addr *, int);
+void                    pfr_kentry_kif_ref(struct pfr_kentry *);
 void                    pfr_destroy_kentries(struct pfr_kentryworkq *);
+void                    pfr_destroy_ioq(struct pfr_kentryworkq *, int);
 void                    pfr_destroy_kentry(struct pfr_kentry *);
 void                    pfr_insert_kentries(struct pfr_ktable *,
                            struct pfr_kentryworkq *, time_t);
@@ -265,13 +270,50 @@ pfr_clr_addrs(struct pfr_table *tbl, int *ndel, int flags)
        return (0);
 }
 
+void
+pfr_fill_feedback(struct pfr_kentry_all *ke, struct pfr_addr *ad)
+{
+       ad->pfra_type = ke->pfrke_type;
+
+       switch (ke->pfrke_type) {
+       case PFRKE_PLAIN:
+               break;
+       case PFRKE_COST:
+               ((struct pfr_kentry_cost *)ke)->weight = ad->pfra_weight;
+               /* FALLTHROUGH */
+       case PFRKE_ROUTE:
+               if (ke->pfrke_rifname[0])
+                       strlcpy(ad->pfra_ifname, ke->pfrke_rifname, IFNAMSIZ);
+               break;
+       }
+
+       switch (ke->pfrke_af) {
+       case AF_INET:
+               ad->pfra_ip4addr = ke->pfrke_sa.sin.sin_addr;
+               break;
+#ifdef INET6
+       case AF_INET6:
+               ad->pfra_ip6addr = ke->pfrke_sa.sin6.sin6_addr;
+               break;
+#endif /* INET6 */
+       default:
+               unhandled_af(ke->pfrke_af);
+       }
+       ad->pfra_weight = ((struct pfr_kentry_cost *)ke)->weight;
+       ad->pfra_af = ke->pfrke_af;
+       ad->pfra_net = ke->pfrke_net;
+       if (ke->pfrke_flags & PFRKE_FLAG_NOT)
+               ad->pfra_not = 1;
+       ad->pfra_fback = ke->pfrke_fb;
+}
+
 int
 pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
     int *nadd, int flags)
 {
        struct pfr_ktable       *kt, *tmpkt;
-       struct pfr_kentryworkq   workq;
-       struct pfr_kentry       *p, *q;
+       struct pfr_kentryworkq   workq, ioq;
+       struct pfr_kentry       *p, *q, *ke;
        struct pfr_addr          ad;
        int                      i, rv, xadd = 0;
        time_t                   tzero = gettime();
@@ -279,9 +321,6 @@ pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
        ACCEPT_FLAGS(flags, PFR_FLAG_DUMMY | PFR_FLAG_FEEDBACK);
        if (pfr_validate_table(tbl, 0, flags & PFR_FLAG_USERIOCTL))
                return (EINVAL);
-       kt = pfr_lookup_table(tbl);
-       if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE))
-               return (ESRCH);
        if (kt->pfrkt_flags & PFR_TFLAG_CONST)
                return (EPERM);
        tmpkt = pfr_create_ktable(&pfr_nulltable, 0, 0,
@@ -289,53 +328,97 @@ pfr_add_addrs(struct pfr_table *tbl, struct pfr_addr *addr, int size,
        if (tmpkt == NULL)
                return (ENOMEM);
        SLIST_INIT(&workq);
+       SLIST_INIT(&ioq);
        for (i = 0; i < size; i++) {
                YIELD(flags & PFR_FLAG_USERIOCTL);
                if (COPYIN(addr+i, &ad, sizeof(ad), flags))
                        senderr(EFAULT);
                if (pfr_validate_addr(&ad))
                        senderr(EINVAL);
-               p = pfr_lookup_addr(kt, &ad, 1);
-               q = pfr_lookup_addr(tmpkt, &ad, 1);
+
+               ke = pfr_create_kentry_unlocked(&ad, flags);
+               if (ke == NULL)
+                       senderr(ENOMEM);
+               ke->pfrke_fb = PFR_FB_NONE;
+               SLIST_INSERT_HEAD(&ioq, ke, pfrke_ioq);
+       }
+
+       NET_LOCK();
+       PF_LOCK();
+       kt = pfr_lookup_table(tbl);
+       if (kt == NULL || !(kt->pfrkt_flags & PFR_TFLAG_ACTIVE)) {
+               PF_UNLOCK();
+               NET_UNLOCK();
+               senderr(ESRCH);
+       }
+       SLIST_FOREACH(ke, &ioq, pfrke_ioq) {
+               pfr_kentry_kif_ref(ke);
+               p = pfr_lookup_kentry(kt, ke, 1);
+               q = pfr_lookup_kentry(tmpkt, ke, 1);
                if (flags & PFR_FLAG_FEEDBACK) {
                        if (q != NULL)
-                               ad.pfra_fback = PFR_FB_DUPLICATE;
+                               ke->pfrke_fb = PFR_FB_DUPLICATE;
                        else if (p == NULL)
-                               ad.pfra_fback = PFR_FB_ADDED;
+                               ke->pfrke_fb = PFR_FB_ADDED;
                        else if ((p->pfrke_flags & PFRKE_FLAG_NOT) !=
-                           ad.pfra_not)
-                               ad.pfra_fback = PFR_FB_CONFLICT;
+                           (ke->pfrke_flags & PFRKE_FLAG_NOT))
+                               ke->pfrke_fb = PFR_FB_CONFLICT;
                        else
-                               ad.pfra_fback = PFR_FB_NONE;
+                               ke->pfrke_fb = PFR_FB_NONE;
                }
                if (p == NULL && q == NULL) {
-                       p = pfr_create_kentry(&ad);
-                       if (p == NULL)
-                               senderr(ENOMEM);
-                       if (pfr_route_kentry(tmpkt, p)) {
-                               pfr_destroy_kentry(p);
-                               ad.pfra_fback = PFR_FB_NONE;
+                       if (pfr_route_kentry(tmpkt, ke)) {
+                               /* defer destroy after feedback is processed */
+                               ke->pfrke_fb = PFR_FB_NONE;
                        } else {
-                               SLIST_INSERT_HEAD(&workq, p, pfrke_workq);
+                               /*
+                                * mark entry as added to table, so we won't
+                                * kill it with rest of the ioq
+                                */
+                               ke->pfrke_fb = PFR_FB_ADDED;
+                               SLIST_INSERT_HEAD(&workq, ke, pfrke_workq);
                                xadd++;
                        }
                }
-               if (flags & PFR_FLAG_FEEDBACK)
-                       if (COPYOUT(&ad, addr+i, sizeof(ad), flags))
-                               senderr(EFAULT);
        }
+       /* remove entries, which we will insert from tmpkt */
        pfr_clean_node_mask(tmpkt, &workq);
-       if (!(flags & PFR_FLAG_DUMMY)) {
+       if (!(flags & PFR_FLAG_DUMMY))
                pfr_insert_kentries(kt, &workq, tzero);
+
+       PF_UNLOCK();
+       NET_UNLOCK();
+
+       if (flags & PFR_FLAG_FEEDBACK) {
+               i = 0;
+               while ((ke = SLIST_FIRST(&ioq)) != NULL) {
+                       YIELD(flags & PFR_FLAG_USERIOCTL);
+                       pfr_fill_feedback((struct pfr_kentry_all *)ke, &ad);
+                       if (COPYOUT(&ad, addr+i, sizeof(ad), flags))
+                               senderr(EFAULT);
+                       i++;
+                       SLIST_REMOVE_HEAD(&ioq, pfrke_ioq);
+                       switch (ke->pfrke_fb) {
+                       case PFR_FB_CONFLICT:
+                       case PFR_FB_DUPLICATE:
+                       case PFR_FB_NONE:
+                               pfr_destroy_kentry(ke);
+                               break;
+                       case PFR_FB_ADDED:
+                               if (flags & PFR_FLAG_DUMMY)
+                                       pfr_destroy_kentry(ke);
+                       }
+               }
        } else
-               pfr_destroy_kentries(&workq);
+               pfr_destroy_ioq(&ioq, flags);
+
        if (nadd != NULL)
                *nadd = xadd;
+
        pfr_destroy_ktable(tmpkt, 0);
        return (0);
 _bad:
-       pfr_clean_node_mask(tmpkt, &workq);
-       pfr_destroy_kentries(&workq);
+       pfr_destroy_ioq(&ioq, flags);
        if (flags & PFR_FLAG_FEEDBACK)
                pfr_reset_feedback(addr, size, flags);
        pfr_destroy_ktable(tmpkt, 0);
@@ -822,6 +905,37 @@ pfr_lookup_addr(struct pfr_ktable *kt, struct pfr_addr *ad, int exact)
        return (ke);
 }
 
+struct pfr_kentry *
+pfr_lookup_kentry(struct pfr_ktable *kt, struct pfr_kentry *key, int exact)
+{
+       union sockaddr_union     mask;
+       struct radix_node_head  *head;
+       struct pfr_kentry       *ke;
+
+       switch (key->pfrke_af) {
+       case AF_INET:
+               head = kt->pfrkt_ip4;
+               break;
+#ifdef INET6
+       case AF_INET6:
+               head = kt->pfrkt_ip6;
+               break;
+#endif /* INET6 */
+       default:
+               unhandled_af(key->pfrke_af);
+       }
+       if (KENTRY_NETWORK(key)) {
+               pfr_prepare_network(&mask, key->pfrke_af, key->pfrke_net);
+               ke = (struct pfr_kentry *)rn_lookup(&key->pfrke_sa, &mask,
+                   head);
+       } else {
+               ke = (struct pfr_kentry *)rn_match(&key->pfrke_sa, head);
+               if (exact && ke && KENTRY_NETWORK(ke))
+                       ke = NULL;
+       }
+       return (ke);
+}
+
 struct pfr_kentry *
 pfr_create_kentry(struct pfr_addr *ad)
 {
@@ -873,6 +987,81 @@ pfr_create_kentry(struct pfr_addr *ad)
        return ((struct pfr_kentry *)ke);
 }
 
+struct pfr_kentry *
+pfr_create_kentry_unlocked(struct pfr_addr *ad, int flags)
+{
+       struct pfr_kentry_all   *ke;
+       int mflags = PR_ZERO;
+
+       if (ad->pfra_type >= PFRKE_MAX)
+               panic("unknown pfra_type %d", ad->pfra_type);
+
+       if (flags & PFR_FLAG_USERIOCTL)
+               mflags |= PR_WAITOK;
+       else
+               mflags |= PR_NOWAIT;
+
+       ke = pool_get(&pfr_kentry_pl[ad->pfra_type], mflags);
+       if (ke == NULL)
+               return (NULL);
+
+       ke->pfrke_type = ad->pfra_type;
+
+       /* set weight allowing implicit weights */
+       if (ad->pfra_weight == 0)
+               ad->pfra_weight = 1;
+
+       switch (ke->pfrke_type) {
+       case PFRKE_PLAIN:
+               break;
+       case PFRKE_COST:
+               ((struct pfr_kentry_cost *)ke)->weight = ad->pfra_weight;
+               /* FALLTHROUGH */
+       case PFRKE_ROUTE:
+               if (ad->pfra_ifname[0])
+                       (void) strlcpy(ke->pfrke_rifname, ad->pfra_ifname,
+                           IFNAMSIZ);
+               break;
+       }
+
+       switch (ad->pfra_af) {
+       case AF_INET:
+               FILLIN_SIN(ke->pfrke_sa.sin, ad->pfra_ip4addr);
+               break;
+#ifdef INET6
+       case AF_INET6:
+               FILLIN_SIN6(ke->pfrke_sa.sin6, ad->pfra_ip6addr);
+               break;
+#endif /* INET6 */
+       default:
+               unhandled_af(ad->pfra_af);
+       }
+       ke->pfrke_af = ad->pfra_af;
+       ke->pfrke_net = ad->pfra_net;
+       if (ad->pfra_not)
+               ke->pfrke_flags |= PFRKE_FLAG_NOT;
+       return ((struct pfr_kentry *)ke);
+}
+
+void
+pfr_kentry_kif_ref(struct pfr_kentry *ke_all)
+{
+       struct pfr_kentry_all   *ke = (struct pfr_kentry_all *)ke_all;
+
+       NET_ASSERT_LOCKED();
+       switch (ke->pfrke_type) {
+       case PFRKE_PLAIN:
+               break;
+       case PFRKE_COST:
+       case PFRKE_ROUTE:
+               if (ke->pfrke_rifname[0])
+                       ke->pfrke_rkif = pfi_kif_get(ke->pfrke_rifname, NULL);
+               if (ke->pfrke_rkif)
+                       pfi_kif_ref(ke->pfrke_rkif, PFI_KIF_REF_ROUTE);
+               break;
+       }
+}
+
 void
 pfr_destroy_kentries(struct pfr_kentryworkq *workq)
 {
@@ -885,6 +1074,23 @@ pfr_destroy_kentries(struct pfr_kentryworkq *workq)
        }
 }
 
+void
+pfr_destroy_ioq(struct pfr_kentryworkq *ioq, int flags)
+{
+       struct pfr_kentry       *p;
+
+       while ((p = SLIST_FIRST(ioq)) != NULL) {
+               YIELD(flags & PFR_FLAG_USERIOCTL);
+               SLIST_REMOVE_HEAD(ioq, pfrke_ioq);
+               /*
+                * we destroy only those entries, which did not make it to
+                * table
+                */
+               if ((p->pfrke_fb != PFR_FB_ADDED) || (flags & PFR_FLAG_DUMMY))
+                       pfr_destroy_kentry(p);
+       }
+}
+
 void
 pfr_destroy_kentry(struct pfr_kentry *ke)
 {

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 1baee0f..514b9e1 100644 (file)
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: pfvar.h,v 1.503 2021/11/11 12:35:01 sashan Exp $ */
+/*     $OpenBSD: pfvar.h,v 1.504 2021/11/16 20:51:31 sashan Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1043,12 +1043,14 @@ struct _pfr_kentry {
        struct radix_node        _pfrke_node[2];
        union pfsockaddr_union   _pfrke_sa;
        SLIST_ENTRY(pfr_kentry)  _pfrke_workq;
+       SLIST_ENTRY(pfr_kentry)  _pfrke_ioq;
        struct pfr_kcounters    *_pfrke_counters;
        time_t                   _pfrke_tzero;
        u_int8_t                 _pfrke_af;
        u_int8_t                 _pfrke_net;
        u_int8_t                 _pfrke_flags;
        u_int8_t                 _pfrke_type;
+       u_int8_t                 _pfrke_fb;
 };
 #define PFRKE_FLAG_NOT         0x01
 #define PFRKE_FLAG_MARK                0x02
@@ -1064,12 +1066,14 @@ struct pfr_kentry {
 #define pfrke_node     u._ke._pfrke_node
 #define pfrke_sa       u._ke._pfrke_sa
 #define pfrke_workq    u._ke._pfrke_workq
+#define pfrke_ioq      u._ke._pfrke_ioq
 #define pfrke_counters u._ke._pfrke_counters
 #define pfrke_tzero    u._ke._pfrke_tzero
 #define pfrke_af       u._ke._pfrke_af
 #define pfrke_net      u._ke._pfrke_net
 #define pfrke_flags    u._ke._pfrke_flags
 #define pfrke_type     u._ke._pfrke_type
+#define pfrke_fb       u._ke._pfrke_fb
 
 struct pfr_kentry_route {
        union {
@@ -1077,6 +1081,7 @@ struct pfr_kentry_route {
        } u;
 
        struct pfi_kif          *kif;
+       char                    ifname[IFNAMSIZ];
 };
 
 struct pfr_kentry_cost {
@@ -1085,6 +1090,7 @@ struct pfr_kentry_cost {
        } u;
 
        struct pfi_kif          *kif;
+       char                    ifname[IFNAMSIZ];
        /* Above overlaps with pfr_kentry route */
 
        u_int16_t                weight;
@@ -1098,6 +1104,7 @@ struct pfr_kentry_all {
        } u;
 };
 #define pfrke_rkif     u.kr.kif
+#define pfrke_rifname  u.kr.ifname
 
 SLIST_HEAD(pfr_ktableworkq, pfr_ktable);
 RB_HEAD(pfr_ktablehead, pfr_ktable);