From owner-svn-src-all@FreeBSD.ORG  Mon Nov 21 19:54:42 2011
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
Delivered-To: svn-src-all@FreeBSD.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 5D99F1065673;
	Mon, 21 Nov 2011 19:54:42 +0000 (UTC)
	(envelope-from glebius@FreeBSD.org)
Received: from cell.glebius.int.ru (glebius.int.ru [81.19.64.117])
	by mx1.freebsd.org (Postfix) with ESMTP id 52FB28FC18;
	Mon, 21 Nov 2011 19:54:41 +0000 (UTC)
Received: from cell.glebius.int.ru (localhost [127.0.0.1])
	by cell.glebius.int.ru (8.14.5/8.14.5) with ESMTP id pALJseCf000315;
	Mon, 21 Nov 2011 23:54:40 +0400 (MSK)
	(envelope-from glebius@FreeBSD.org)
Received: (from glebius@localhost)
	by cell.glebius.int.ru (8.14.5/8.14.5/Submit) id pALJsdHd000314;
	Mon, 21 Nov 2011 23:54:39 +0400 (MSK)
	(envelope-from glebius@FreeBSD.org)
X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to
	glebius@FreeBSD.org using -f
Date: Mon, 21 Nov 2011 23:54:39 +0400
From: Gleb Smirnoff <glebius@FreeBSD.org>
To: Qing Li <qingli@FreeBSD.org>
Message-ID: <20111121195439.GE96616@FreeBSD.org>
References: <201111211410.pALEAD9B046139@svn.freebsd.org>
	<CAGnGRdLpWwTkfjirBYe7x-1TVOMtHiRJNX4dM-iQXwQgP3mCVQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAGnGRdLpWwTkfjirBYe7x-1TVOMtHiRJNX4dM-iQXwQgP3mCVQ@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org,
	src-committers@FreeBSD.org
Subject: Re: svn commit: r227791 - head/sys/netinet
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for &quot;
	user&quot; and &quot; projects&quot; \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
	<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all>
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
	<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Mon, 21 Nov 2011 19:54:42 -0000

  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.