[PATCH] Minor fixes to netinet6/icmp6.c

Sergey Kandaurov pluknet at gmail.com
Sat Dec 24 00:07:13 UTC 2011


On 23 December 2011 23:46, John Baldwin <jhb at freebsd.org> wrote:
> I found these nits while working on the patches to convert if_addr_mtx to an
> rwlock.  The first change is cosmetic, it just un-inlines a TAILQ_FOREACH().
> The second change is an actual bug.  The code is currently reading
> TAILQ_FIRST(&V_ifnet) without holding the appropriate lock.
>
> Index: icmp6.c
> ===================================================================
> --- icmp6.c     (revision 228777)
> +++ icmp6.c     (working copy)
> @@ -1780,7 +1780,7 @@ ni6_addrs(struct icmp6_nodeinfo *ni6, struct mbuf
>        }
>
>        IFNET_RLOCK_NOSLEEP();
> -       for (ifp = TAILQ_FIRST(&V_ifnet); ifp; ifp = TAILQ_NEXT(ifp, if_list)) {
> +       TAILQ_FOREACH(ifp, &V_ifnet, if_list) {
>                addrsofif = 0;
>                IF_ADDR_LOCK(ifp);
>                TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {

FWIW, there are much more of them in netinet6.
Some time ago I started to un-expand them to queue(3).
[not unfinished yet..]

Index: /sys/netinet6/in6_ifattach.c
===================================================================
--- /sys/netinet6/in6_ifattach.c        (revision 228686)
+++ /sys/netinet6/in6_ifattach.c        (working copy)
@@ -405,7 +405,7 @@

        /* next, try to get it from some other hardware interface */
        IFNET_RLOCK_NOSLEEP();
-       for (ifp = V_ifnet.tqh_first; ifp; ifp = ifp->if_list.tqe_next) {
+       TAILQ_FOREACH(ifp, &V_ifnet, if_list) {
                if (ifp == ifp0)
                        continue;
                if (in6_get_hw_ifid(ifp, in6) != 0)
@@ -820,7 +820,7 @@
                /*
                 * leave from multicast groups we have joined for the interface
                 */
-               while ((imm = ia->ia6_memberships.lh_first) != NULL) {
+               while ((imm = LIST_FIRST(&ia->ia6_memberships)) != NULL) {
                        LIST_REMOVE(imm, i6mm_chain);
                        in6_leavegroup(imm);
                }
@@ -923,8 +923,7 @@
            V_ip6_temp_regen_advance) * hz, in6_tmpaddrtimer, curvnet);

        bzero(nullbuf, sizeof(nullbuf));
-       for (ifp = TAILQ_FIRST(&V_ifnet); ifp;
-           ifp = TAILQ_NEXT(ifp, if_list)) {
+       TAILQ_FOREACH(ifp, &V_ifnet, if_list) {
                ndi = ND_IFINFO(ifp);
                if (bcmp(ndi->randomid, nullbuf, sizeof(nullbuf)) != 0) {
                        /*
Index: /sys/netinet6/icmp6.c
===================================================================
--- /sys/netinet6/icmp6.c       (revision 228686)
+++ /sys/netinet6/icmp6.c       (working copy)
@@ -779,9 +779,8 @@
                                /* -1 == no app on SEND socket */
                                if (error == 0)
                                        return (IPPROTO_DONE);
-                               nd6_rs_input(m, off, icmp6len);
-                       } else
-                               nd6_rs_input(m, off, icmp6len);
+                       }
+                       nd6_rs_input(m, off, icmp6len);
                        m = NULL;
                        goto freeit;
                }
@@ -793,9 +792,8 @@
                        if (error == 0)
                                goto freeit;
                        /* -1 == no app on SEND socket */
-                       nd6_rs_input(n, off, icmp6len);
-               } else
-                       nd6_rs_input(n, off, icmp6len);
+               }
+               nd6_rs_input(n, off, icmp6len);
                /* m stays. */
                break;

@@ -813,9 +811,8 @@
                                    SND_IN, ip6len);
                                if (error == 0)
                                        return (IPPROTO_DONE);
-                               nd6_ra_input(m, off, icmp6len);
-                       } else
-                               nd6_ra_input(m, off, icmp6len);
+                       }
+                       nd6_ra_input(m, off, icmp6len);
                        m = NULL;
                        goto freeit;
                }
