Re: svn commit: r225947 - head/sys/netinet

[Gleb Smirnoff]

On Sun, Oct 09, 2011 at 10:11:56PM -0700, Qing Li wrote:
Q   What confuses me most, is that in lines 1435-1445 you are
Q  assigning error to a positive value, BUT proceeding further
Q  with function.
Q 
QThis is what was there before (meaning returning error immediately),
Qbut I guess a couple of folks felt it looked a bit cluttered.
QThis is mostly due to the fact the RTFREE_LOCKED() operation
Qhas to be performed before returning.

Well, we can assign error and then goto done label. Assigning error
and continuing processing is confusing, isn't it?

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 
QNot 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.

-- 
Totus tuus, Glebius.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r225947 - head/sys/netinet

[Qing Li]

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.



in.c.diff
Description: Binary data
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r225947 - head/sys/netinet

[Bjoern A. Zeeb]

On 10. Oct 2011, at 08:44 , Qing Li wrote:

Hey,

 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
 QNot 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.

I am a bit confused by this entire thing; it seems it's only parts of what
I had looked at initially but maybe the commit was split due to follow-ups
on an early change.

I am however happy that some things I had mentioned initially are now being
addressed in the cleanup patch.  I have not checked the logic changes on it
however.

Thanks,


Bjoern

-- 
Bjoern A. Zeeb You have to have visions!
 Stop bit received. Insert coin for new address 
family.___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r225947 - head/sys/netinet

[Gleb Smirnoff]

  Qing,

  [cced Bjoern as reviewer]

On Mon, Oct 03, 2011 at 07:51:19PM +, 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.
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);
 }
 
 /*
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

Re: svn commit: r225947 - head/sys/netinet

[Qing Li]

Hi Gleb,


 On Mon, Oct 03, 2011 at 07:51:19PM +, 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.


   This is what was there before (meaning returning error immediately),
   but I guess a couple of folks felt it looked a bit cluttered.
   This is mostly due to the fact the RTFREE_LOCKED() operation
   has to be performed before returning.


 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.


   Not so, only for an indirect prefix route.


 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.


  No, the RTF_GATEWAY check cannot be shifted up because if we did
  that, the (indirect host route, with destination matching the gateway IP)
  would never be executed, if when that set of conditions are true, which is
  allowed and the reason for the patch in the first place.


 Also, in this commit you really do not need the __DECONST hacks.


  Okay, I must had a temporary short circuit. I will fix that.

  Thanks

  --Qing
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org

svn commit: r225947 - head/sys/netinet

[Qing Li]

Author: qingli
Date: Mon Oct  3 19:51:18 2011
New Revision: 225947
URL: http://svn.freebsd.org/changeset/base/225947

Log:
  A system may have multiple physical interfaces, all of which are on the
  same prefix. Since a single route entry is installed for the prefix
  (without RADIX_MPATH), incoming packets on the interfaces that are not
  associated with the prefix route may trigger an error message about
  unable to allocation LLE entry, and fails L2. This patch makes sure a
  valid route is present in the system, and allow the aforementioned
  condition to exist and treats as valid.
  
  Reviewed by:  bz
  MFC after:5 days

Modified:
  head/sys/netinet/in.c

Modified: head/sys/netinet/in.c
==
--- head/sys/netinet/in.c   Mon Oct  3 19:06:55 2011(r225946)
+++ head/sys/netinet/in.c   Mon Oct  3 19:51:18 2011(r225947)
@@ -1439,14 +1439,43 @@ in_lltable_rtcheck(struct ifnet *ifp, u_
if (memcmp(rt-rt_gateway-sa_data, l3addr-sa_data,
sizeof(in_addr_t)) != 0)
error = EINVAL;
-   } else if (!(flags  LLE_PUB)  ((rt-rt_flags  RTF_GATEWAY) ||
-   (rt-rt_ifp != ifp))) {
+   }
+
+   if (rt-rt_flags  RTF_GATEWAY) {
+   RTFREE_LOCKED(rt);
+   return (EINVAL);
+   }
+
+   /*
+* Make sure that at least the destination address is covered 
+* by the route. This is for handling the case where 2 or more 
+* 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;
+   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;
+   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));
+   log(LOG_INFO, IPv4 address: \%s\ is not on 
the network\n,
+   inet_ntoa(((const struct sockaddr_in 
*)l3addr)-sin_addr));
 #endif
-   error = EINVAL;
+   break;
+   }
+   }
}
+
RTFREE_LOCKED(rt);
return (error);
 }
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org