svn commit: r227791 - head/sys/netinet
Qing Li
qingli at freebsd.org
Mon Nov 21 16:48:33 UTC 2011
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 <glebius at 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
>
More information about the svn-src-all
mailing list