svn commit: r267186 - in stable/10: sys/net sys/netinet tests/sys/netinet

Alan Somers asomers at FreeBSD.org
Fri Jun 6 20:35:42 UTC 2014


Author: asomers
Date: Fri Jun  6 20:35:40 2014
New Revision: 267186
URL: http://svnweb.freebsd.org/changeset/base/267186

Log:
  MFC changes relating to running multiple interfaces on different fibs but
  with addresses on the same subnet.
  
  MFC r266860
  
  Fix unintended KBI change from r264905.  Add _fib versions of
  ifa_ifwithnet() and ifa_ifwithdstaddr()  The legacy functions will call the
  _fib() versions with RT_ALL_FIBS, preserving legacy behavior.
  
  sys/net/if_var.h
  sys/net/if.c
          Add legacy-compatible functions as described above.  Ensure legacy
          behavior when RT_ALL_FIBS is passed as fibnum.
  
  sys/netinet/in_pcb.c
  sys/netinet/ip_output.c
  sys/netinet/ip_options.c
  sys/net/route.c
  sys/net/rtsock.c
  sys/netinet6/nd6.c
          Call with _fib() functions if we must use a specific fib, or the
          legacy functions otherwise.
  
  tests/sys/netinet/fibs_test.sh
  tests/sys/netinet/udp_dontroute.c
          Improve the udp_dontroute test.  The bug that this test exercises is
          that ifa_ifwithnet() will return the wrong address, if multiple
          interfaces have addresses on the same subnet but with different
          fibs.  The previous version of the test only considered one possible
          failure mode: that ifa_ifwithnet_fib() might fail to find any
          suitable address at all.  The new version also checks whether
          ifa_ifwithnet_fib() finds the correct address by checking where the
          ARP request goes.
  
  MFC r264917
  
  Style fixes, mostly trailing whitespace elimination.  No functional change.
  
  MFC r264905
  
  Fix subnet and default routes on different FIBs on the same subnet.
  
  These two bugs are closely related.  The root cause is that ifa_ifwithnet
  does not consider FIBs when searching for an interface address.
  
  sys/net/if_var.h
  sys/net/if.c
          Add a fib argument to ifa_ifwithnet and ifa_ifwithdstadddr.  Those
          functions will only return an address whose interface fib equals the
          argument.
  
  sys/net/route.c
          Update calls to ifa_ifwithnet and ifa_ifwithdstaddr with fib
          arguments.
  
  sys/netinet/in.c
          Update in_addprefix to consider the interface fib when adding
          prefixes.  This will prevent it from not adding a subnet route when
          one already exists on a different fib.
  
  sys/net/rtsock.c
  sys/netinet/in_pcb.c
  sys/netinet/ip_output.c
  sys/netinet/ip_options.c
  sys/netinet6/nd6.c
          Add RT_DEFAULT_FIB arguments to ifa_ifwithdstaddr and ifa_ifwithnet.
          In some cases it there wasn't a clear specific fib number to use.
          In others, I was unable to test those functions so I chose
          RT_DEFAULT_FIB to minimize divergence from current behavior.  I will
          fix some of the latter changes along with PR kern/187553.
  
  tests/sys/netinet/fibs_test.sh
  tests/sys/netinet/udp_dontroute.c
  tests/sys/netinet/Makefile
          Revert r263738.  The udp_dontroute test was right all along.
          However, bugs kern/187550 and kern/187553 cancelled each other out
          when it came to this test.  Because of kern/187553, ifa_ifwithnet
          searched the default fib instead of the requested one, but because
          of kern/187550, there was an applicable subnet route on the default
          fib.  The new test added in r263738 doesn't work right, however.  I
          can verify with dtrace that ifa_ifwithnet returned the wrong address
          before I applied this commit, but route(8) miraculously found the
          correct interface to use anyway.  I don't know how.
  
          Clear expected failure messages for kern/187550 and kern/187552.
  
  MFC r263738
  
  tests/sys/netinet/Makefile
  tests/sys/netinet/fibs.sh
          Replace fibs:udp_dontroute with fibs:src_addr_selection_by_subnet.
          The original test was poorly written; it was actually testing
          kern/167947 instead of the desired kern/187553.  The root cause of the
          bug is that ifa_ifwithnet did not have a fib argument.  The new test
          more directly targets that behavior.
  
  tests/sys/netinet/udp_dontroute.c
          Delete the auxilliary binary used by the old test

Modified:
  stable/10/sys/net/if.c
  stable/10/sys/net/if_var.h
  stable/10/sys/net/route.c
  stable/10/sys/netinet/in.c
  stable/10/tests/sys/netinet/fibs_test.sh
  stable/10/tests/sys/netinet/udp_dontroute.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/net/if.c
==============================================================================
--- stable/10/sys/net/if.c	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/sys/net/if.c	Fri Jun  6 20:35:40 2014	(r267186)
@@ -1607,7 +1607,7 @@ done:
  */
 /*ARGSUSED*/
 struct ifaddr *
-ifa_ifwithdstaddr(struct sockaddr *addr)
+ifa_ifwithdstaddr_fib(struct sockaddr *addr, int fibnum)
 {
 	struct ifnet *ifp;
 	struct ifaddr *ifa;
@@ -1616,6 +1616,8 @@ ifa_ifwithdstaddr(struct sockaddr *addr)
 	TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
 		if ((ifp->if_flags & IFF_POINTOPOINT) == 0)
 			continue;
+		if ((fibnum != RT_ALL_FIBS) && (ifp->if_fib != fibnum))
+			continue;
 		IF_ADDR_RLOCK(ifp);
 		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
 			if (ifa->ifa_addr->sa_family != addr->sa_family)
@@ -1635,12 +1637,19 @@ done:
 	return (ifa);
 }
 
+struct ifaddr *
+ifa_ifwithdstaddr(struct sockaddr *addr)
+{
+
+	return (ifa_ifwithdstaddr_fib(addr, RT_ALL_FIBS));
+}
+
 /*
  * Find an interface on a specific network.  If many, choice
  * is most specific found.
  */
 struct ifaddr *