@@ -824,9 +821,8 @@
                            SND_IN, ip6len);
                        if (error == 0)
                                goto freeit;
-                       nd6_ra_input(n, off, icmp6len);
-               } else
-                       nd6_ra_input(n, off, icmp6len);
+               }
+               nd6_ra_input(n, off, icmp6len);
                /* m stays. */
                break;

@@ -842,9 +838,8 @@
                                    SND_IN, ip6len);
                                if (error == 0)
                                        return (IPPROTO_DONE);
-                               nd6_ns_input(m, off, icmp6len);
-                       } else
-                               nd6_ns_input(m, off, icmp6len);
+                       }
+                       nd6_ns_input(m, off, icmp6len);
                        m = NULL;
                        goto freeit;
                }
@@ -853,9 +848,8 @@
                            SND_IN, ip6len);
                        if (error == 0)
                                goto freeit;
-                       nd6_ns_input(n, off, icmp6len);
-               } else
-                       nd6_ns_input(n, off, icmp6len);
+               }
+               nd6_ns_input(n, off, icmp6len);
                /* m stays. */
                break;

@@ -873,9 +867,8 @@
                                    SND_IN, ip6len);
                                if (error == 0)
                                        return (IPPROTO_DONE);
-                               nd6_na_input(m, off, icmp6len);
-                       } else
-                               nd6_na_input(m, off, icmp6len);
+                       }
+                       nd6_na_input(m, off, icmp6len);
                        m = NULL;
                        goto freeit;
                }
@@ -884,9 +877,8 @@
                            SND_IN, ip6len);
                        if (error == 0)
                                goto freeit;
-                       nd6_na_input(n, off, icmp6len);
-               } else
-                       nd6_na_input(n, off, icmp6len);
+               }
+               nd6_na_input(n, off, icmp6len);
                /* m stays. */
                break;

@@ -902,9 +894,8 @@
                                    SND_IN, ip6len);
                                if (error == 0)
                                        return (IPPROTO_DONE);
-                           icmp6_redirect_input(m, off);
-                       } else
-                               icmp6_redirect_input(m, off);
+                       }
+                       icmp6_redirect_input(m, off);
                        m = NULL;
                        goto freeit;
                }
@@ -913,9 +904,8 @@
                            SND_IN, ip6len);
                        if (error == 0)
                                goto freeit;
-                       icmp6_redirect_input(n, off);
-               } else
-                       icmp6_redirect_input(n, off);
+               }
+               icmp6_redirect_input(n, off);
                /* m stays. */
                break;

Index: /sys/netinet6/nd6.c
===================================================================
--- /sys/netinet6/nd6.c (revision 228686)
+++ /sys/netinet6/nd6.c (working copy)
@@ -575,7 +575,6 @@
        struct nd_defrouter *dr;
        struct nd_prefix *pr;
        struct in6_ifaddr *ia6, *nia6;
-       struct in6_addrlifetime *lt6;

        callout_reset(&V_nd6_timer_ch, V_nd6_prune * hz,
            nd6_timer, curvnet);
@@ -604,8 +603,6 @@
         */
   addrloop:
        TAILQ_FOREACH_SAFE(ia6, &V_in6_ifaddrhead, ia_link, nia6) {
-               /* check address lifetime */
-               lt6 = &ia6->ia6_lifetime;
                if (IFA6_IS_INVALID(ia6)) {
                        int regen = 0;

@@ -668,7 +665,7 @@
        }

        /* expire prefix list */
-       pr = V_nd_prefix.lh_first;
+       pr = LIST_FIRST(&V_nd_prefix);
        while (pr) {
                /*
                 * check prefix lifetime.
@@ -800,7 +797,7 @@
        }

        /* Nuke prefix list entries toward ifp */
-       for (pr = V_nd_prefix.lh_first; pr; pr = npr) {
+       for (pr = LIST_FIRST(&V_nd_prefix); pr; pr = npr) {
                npr = pr->ndpr_next;
                if (pr->ndpr_ifp == ifp) {
                        /*
@@ -912,7 +909,7 @@
         * If the address matches one of our on-link prefixes, it should be a
         * neighbor.
         */
-       for (pr = V_nd_prefix.lh_first; pr; pr = pr->ndpr_next) {
+       for (pr = LIST_FIRST(&V_nd_prefix); pr; pr = pr->ndpr_next) {
                if (pr->ndpr_ifp != ifp)
                        continue;

@@ -1263,7 +1260,7 @@
                 */
                bzero(oprl, sizeof(*oprl));
                s = splnet();
-               pr = V_nd_prefix.lh_first;
+               pr = LIST_FIRST(&V_nd_prefix);
                while (pr && i < PRLSTSIZ) {
                        struct nd_pfxrouter *pfr;
                        int j;
@@ -1292,7 +1289,7 @@
                                        oprl->prefix[i].expire = maxexpire;
                        }

-                       pfr = pr->ndpr_advrtrs.lh_first;
+                       pfr = LIST_FIRST(&pr->ndpr_advrtrs);
                        j = 0;
                        while (pfr) {
                                if (j < DRLSTSIZ) {
@@ -1470,7 +1467,7 @@
                struct nd_prefix *pr, *next;

                s = splnet();
-               for (pr = V_nd_prefix.lh_first; pr; pr = next) {
+               for (pr = LIST_FIRST(&V_nd_prefix); pr; pr = next) {
                        struct in6_ifaddr *ia, *ia_next;

                        next = pr->ndpr_next;
@@ -2335,7 +2332,7 @@
                return EPERM;
        error = 0;

-       for (pr = V_nd_prefix.lh_first; pr; pr = pr->ndpr_next) {
+       for (pr = LIST_FIRST(&V_nd_prefix); pr; pr = pr->ndpr_next) {
                u_short advrtrs;
                size_t advance;
                struct sockaddr_in6 *sin6, *s6;
@@ -2380,7 +2377,7 @@
                        p->flags = pr->ndpr_stateflags;
                        p->origin = PR_ORIG_RA;
                        advrtrs = 0;
-                       for (pfr = pr->ndpr_advrtrs.lh_first; pfr;
+                       for (pfr = LIST_FIRST(&pr->ndpr_advrtrs); pfr;
                             pfr = pfr->pfr_next) {
                                if ((void *)&sin6[advrtrs + 1] > (void *)pe) {
                                        advrtrs++;
Index: /sys/netinet6/in6.c
===================================================================
--- /sys/netinet6/in6.c (revision 228686)
+++ /sys/netinet6/in6.c (working copy)
@@ -1362,7 +1362,7 @@
        /*
         * leave from multicast groups we have joined for the interface
         */
-       while ((imm = ia->ia6_memberships.lh_first) != NULL) {
+       while ((imm = LIST_FIRST(&ia->ia6_memberships)) != NULL) {
                LIST_REMOVE(imm, i6mm_chain);
                in6_leavegroup(imm);
        }
Index: /sys/netinet6/nd6_rtr.c
===================================================================
--- /sys/netinet6/nd6_rtr.c     (revision 228686)
+++ /sys/netinet6/nd6_rtr.c     (working copy)
@@ -581,7 +581,7 @@
        /*
         * Also delete all the pointers to the router in each prefix lists.
         */
-       for (pr = V_nd_prefix.lh_first; pr; pr = pr->ndpr_next) {
+       LIST_FOREACH(pr, &V_nd_prefix, ndpr_entry) {
                struct nd_pfxrouter *pfxrtr;
                if ((pfxrtr = pfxrtr_lookup(pr, dr)) != NULL)
                        pfxrtr_del(pfxrtr);

-- 
wbr,
pluknet


More information about the freebsd-net mailing list