svn commit: r227791 - head/sys/netinet
Gleb Smirnoff
glebius at FreeBSD.org
Mon Nov 21 19:54:42 UTC 2011
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 at 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.
More information about the svn-src-all
mailing list