From bd6eb7be79d81290efa6dcaa9f492a05b1966344 Mon Sep 17 00:00:00 2001 From: rwatson Date: Thu, 25 Jun 2009 16:35:28 +0000 Subject: [PATCH] Add address list locking for in6_ifaddrhead/ia_link: as with locking for in_ifaddrhead, we stick with an rwlock for the time being, which we will revisit in the future with a possible move to rmlocks. Some pieces of code require significant further reworking to be safe from all classes of writer-writer races. Reviewed by: bz MFC after: 6 weeks --- sys/netinet/ip_carp.c | 13 +++++++++++-- sys/netinet6/in6.c | 21 +++++++++++++++++---- sys/netinet6/in6_ifattach.c | 2 ++ sys/netinet6/in6_src.c | 6 +++++- sys/netinet6/in6_var.h | 10 ++++++++++ sys/netinet6/ip6_input.c | 3 +++ sys/netinet6/nd6.c | 3 +++ sys/netinet6/nd6_rtr.c | 6 ++++++ sys/netipsec/key.c | 10 ++++++++-- 9 files changed, 65 insertions(+), 9 deletions(-) diff --git a/sys/netinet/ip_carp.c b/sys/netinet/ip_carp.c index 8c7bd0cab5b2..152f5e6cd422 100644 --- a/sys/netinet/ip_carp.c +++ b/sys/netinet/ip_carp.c @@ -1680,6 +1680,7 @@ carp_set_addr6(struct carp_softc *sc, struct sockaddr_in6 *sin6) /* we have to do it by hands to check we won't match on us */ ia_if = NULL; own = 0; + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { int i; @@ -1702,14 +1703,20 @@ carp_set_addr6(struct carp_softc *sc, struct sockaddr_in6 *sin6) } } - if (!ia_if) + if (!ia_if) { + IN6_IFADDR_RUNLOCK(); return (EADDRNOTAVAIL); + } ia = ia_if; + ifa_ref(&ia->ia_ifa); + IN6_IFADDR_RUNLOCK(); ifp = ia->ia_ifp; if (ifp == NULL || (ifp->if_flags & IFF_MULTICAST) == 0 || - (im6o->im6o_multicast_ifp && im6o->im6o_multicast_ifp != ifp)) + (im6o->im6o_multicast_ifp && im6o->im6o_multicast_ifp != ifp)) { + ifa_free(&ia->ia_ifa); return (EADDRNOTAVAIL); + } if (!sc->sc_naddrs6) { struct in6_multi *in6m; @@ -1811,12 +1818,14 @@ carp_set_addr6(struct carp_softc *sc, struct sockaddr_in6 *sin6) carp_setrun(sc, 0); CARP_UNLOCK(cif); + ifa_free(&ia->ia_ifa); /* XXXRW: should hold reference for softc. */ return (0); cleanup: if (!sc->sc_naddrs6) carp_multicast6_cleanup(sc); + ifa_free(&ia->ia_ifa); return (error); } diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c index b280e891ee5e..f415877df68f 100644 --- a/sys/netinet6/in6.c +++ b/sys/netinet6/in6.c @@ -831,8 +831,10 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq *ifra, TAILQ_INSERT_TAIL(&ifp->if_addrhead, &ia->ia_ifa, ifa_link); IF_ADDR_UNLOCK(ifp); - ifa_ref(&ia->ia_ifa); /* in6_if_addrhead */ + ifa_ref(&ia->ia_ifa); /* in6_ifaddrhead */ + IN6_IFADDR_WLOCK(); TAILQ_INSERT_TAIL(&V_in6_ifaddrhead, ia, ia_link); + IN6_IFADDR_WUNLOCK(); } /* update timestamp */ @@ -1376,7 +1378,9 @@ in6_unlink_ifa(struct in6_ifaddr *ia, struct ifnet *ifp) IF_ADDR_UNLOCK(ifp); ifa_free(&ia->ia_ifa); /* if_addrhead */ + IN6_IFADDR_WLOCK(); TAILQ_REMOVE(&V_in6_ifaddrhead, ia, ia_link); + IN6_IFADDR_WUNLOCK(); ifa_free(&ia->ia_ifa); /* in6_ifaddrhead */ /* @@ -1917,12 +1921,15 @@ in6_localaddr(struct in6_addr *in6) if (IN6_IS_ADDR_LOOPBACK(in6) || IN6_IS_ADDR_LINKLOCAL(in6)) return 1; + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { if (IN6_ARE_MASKED_ADDR_EQUAL(in6, &ia->ia_addr.sin6_addr, &ia->ia_prefixmask.sin6_addr)) { + IN6_IFADDR_RUNLOCK(); return 1; } } + IN6_IFADDR_RUNLOCK(); return (0); } @@ -1933,14 +1940,18 @@ in6_is_addr_deprecated(struct sockaddr_in6 *sa6) INIT_VNET_INET6(curvnet); struct in6_ifaddr *ia; + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { if (IN6_ARE_ADDR_EQUAL(&ia->ia_addr.sin6_addr, &sa6->sin6_addr) && - (ia->ia6_flags & IN6_IFF_DEPRECATED) != 0) + (ia->ia6_flags & IN6_IFF_DEPRECATED) != 0) { + IN6_IFADDR_RUNLOCK(); return (1); /* true */ + } /* XXX: do we still have to go thru the rest of the list? */ } + IN6_IFADDR_RUNLOCK(); return (0); /* false */ } @@ -2074,7 +2085,9 @@ in6_ifawithifp(struct ifnet *ifp, struct in6_addr *dst) IF_ADDR_UNLOCK(ifp); return (besta); } + IF_ADDR_UNLOCK(ifp); + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) { if (ifa->ifa_addr->sa_family != AF_INET6) continue; @@ -2092,10 +2105,10 @@ in6_ifawithifp(struct ifnet *ifp, struct in6_addr *dst) if (ifa != NULL) ifa_ref(ifa); - IF_ADDR_UNLOCK(ifp); + IN6_IFADDR_RUNLOCK(); return (struct in6_ifaddr *)ifa; } - IF_ADDR_UNLOCK(ifp); + IN6_IFADDR_RUNLOCK(); /* use the last-resort values, that are, deprecated addresses */ if (dep[0]) diff --git a/sys/netinet6/in6_ifattach.c b/sys/netinet6/in6_ifattach.c index 3069c467e5a6..7fc1251f235e 100644 --- a/sys/netinet6/in6_ifattach.c +++ b/sys/netinet6/in6_ifattach.c @@ -836,7 +836,9 @@ in6_ifdetach(struct ifnet *ifp) IF_ADDR_UNLOCK(ifp); ifa_free(ifa); /* if_addrhead */ + IN6_IFADDR_WLOCK(); TAILQ_REMOVE(&V_in6_ifaddrhead, ia, ia_link); + IN6_IFADDR_WUNLOCK(); ifa_free(ifa); } diff --git a/sys/netinet6/in6_src.c b/sys/netinet6/in6_src.c index b38fbc703a2a..f1ccca15ffec 100644 --- a/sys/netinet6/in6_src.c +++ b/sys/netinet6/in6_src.c @@ -289,6 +289,7 @@ in6_selectsrc(struct sockaddr_in6 *dstsock, struct ip6_pktopts *opts, if (error) return (error); + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { int new_scope = -1, new_matchlen = -1; struct in6_addrpolicy *new_policy = NULL; @@ -466,13 +467,16 @@ in6_selectsrc(struct sockaddr_in6 *dstsock, struct ip6_pktopts *opts, break; } - if ((ia = ia_best) == NULL) + if ((ia = ia_best) == NULL) { + IN6_IFADDR_RUNLOCK(); return (EADDRNOTAVAIL); + } if (ifpp) *ifpp = ifp; bcopy(&ia->ia_addr.sin6_addr, srcp, sizeof(*srcp)); + IN6_IFADDR_RUNLOCK(); return (0); } diff --git a/sys/netinet6/in6_var.h b/sys/netinet6/in6_var.h index 4ed407be5ef4..6a844db0ccfa 100644 --- a/sys/netinet6/in6_var.h +++ b/sys/netinet6/in6_var.h @@ -493,6 +493,16 @@ extern struct icmp6stat icmp6stat; extern unsigned long in6_maxmtu; #endif /* VIMAGE_GLOBALS */ + +extern struct rwlock in6_ifaddr_lock; +#define IN6_IFADDR_LOCK_ASSERT( ) rw_assert(&in6_ifaddr_lock, RA_LOCKED) +#define IN6_IFADDR_RLOCK() rw_rlock(&in6_ifaddr_lock) +#define IN6_IFADDR_RLOCK_ASSERT() rw_assert(&in6_ifaddr_lock, RA_RLOCKED) +#define IN6_IFADDR_RUNLOCK() rw_runlock(&in6_ifaddr_lock) +#define IN6_IFADDR_WLOCK() rw_wlock(&in6_ifaddr_lock) +#define IN6_IFADDR_WLOCK_ASSERT() rw_assert(&in6_ifaddr_lock, RA_WLOCKED) +#define IN6_IFADDR_WUNLOCK() rw_wunlock(&in6_ifaddr_lock) + #define in6_ifstat_inc(ifp, tag) \ do { \ if (ifp) \ diff --git a/sys/netinet6/ip6_input.c b/sys/netinet6/ip6_input.c index 47f83e20a130..b9d594c2bc4b 100644 --- a/sys/netinet6/ip6_input.c +++ b/sys/netinet6/ip6_input.c @@ -150,6 +150,9 @@ extern int udp6_sendspace; extern int udp6_recvspace; #endif +struct rwlock in6_ifaddr_lock; +RW_SYSINIT(in6_ifaddr_lock, &in6_ifaddr_lock, "in6_ifaddr_lock"); + struct pfil_head inet6_pfil_hook; static void ip6_init2(void *); diff --git a/sys/netinet6/nd6.c b/sys/netinet6/nd6.c index 8cfb13537301..0e93aec573da 100644 --- a/sys/netinet6/nd6.c +++ b/sys/netinet6/nd6.c @@ -624,6 +624,8 @@ nd6_timer(void *arg) * in the past the loop was inside prefix expiry processing. * However, from a stricter speci-confrmance standpoint, we should * rather separate address lifetimes and prefix lifetimes. + * + * XXXRW: in6_ifaddrhead locking. */ addrloop: TAILQ_FOREACH_SAFE(ia6, &V_in6_ifaddrhead, ia_link, nia6) { @@ -1328,6 +1330,7 @@ nd6_ioctl(u_long cmd, caddr_t data, struct ifnet *ifp) continue; /* XXX */ /* do we really have to remove addresses as well? */ + /* XXXRW: in6_ifaddrhead locking. */ TAILQ_FOREACH_SAFE(ia, &V_in6_ifaddrhead, ia_link, ia_next) { if ((ia->ia6_flags & IN6_IFF_AUTOCONF) == 0) diff --git a/sys/netinet6/nd6_rtr.c b/sys/netinet6/nd6_rtr.c index c5021f672ca9..c746fcd80740 100644 --- a/sys/netinet6/nd6_rtr.c +++ b/sys/netinet6/nd6_rtr.c @@ -1500,6 +1500,8 @@ pfxlist_onlink_check() * detached. Note, however, that a manually configured address should * always be attached. * The precise detection logic is same as the one for prefixes. + * + * XXXRW: in6_ifaddrhead locking. */ TAILQ_FOREACH(ifa, &V_in6_ifaddrhead, ia_link) { if (!(ifa->ia6_flags & IN6_IFF_AUTOCONF)) @@ -1949,10 +1951,12 @@ in6_tmpifadd(const struct in6_ifaddr *ia0, int forcegen, int delay) * there may be a time lag between generation of the ID and generation * of the address. So, we'll do one more sanity check. */ + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { if (IN6_ARE_ADDR_EQUAL(&ia->ia_addr.sin6_addr, &ifra.ifra_addr.sin6_addr)) { if (trylimit-- == 0) { + IN6_IFADDR_RUNLOCK(); /* * Give up. Something strange should have * happened. @@ -1961,10 +1965,12 @@ in6_tmpifadd(const struct in6_ifaddr *ia0, int forcegen, int delay) "find a unique random IFID\n")); return (EEXIST); } + IN6_IFADDR_RUNLOCK(); forcegen = 1; goto again; } } + IN6_IFADDR_RUNLOCK(); /* * The Valid Lifetime is the lower of the Valid Lifetime of the diff --git a/sys/netipsec/key.c b/sys/netipsec/key.c index 08547a8d58d2..3dc687842f6e 100644 --- a/sys/netipsec/key.c +++ b/sys/netipsec/key.c @@ -3982,10 +3982,13 @@ key_ismyaddr6(sin6) struct in6_multi *in6m; #endif + IN6_IFADDR_RLOCK(); TAILQ_FOREACH(ia, &V_in6_ifaddrhead, ia_link) { if (key_sockaddrcmp((struct sockaddr *)&sin6, - (struct sockaddr *)&ia->ia_addr, 0) == 0) + (struct sockaddr *)&ia->ia_addr, 0) == 0) { + IN6_IFADDR_RUNLOCK(); return 1; + } #if 0 /* @@ -3996,10 +3999,13 @@ key_ismyaddr6(sin6) */ in6m = NULL; IN6_LOOKUP_MULTI(sin6->sin6_addr, ia->ia_ifp, in6m); - if (in6m) + if (in6m) { + IN6_IFADDR_RUNLOCK(); return 1; + } #endif } + IN6_IFADDR_RUNLOCK(); /* loopback, just for safety */ if (IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr))