Module Name:    src
Committed By:   ozaki-r
Date:           Thu Nov 10 04:13:53 UTC 2016

Modified Files:
        src/sys/netinet6: in6_src.c ip6_output.c ip6_var.h

Log Message:
Tidy up in6_select*

This change tidies up in6_select* functions, especially
selectroute.

selectroute is annoying because:
- It returns both/either of a rtentry and/or an ifp
  - Yes, it may return only an ifp!
    - It is valid but selectroute shouldn't handle the case
  - Such conditional behavior makes it difficult
    to apply locking/psref thingy
- It may return a rtentry even if error
- It may use opt->ip6po_nextroute rtcache implicitly
  - The caller can know if it is used
    by rtcache_validate(&opt->ip6po_nextroute)
    but it's racy in MP-safe world
  - Even if it uses opt->ip6po_nextroute, it may
    return a rtentry that isn't derived from the rtcache

The change includes:
- Rename selectroute to in6_selectroute
  - Let a remaining caller of selectroute, in6_selectif,
    use in6_selectroute instead
- Let in6_selectroute return only an rtentry
  - If error, it doesn't return an rtentry
  - A caller gets an ifp from a returned rtentry
- Allow in6_selectroute to modify a passed rtcache
  and a caller can know if opt->ip6po_nextroute is
  used via the rtcache
- Let callers (ip6_output and in6_selectif) handle
  the case that only an ifp is required

Inspired by OpenBSD
Proposed on tech-kern and tech-net
LGTM by roy@


To generate a diff of this commit:
cvs rdiff -u -r1.73 -r1.74 src/sys/netinet6/in6_src.c
cvs rdiff -u -r1.177 -r1.178 src/sys/netinet6/ip6_output.c
cvs rdiff -u -r1.69 -r1.70 src/sys/netinet6/ip6_var.h

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/sys/netinet6/in6_src.c
diff -u src/sys/netinet6/in6_src.c:1.73 src/sys/netinet6/in6_src.c:1.74
--- src/sys/netinet6/in6_src.c:1.73	Mon Oct 31 04:57:10 2016
+++ src/sys/netinet6/in6_src.c	Thu Nov 10 04:13:53 2016
@@ -1,4 +1,4 @@
-/*	$NetBSD: in6_src.c,v 1.73 2016/10/31 04:57:10 ozaki-r Exp $	*/
+/*	$NetBSD: in6_src.c,v 1.74 2016/11/10 04:13:53 ozaki-r Exp $	*/
 /*	$KAME: in6_src.c,v 1.159 2005/10/19 01:40:32 t-momose Exp $	*/
 
 /*
@@ -66,7 +66,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: in6_src.c,v 1.73 2016/10/31 04:57:10 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: in6_src.c,v 1.74 2016/11/10 04:13:53 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -122,9 +122,6 @@ struct in6_addrpolicy defaultaddrpolicy;
 
 int ip6_prefer_tempaddr = 0;
 
-static int selectroute(struct sockaddr_in6 *, struct ip6_pktopts *,
-	struct ip6_moptions *, struct route *, struct ifnet **, struct psref *,
-	struct rtentry **, int, int);
 static int in6_selectif(struct sockaddr_in6 *, struct ip6_pktopts *,
 	struct ip6_moptions *, struct route *, struct ifnet **, struct psref *);
 
@@ -590,28 +587,20 @@ exit:
 #undef PSREF
 }
 
-static int
-selectroute(struct sockaddr_in6 *dstsock, struct ip6_pktopts *opts, 
-	struct ip6_moptions *mopts, struct route *ro, struct ifnet **retifp, 
-	struct psref *psref, struct rtentry **retrt, int clone, int norouteok)
+int
+in6_selectroute(struct sockaddr_in6 *dstsock, struct ip6_pktopts *opts,
+    struct route **ro, struct rtentry **retrt, bool count_discard)
 {
 	int error = 0;
-	struct ifnet *ifp = NULL;
 	struct rtentry *rt = NULL;
-	struct sockaddr_in6 *sin6_next;
-	struct in6_pktinfo *pi = NULL;
-	struct in6_addr *dst;
 	union {
 		struct sockaddr		dst;
 		struct sockaddr_in6	dst6;
 	} u;
 
 	KASSERT(ro != NULL);
-	KASSERT(retifp != NULL && psref != NULL);
 	KASSERT(retrt != NULL);
 
-	dst = &dstsock->sin6_addr;
-
 #if 0
 	if (dstsock->sin6_addr.s6_addr32[0] == 0 &&
 	    dstsock->sin6_addr.s6_addr32[1] == 0 &&
@@ -625,41 +614,13 @@ selectroute(struct sockaddr_in6 *dstsock
 	}
 #endif
 
-	/* If the caller specify the outgoing interface explicitly, use it. */
-	if (opts && (pi = opts->ip6po_pktinfo) != NULL && pi->ipi6_ifindex) {
-		/* XXX boundary check is assumed to be already done. */
-		ifp = if_get_byindex(pi->ipi6_ifindex, psref);
-		if (ifp != NULL &&
-		    (norouteok || IN6_IS_ADDR_MULTICAST(dst))) {
-			/*
-			 * we do not have to check or get the route for
-			 * multicast.
-			 */
-			goto done;
-		} else {
-			if_put(ifp, psref);
-			ifp = NULL;
-			goto getroute;
-		}
-	}
-
-	/*
-	 * If the destination address is a multicast address and the outgoing
-	 * interface for the address is specified by the caller, use it.
-	 */
-	if (IN6_IS_ADDR_MULTICAST(dst) && mopts != NULL) {
-		ifp = if_get_byindex(mopts->im6o_multicast_if_index, psref);
-		if (ifp != NULL)
-			goto done; /* we do not need a route for multicast. */
-	}
-
-  getroute:
 	/*
 	 * If the next hop address for the packet is specified by the caller,
 	 * use it as the gateway.
 	 */
 	if (opts && opts->ip6po_nexthop) {
 		struct route *ron;
+		struct sockaddr_in6 *sin6_next;
 
 		sin6_next = satosin6(opts->ip6po_nexthop);
 
@@ -674,28 +635,19 @@ selectroute(struct sockaddr_in6 *dstsock
 		 * by that address must be a neighbor of the sending host.
 		 */
 		ron = &opts->ip6po_nextroute;
-		if ((rt = rtcache_lookup(ron, sin6tosa(sin6_next))) == NULL ||
-		    (rt->rt_flags & RTF_GATEWAY) != 0 ||
+		rt = rtcache_lookup(ron, sin6tosa(sin6_next));
+		if (rt == NULL || (rt->rt_flags & RTF_GATEWAY) != 0 ||
 		    !nd6_is_addr_neighbor(sin6_next, rt->rt_ifp)) {
 			rtcache_free(ron);
+			if (rt != NULL && count_discard)
+				in6_ifstat_inc(rt->rt_ifp, ifs6_out_discard);
+			rt = NULL;
 			error = EHOSTUNREACH;
 			goto done;
 		}
-		ifp = rt->rt_ifp;
-		if (ifp != NULL) {
-			if (!if_is_deactivated(ifp))
-				if_acquire_NOMPSAFE(ifp, psref);
-			else
-				ifp = NULL;
-		}
+		*ro = ron;
 
-		/*
-		 * When cloning is required, try to allocate a route to the
-		 * destination so that the caller can store path MTU
-		 * information.
-		 */
-		if (!clone)
-			goto done;
+		goto done;
 	}
 
 	/*
@@ -705,27 +657,10 @@ selectroute(struct sockaddr_in6 *dstsock
 	 */
 	u.dst6 = *dstsock;
 	u.dst6.sin6_scope_id = 0;
-	rt = rtcache_lookup1(ro, &u.dst, clone);
-
-	/*
-	 * do not care about the result if we have the nexthop
-	 * explicitly specified.
-	 */
-	if (opts && opts->ip6po_nexthop)
-		goto done;
+	rt = rtcache_lookup1(*ro, &u.dst, 1);
 
 	if (rt == NULL)
 		error = EHOSTUNREACH;
-	else {
-		if_put(ifp, psref);
-		ifp = rt->rt_ifp;
-		if (ifp != NULL) {
-			if (!if_is_deactivated(ifp))
-				if_acquire_NOMPSAFE(ifp, psref);
-			else
-				ifp = NULL;
-		}
-	}
 
 	/*
 	 * Check if the outgoing interface conflicts with
@@ -735,28 +670,20 @@ selectroute(struct sockaddr_in6 *dstsock
 	 *  our own addresses.)
 	 */
 	if (opts && opts->ip6po_pktinfo && opts->ip6po_pktinfo->ipi6_ifindex) {
-		if (!(ifp->if_flags & IFF_LOOPBACK) &&
-		    ifp->if_index != opts->ip6po_pktinfo->ipi6_ifindex) {
+		if (!(rt->rt_ifp->if_flags & IFF_LOOPBACK) &&
+		    rt->rt_ifp->if_index != opts->ip6po_pktinfo->ipi6_ifindex) {
+			if (rt != NULL && count_discard)
+				in6_ifstat_inc(rt->rt_ifp, ifs6_out_discard);
 			error = EHOSTUNREACH;
-			goto done;
+			rt = NULL;
 		}
 	}
 
-  done:
-	if (ifp == NULL && rt == NULL) {
-		/*
-		 * This can happen if the caller did not pass a cached route
-		 * nor any other hints.  We treat this case an error.
-		 */
-		error = EHOSTUNREACH;
-	}
+done:
 	if (error == EHOSTUNREACH)
 		IP6_STATINC(IP6_STAT_NOROUTE);
-
-	*retifp = ifp;
-	*retrt = rt;	/* rt may be NULL */
-
-	return (error);
+	*retrt = rt;
+	return error;
 }
 
 static int
@@ -764,19 +691,42 @@ in6_selectif(struct sockaddr_in6 *dstsoc
 	struct ip6_moptions *mopts, struct route *ro, struct ifnet **retifp,
 	struct psref *psref)
 {
-	int error, clone;
+	int error;
 	struct rtentry *rt = NULL;
+	struct in6_addr *dst;
+	struct in6_pktinfo *pi = NULL;
 
 	KASSERT(retifp != NULL);
 	*retifp = NULL;
+	dst = &dstsock->sin6_addr;
 
-	clone = IN6_IS_ADDR_MULTICAST(&dstsock->sin6_addr) ? 0 : 1;
-	if ((error = selectroute(dstsock, opts, mopts, ro, retifp, psref,
-	    &rt, clone, 1)) != 0) {
-		return (error);
+	/* If the caller specify the outgoing interface explicitly, use it. */
+	if (opts && (pi = opts->ip6po_pktinfo) != NULL && pi->ipi6_ifindex) {
+		/* XXX boundary check is assumed to be already done. */
+		*retifp = if_get_byindex(pi->ipi6_ifindex, psref);
+		if (*retifp != NULL)
+			return 0;
+		goto getroute;
 	}
 
 	/*
+	 * If the destination address is a multicast address and the outgoing
+	 * interface for the address is specified by the caller, use it.
+	 */
+	if (IN6_IS_ADDR_MULTICAST(dst) && mopts != NULL) {
+		*retifp = if_get_byindex(mopts->im6o_multicast_if_index, psref);
+		if (*retifp != NULL)
+			return 0; /* we do not need a route for multicast. */
+	}
+
+getroute:
+	error = in6_selectroute(dstsock, opts, &ro, &rt, false);
+	if (error != 0)
+		return error;
+
+	*retifp = if_get_byindex(rt->rt_ifp->if_index, psref);
+
+	/*
 	 * do not use a rejected or black hole route.
 	 * XXX: this check should be done in the L2 output routine.
 	 * However, if we skipped this check here, we'd see the following
@@ -793,7 +743,7 @@ in6_selectif(struct sockaddr_in6 *dstsoc
 	 * Although this may not be very harmful, it should still be confusing.
 	 * We thus reject the case here.
 	 */
-	if (rt && (rt->rt_flags & (RTF_REJECT | RTF_BLACKHOLE)))
+	if ((rt->rt_flags & (RTF_REJECT | RTF_BLACKHOLE)))
 		return (rt->rt_flags & RTF_HOST ? EHOSTUNREACH : ENETUNREACH);
 
 	/*
@@ -803,7 +753,7 @@ in6_selectif(struct sockaddr_in6 *dstsoc
 	 * destination address (which should probably be one of our own
 	 * addresses.)
 	 */
-	if (rt && rt->rt_ifa && rt->rt_ifa->ifa_ifp &&
+	if (rt->rt_ifa && rt->rt_ifa->ifa_ifp &&
 	    rt->rt_ifa->ifa_ifp != *retifp &&
 	    !if_is_deactivated(rt->rt_ifa->ifa_ifp)) {
 		if_put(*retifp, psref);
@@ -815,19 +765,6 @@ in6_selectif(struct sockaddr_in6 *dstsoc
 }
 
 /*
- * close - meaningful only for bsdi and freebsd.
- */
-
-int
-in6_selectroute(struct sockaddr_in6 *dstsock, struct ip6_pktopts *opts, 
-	struct ip6_moptions *mopts, struct route *ro, struct ifnet **retifp, 
-	struct psref *psref, struct rtentry **retrt, int clone)
-{
-	return selectroute(dstsock, opts, mopts, ro, retifp, psref,
-	    retrt, clone, 0);
-}
-
-/*
  * Default hop limit selection. The precedence is as follows:
  * 1. Hoplimit value specified via ioctl.
  * 2. (If the outgoing interface is detected) the current

Index: src/sys/netinet6/ip6_output.c
diff -u src/sys/netinet6/ip6_output.c:1.177 src/sys/netinet6/ip6_output.c:1.178
--- src/sys/netinet6/ip6_output.c:1.177	Mon Nov  7 01:55:17 2016
+++ src/sys/netinet6/ip6_output.c	Thu Nov 10 04:13:53 2016
@@ -1,4 +1,4 @@
-/*	$NetBSD: ip6_output.c,v 1.177 2016/11/07 01:55:17 ozaki-r Exp $	*/
+/*	$NetBSD: ip6_output.c,v 1.178 2016/11/10 04:13:53 ozaki-r Exp $	*/
 /*	$KAME: ip6_output.c,v 1.172 2001/03/25 09:55:56 itojun Exp $	*/
 
 /*
@@ -62,7 +62,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: ip6_output.c,v 1.177 2016/11/07 01:55:17 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: ip6_output.c,v 1.178 2016/11/10 04:13:53 ozaki-r Exp $");
 
 #ifdef _KERNEL_OPT
 #include "opt_inet.h"
@@ -408,6 +408,9 @@ ip6_output(
 		    sizeof(struct ip6_hdr) + optlen);
 	}
 
+	/* Need to save for pmtu */
+	finaldst = ip6->ip6_dst;
+
 	/*
 	 * If there is a routing header, replace destination address field
 	 * with the first hop of the routing header.
@@ -417,7 +420,6 @@ ip6_output(
 
 		rh = (struct ip6_rthdr *)(mtod(exthdrs.ip6e_rthdr,
 		    struct ip6_rthdr *));
-		finaldst = ip6->ip6_dst; /* need to save for pmtu */
 
 		error = ip6_handle_rthdr(rh, ip6);
 		if (error != 0)
@@ -499,12 +501,31 @@ ip6_output(
 	ip6 = mtod(m, struct ip6_hdr *);
 
 	sockaddr_in6_init(&dst_sa, &ip6->ip6_dst, 0, 0, 0);
-	if ((error = in6_selectroute(&dst_sa, opt, im6o, ro,
-	    &ifp, &psref, &rt, 0)) != 0) {
-		if (ifp != NULL)
-			in6_ifstat_inc(ifp, ifs6_out_discard);
-		goto bad;
+
+	/* We do not need a route for multicast */
+	if (IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst)) {
+		struct in6_pktinfo *pi = NULL;
+
+		/*
+		 * If the outgoing interface for the address is specified by
+		 * the caller, use it.
+		 */
+		if (opt && (pi = opt->ip6po_pktinfo) != NULL) {
+			/* XXX boundary check is assumed to be already done. */
+			ifp = if_get_byindex(pi->ipi6_ifindex, &psref);
+		} else if (im6o != NULL) {
+			ifp = if_get_byindex(im6o->im6o_multicast_if_index,
+			    &psref);
+		}
 	}
+
+	if (ifp == NULL) {
+		error = in6_selectroute(&dst_sa, opt, &ro, &rt, true);
+		if (error != 0)
+			goto bad;
+		ifp = if_get_byindex(rt->rt_ifp->if_index, &psref);
+	}
+
 	if (rt == NULL) {
 		/*
 		 * If in6_selectroute() does not return a route entry,
@@ -578,18 +599,9 @@ ip6_output(
 		goto bad;
 	}
 
-	if (rt == NULL || IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst)) {
-		dst = satocsin6(rtcache_getdst(ro));
-		KASSERT(dst != NULL);
-	} else if (opt && rtcache_validate(&opt->ip6po_nextroute) != NULL) {
-		/*
-		 * The nexthop is explicitly specified by the
-		 * application.  We assume the next hop is an IPv6
-		 * address.
-		 */
-		dst = (struct sockaddr_in6 *)opt->ip6po_nexthop;
-	} else if ((rt->rt_flags & RTF_GATEWAY))
-		dst = (struct sockaddr_in6 *)rt->rt_gateway;
+	if (rt != NULL && (rt->rt_flags & RTF_GATEWAY) &&
+	    !IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst))
+		dst = satocsin6(rt->rt_gateway);
 	else
 		dst = satocsin6(rtcache_getdst(ro));
 

Index: src/sys/netinet6/ip6_var.h
diff -u src/sys/netinet6/ip6_var.h:1.69 src/sys/netinet6/ip6_var.h:1.70
--- src/sys/netinet6/ip6_var.h:1.69	Mon Oct 31 04:16:25 2016
+++ src/sys/netinet6/ip6_var.h	Thu Nov 10 04:13:53 2016
@@ -1,4 +1,4 @@
-/*	$NetBSD: ip6_var.h,v 1.69 2016/10/31 04:16:25 ozaki-r Exp $	*/
+/*	$NetBSD: ip6_var.h,v 1.70 2016/11/10 04:13:53 ozaki-r Exp $	*/
 /*	$KAME: ip6_var.h,v 1.33 2000/06/11 14:59:20 jinmei Exp $	*/
 
 /*
@@ -400,8 +400,7 @@ int	in6_selectsrc(struct sockaddr_in6 *,
 	   struct ip6_moptions *, struct route *, struct in6_addr *,
 	   struct ifnet **, struct psref *, struct in6_addr *);
 int in6_selectroute(struct sockaddr_in6 *, struct ip6_pktopts *,
-	struct ip6_moptions *, struct route *, struct ifnet **,
-	struct psref *, struct rtentry **, int);
+	struct route **, struct rtentry **, bool);
 int	ip6_get_membership(const struct sockopt *, struct ifnet **, void *,
 	size_t);
 

Reply via email to