Date: Mon, 10 Oct 2011 01:44:30 -0700 From: Qing Li <qingli@freebsd.org> To: Gleb Smirnoff <glebius@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: <CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA@mail.gmail.com> In-Reply-To: <20111010061313.GC94905@FreeBSD.org> References: <201110031951.p93JpJLA025249@svn.freebsd.org> <20111009194732.GB94905@glebius.int.ru> <CAGnGRd%2BUfDYb=3y=8DoeArT7Qp2S1rUyTbQMTjD6cgybyoMKhA@mail.gmail.com> <20111010061313.GC94905@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Okay, now I know what's confusing you ... it's that bug I introduced :-)
The 2nd "if()" check on the RTF_GATEWAY flag should have been
an "else if()".
In a nutshell, the logic is any indirect route should fail the check,
except for that special host route.
Attached is the rework of that part of the patch, with some cleaning up
per your suggestion.
Thank you very much for catching that bug.
--Qing
> Q> > Well, after third review it is clear, that
> Q> > next if() case would definitely be true, and you would proceed
> Q> > with return. But that is difficult to see from first glance.
> Q>
> Q> Not so, only for an indirect prefix route.
> Q>
> Q> > I'd suggest to remove error variable, return immediately in
> Q> > all error cases, and also the RTF_GATEWAY check can be shifted up,
> Q> > since it is the most simple and the most usual to be true.
> Q> >
> Q>
> Q> No, the RTF_GATEWAY check cannot be shifted up because if we did
> Q> that, the (indirect host route, with destination matching the gateway IP)
> Q> would never be executed, if when that set of conditions are true, which is
> Q> allowed and the reason for the patch in the first place.
>
> Can you elaborate on that please? As far as I see, any rtentry that has
> RTF_GATEWAY would return with EINVAL. The first if() clause doesn't
> do any actual processing, only checking flags and memcmp()ing. The third
> clause either. The error is never reset to 0. So, I don't see any
> difference in returning EINVAL for RTF_GATEWAY immediately or later
> after other checks.
>
[-- Attachment #2 --]
Index: in.c
===================================================================
--- in.c (revision 226184)
+++ in.c (working copy)
@@ -1414,8 +1414,6 @@ static int
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));
@@ -1432,21 +1430,16 @@ in_lltable_rtcheck(struct ifnet *ifp, u_int flags,
* 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) {
- RTFREE_LOCKED(rt);
- return (EINVAL);
+ 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 +1448,31 @@ in_lltable_rtcheck(struct ifnet *ifp, u_int flags,
* 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);
}
/*
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGnGRdL8PraLk6BrMoC7XOxKBab_oNrRzvK7NXjVpgRgfeeknA>