Date: Mon, 21 Nov 2011 23:54:39 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Qing Li <qingli@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r227791 - head/sys/netinet Message-ID: <20111121195439.GE96616@FreeBSD.org> In-Reply-To: <CAGnGRdLpWwTkfjirBYe7x-1TVOMtHiRJNX4dM-iQXwQgP3mCVQ@mail.gmail.com> References: <201111211410.pALEAD9B046139@svn.freebsd.org> <CAGnGRdLpWwTkfjirBYe7x-1TVOMtHiRJNX4dM-iQXwQgP3mCVQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Qing, On Mon, Nov 21, 2011 at 08:23:31AM -0800, Qing Li wrote: Q> Logically speaking the prefix route should not be removed until all of the Q> address related housing keeping tasks have been completed successfully. >From my point of view logically speaking, we should first remove route, then remove address. Otherwise, for a short time we've got an invalid route in table. Q> Putting "in_scrubprefix()" at the top does not gain you anything at Q> all, but can Q> potentially be problematic if additional tasks are in fact performed Q> in "if_ioctl()" Q> that may in fact affect the logic in "in_ifinit()". Q> Q> Case in point, I had to perform additional routing related tasks so I Q> re-introduced "nd6_rtrequest()" in r227460. Pardon, can you please elaborate on this? I don't see any problems that I intoroduced, but if ther are any, we can either push in_scrubprefix() down the function as it was before, of fix them some other way. Q> You are not simplifying much logic by removing the error recovery code from Q> the return of "if_ioctl()". So why removing the flexibility of what Q> "if_ioctl()" is Q> intended for as part of the address configuration logic ? Because in_ifinit() was inconsistent. It tried to recover in case of (*ifp->if_ioctl) failure, but did not try to recover in case of failure of: in_addprefix() ifa_add_loopback_route() Q> --Qing Q> Q> On Mon, Nov 21, 2011 at 6:10 AM, Gleb Smirnoff <glebius@freebsd.org> wrote: Q> > Author: glebius Q> > Date: Mon Nov 21 14:10:13 2011 Q> > New Revision: 227791 Q> > URL: http://svn.freebsd.org/changeset/base/227791 Q> > Q> > Log: Q> > Historically in_control() did not check sockaddrs supplied with Q> > structs ifreq/in_aliasreq and there've been several panics due Q> > to that problem. All these panics were fixed just a couple of Q> > lines above the panicing code. Q> > Q> > Take a more general approach: sanity check sockaddrs supplied Q> > with SIOCAIFADDR and SIOCSIF*ADDR at the beggining of the Q> > function and drop all checks below. Q> > Q> > One check is now disabled due to strange code in ifconfig(8) Q> > that I've removed recently. I'm going to enable it with next Q> > __FreeBSD_version bump. Q> > Q> > Historically in_ifinit() was able to recover from an error Q> > and restore old address. Nowadays this feature isn't working Q> > for all error cases, but for some of them. I suppose no software Q> > relies on this behavior, so I'd like to remove it, since this Q> > simplifies code a lot. Q> > Q> > Also, move if_scrub() earlier in the in_ifinit(). It is more Q> > correct to wipe routes before removing address from local Q> > address list, and interface address list. Q> > Q> > Silence from: bz, brooks, andre, rwatson, 3 weeks Q> > Q> > Modified: Q> > head/sys/netinet/in.c Q> > Q> > Modified: head/sys/netinet/in.c Q> > ============================================================================== Q> > --- head/sys/netinet/in.c Mon Nov 21 13:40:35 2011 (r227790) Q> > +++ head/sys/netinet/in.c Mon Nov 21 14:10:13 2011 (r227791) Q> > @@ -234,16 +234,42 @@ in_control(struct socket *so, u_long cmd Q> > * in_lifaddr_ioctl() and ifp->if_ioctl(). Q> > */ Q> > switch (cmd) { Q> > - case SIOCAIFADDR: Q> > - case SIOCDIFADDR: Q> > case SIOCGIFADDR: Q> > case SIOCGIFBRDADDR: Q> > case SIOCGIFDSTADDR: Q> > case SIOCGIFNETMASK: Q> > + case SIOCDIFADDR: Q> > + break; Q> > + case SIOCAIFADDR: Q> > + /* Q> > + * ifra_addr must be present and be of INET family. Q> > + * ifra_broadaddr and ifra_mask are optional. Q> > + */ Q> > + if (ifra->ifra_addr.sin_len != sizeof(struct sockaddr_in) || Q> > + ifra->ifra_addr.sin_family != AF_INET) Q> > + return (EINVAL); Q> > + if (ifra->ifra_broadaddr.sin_len != 0 && Q> > + (ifra->ifra_broadaddr.sin_len != sizeof(struct sockaddr_in) || Q> > + ifra->ifra_broadaddr.sin_family != AF_INET)) Q> > + return (EINVAL); Q> > +#if 0 Q> > + /* Q> > + * ifconfig(8) historically doesn't set af_family for mask Q> > + * for unknown reason. Q> > + */ Q> > + if (ifra->ifra_mask.sin_len != 0 && Q> > + (ifra->ifra_mask.sin_len != sizeof(struct sockaddr_in) || Q> > + ifra->ifra_mask.sin_family != AF_INET)) Q> > + return (EINVAL); Q> > +#endif Q> > + break; Q> > case SIOCSIFADDR: Q> > case SIOCSIFBRDADDR: Q> > case SIOCSIFDSTADDR: Q> > case SIOCSIFNETMASK: Q> > + if (ifr->ifr_addr.sa_family != AF_INET || Q> > + ifr->ifr_addr.sa_len != sizeof(struct sockaddr_in)) Q> > + return (EINVAL); Q> > break; Q> > Q> > case SIOCALIFADDR: Q> > @@ -349,7 +375,7 @@ in_control(struct socket *so, u_long cmd Q> > switch (cmd) { Q> > case SIOCAIFADDR: Q> > case SIOCDIFADDR: Q> > - if (ifra->ifra_addr.sin_family == AF_INET) { Q> > + { Q> > struct in_ifaddr *oia; Q> > Q> > IN_IFADDR_RLOCK(); Q> > @@ -506,7 +532,7 @@ in_control(struct socket *so, u_long cmd Q> > goto out; Q> > Q> > case SIOCSIFNETMASK: Q> > - ia->ia_sockmask.sin_addr = ifra->ifra_addr.sin_addr; Q> > + ia->ia_sockmask = *(struct sockaddr_in *)&ifr->ifr_addr; Q> > ia->ia_subnetmask = ntohl(ia->ia_sockmask.sin_addr.s_addr); Q> > goto out; Q> > Q> > @@ -514,14 +540,12 @@ in_control(struct socket *so, u_long cmd Q> > maskIsNew = 0; Q> > hostIsNew = 1; Q> > error = 0; Q> > - if (ia->ia_addr.sin_family == AF_INET) { Q> > - if (ifra->ifra_addr.sin_len == 0) { Q> > - ifra->ifra_addr = ia->ia_addr; Q> > - hostIsNew = 0; Q> > - } else if (ifra->ifra_addr.sin_addr.s_addr == Q> > - ia->ia_addr.sin_addr.s_addr) Q> > - hostIsNew = 0; Q> > - } Q> > + if (ifra->ifra_addr.sin_len == 0) { Q> > + ifra->ifra_addr = ia->ia_addr; Q> > + hostIsNew = 0; Q> > + } else if (ifra->ifra_addr.sin_addr.s_addr == Q> > + ia->ia_addr.sin_addr.s_addr) Q> > + hostIsNew = 0; Q> > if (ifra->ifra_mask.sin_len) { Q> > /* Q> > * QL: XXX Q> > @@ -552,7 +576,7 @@ in_control(struct socket *so, u_long cmd Q> > break; Q> > Q> > if ((ifp->if_flags & IFF_BROADCAST) && Q> > - (ifra->ifra_broadaddr.sin_family == AF_INET)) Q> > + ifra->ifra_broadaddr.sin_len) Q> > ia->ia_broadaddr = ifra->ifra_broadaddr; Q> > if (error == 0) { Q> > ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); Q> > @@ -608,31 +632,26 @@ in_control(struct socket *so, u_long cmd Q> > Q> > IN_IFADDR_WLOCK(); Q> > TAILQ_REMOVE(&V_in_ifaddrhead, ia, ia_link); Q> > - if (ia->ia_addr.sin_family == AF_INET) { Q> > - struct in_ifaddr *if_ia; Q> > Q> > - LIST_REMOVE(ia, ia_hash); Q> > - IN_IFADDR_WUNLOCK(); Q> > - /* Q> > - * If this is the last IPv4 address configured on this Q> > - * interface, leave the all-hosts group. Q> > - * No state-change report need be transmitted. Q> > - */ Q> > - if_ia = NULL; Q> > - IFP_TO_IA(ifp, if_ia); Q> > - if (if_ia == NULL) { Q> > - ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); Q> > - IN_MULTI_LOCK(); Q> > - if (ii->ii_allhosts) { Q> > - (void)in_leavegroup_locked(ii->ii_allhosts, Q> > - NULL); Q> > - ii->ii_allhosts = NULL; Q> > - } Q> > - IN_MULTI_UNLOCK(); Q> > - } else Q> > - ifa_free(&if_ia->ia_ifa); Q> > + LIST_REMOVE(ia, ia_hash); Q> > + IN_IFADDR_WUNLOCK(); Q> > + /* Q> > + * If this is the last IPv4 address configured on this Q> > + * interface, leave the all-hosts group. Q> > + * No state-change report need be transmitted. Q> > + */ Q> > + IFP_TO_IA(ifp, iap); Q> > + if (iap == NULL) { Q> > + ii = ((struct in_ifinfo *)ifp->if_afdata[AF_INET]); Q> > + IN_MULTI_LOCK(); Q> > + if (ii->ii_allhosts) { Q> > + (void)in_leavegroup_locked(ii->ii_allhosts, NULL); Q> > + ii->ii_allhosts = NULL; Q> > + } Q> > + IN_MULTI_UNLOCK(); Q> > } else Q> > - IN_IFADDR_WUNLOCK(); Q> > + ifa_free(&iap->ia_ifa); Q> > + Q> > ifa_free(&ia->ia_ifa); /* in_ifaddrhead */ Q> > out: Q> > if (ia != NULL) Q> > @@ -828,50 +847,29 @@ in_ifinit(struct ifnet *ifp, struct in_i Q> > int scrub) Q> > { Q> > register u_long i = ntohl(sin->sin_addr.s_addr); Q> > - struct sockaddr_in oldaddr; Q> > int flags = RTF_UP, error = 0; Q> > Q> > - oldaddr = ia->ia_addr; Q> > - if (oldaddr.sin_family == AF_INET) Q> > + if (scrub) Q> > + in_scrubprefix(ia, LLE_STATIC); Q> > + Q> > + IN_IFADDR_WLOCK(); Q> > + if (ia->ia_addr.sin_family == AF_INET) Q> > LIST_REMOVE(ia, ia_hash); Q> > ia->ia_addr = *sin; Q> > - if (ia->ia_addr.sin_family == AF_INET) { Q> > - IN_IFADDR_WLOCK(); Q> > - LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), Q> > - ia, ia_hash); Q> > - IN_IFADDR_WUNLOCK(); Q> > - } Q> > + LIST_INSERT_HEAD(INADDR_HASH(ia->ia_addr.sin_addr.s_addr), Q> > + ia, ia_hash); Q> > + IN_IFADDR_WUNLOCK(); Q> > + Q> > /* Q> > * Give the interface a chance to initialize Q> > * if this is its first address, Q> > * and to validate the address if necessary. Q> > */ Q> > - if (ifp->if_ioctl != NULL) { Q> > - error = (*ifp->if_ioctl)(ifp, SIOCSIFADDR, (caddr_t)ia); Q> > - if (error) { Q> > + if (ifp->if_ioctl != NULL && Q> > + (error = (*ifp->if_ioctl)(ifp, SIOCSIFADDR, (caddr_t)ia)) != 0) Q> > /* LIST_REMOVE(ia, ia_hash) is done in in_control */ Q> > - ia->ia_addr = oldaddr; Q> > - IN_IFADDR_WLOCK(); Q> > - if (ia->ia_addr.sin_family == AF_INET) Q> > - LIST_INSERT_HEAD(INADDR_HASH( Q> > - ia->ia_addr.sin_addr.s_addr), ia, ia_hash); Q> > - else Q> > - /* Q> > - * If oldaddr family is not AF_INET (e.g. Q> > - * interface has been just created) in_control Q> > - * does not call LIST_REMOVE, and we end up Q> > - * with bogus ia entries in hash Q> > - */ Q> > - LIST_REMOVE(ia, ia_hash); Q> > - IN_IFADDR_WUNLOCK(); Q> > return (error); Q> > - } Q> > - } Q> > - if (scrub) { Q> > - ia->ia_ifa.ifa_addr = (struct sockaddr *)&oldaddr; Q> > - in_ifscrub(ifp, ia, LLE_STATIC); Q> > - ia->ia_ifa.ifa_addr = (struct sockaddr *)&ia->ia_addr; Q> > - } Q> > + Q> > /* Q> > * Be compatible with network classes, if netmask isn't supplied, Q> > * guess it based on classes. Q> > @@ -916,11 +914,9 @@ in_ifinit(struct ifnet *ifp, struct in_i Q> > if (ia->ia_addr.sin_addr.s_addr == INADDR_ANY) Q> > return (0); Q> > Q> > - if (ifp->if_flags & IFF_POINTOPOINT) { Q> > - if (ia->ia_dstaddr.sin_addr.s_addr == ia->ia_addr.sin_addr.s_addr) Q> > + if (ifp->if_flags & IFF_POINTOPOINT && Q> > + ia->ia_dstaddr.sin_addr.s_addr == ia->ia_addr.sin_addr.s_addr) Q> > return (0); Q> > - } Q> > - Q> > Q> > /* Q> > * add a loopback route to self Q> > -- Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111121195439.GE96616>