From owner-freebsd-net@FreeBSD.ORG Sat Dec 24 00:07:13 2011 Return-Path: Delivered-To: net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8B0641065676; Sat, 24 Dec 2011 00:07:13 +0000 (UTC) (envelope-from pluknet@gmail.com) Received: from mail-tul01m020-f182.google.com (mail-tul01m020-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id 47B248FC08; Sat, 24 Dec 2011 00:07:13 +0000 (UTC) Received: by obbwd18 with SMTP id wd18so8299744obb.13 for ; Fri, 23 Dec 2011 16:07:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=MivxwOW+c0KwMOLPRNniIDVBKBUR5MfmkG39VnsRyWg=; b=VPYjF0Tgyu1zQfUwQpMFzmVc4mPOQxcMxFwZLZ2LKcYnZwyjidE2Zd5egidxqP59qp WDtO+XDVUgepNGGK5vc/btQ5kNDmNbbc3qPlqrmuQgURJH5PT7anGYtOe8jmplIHl2e5 GvRGkhOCmUl3XUH+dtudbeiF2UHMpMmJo8y3o= MIME-Version: 1.0 Received: by 10.182.89.5 with SMTP id bk5mr14312101obb.50.1324683508017; Fri, 23 Dec 2011 15:38:28 -0800 (PST) Received: by 10.182.171.67 with HTTP; Fri, 23 Dec 2011 15:38:27 -0800 (PST) In-Reply-To: <201112231446.38057.jhb@freebsd.org> References: <201112231446.38057.jhb@freebsd.org> Date: Sat, 24 Dec 2011 02:38:27 +0300 Message-ID: From: Sergey Kandaurov To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Bjoern Zeeb , net@freebsd.org Subject: Re: [PATCH] Minor fixes to netinet6/icmp6.c X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 24 Dec 2011 00:07:13 -0000 On 23 December 2011 23:46, John Baldwin wrote: > I found these nits while working on the patches to convert if_addr_mtx to= an > rwlock. =A0The first change is cosmetic, it just un-inlines a TAILQ_FOREA= CH(). > The second change is an actual bug. =A0The code is currently reading > TAILQ_FIRST(&V_ifnet) without holding the appropriate lock. > > Index: icmp6.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- icmp6.c =A0 =A0 (revision 228777) > +++ icmp6.c =A0 =A0 (working copy) > @@ -1780,7 +1780,7 @@ ni6_addrs(struct icmp6_nodeinfo *ni6, struct mbuf > =A0 =A0 =A0 =A0} > > =A0 =A0 =A0 =A0IFNET_RLOCK_NOSLEEP(); > - =A0 =A0 =A0 for (ifp =3D TAILQ_FIRST(&V_ifnet); ifp; ifp =3D TAILQ_NEXT= (ifp, if_list)) { > + =A0 =A0 =A0 TAILQ_FOREACH(ifp, &V_ifnet, if_list) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0addrsofif =3D 0; > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0IF_ADDR_LOCK(ifp); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0TAILQ_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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- /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 =3D V_ifnet.tqh_first; ifp; ifp =3D ifp->if_list.tqe_next)= { + TAILQ_FOREACH(ifp, &V_ifnet, if_list) { if (ifp =3D=3D ifp0) continue; if (in6_get_hw_ifid(ifp, in6) !=3D 0) @@ -820,7 +820,7 @@ /* * leave from multicast groups we have joined for the inter= face */ - while ((imm =3D ia->ia6_memberships.lh_first) !=3D NULL) { + while ((imm =3D LIST_FIRST(&ia->ia6_memberships)) !=3D 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 =3D TAILQ_FIRST(&V_ifnet); ifp; - ifp =3D TAILQ_NEXT(ifp, if_list)) { + TAILQ_FOREACH(ifp, &V_ifnet, if_list) { ndi =3D ND_IFINFO(ifp); if (bcmp(ndi->randomid, nullbuf, sizeof(nullbuf)) !=3D 0) { /* Index: /sys/netinet6/icmp6.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- /sys/netinet6/icmp6.c (revision 228686) +++ /sys/netinet6/icmp6.c (working copy) @@ -779,9 +779,8 @@ /* -1 =3D=3D no app on SEND socket */ if (error =3D=3D 0) return (IPPROTO_DONE); - nd6_rs_input(m, off, icmp6len); - } else - nd6_rs_input(m, off, icmp6len); + } + nd6_rs_input(m, off, icmp6len); m =3D NULL; goto freeit; } @@ -793,9 +792,8 @@ if (error =3D=3D 0) goto freeit; /* -1 =3D=3D 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 =3D=3D 0) return (IPPROTO_DONE); - nd6_ra_input(m, off, icmp6len); - } else - nd6_ra_input(m, off, icmp6len); + } + nd6_ra_input(m, off, icmp6len); m =3D NULL; goto freeit; } @@ -824,9 +821,8 @@ SND_IN, ip6len); if (error =3D=3D 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 =3D=3D 0) return (IPPROTO_DONE); - nd6_ns_input(m, off, icmp6len); - } else - nd6_ns_input(m, off, icmp6len); + } + nd6_ns_input(m, off, icmp6len); m =3D NULL; goto freeit; } @@ -853,9 +848,8 @@ SND_IN, ip6len); if (error =3D=3D 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 =3D=3D 0) return (IPPROTO_DONE); - nd6_na_input(m, off, icmp6len); - } else - nd6_na_input(m, off, icmp6len); + } + nd6_na_input(m, off, icmp6len); m =3D NULL; goto freeit; } @@ -884,9 +877,8 @@ SND_IN, ip6len); if (error =3D=3D 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 =3D=3D 0) return (IPPROTO_DONE); - icmp6_redirect_input(m, off); - } else - icmp6_redirect_input(m, off); + } + icmp6_redirect_input(m, off); m =3D NULL; goto freeit; } @@ -913,9 +904,8 @@ SND_IN, ip6len); if (error =3D=3D 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- /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 =3D &ia6->ia6_lifetime; if (IFA6_IS_INVALID(ia6)) { int regen =3D 0; @@ -668,7 +665,7 @@ } /* expire prefix list */ - pr =3D V_nd_prefix.lh_first; + pr =3D LIST_FIRST(&V_nd_prefix); while (pr) { /* * check prefix lifetime. @@ -800,7 +797,7 @@ } /* Nuke prefix list entries toward ifp */ - for (pr =3D V_nd_prefix.lh_first; pr; pr =3D npr) { + for (pr =3D LIST_FIRST(&V_nd_prefix); pr; pr =3D npr) { npr =3D pr->ndpr_next; if (pr->ndpr_ifp =3D=3D ifp) { /* @@ -912,7 +909,7 @@ * If the address matches one of our on-link prefixes, it should be= a * neighbor. */ - for (pr =3D V_nd_prefix.lh_first; pr; pr =3D pr->ndpr_next) { + for (pr =3D LIST_FIRST(&V_nd_prefix); pr; pr =3D pr->ndpr_next) { if (pr->ndpr_ifp !=3D ifp) continue; @@ -1263,7 +1260,7 @@ */ bzero(oprl, sizeof(*oprl)); s =3D splnet(); - pr =3D V_nd_prefix.lh_first; + pr =3D LIST_FIRST(&V_nd_prefix); while (pr && i < PRLSTSIZ) { struct nd_pfxrouter *pfr; int j; @@ -1292,7 +1289,7 @@ oprl->prefix[i].expire =3D maxexpir= e; } - pfr =3D pr->ndpr_advrtrs.lh_first; + pfr =3D LIST_FIRST(&pr->ndpr_advrtrs); j =3D 0; while (pfr) { if (j < DRLSTSIZ) { @@ -1470,7 +1467,7 @@ struct nd_prefix *pr, *next; s =3D splnet(); - for (pr =3D V_nd_prefix.lh_first; pr; pr =3D next) { + for (pr =3D LIST_FIRST(&V_nd_prefix); pr; pr =3D next) { struct in6_ifaddr *ia, *ia_next; next =3D pr->ndpr_next; @@ -2335,7 +2332,7 @@ return EPERM; error =3D 0; - for (pr =3D V_nd_prefix.lh_first; pr; pr =3D pr->ndpr_next) { + for (pr =3D LIST_FIRST(&V_nd_prefix); pr; pr =3D pr->ndpr_next) { u_short advrtrs; size_t advance; struct sockaddr_in6 *sin6, *s6; @@ -2380,7 +2377,7 @@ p->flags =3D pr->ndpr_stateflags; p->origin =3D PR_ORIG_RA; advrtrs =3D 0; - for (pfr =3D pr->ndpr_advrtrs.lh_first; pfr; + for (pfr =3D LIST_FIRST(&pr->ndpr_advrtrs); pfr; pfr =3D pfr->pfr_next) { if ((void *)&sin6[advrtrs + 1] > (void *)pe= ) { advrtrs++; Index: /sys/netinet6/in6.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- /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 =3D ia->ia6_memberships.lh_first) !=3D NULL) { + while ((imm =3D LIST_FIRST(&ia->ia6_memberships)) !=3D NULL) { LIST_REMOVE(imm, i6mm_chain); in6_leavegroup(imm); } Index: /sys/netinet6/nd6_rtr.c =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- /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 =3D V_nd_prefix.lh_first; pr; pr =3D pr->ndpr_next) { + LIST_FOREACH(pr, &V_nd_prefix, ndpr_entry) { struct nd_pfxrouter *pfxrtr; if ((pfxrtr =3D pfxrtr_lookup(pr, dr)) !=3D NULL) pfxrtr_del(pfxrtr); --=20 wbr, pluknet