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