Date: Sat, 24 Dec 2011 02:38:27 +0300 From: Sergey Kandaurov <pluknet@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Bjoern Zeeb <bz@freebsd.org>, net@freebsd.org Subject: Re: [PATCH] Minor fixes to netinet6/icmp6.c Message-ID: <CAE-mSOLryQb6n0RcxM=LHFCLN=25Tfk5bLnLjK1e5XbU5ncmWw@mail.gmail.com> In-Reply-To: <201112231446.38057.jhb@freebsd.org> References: <201112231446.38057.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 23 December 2011 23:46, John Baldwin <jhb@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSOLryQb6n0RcxM=LHFCLN=25Tfk5bLnLjK1e5XbU5ncmWw>