Revert "pf: Allow disappearing or not yet existing interfaces for ALTQ"
authorAaron LI <aly@aaronly.me>
Thu, 9 Aug 2018 02:40:36 +0000 (10:40 +0800)
committerAaron LI <aly@aaronly.me>
Thu, 9 Aug 2018 02:40:36 +0000 (10:40 +0800)
This reverts commit 0a887f91f9633448c99b9a5b7c6116a0a22d25d6.

1. It's incorrect to change the ifnet_unlock().  The original protection
   range is used to make sure that the ifp does not get ripped out behind
   our back.

2. We don't suffer from the issue that that commit was intended to fix.

Thanks-to: sephe

sys/net/pf/pf_if.c
sys/net/pf/pf_ioctl.c
sys/net/pf/pfvar.h
usr.sbin/pfctl/pfctl_altq.c
usr.sbin/pfctl/pfctl_qstats.c

index 33960e5..7880fa0 100644 (file)
@@ -866,22 +866,12 @@ static void
 pfi_attach_ifnet_event(void *arg __unused, struct ifnet *ifp)
 {
        pfi_attach_ifnet(ifp);
-#ifdef ALTQ
-       crit_enter();
-       pf_altq_ifnet_event(ifp, 0);
-       crit_exit();
-#endif
 }
 
 static void
 pfi_detach_ifnet_event(void *arg __unused, struct ifnet *ifp)
 {
        pfi_detach_ifnet(ifp);
-#ifdef ALTQ
-       crit_enter();
-       pf_altq_ifnet_event(ifp, 1);
-       crit_exit();
-#endif
 }
 
 static void
