Date: Sun, 9 Oct 2011 23:47:32 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Qing Li <qingli@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, bz@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r225947 - head/sys/netinet Message-ID: <20111009194732.GB94905@glebius.int.ru> In-Reply-To: <201110031951.p93JpJLA025249@svn.freebsd.org> References: <201110031951.p93JpJLA025249@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--jI8keyz6grp/JLjh Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Qing, [cced Bjoern as reviewer] On Mon, Oct 03, 2011 at 07:51:19PM +0000, Qing Li wrote: Q> Author: qingli Q> Date: Mon Oct 3 19:51:18 2011 Q> New Revision: 225947 Q> URL: http://svn.freebsd.org/changeset/base/225947 Q> Q> Log: Q> A system may have multiple physical interfaces, all of which are on the Q> same prefix. Since a single route entry is installed for the prefix Q> (without RADIX_MPATH), incoming packets on the interfaces that are not Q> associated with the prefix route may trigger an error message about Q> unable to allocation LLE entry, and fails L2. This patch makes sure a Q> valid route is present in the system, and allow the aforementioned Q> condition to exist and treats as valid. Q> Q> Reviewed by: bz Q> MFC after: 5 days this commit together with r225946 makes the in_lltable_rtcheck() quite difficult to understand. What confuses me most, is that in lines 1435-1445 you are assigning error to a positive value, BUT proceeding further with function. Well, after third review it is clear, that next if() case would definitely be true, and you would proceed with return. But that is difficult to see from first glance. I'd suggest to remove error variable, return immediately in all error cases, and also the RTF_GATEWAY check can be shifted up, since it is the most simple and the most usual to be true. Also, in this commit you really do not need the __DECONST hacks. Here is a snap, only compile-tested patch. -- Totus tuus, Glebius. --jI8keyz6grp/JLjh Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="in.c.diff" Index: in.c =================================================================== --- in.c (revision 226163) +++ in.c (working copy) @@ -1414,8 +1414,6 @@ in_lltable_rtcheck(struct ifnet *ifp, u_int flags, const struct sockaddr *l3addr) { struct rtentry *rt; - struct ifnet *xifp; - int error = 0; KASSERT(l3addr->sa_family == AF_INET, ("sin_family %d", l3addr->sa_family)); @@ -1426,25 +1424,22 @@ if (rt == NULL) return (EINVAL); + if (rt->rt_flags & RTF_GATEWAY) { + RTFREE_LOCKED(rt); + return (EINVAL); + } + /* * If the gateway for an existing host route matches the target L3 * address, which is a special route inserted by some implementation * such as MANET, and the interface is of the correct type, then * allow for ARP to proceed. */ - if (rt->rt_flags & (RTF_GATEWAY | RTF_HOST)) { - xifp = rt->rt_ifp; - - if (xifp && (xifp->if_type != IFT_ETHER || - (xifp->if_flags & (IFF_NOARP | IFF_STATICARP)) != 0)) - error = EINVAL; - - if (memcmp(rt->rt_gateway->sa_data, l3addr->sa_data, - sizeof(in_addr_t)) != 0) - error = EINVAL; - } - - if (rt->rt_flags & RTF_GATEWAY) { + if (rt->rt_flags & RTF_HOST && + ((rt->rt_ifp && (rt->rt_ifp->if_type != IFT_ETHER || + (rt->rt_ifp->if_flags & (IFF_NOARP | IFF_STATICARP)) != 0)) || + (memcmp(rt->rt_gateway->sa_data, l3addr->sa_data, + sizeof(in_addr_t)) != 0))) { RTFREE_LOCKED(rt); return (EINVAL); } @@ -1455,32 +1450,31 @@ * interfaces have the same prefix. An incoming packet arrives * on one interface and the corresponding outgoing packet leaves * another interface. - * */ if (rt->rt_ifp != ifp) { - char *sa, *mask, *addr, *lim; + const char *sa, *mask, *addr, *lim; int len; - sa = (char *)rt_key(rt); - mask = (char *)rt_mask(rt); - addr = (char *)__DECONST(struct sockaddr *, l3addr); - len = ((struct sockaddr_in *)__DECONST(struct sockaddr *, l3addr))->sin_len; + sa = (const char *)rt_key(rt); + mask = (const char *)rt_mask(rt); + addr = (const char *)l3addr; + len = ((const struct sockaddr_in *)l3addr)->sin_len; lim = addr + len; for ( ; addr < lim; sa++, mask++, addr++) { if ((*sa ^ *addr) & *mask) { - error = EINVAL; #ifdef DIAGNOSTIC log(LOG_INFO, "IPv4 address: \"%s\" is not on the network\n", inet_ntoa(((const struct sockaddr_in *)l3addr)->sin_addr)); #endif - break; + RTFREE_LOCKED(rt); + return (EINVAL); } } } RTFREE_LOCKED(rt); - return (error); + return (0); } /* --jI8keyz6grp/JLjh--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111009194732.GB94905>