-ifa_ifwithnet(struct sockaddr *addr, int ignore_ptp)
+ifa_ifwithnet_fib(struct sockaddr *addr, int ignore_ptp, int fibnum)
 {
 	struct ifnet *ifp;
 	struct ifaddr *ifa;
@@ -1660,12 +1669,14 @@ ifa_ifwithnet(struct sockaddr *addr, int
 
 	/*
 	 * Scan though each interface, looking for ones that have addresses
-	 * in this address family.  Maintain a reference on ifa_maybe once
-	 * we find one, as we release the IF_ADDR_RLOCK() that kept it stable
-	 * when we move onto the next interface.
+	 * in this address family and the requested fib.  Maintain a reference
+	 * on ifa_maybe once we find one, as we release the IF_ADDR_RLOCK() that
+	 * kept it stable when we move onto the next interface.
 	 */
 	IFNET_RLOCK_NOSLEEP();
 	TAILQ_FOREACH(ifp, &V_ifnet, if_link) {
+		if ((fibnum != RT_ALL_FIBS) && (ifp->if_fib != fibnum))
+			continue;
 		IF_ADDR_RLOCK(ifp);
 		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
 			char *cp, *cp2, *cp3;
@@ -1749,6 +1760,13 @@ done:
 	return (ifa);
 }
 
+struct ifaddr *
+ifa_ifwithnet(struct sockaddr *addr, int ignore_ptp)
+{
+
+	return (ifa_ifwithnet_fib(addr, ignore_ptp, RT_ALL_FIBS));
+}
+
 /*
  * Find an interface address specific to an interface best matching
  * a given address.

Modified: stable/10/sys/net/if_var.h
==============================================================================
--- stable/10/sys/net/if_var.h	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/sys/net/if_var.h	Fri Jun  6 20:35:40 2014	(r267186)
@@ -941,7 +941,9 @@ struct	ifaddr *ifa_ifwithaddr(struct soc
 int		ifa_ifwithaddr_check(struct sockaddr *);
 struct	ifaddr *ifa_ifwithbroadaddr(struct sockaddr *);
 struct	ifaddr *ifa_ifwithdstaddr(struct sockaddr *);
+struct	ifaddr *ifa_ifwithdstaddr_fib(struct sockaddr *, int);
 struct	ifaddr *ifa_ifwithnet(struct sockaddr *, int);
+struct	ifaddr *ifa_ifwithnet_fib(struct sockaddr *, int, int);
 struct	ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr *);
 struct	ifaddr *ifa_ifwithroute_fib(int, struct sockaddr *, struct sockaddr *, u_int);
 struct	ifaddr *ifaof_ifpforaddr(struct sockaddr *, struct ifnet *);

Modified: stable/10/sys/net/route.c
==============================================================================
--- stable/10/sys/net/route.c	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/sys/net/route.c	Fri Jun  6 20:35:40 2014	(r267186)
@@ -589,7 +589,7 @@ rtredirect_fib(struct sockaddr *dst,
 	}
 
 	/* verify the gateway is directly reachable */
-	if ((ifa = ifa_ifwithnet(gateway, 0)) == NULL) {
+	if ((ifa = ifa_ifwithnet_fib(gateway, 0, fibnum)) == NULL) {
 		error = ENETUNREACH;
 		goto out;
 	}
@@ -746,7 +746,7 @@ ifa_ifwithroute_fib(int flags, struct so
 		 */
 		ifa = NULL;
 		if (flags & RTF_HOST)
-			ifa = ifa_ifwithdstaddr(dst);
+			ifa = ifa_ifwithdstaddr_fib(dst, fibnum);
 		if (ifa == NULL)
 			ifa = ifa_ifwithaddr(gateway);
 	} else {
@@ -755,10 +755,10 @@ ifa_ifwithroute_fib(int flags, struct so
 		 * or host, the gateway may still be on the
 		 * other end of a pt to pt link.
 		 */
-		ifa = ifa_ifwithdstaddr(gateway);
+		ifa = ifa_ifwithdstaddr_fib(gateway, fibnum);
 	}
 	if (ifa == NULL)
-		ifa = ifa_ifwithnet(gateway, 0);
+		ifa = ifa_ifwithnet_fib(gateway, 0, fibnum);
 	if (ifa == NULL) {
 		struct rtentry *rt = rtalloc1_fib(gateway, 0, RTF_RNH_LOCKED, fibnum);
 		if (rt == NULL)
@@ -872,7 +872,7 @@ rt_getifa_fib(struct rt_addrinfo *info, 
 	 */
 	if (info->rti_ifp == NULL && ifpaddr != NULL &&
 	    ifpaddr->sa_family == AF_LINK &&
-	    (ifa = ifa_ifwithnet(ifpaddr, 0)) != NULL) {
+	    (ifa = ifa_ifwithnet_fib(ifpaddr, 0, fibnum)) != NULL) {
 		info->rti_ifp = ifa->ifa_ifp;
 		ifa_free(ifa);
 	}

Modified: stable/10/sys/netinet/in.c
==============================================================================
--- stable/10/sys/netinet/in.c	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/sys/netinet/in.c	Fri Jun  6 20:35:40 2014	(r267186)
@@ -908,7 +908,7 @@ in_addprefix(struct in_ifaddr *target, i
 {
 	struct in_ifaddr *ia;
 	struct in_addr prefix, mask, p, m;
-	int error, fibnum;
+	int error;
 
 	if ((flags & RTF_HOST) != 0) {
 		prefix = target->ia_dstaddr.sin_addr;
@@ -919,9 +919,8 @@ in_addprefix(struct in_ifaddr *target, i
 		prefix.s_addr &= mask.s_addr;
 	}
 
-	fibnum = rt_add_addr_allfibs ? RT_ALL_FIBS : target->ia_ifp->if_fib;
-
 	IN_IFADDR_RLOCK();
+	/* Look for an existing address with the same prefix, mask, and fib */
 	TAILQ_FOREACH(ia, &V_in_ifaddrhead, ia_link) {
 		if (rtinitflags(ia)) {
 			p = ia->ia_dstaddr.sin_addr;
@@ -937,6 +936,8 @@ in_addprefix(struct in_ifaddr *target, i
 			    mask.s_addr != m.s_addr)
 				continue;
 		}
+		if (target->ia_ifp->if_fib != ia->ia_ifp->if_fib)
+			continue;
 
 		/*
 		 * If we got a matching prefix route inserted by other
@@ -955,6 +956,10 @@ in_addprefix(struct in_ifaddr *target, i
 				IN_IFADDR_RUNLOCK();
 				return (EEXIST);
 			} else {
+				int fibnum;
+
+				fibnum = rt_add_addr_allfibs ? RT_ALL_FIBS :
+					target->ia_ifp->if_fib;
 				rt_addrmsg(RTM_ADD, &target->ia_ifa, fibnum);
 				IN_IFADDR_RUNLOCK();
 				return (0);

Modified: stable/10/tests/sys/netinet/fibs_test.sh
==============================================================================
--- stable/10/tests/sys/netinet/fibs_test.sh	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/tests/sys/netinet/fibs_test.sh	Fri Jun  6 20:35:40 2014	(r267186)
@@ -176,7 +176,6 @@ default_route_with_multiple_fibs_on_same
 
 default_route_with_multiple_fibs_on_same_subnet_body()
 {
-	atf_expect_fail "kern/187552 default route uses the wrong interface when multiple interfaces have the same subnet but different fibs"
 	# Configure the TAP interfaces to use a RFC5737 nonrouteable addresses
 	# and a non-default fib
 	ADDR0="192.0.2.2"
@@ -226,7 +225,6 @@ subnet_route_with_multiple_fibs_on_same_
 
 subnet_route_with_multiple_fibs_on_same_subnet_body()
 {
-	atf_expect_fail "kern/187550 Multiple interfaces on different FIBs but the same subnet don't all have a subnet route"
 	# Configure the TAP interfaces to use a RFC5737 nonrouteable addresses
 	# and a non-default fib
 	ADDR0="192.0.2.2"
@@ -258,6 +256,15 @@ subnet_route_with_multiple_fibs_on_same_
 # SO_DONTROUTE set that are sent on non-default FIBs.
 # This bug was discovered with "setfib 1 netperf -t UDP_STREAM -H some_host"
 # Regression test for kern/187553
+#
+# The root cause was that ifa_ifwithnet() did not have a fib argument.  It
+# would return an address from an interface on any FIB that had a subnet route
+# for the destination.  If more than one were available, it would choose the
+# most specific.  This is most easily tested by creating a FIB without a
+# default route, then trying to send a UDP packet with SO_DONTROUTE set to an
+# address which is not routable on that FIB.  Absent the fix for this bug,
+# in_pcbladdr would choose an interface on any FIB with a default route.  With
+# the fix, you will get EUNREACH or ENETUNREACH.
 atf_test_case udp_dontroute cleanup
 udp_dontroute_head()
 {
@@ -271,25 +278,38 @@ udp_dontroute_body()
 	atf_expect_fail "kern/187553 Source address selection for UDP packets with SO_DONTROUTE uses the default FIB"
 	# Configure the TAP interface to use an RFC5737 nonrouteable address
 	# and a non-default fib
-	ADDR="192.0.2.2"
+	ADDR0="192.0.2.2"
+	ADDR1="192.0.2.3"
 	SUBNET="192.0.2.0"
 	MASK="24"
 	# Use a different IP on the same subnet as the target
 	TARGET="192.0.2.100"
+	SRCDIR=`atf_get_srcdir`
 
 	# Check system configuration
 	if [ 0 != `sysctl -n net.add_addr_allfibs` ]; then
 		atf_skip "This test requires net.add_addr_allfibs=0"
 	fi
-	get_fibs 1
+	get_fibs 2
 
-	# Configure a TAP interface
-	setup_tap ${FIB0} ${ADDR} ${MASK}
+	# Configure the TAP interfaces
+	setup_tap ${FIB0} ${ADDR0} ${MASK}
+	TARGET_TAP=${TAP}
+	setup_tap ${FIB1} ${ADDR1} ${MASK}
 
 	# Send a UDP packet with SO_DONTROUTE.  In the failure case, it will
-	# return ENETUNREACH
-	SRCDIR=`atf_get_srcdir`
-	atf_check -o ignore setfib ${FIB0} ${SRCDIR}/udp_dontroute ${TARGET}
+	# return ENETUNREACH, or send the packet to the wrong tap
+	atf_check -o ignore setfib ${FIB0} \
+		${SRCDIR}/udp_dontroute ${TARGET} /dev/${TARGET_TAP}
+	cleanup_tap
+
+	# Repeat, but this time target the other tap
+	setup_tap ${FIB0} ${ADDR0} ${MASK}
+	setup_tap ${FIB1} ${ADDR1} ${MASK}
+	TARGET_TAP=${TAP}
+
+	atf_check -o ignore setfib ${FIB1} \
+		${SRCDIR}/udp_dontroute ${TARGET} /dev/${TARGET_TAP}
 }
 
 udp_dontroute_cleanup()
@@ -367,4 +387,5 @@ cleanup_tap()
 	for TAPD in `cat "tap_devices_to_cleanup"`; do
 		ifconfig ${TAPD} destroy
 	done
+	rm "tap_devices_to_cleanup"
 }

Modified: stable/10/tests/sys/netinet/udp_dontroute.c
==============================================================================
--- stable/10/tests/sys/netinet/udp_dontroute.c	Fri Jun  6 20:01:45 2014	(r267185)
+++ stable/10/tests/sys/netinet/udp_dontroute.c	Fri Jun  6 20:35:40 2014	(r267186)
@@ -39,9 +39,11 @@
 
 #include <err.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <unistd.h>
 
 /*
  * Sends a single UDP packet to the provided address, with SO_DONTROUTE set
@@ -51,23 +53,31 @@ int
 main(int argc, char **argv)
 {
 	struct sockaddr_in dst;
-	int s;
+	int s, t;
 	int opt;
 	int ret;
-	const char* buf = "Hello, World!";
+	ssize_t len;
+	const char* sendbuf = "Hello, World!";
+	const size_t buflen = 80;
+	char recvbuf[buflen];
 
-	if (argc != 2) {
-		fprintf(stderr, "Usage: %s ip_address\n", argv[0]);
+	if (argc != 3) {
+		fprintf(stderr, "Usage: %s ip_address tapdev\n", argv[0]);
 		exit(2);
 	}
+
+	t = open(argv[2], O_RDWR | O_NONBLOCK);
+	if (t < 0)
+		err(EXIT_FAILURE, "open");
+
 	s = socket(PF_INET, SOCK_DGRAM, 0);
 	if (s < 0)
-		err(errno, "socket");
+		err(EXIT_FAILURE, "socket");
 	opt = 1;
 
 	ret = setsockopt(s, SOL_SOCKET, SO_DONTROUTE, &opt, sizeof(opt));
 	if (ret == -1)
-		err(errno, "setsockopt(SO_DONTROUTE)");
+		err(EXIT_FAILURE, "setsockopt(SO_DONTROUTE)");
 
 	dst.sin_len = sizeof(dst);
 	dst.sin_family = AF_INET;
@@ -77,10 +87,25 @@ main(int argc, char **argv)
 		fprintf(stderr, "Invalid address: %s\n", argv[1]);
 		exit(2);
 	}
-	ret = sendto(s, buf, strlen(buf), 0, (struct sockaddr*)&dst,
+	ret = sendto(s, sendbuf, strlen(sendbuf), 0, (struct sockaddr*)&dst,
 	    dst.sin_len);
 	if (ret == -1)
-		err(errno, "sendto");
+		err(EXIT_FAILURE, "sendto");
+
+	/* Verify that the packet went to the desired tap device */
 
+	len = read(t, recvbuf, buflen);
+	if (len == 0)
+		errx(EXIT_FAILURE, "read returned EOF");
+	else if (len < 0 && errno == EAGAIN)
+		errx(EXIT_FAILURE, "Did not receive any packets");
+	else if (len < 0)
+		err(EXIT_FAILURE, "read");
+
+	/*
+	 * If read returned anything at all, consider it a success.  The packet
+	 * should be an Ethernet frame containing an ARP request for
+	 * ip_address.  We won't bother to decode it
+	 */
 	return (0);
 }


More information about the svn-src-all mailing list