index b245de3..0cb20ed 100644 (file)
@@ -565,8 +565,7 @@ pf_begin_altq(u_int32_t *ticket)
        /* Purge the old altq list */
        while ((altq = TAILQ_FIRST(pf_altqs_inactive)) != NULL) {
                TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
-               if (altq->qname[0] == 0 &&
-                   (altq->local_flags & PFALTQ_FLAG_IF_REMOVED) == 0) {
+               if (altq->qname[0] == 0) {
                        /* detach and destroy the discipline */
                        error = altq_remove(altq);
                } else
@@ -591,8 +590,7 @@ pf_rollback_altq(u_int32_t ticket)
        /* Purge the old altq list */
        while ((altq = TAILQ_FIRST(pf_altqs_inactive)) != NULL) {
                TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
-               if (altq->qname[0] == 0 &&
-                   (altq->local_flags & PFALTQ_FLAG_IF_REMOVED) == 0) {
+               if (altq->qname[0] == 0) {
                        /* detach and destroy the discipline */
                        error = altq_remove(altq);
                } else
@@ -622,8 +620,7 @@ pf_commit_altq(u_int32_t ticket)
 
        /* Attach new disciplines */
        TAILQ_FOREACH(altq, pf_altqs_active, entries) {
-               if (altq->qname[0] == 0 &&
-                   (altq->local_flags & PFALTQ_FLAG_IF_REMOVED) == 0) {
+               if (altq->qname[0] == 0) {
                        /* attach the discipline */
                        error = altq_pfattach(altq);
                        if (error) {
@@ -636,8 +633,7 @@ pf_commit_altq(u_int32_t ticket)
        /* Purge the old altq list */
        while ((altq = TAILQ_FIRST(pf_altqs_inactive)) != NULL) {
                TAILQ_REMOVE(pf_altqs_inactive, altq, entries);
-               if (altq->qname[0] == 0 &&
-                   (altq->local_flags & PFALTQ_FLAG_IF_REMOVED) == 0) {
+               if (altq->qname[0] == 0) {
                        /* detach and destroy the discipline */
                        if (pf_altq_running)
                                error = pf_disable_altq(altq);
@@ -665,11 +661,11 @@ pf_enable_altq(struct pf_altq *altq)
        int                      error = 0;
 
        ifnet_lock();
-       ifp = ifunit(altq->ifname);
-       ifnet_unlock();
 
-       if (ifp == NULL)
+       if ((ifp = ifunit(altq->ifname)) == NULL) {
+               ifnet_unlock();
                return (EINVAL);
+       }
 
        if (ifp->if_snd.altq_type != ALTQT_NONE)
                error = altq_enable(&ifp->if_snd);
@@ -683,6 +679,7 @@ pf_enable_altq(struct pf_altq *altq)
                crit_exit();
        }
 
+       ifnet_unlock();
        return (error);
 }
 
@@ -694,18 +691,20 @@ pf_disable_altq(struct pf_altq *altq)
        int                      error;
 
        ifnet_lock();
-       ifp = ifunit(altq->ifname);
-       ifnet_unlock();
 
-       if (ifp == NULL)
+       if ((ifp = ifunit(altq->ifname)) == NULL) {
+               ifnet_unlock();
                return (EINVAL);
+       }
 
        /*
         * when the discipline is no longer referenced, it was overridden
         * by a new one.  if so, just return.
         */
-       if (altq->altq_disc != ifp->if_snd.altq_disc)
+       if (altq->altq_disc != ifp->if_snd.altq_disc) {
+               ifnet_unlock();
                return (0);
+       }
 
        error = altq_disable(&ifp->if_snd);
 
@@ -717,76 +716,9 @@ pf_disable_altq(struct pf_altq *altq)
                crit_exit();
        }
 
+       ifnet_unlock();
        return (error);
 }
-
-void
-pf_altq_ifnet_event(struct ifnet *ifp, int remove)
-{
-       struct ifnet    *ifp1;
-       struct pf_altq  *a1, *a2, *a3;
-       u_int32_t        ticket;
-       int              error = 0;
-
-       /* Interrupt userland queue modifications */
-       if (altqs_inactive_open)
-               pf_rollback_altq(ticket_altqs_inactive);
-
-       /* Start new altq ruleset */
-       if (pf_begin_altq(&ticket))
-               return;
-
-       /* Copy the current active set */
-       TAILQ_FOREACH(a1, pf_altqs_active, entries) {
-               a2 = kmalloc(sizeof(*a2), M_PFALTQPL, M_INTWAIT);
-               if (a2 == NULL) {
-                       error = ENOMEM;
-                       break;
-               }
-               bcopy(a1, a2, sizeof(struct pf_altq));
-
-               if (a2->qname[0] != 0) {
-                       if ((a2->qid = pf_qname2qid(a2->qname)) == 0) {
-                               error = EBUSY;
-                               kfree(a2, M_PFALTQPL);
-                               break;
-                       }
-                       a2->altq_disc = NULL;
-                       TAILQ_FOREACH(a3, pf_altqs_inactive, entries) {
-                               if (strncmp(a3->ifname, a2->ifname,
-                                   IFNAMSIZ) == 0 && a3->qname[0] == 0) {
-                                       a2->altq_disc = a3->altq_disc;
-                                       break;
-                               }
-                       }
-               }
-               /* Deactivate the interface in question */
-               a2->local_flags &= ~PFALTQ_FLAG_IF_REMOVED;
-               ifnet_lock();
-               ifp1 = ifunit(a2->ifname);
-               ifnet_unlock();
-               if ((ifp1 == NULL) || (remove && ifp1 == ifp)) {
-                       a2->local_flags |= PFALTQ_FLAG_IF_REMOVED;
-               } else {
-                       error = altq_add(a2);
-
-                       if (ticket != ticket_altqs_inactive)
-                               error = EBUSY;
-
-                       if (error) {
-                               kfree(a2, M_PFALTQPL);
-                               break;
-                       }
-               }
-
-               TAILQ_INSERT_TAIL(pf_altqs_inactive, a2, entries);
-       }
-
-       if (error != 0)
-               pf_rollback_altq(ticket);
-       else
-               pf_commit_altq(ticket);
-}
 #endif /* ALTQ */
 
 int
@@ -1986,11 +1918,11 @@ pfioctl(struct dev_ioctl_args *ap)
                        strlcpy(ps.ifname, psp->ifname, IFNAMSIZ);
                        ifnet_lock();
                        ifp = ifunit(ps.ifname);
-                       ifnet_unlock();
-                       if (ifp)
+                       if (ifp )
                                psp->baudrate = ifp->if_baudrate;
                        else
                                error = EINVAL;
+                       ifnet_unlock();
                } else
                        error = EINVAL;
                break;
@@ -2000,10 +1932,8 @@ pfioctl(struct dev_ioctl_args *ap)
                struct pf_altq          *altq;
 
                /* enable all altq interfaces on active list */
-               /* XXX: locking? */
                TAILQ_FOREACH(altq, pf_altqs_active, entries) {
-                       if (altq->qname[0] == 0 && (altq->local_flags &
-                           PFALTQ_FLAG_IF_REMOVED) == 0) {
+                       if (altq->qname[0] == 0) {
                                error = pf_enable_altq(altq);
                                if (error != 0)
                                        break;
@@ -2020,8 +1950,7 @@ pfioctl(struct dev_ioctl_args *ap)
 
                /* disable all altq interfaces on active list */
                TAILQ_FOREACH(altq, pf_altqs_active, entries) {
-                       if (altq->qname[0] == 0 && (altq->local_flags &
-                           PFALTQ_FLAG_IF_REMOVED) == 0) {
+                       if (altq->qname[0] == 0) {
                                error = pf_disable_altq(altq);
                                if (error != 0)
                                        break;
@@ -2036,7 +1965,6 @@ pfioctl(struct dev_ioctl_args *ap)
        case DIOCADDALTQ: {
                struct pfioc_altq       *pa = (struct pfioc_altq *)addr;
                struct pf_altq          *altq, *a;
-               struct ifnet            *ifp;
 
                if (pa->ticket != ticket_altqs_inactive) {
                        error = EBUSY;
@@ -2069,14 +1997,7 @@ pfioctl(struct dev_ioctl_args *ap)
                        }
                }
 
-               ifnet_lock();
-               ifp = ifunit(altq->ifname);
-               ifnet_unlock();
-               if (ifp == NULL)
-                       altq->local_flags |= PFALTQ_FLAG_IF_REMOVED;
-               else
-                       error = altq_add(altq);
-
+               error = altq_add(altq);
                if (error) {
                        kfree(altq, M_PFALTQPL);
                        break;
@@ -2147,10 +2068,6 @@ pfioctl(struct dev_ioctl_args *ap)
                        error = EBUSY;
                        break;
                }
-               if ((altq->local_flags & PFALTQ_FLAG_IF_REMOVED) != 0) {
-                       error = ENXIO;
-                       break;
-               }
                error = altq_getqstats(altq, pq->buf, &nbytes);
                if (error == 0) {
                        pq->scheduler = altq->scheduler;
index 72b4344..8ea12e7 100644 (file)
@@ -1434,9 +1434,6 @@ struct pf_altq {
        u_int32_t                parent_qid;    /* parent queue id */
        u_int32_t                bandwidth;     /* queue bandwidth */
        u_int8_t                 priority;      /* priority */
-       uint8_t                  local_flags;   /* dynamic interface */
-#define        PFALTQ_FLAG_IF_REMOVED          0x01
-
        u_int16_t                qlimit;        /* queue size limit */
        u_int16_t                flags;         /* misc flags */
        union {
@@ -1772,9 +1769,6 @@ extern int                         pf_tbladdr_setup(struct pf_ruleset *,
 extern void                     pf_tbladdr_remove(struct pf_addr_wrap *);
 extern void                     pf_tbladdr_copyout(struct pf_addr_wrap *);
 extern void                     pf_calc_skip_steps(struct pf_rulequeue *);
-#ifdef ALTQ
-extern void                     pf_altq_ifnet_event(struct ifnet *, int);
-#endif
 extern struct malloc_type      *pf_src_tree_pl, *pf_rule_pl;
 extern struct malloc_type      *pf_state_pl, *pf_state_key_pl, *pf_state_item_pl,
                                        *pf_altq_pl, *pf_pooladdr_pl;
index 2064bb0..de21e33 100644 (file)
@@ -156,8 +156,6 @@ print_altq(const struct pf_altq *a, unsigned int level,
                return;
        }
 
-       if (a->local_flags & PFALTQ_FLAG_IF_REMOVED)
-               printf("INACTIVE ");
        printf("altq on %s ", a->ifname);
 
        switch (a->scheduler) {
@@ -197,8 +195,6 @@ print_queue(const struct pf_altq *a, unsigned int level,
 {
        unsigned int    i;
 
-       if (a->local_flags & PFALTQ_FLAG_IF_REMOVED)
-               printf("INACTIVE ");
        printf("queue ");
        for (i = 0; i < level; ++i)
                printf(" ");
index 033897a..4a22c2f 100644 (file)
@@ -96,8 +96,6 @@ pfctl_show_altq(int dev, const char *iface, int opts, int verbose2)
        for (node = root; node != NULL; node = node->next) {
                if (iface != NULL && strcmp(node->altq.ifname, iface))
                        continue;
-               if (node->altq.local_flags & PFALTQ_FLAG_IF_REMOVED)
-                       continue;
                if (dotitle) {
                        pfctl_print_title("ALTQ:");
                        dotitle = 0;
@@ -153,8 +151,7 @@ pfctl_update_qstats(int dev, struct pf_altq_node **root)
                        warn("DIOCGETALTQ");
                        return (-1);
                }
-               if ((pa.altq.qid > 0) &&
-                   !(pa.altq.local_flags & PFALTQ_FLAG_IF_REMOVED)) {
+               if (pa.altq.qid > 0) {
                        pq.nr = nr;
                        pq.ticket = pa.ticket;
                        pq.buf = &qstats.data;
@@ -171,16 +168,6 @@ pfctl_update_qstats(int dev, struct pf_altq_node **root)
                        } else {
                                pfctl_insert_altq_node(root, pa.altq, qstats);
                        }
-               } else if (pa.altq.local_flags & PFALTQ_FLAG_IF_REMOVED) {
-                       memset(&qstats.data, 0, sizeof(qstats.data));
-                       if ((node = pfctl_find_altq_node(*root, pa.altq.qname,
-                            pa.altq.ifname)) != NULL) {
-                               memcpy(&node->qstats.data, &qstats.data,
-                                   sizeof(qstats.data));
-                               update_avg(node);
-                       } else {
-                               pfctl_insert_altq_node(root, pa.altq, qstats);
-                       }
                }
        }
        return (mnr);
@@ -287,8 +274,6 @@ pfctl_print_altq_nodestat(int dev __unused, const struct pf_altq_node *a)
 {
        if (a->altq.qid == 0)
                return;
-       if (a->altq.local_flags & PFALTQ_FLAG_IF_REMOVED)
-               return;
 
        switch (a->altq.scheduler) {
        case ALTQT_CBQ: