[PATCH net-next 00/10] ipv6: Refactor nexthop selection helpers during a fib lookup

[PATCH net-next 00/10] ipv6: Refactor nexthop selection helpers during a fib lookup

[David Ahern]

From: David Ahern <dsahern@gmail.com>

IPv6 has a fib6_nh embedded within each fib6_info and a separate
fib6_info for each path in a multipath route. A side effect is that
a fib6_info is passed all the way down the stack when selecting a path
on a fib lookup. Refactor the fib lookup functions and associated
helper functions to take a fib6_nh when appropriate to enable IPv6
to work with nexthop objects where the fib6_nh is not directly part
of a fib entry.

David Ahern (10):
  ipv6: only call rt6_check_neigh for nexthop with gateway
  ipv6: Remove rt6_check_dev
  ipv6: Change rt6_probe to take a fib6_nh
  ipv6: Pass fib6_nh and flags to rt6_score_route
  ipv6: Refactor find_match
  ipv6: Refactor find_rr_leaf
  ipv6: Be smarter with null_entry handling in ip6_pol_route_lookup
  ipv6: Move fib6_multipath_select down in ip6_pol_route
  ipv6: Refactor rt6_device_match
  ipv6: Refactor __ip6_route_redirect

 include/net/ip6_fib.h |   8 +-
 net/ipv6/route.c      | 266 +++++++++++++++++++++++++++-----------------------
 2 files changed, 149 insertions(+), 125 deletions(-)

-- 
2.11.0

[PATCH net-next 01/10] ipv6: Only call rt6_check_neigh for nexthop with gateway

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Change rt6_check_neigh to take a fib6_nh instead of a fib entry.
Move the check on fib_flags and whether the nexthop has a gateway
up to the one caller.

Remove the inline from the definition as well. Not necessary.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 69f92d2b780e..b515fa8f787e 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -589,18 +589,14 @@ static inline int rt6_check_dev(struct fib6_info *rt, int oif)
 	return 0;
 }
 
-static inline enum rt6_nud_state rt6_check_neigh(struct fib6_info *rt)
+static enum rt6_nud_state rt6_check_neigh(const struct fib6_nh *fib6_nh)
 {
 	enum rt6_nud_state ret = RT6_NUD_FAIL_HARD;
 	struct neighbour *neigh;
 
-	if (rt->fib6_flags & RTF_NONEXTHOP ||
-	    !rt->fib6_nh.fib_nh_gw_family)
-		return RT6_NUD_SUCCEED;
-
 	rcu_read_lock_bh();
-	neigh = __ipv6_neigh_lookup_noref(rt->fib6_nh.fib_nh_dev,
-					  &rt->fib6_nh.fib_nh_gw6);
+	neigh = __ipv6_neigh_lookup_noref(fib6_nh->fib_nh_dev,
+					  &fib6_nh->fib_nh_gw6);
 	if (neigh) {
 		read_lock(&neigh->lock);
 		if (neigh->nud_state & NUD_VALID)
@@ -623,6 +619,7 @@ static inline enum rt6_nud_state rt6_check_neigh(struct fib6_info *rt)
 
 static int rt6_score_route(struct fib6_info *rt, int oif, int strict)
 {
+	struct fib6_nh *nh = &rt->fib6_nh;
 	int m;
 
 	m = rt6_check_dev(rt, oif);
@@ -631,8 +628,9 @@ static int rt6_score_route(struct fib6_info *rt, int oif, int strict)
 #ifdef CONFIG_IPV6_ROUTER_PREF
 	m |= IPV6_DECODE_PREF(IPV6_EXTRACT_PREF(rt->fib6_flags)) << 2;
 #endif
-	if (strict & RT6_LOOKUP_F_REACHABLE) {
-		int n = rt6_check_neigh(rt);
+	if ((strict & RT6_LOOKUP_F_REACHABLE) &&
+	    !(rt->fib6_flags & RTF_NONEXTHOP) && nh->fib_nh_gw_family) {
+		int n = rt6_check_neigh(nh);
 		if (n < 0)
 			return n;
 	}
-- 
2.11.0

[PATCH net-next 02/10] ipv6: Remove rt6_check_dev

[David Ahern]

From: David Ahern <dsahern@gmail.com>

rt6_check_dev is a simpler helper with only 1 caller. Fold the code
into rt6_score_route.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b515fa8f787e..9630339d4b76 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -580,15 +580,6 @@ static inline void rt6_probe(struct fib6_info *rt)
 /*
  * Default Router Selection (RFC 2461 6.3.6)
  */
-static inline int rt6_check_dev(struct fib6_info *rt, int oif)
-{
-	const struct net_device *dev = rt->fib6_nh.fib_nh_dev;
-
-	if (!oif || dev->ifindex == oif)
-		return 2;
-	return 0;
-}
-
 static enum rt6_nud_state rt6_check_neigh(const struct fib6_nh *fib6_nh)
 {
 	enum rt6_nud_state ret = RT6_NUD_FAIL_HARD;
@@ -620,9 +611,11 @@ static enum rt6_nud_state rt6_check_neigh(const struct fib6_nh *fib6_nh)
 static int rt6_score_route(struct fib6_info *rt, int oif, int strict)
 {
 	struct fib6_nh *nh = &rt->fib6_nh;
-	int m;
+	int m = 0;
+
+	if (!oif || nh->fib_nh_dev->ifindex == oif)
+		m = 2;
 
-	m = rt6_check_dev(rt, oif);
 	if (!m && (strict & RT6_LOOKUP_F_IFACE))
 		return RT6_NUD_FAIL_HARD;
 #ifdef CONFIG_IPV6_ROUTER_PREF
-- 
2.11.0

[PATCH net-next 03/10] ipv6: Change rt6_probe to take a fib6_nh

[David Ahern]

From: David Ahern <dsahern@gmail.com>

rt6_probe sends probes for gateways in a nexthop. As such it really
depends on a fib6_nh, not a fib entry. Move last_probe to fib6_nh and
update rt6_probe to a fib6_nh struct.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 include/net/ip6_fib.h |  8 ++++----
 net/ipv6/route.c      | 16 ++++++++--------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 58dbb4e82908..2e9235adfa0d 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -127,6 +127,10 @@ struct rt6_exception {
 
 struct fib6_nh {
 	struct fib_nh_common	nh_common;
+
+#ifdef CONFIG_IPV6_ROUTER_PREF
+	unsigned long		last_probe;
+#endif
 };
 
 struct fib6_info {
@@ -155,10 +159,6 @@ struct fib6_info {
 	struct rt6_info * __percpu	*rt6i_pcpu;
 	struct rt6_exception_bucket __rcu *rt6i_exception_bucket;
 
-#ifdef CONFIG_IPV6_ROUTER_PREF
-	unsigned long			last_probe;
-#endif
-
 	u32				fib6_metric;
 	u8				fib6_protocol;
 	u8				fib6_type;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 9630339d4b76..c2b0d6f049e3 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -517,7 +517,7 @@ static void rt6_probe_deferred(struct work_struct *w)
 	kfree(work);
 }
 
-static void rt6_probe(struct fib6_info *rt)
+static void rt6_probe(struct fib6_nh *fib6_nh)
 {
 	struct __rt6_probe_work *work = NULL;
 	const struct in6_addr *nh_gw;
@@ -533,11 +533,11 @@ static void rt6_probe(struct fib6_info *rt)
 	 * Router Reachability Probe MUST be rate-limited
 	 * to no more than one per minute.
 	 */
-	if (!rt || !rt->fib6_nh.fib_nh_gw_family)
+	if (fib6_nh->fib_nh_gw_family)
 		return;
 
-	nh_gw = &rt->fib6_nh.fib_nh_gw6;
-	dev = rt->fib6_nh.fib_nh_dev;
+	nh_gw = &fib6_nh->fib_nh_gw6;
+	dev = fib6_nh->fib_nh_dev;
 	rcu_read_lock_bh();
 	idev = __in6_dev_get(dev);
 	neigh = __ipv6_neigh_lookup_noref(dev, nh_gw);
@@ -554,13 +554,13 @@ static void rt6_probe(struct fib6_info *rt)
 				__neigh_set_probe_once(neigh);
 		}
 		write_unlock(&neigh->lock);
-	} else if (time_after(jiffies, rt->last_probe +
+	} else if (time_after(jiffies, fib6_nh->last_probe +
 				       idev->cnf.rtr_probe_interval)) {
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 	}
 
 	if (work) {
-		rt->last_probe = jiffies;
+		fib6_nh->last_probe = jiffies;
 		INIT_WORK(&work->work, rt6_probe_deferred);
 		work->target = *nh_gw;
 		dev_hold(dev);
@@ -572,7 +572,7 @@ static void rt6_probe(struct fib6_info *rt)
 	rcu_read_unlock_bh();
 }
 #else
-static inline void rt6_probe(struct fib6_info *rt)
+static inline void rt6_probe(struct fib6_nh *fib6_nh)
 {
 }
 #endif
@@ -657,7 +657,7 @@ static struct fib6_info *find_match(struct fib6_info *rt, int oif, int strict,
 	}
 
 	if (strict & RT6_LOOKUP_F_REACHABLE)
-		rt6_probe(rt);
+		rt6_probe(&rt->fib6_nh);
 
 	/* note that m can be RT6_NUD_FAIL_PROBE at this point */
 	if (m > *mpri) {
-- 
2.11.0

[PATCH net-next 04/10] ipv6: Pass fib6_nh and flags to rt6_score_route

[David Ahern]

From: David Ahern <dsahern@gmail.com>

rt6_score_route only needs the fib6_flags and nexthop data. Change
it accordingly. Allows re-use later for nexthop based fib6_nh.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c2b0d6f049e3..22d1933278ae 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -102,7 +102,8 @@ static void		ip6_rt_update_pmtu(struct dst_entry *dst, struct sock *sk,
 					   struct sk_buff *skb, u32 mtu);
 static void		rt6_do_redirect(struct dst_entry *dst, struct sock *sk,
 					struct sk_buff *skb);
-static int rt6_score_route(struct fib6_info *rt, int oif, int strict);
+static int rt6_score_route(const struct fib6_nh *nh, u32 fib6_flags, int oif,
+			   int strict);
 static size_t rt6_nlmsg_size(struct fib6_info *rt);
 static int rt6_fill_node(struct net *net, struct sk_buff *skb,
 			 struct fib6_info *rt, struct dst_entry *dst,
@@ -446,12 +447,13 @@ struct fib6_info *fib6_multipath_select(const struct net *net,
 
 	list_for_each_entry_safe(sibling, next_sibling, &match->fib6_siblings,
 				 fib6_siblings) {
+		const struct fib6_nh *nh = &sibling->fib6_nh;
 		int nh_upper_bound;
 
-		nh_upper_bound = atomic_read(&sibling->fib6_nh.fib_nh_upper_bound);
+		nh_upper_bound = atomic_read(&nh->fib_nh_upper_bound);
 		if (fl6->mp_hash > nh_upper_bound)
 			continue;
-		if (rt6_score_route(sibling, oif, strict) < 0)
+		if (rt6_score_route(nh, sibling->fib6_flags, oif, strict) < 0)
 			break;
 		match = sibling;
 		break;
@@ -608,9 +610,9 @@ static enum rt6_nud_state rt6_check_neigh(const struct fib6_nh *fib6_nh)
 	return ret;
 }
 
-static int rt6_score_route(struct fib6_info *rt, int oif, int strict)
+static int rt6_score_route(const struct fib6_nh *nh, u32 fib6_flags, int oif,
+			   int strict)
 {
-	struct fib6_nh *nh = &rt->fib6_nh;
 	int m = 0;
 
 	if (!oif || nh->fib_nh_dev->ifindex == oif)
@@ -619,10 +621,10 @@ static int rt6_score_route(struct fib6_info *rt, int oif, int strict)
 	if (!m && (strict & RT6_LOOKUP_F_IFACE))
 		return RT6_NUD_FAIL_HARD;
 #ifdef CONFIG_IPV6_ROUTER_PREF
-	m |= IPV6_DECODE_PREF(IPV6_EXTRACT_PREF(rt->fib6_flags)) << 2;
+	m |= IPV6_DECODE_PREF(IPV6_EXTRACT_PREF(fib6_flags)) << 2;
 #endif
 	if ((strict & RT6_LOOKUP_F_REACHABLE) &&
-	    !(rt->fib6_flags & RTF_NONEXTHOP) && nh->fib_nh_gw_family) {
+	    !(fib6_flags & RTF_NONEXTHOP) && nh->fib_nh_gw_family) {
 		int n = rt6_check_neigh(nh);
 		if (n < 0)
 			return n;
@@ -648,7 +650,7 @@ static struct fib6_info *find_match(struct fib6_info *rt, int oif, int strict,
 	if (fib6_check_expired(rt))
 		goto out;
 
-	m = rt6_score_route(rt, oif, strict);
+	m = rt6_score_route(&rt->fib6_nh, rt->fib6_flags, oif, strict);
 	if (m == RT6_NUD_FAIL_DO_RR) {
 		match_do_rr = true;
 		m = 0; /* lowest valid score */
-- 
2.11.0

[PATCH net-next 05/10] ipv6: Refactor find_match

[David Ahern]

From: David Ahern <dsahern@gmail.com>

find_match primarily needs a fib6_nh (and fib6_flags which it passes
through to rt6_score_route). Move fib6_check_expired up to the call
sites so find_match is only called for relevant entries. Remove the
match argument which is mostly a pass through and use the return
boolean to decide if match gets set in the call sites.

The end result is a helper that can be called per fib6_nh struct
which is needed once fib entries reference nexthop objects that
have more than one fib6_nh.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 22d1933278ae..200bd5bb56bf 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -632,25 +632,22 @@ static int rt6_score_route(const struct fib6_nh *nh, u32 fib6_flags, int oif,
 	return m;
 }
 
-static struct fib6_info *find_match(struct fib6_info *rt, int oif, int strict,
-				   int *mpri, struct fib6_info *match,
-				   bool *do_rr)
+static bool find_match(struct fib6_nh *nh, u32 fib6_flags,
+		       int oif, int strict, int *mpri, bool *do_rr)
 {
-	int m;
 	bool match_do_rr = false;
+	bool rc = false;
+	int m;
 
-	if (rt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
+	if (nh->fib_nh_flags & RTNH_F_DEAD)
 		goto out;
 
-	if (ip6_ignore_linkdown(rt->fib6_nh.fib_nh_dev) &&
-	    rt->fib6_nh.fib_nh_flags & RTNH_F_LINKDOWN &&
+	if (ip6_ignore_linkdown(nh->fib_nh_dev) &&
+	    nh->fib_nh_flags & RTNH_F_LINKDOWN &&
 	    !(strict & RT6_LOOKUP_F_IGNORE_LINKSTATE))
 		goto out;
 
-	if (fib6_check_expired(rt))
-		goto out;
-
-	m = rt6_score_route(&rt->fib6_nh, rt->fib6_flags, oif, strict);
+	m = rt6_score_route(nh, fib6_flags, oif, strict);
 	if (m == RT6_NUD_FAIL_DO_RR) {
 		match_do_rr = true;
 		m = 0; /* lowest valid score */
@@ -659,16 +656,16 @@ static struct fib6_info *find_match(struct fib6_info *rt, int oif, int strict,
 	}
 
 	if (strict & RT6_LOOKUP_F_REACHABLE)
-		rt6_probe(&rt->fib6_nh);
+		rt6_probe(nh);
 
 	/* note that m can be RT6_NUD_FAIL_PROBE at this point */
 	if (m > *mpri) {
 		*do_rr = match_do_rr;
 		*mpri = m;
-		match = rt;
+		rc = true;
 	}
 out:
-	return match;
+	return rc;
 }
 
 static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
@@ -678,6 +675,7 @@ static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
 				     bool *do_rr)
 {
 	struct fib6_info *rt, *match, *cont;
+	struct fib6_nh *nh;
 	int mpri = -1;
 
 	match = NULL;
@@ -688,7 +686,12 @@ static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
 			break;
 		}
 
-		match = find_match(rt, oif, strict, &mpri, match, do_rr);
+		if (fib6_check_expired(rt))
+			continue;
+
+		nh = &rt->fib6_nh;
+		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
+			match = rt;
 	}
 
 	for (rt = leaf; rt && rt != rr_head;
@@ -698,14 +701,25 @@ static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
 			break;
 		}
 
-		match = find_match(rt, oif, strict, &mpri, match, do_rr);
+		if (fib6_check_expired(rt))
+			continue;
+
+		nh = &rt->fib6_nh;
+		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
+			match = rt;
 	}
 
 	if (match || !cont)
 		return match;
 
-	for (rt = cont; rt; rt = rcu_dereference(rt->fib6_next))
-		match = find_match(rt, oif, strict, &mpri, match, do_rr);
+	for (rt = cont; rt; rt = rcu_dereference(rt->fib6_next)) {
+		if (fib6_check_expired(rt))
+			continue;
+
+		nh = &rt->fib6_nh;
+		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
+			match = rt;
+	}
 
 	return match;
 }
-- 
2.11.0

[PATCH net-next 06/10] ipv6: Refactor find_rr_leaf

[David Ahern]

From: David Ahern <dsahern@gmail.com>

find_rr_leaf has 3 loops over fib_entries calling find_match. The loops
are very similar with differences in start point and whether the metric
is evaluated:
    1. start at rr_head, no extra loop compare, check fib metric
    2. start at leaf, compare rt against rr_head, check metric
    3. start at cont (potential saved point from earlier loops), no
       extra loop compare, no metric check

Create 1 loop that is called 3 different times. This will make a
later change with multipath nexthop objects much simpler.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 66 ++++++++++++++++++++++++++------------------------------
 1 file changed, 30 insertions(+), 36 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 200bd5bb56bf..52aa48a8dbda 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -668,58 +668,52 @@ static bool find_match(struct fib6_nh *nh, u32 fib6_flags,
 	return rc;
 }
 
-static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
-				     struct fib6_info *leaf,
-				     struct fib6_info *rr_head,
-				     u32 metric, int oif, int strict,
-				     bool *do_rr)
+static void __find_rr_leaf(struct fib6_info *rt_start,
+			   struct fib6_info *nomatch, u32 metric,
+			   struct fib6_info **match, struct fib6_info **cont,
+			   int oif, int strict, bool *do_rr, int *mpri)
 {
-	struct fib6_info *rt, *match, *cont;
-	struct fib6_nh *nh;
-	int mpri = -1;
+	struct fib6_info *rt;
 
-	match = NULL;
-	cont = NULL;
-	for (rt = rr_head; rt; rt = rcu_dereference(rt->fib6_next)) {
-		if (rt->fib6_metric != metric) {
-			cont = rt;
-			break;
+	for (rt = rt_start;
+	     rt && rt != nomatch;
+	     rt = rcu_dereference(rt->fib6_next)) {
+		struct fib6_nh *nh;
+
+		if (cont && rt->fib6_metric != metric) {
+			*cont = rt;
+			return;
 		}
 
 		if (fib6_check_expired(rt))
 			continue;
 
 		nh = &rt->fib6_nh;
-		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
-			match = rt;
+		if (find_match(nh, rt->fib6_flags, oif, strict, mpri, do_rr))
+			*match = rt;
 	}
+}
 
-	for (rt = leaf; rt && rt != rr_head;
-	     rt = rcu_dereference(rt->fib6_next)) {
-		if (rt->fib6_metric != metric) {
-			cont = rt;
-			break;
-		}
+static struct fib6_info *find_rr_leaf(struct fib6_node *fn,
+				      struct fib6_info *leaf,
+				      struct fib6_info *rr_head,
+				      u32 metric, int oif, int strict,
+				      bool *do_rr)
+{
+	struct fib6_info *match = NULL, *cont = NULL;
+	int mpri = -1;
 
-		if (fib6_check_expired(rt))
-			continue;
+	__find_rr_leaf(rr_head, NULL, metric, &match, &cont,
+		       oif, strict, do_rr, &mpri);
 
-		nh = &rt->fib6_nh;
-		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
-			match = rt;
-	}
+	__find_rr_leaf(leaf, rr_head, metric, &match, &cont,
+		       oif, strict, do_rr, &mpri);
 
 	if (match || !cont)
 		return match;
 
-	for (rt = cont; rt; rt = rcu_dereference(rt->fib6_next)) {
-		if (fib6_check_expired(rt))
-			continue;
-
-		nh = &rt->fib6_nh;
-		if (find_match(nh, rt->fib6_flags, oif, strict, &mpri, do_rr))
-			match = rt;
-	}
+	__find_rr_leaf(cont, NULL, metric, &match, NULL,
+		       oif, strict, do_rr, &mpri);
 
 	return match;
 }
-- 
2.11.0

[PATCH net-next 07/10] ipv6: Be smarter with null_entry handling in ip6_pol_route_lookup

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Clean up the fib6_null_entry handling in ip6_pol_route_lookup.
rt6_device_match can return fib6_null_entry, but fib6_multipath_select
can not. Consolidate the fib6_null_entry handling and on the final
null_entry check set rt and goto out - no need to defer to a second
check after rt6_find_cached_rt.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 52aa48a8dbda..0745ed872e5b 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1062,36 +1062,37 @@ static struct rt6_info *ip6_pol_route_lookup(struct net *net,
 	fn = fib6_node_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
 restart:
 	f6i = rcu_dereference(fn->leaf);
-	if (!f6i) {
+	if (!f6i)
 		f6i = net->ipv6.fib6_null_entry;
-	} else {
+	else
 		f6i = rt6_device_match(net, f6i, &fl6->saddr,
 				      fl6->flowi6_oif, flags);
-		if (f6i->fib6_nsiblings && fl6->flowi6_oif == 0)
-			f6i = fib6_multipath_select(net, f6i, fl6,
-						    fl6->flowi6_oif, skb,
-						    flags);
-	}
+
 	if (f6i == net->ipv6.fib6_null_entry) {
 		fn = fib6_backtrack(fn, &fl6->saddr);
 		if (fn)
 			goto restart;
-	}
 
-	trace_fib6_table_lookup(net, f6i, table, fl6);
+		rt = net->ipv6.ip6_null_entry;
+		dst_hold(&rt->dst);
+		goto out;
+	}
 
+	if (f6i->fib6_nsiblings && fl6->flowi6_oif == 0)
+		f6i = fib6_multipath_select(net, f6i, fl6, fl6->flowi6_oif, skb,
+					    flags);
 	/* Search through exception table */
 	rt = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
 	if (rt) {
 		if (ip6_hold_safe(net, &rt))
 			dst_use_noref(&rt->dst, jiffies);
-	} else if (f6i == net->ipv6.fib6_null_entry) {
-		rt = net->ipv6.ip6_null_entry;
-		dst_hold(&rt->dst);
 	} else {
 		rt = ip6_create_rt_rcu(f6i);
 	}
 
+out:
+	trace_fib6_table_lookup(net, f6i, table, fl6);
+
 	rcu_read_unlock();
 
 	return rt;
-- 
2.11.0

[PATCH net-next 08/10] ipv6: Move fib6_multipath_select down in ip6_pol_route

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Move the siblings and fib6_multipath_select after the null entry check
since a null entry can not have siblings.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0745ed872e5b..4acb71f0bc55 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1843,9 +1843,6 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 	rcu_read_lock();
 
 	f6i = fib6_table_lookup(net, table, oif, fl6, strict);
-	if (f6i->fib6_nsiblings)
-		f6i = fib6_multipath_select(net, f6i, fl6, oif, skb, strict);
-
 	if (f6i == net->ipv6.fib6_null_entry) {
 		rt = net->ipv6.ip6_null_entry;
 		rcu_read_unlock();
@@ -1853,6 +1850,9 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 		return rt;
 	}
 
+	if (f6i->fib6_nsiblings)
+		f6i = fib6_multipath_select(net, f6i, fl6, oif, skb, strict);
+
 	/*Search through exception table */
 	rt = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
 	if (rt) {
-- 
2.11.0

[PATCH net-next 09/10] ipv6: Refactor rt6_device_match

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Move the device and gateway checks in the fib6_next loop to a helper
that can be called per fib6_nh entry.

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4acb71f0bc55..0e8becb1e455 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -466,12 +466,34 @@ struct fib6_info *fib6_multipath_select(const struct net *net,
  *	Route lookup. rcu_read_lock() should be held.
  */
 
+static bool __rt6_device_match(struct net *net, const struct fib6_nh *nh,
+			       const struct in6_addr *saddr, int oif, int flags)
+{
+	const struct net_device *dev;
+
+	if (nh->fib_nh_flags & RTNH_F_DEAD)
+		return false;
+
+	dev = nh->fib_nh_dev;
+	if (oif) {
+		if (dev->ifindex == oif)
+			return true;
+	} else {
+		if (ipv6_chk_addr(net, saddr, dev,
+				  flags & RT6_LOOKUP_F_IFACE))
+			return true;
+	}
+
+	return false;
+}
+
 static inline struct fib6_info *rt6_device_match(struct net *net,
 						 struct fib6_info *rt,
 						    const struct in6_addr *saddr,
 						    int oif,
 						    int flags)
 {
+	const struct fib6_nh *nh;
 	struct fib6_info *sprt;
 
 	if (!oif && ipv6_addr_any(saddr) &&
@@ -479,19 +501,9 @@ static inline struct fib6_info *rt6_device_match(struct net *net,
 		return rt;
 
 	for (sprt = rt; sprt; sprt = rcu_dereference(sprt->fib6_next)) {
-		const struct net_device *dev = sprt->fib6_nh.fib_nh_dev;
-
-		if (sprt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
-			continue;
-
-		if (oif) {
-			if (dev->ifindex == oif)
-				return sprt;
-		} else {
-			if (ipv6_chk_addr(net, saddr, dev,
-					  flags & RT6_LOOKUP_F_IFACE))
-				return sprt;
-		}
+		nh = &sprt->fib6_nh;
+		if (__rt6_device_match(net, nh, saddr, oif, flags))
+			return sprt;
 	}
 
 	if (oif && flags & RT6_LOOKUP_F_IFACE)
-- 
2.11.0

[PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

[David Ahern]

From: David Ahern <dsahern@gmail.com>

Move the nexthop evaluation of a fib entry to a helper that can be
leveraged for each fib6_nh in a multipath nexthop object.

In the move, 'continue' statements means the helper returns false
(loop should continue) and 'break' means return true (found the entry
of interest).

Signed-off-by: David Ahern <dsahern@gmail.com>
---
 net/ipv6/route.c | 56 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0e8becb1e455..d555edaaff13 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2407,6 +2407,35 @@ void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
 		      NULL);
 }
 
+static bool ip6_redirect_nh_match(struct fib6_info *f6i,
+				  struct fib6_nh *nh,
+				  struct flowi6 *fl6,
+				  const struct in6_addr *gw,
+				  struct rt6_info **ret)
+{
+	if (nh->fib_nh_flags & RTNH_F_DEAD || !nh->fib_nh_gw_family ||
+	    fl6->flowi6_oif != nh->fib_nh_dev->ifindex)
+		return false;
+
+	/* rt_cache's gateway might be different from its 'parent'
+	 * in the case of an ip redirect.
+	 * So we keep searching in the exception table if the gateway
+	 * is different.
+	 */
+	if (!ipv6_addr_equal(gw, &nh->fib_nh_gw6)) {
+		struct rt6_info *rt_cache;
+
+		rt_cache = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
+		if (rt_cache &&
+		    ipv6_addr_equal(gw, &rt_cache->rt6i_gateway)) {
+			*ret = rt_cache;
+			return true;
+		}
+		return false;
+	}
+	return true;
+}
+
 /* Handle redirects */
 struct ip6rd_flowi {
 	struct flowi6 fl6;
@@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
 					     int flags)
 {
 	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
-	struct rt6_info *ret = NULL, *rt_cache;
+	struct rt6_info *ret = NULL;
 	struct fib6_info *rt;
 	struct fib6_node *fn;
 
@@ -2438,34 +2467,15 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
 	fn = fib6_node_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
 restart:
 	for_each_fib6_node_rt_rcu(fn) {
-		if (rt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
-			continue;
 		if (fib6_check_expired(rt))
 			continue;
 		if (rt->fib6_flags & RTF_REJECT)
 			break;
-		if (!rt->fib6_nh.fib_nh_gw_family)
-			continue;
 		if (fl6->flowi6_oif != rt->fib6_nh.fib_nh_dev->ifindex)
 			continue;
-		/* rt_cache's gateway might be different from its 'parent'
-		 * in the case of an ip redirect.
-		 * So we keep searching in the exception table if the gateway
-		 * is different.
-		 */
-		if (!ipv6_addr_equal(&rdfl->gateway, &rt->fib6_nh.fib_nh_gw6)) {
-			rt_cache = rt6_find_cached_rt(rt,
-						      &fl6->daddr,
-						      &fl6->saddr);
-			if (rt_cache &&
-			    ipv6_addr_equal(&rdfl->gateway,
-					    &rt_cache->rt6i_gateway)) {
-				ret = rt_cache;
-				break;
-			}
-			continue;
-		}
-		break;
+		if (ip6_redirect_nh_match(rt, &rt->fib6_nh, fl6,
+					  &rdfl->gateway, &ret))
+			goto out;
 	}
 
 	if (!rt)
-- 
2.11.0

Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

[Martin Lau]

On Tue, Apr 09, 2019 at 02:41:19PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
> 
> Move the nexthop evaluation of a fib entry to a helper that can be
> leveraged for each fib6_nh in a multipath nexthop object.
> 
> In the move, 'continue' statements means the helper returns false
> (loop should continue) and 'break' means return true (found the entry
> of interest).
> 
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
>  net/ipv6/route.c | 56 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 0e8becb1e455..d555edaaff13 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2407,6 +2407,35 @@ void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
>  		      NULL);
>  }
>  
> +static bool ip6_redirect_nh_match(struct fib6_info *f6i,
> +				  struct fib6_nh *nh,
Why both "struct fib6_info" and "struct fib6_nh" are needed?
I assume nh cannot be obtained from f6i in the later patch
when nh is decoupled from f6i?

> +				  struct flowi6 *fl6,
> +				  const struct in6_addr *gw,
> +				  struct rt6_info **ret)
> +{
> +	if (nh->fib_nh_flags & RTNH_F_DEAD || !nh->fib_nh_gw_family ||
> +	    fl6->flowi6_oif != nh->fib_nh_dev->ifindex)
> +		return false;
> +
> +	/* rt_cache's gateway might be different from its 'parent'
> +	 * in the case of an ip redirect.
> +	 * So we keep searching in the exception table if the gateway
> +	 * is different.
> +	 */
> +	if (!ipv6_addr_equal(gw, &nh->fib_nh_gw6)) {
> +		struct rt6_info *rt_cache;
> +
> +		rt_cache = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
> +		if (rt_cache &&
> +		    ipv6_addr_equal(gw, &rt_cache->rt6i_gateway)) {
> +			*ret = rt_cache;
> +			return true;
> +		}
> +		return false;
> +	}
> +	return true;
> +}
> +
>  /* Handle redirects */
>  struct ip6rd_flowi {
>  	struct flowi6 fl6;
> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>  					     int flags)
>  {
>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
> -	struct rt6_info *ret = NULL, *rt_cache;
> +	struct rt6_info *ret = NULL;
>  	struct fib6_info *rt;
The "rt" naming is becoming hard to review.
It is fortunate that the type context is kept in this diff.

In this local case, the s/rt/f6i/ rename is quite doable?
It can be the first clean-up step.

>  	struct fib6_node *fn;
>  
> @@ -2438,34 +2467,15 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>  	fn = fib6_node_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>  restart:
>  	for_each_fib6_node_rt_rcu(fn) {
> -		if (rt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
> -			continue;
>  		if (fib6_check_expired(rt))
>  			continue;
>  		if (rt->fib6_flags & RTF_REJECT)
>  			break;
> -		if (!rt->fib6_nh.fib_nh_gw_family)
> -			continue;
>  		if (fl6->flowi6_oif != rt->fib6_nh.fib_nh_dev->ifindex)
This check is also copied to the new ip6_redirect_nh_match.
Should this one be removed from here?

>  			continue;
> -		/* rt_cache's gateway might be different from its 'parent'
> -		 * in the case of an ip redirect.
> -		 * So we keep searching in the exception table if the gateway
> -		 * is different.
> -		 */
> -		if (!ipv6_addr_equal(&rdfl->gateway, &rt->fib6_nh.fib_nh_gw6)) {
> -			rt_cache = rt6_find_cached_rt(rt,
> -						      &fl6->daddr,
> -						      &fl6->saddr);
> -			if (rt_cache &&
> -			    ipv6_addr_equal(&rdfl->gateway,
> -					    &rt_cache->rt6i_gateway)) {
> -				ret = rt_cache;
> -				break;
> -			}
> -			continue;
> -		}
> -		break;
> +		if (ip6_redirect_nh_match(rt, &rt->fib6_nh, fl6,
> +					  &rdfl->gateway, &ret))
> +			goto out;
>  	}
>  
>  	if (!rt)
> -- 
> 2.11.0
> 

Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

[David Ahern]

On 4/10/19 10:36 AM, Martin Lau wrote:
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 0e8becb1e455..d555edaaff13 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2407,6 +2407,35 @@ void ip6_sk_dst_store_flow(struct sock *sk, struct dst_entry *dst,
>>  		      NULL);
>>  }
>>  
>> +static bool ip6_redirect_nh_match(struct fib6_info *f6i,
>> +				  struct fib6_nh *nh,
> Why both "struct fib6_info" and "struct fib6_nh" are needed?

Because of rt6_find_cached_rt. At the moment it needs 2 entries from the
fib6_info. Patches in follow on sets affect rt6_find_cached_rt in 2 ways:
1. Exceptions and cached routes are moved to per-fib6_nh similar to what
is done for IPv4 in which case the need for fib6_info reduces to just
the fib6_src.plen

2. fib6_result is introduced and passed to rt6_find_cached_rt instead of
fib6_info and fib6_nh. It satisfies the need for fib6_src.plen from the
fib entry and the fib6_nh for everything else.

Something has to come first and in my experience with this overall
change, I feel this order has the least duplicity in changes.

> I assume nh cannot be obtained from f6i in the later patch
> when nh is decoupled from f6i?

correct

> 
>> +				  struct flowi6 *fl6,
>> +				  const struct in6_addr *gw,
>> +				  struct rt6_info **ret)
>> +{
>> +	if (nh->fib_nh_flags & RTNH_F_DEAD || !nh->fib_nh_gw_family ||
>> +	    fl6->flowi6_oif != nh->fib_nh_dev->ifindex)
>> +		return false;
>> +
>> +	/* rt_cache's gateway might be different from its 'parent'
>> +	 * in the case of an ip redirect.
>> +	 * So we keep searching in the exception table if the gateway
>> +	 * is different.
>> +	 */
>> +	if (!ipv6_addr_equal(gw, &nh->fib_nh_gw6)) {
>> +		struct rt6_info *rt_cache;
>> +
>> +		rt_cache = rt6_find_cached_rt(f6i, &fl6->daddr, &fl6->saddr);
>> +		if (rt_cache &&
>> +		    ipv6_addr_equal(gw, &rt_cache->rt6i_gateway)) {
>> +			*ret = rt_cache;
>> +			return true;
>> +		}
>> +		return false;
>> +	}
>> +	return true;
>> +}
>> +
>>  /* Handle redirects */
>>  struct ip6rd_flowi {
>>  	struct flowi6 fl6;
>> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>>  					     int flags)
>>  {
>>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>> -	struct rt6_info *ret = NULL, *rt_cache;
>> +	struct rt6_info *ret = NULL;
>>  	struct fib6_info *rt;
> The "rt" naming is becoming hard to review.
> It is fortunate that the type context is kept in this diff.
> 
> In this local case, the s/rt/f6i/ rename is quite doable?

I looked into it and I feel such a mass rename just causes noise:
$ egrep -r 'fib6_info \*rt[,;]' net | wc -l
      50

so that's 50 functions that need to be updated and then all rt
references within the functions. A lot of noise which is why I have not
sent a name change patch.

> It can be the first clean-up step.
> 
>>  	struct fib6_node *fn;
>>  
>> @@ -2438,34 +2467,15 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>>  	fn = fib6_node_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>>  restart:
>>  	for_each_fib6_node_rt_rcu(fn) {
>> -		if (rt->fib6_nh.fib_nh_flags & RTNH_F_DEAD)
>> -			continue;
>>  		if (fib6_check_expired(rt))
>>  			continue;
>>  		if (rt->fib6_flags & RTF_REJECT)
>>  			break;
>> -		if (!rt->fib6_nh.fib_nh_gw_family)
>> -			continue;
>>  		if (fl6->flowi6_oif != rt->fib6_nh.fib_nh_dev->ifindex)
> This check is also copied to the new ip6_redirect_nh_match.
> Should this one be removed from here?

good catch. It should be removed.

Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

[Martin Lau]

On Wed, Apr 10, 2019 at 11:45:04AM -0700, David Ahern wrote:
> >> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
> >>  					     int flags)
> >>  {
> >>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
> >> -	struct rt6_info *ret = NULL, *rt_cache;
> >> +	struct rt6_info *ret = NULL;
> >>  	struct fib6_info *rt;
> > The "rt" naming is becoming hard to review.
> > It is fortunate that the type context is kept in this diff.
> > 
> > In this local case, the s/rt/f6i/ rename is quite doable?
> 
> I looked into it and I feel such a mass rename just causes noise:
> $ egrep -r 'fib6_info \*rt[,;]' net | wc -l
>       50
> 
> so that's 50 functions that need to be updated and then all rt
> references within the functions. A lot of noise which is why I have not
> sent a name change patch.
Agree that it is a bit noisy (but obvious) change.
I think it was a mistake to decide to postpone this name change while
introducing fib6_info back then.  It would be easier to review if
they were done together:
https://marc.info/?l=linux-netdev&m=151968617902168&w=2

I think a few of us are still ok-ish because we know what
have been going on.  For some, it will be confusing and
it should not be suprised to see this 'struct fib6_info *rt'
being copy-and-pasted to the future patch that makes it
even harder to read/maintain.

Re: [PATCH net-next 10/10] ipv6: Refactor __ip6_route_redirect

[David Ahern]

On 4/10/19 1:45 PM, Martin Lau wrote:
> On Wed, Apr 10, 2019 at 11:45:04AM -0700, David Ahern wrote:
>>>> @@ -2420,7 +2449,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
>>>>  					     int flags)
>>>>  {
>>>>  	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>>>> -	struct rt6_info *ret = NULL, *rt_cache;
>>>> +	struct rt6_info *ret = NULL;
>>>>  	struct fib6_info *rt;
>>> The "rt" naming is becoming hard to review.
>>> It is fortunate that the type context is kept in this diff.
>>>
>>> In this local case, the s/rt/f6i/ rename is quite doable?
>>
>> I looked into it and I feel such a mass rename just causes noise:
>> $ egrep -r 'fib6_info \*rt[,;]' net | wc -l
>>       50
>>
>> so that's 50 functions that need to be updated and then all rt
>> references within the functions. A lot of noise which is why I have not
>> sent a name change patch.
> Agree that it is a bit noisy (but obvious) change.
> I think it was a mistake to decide to postpone this name change while
> introducing fib6_info back then.  It would be easier to review if
> they were done together:
> https://marc.info/?l=linux-netdev&m=151968617902168&w=2
> 
> I think a few of us are still ok-ish because we know what
> have been going on.  For some, it will be confusing and
> it should not be suprised to see this 'struct fib6_info *rt'
> being copy-and-pasted to the future patch that makes it
> even harder to read/maintain.
> 

As I mentioned in that URL:
"My thought is to follow up with another patch that does the rename of
rt to f6i for all fib6_info. Given how large this change is already I
did not want to add extra diffs for that. If there is agreement to fold
that part in now, I can do it."

That was the theory. The reality is that it is a non-trivial change that
can not be done cleanly: 'rt' does not allow a clean search and replace
and when you consider the related changes (e.g., fib6_info *rt_last). I
am 80% through net/ipv6/route.c with ip6_fib.c to go. This is the
current diff stat:

$ git diff --stat
 include/net/if_inet6.h |   2 +-
 include/net/ip6_fib.h  |  20 +--
 net/ipv6/addrconf.c    |  84 ++++-----
 net/ipv6/route.c       | 674
++++++++++++++++++++++++++++++++++-----------------------------------
 4 files changed, 390 insertions(+), 390 deletions(-)

and that assumes I have done this right (ie., a clean compile). By the
time I am done that is going to be on the order of 500 insert / 500
delete of manual name changes. I see that as unnecessary churn with no
benefit.

Re: [PATCH net-next 00/10] ipv6: Refactor nexthop selection helpers during a fib lookup

[David Miller]

From: David Ahern <dsahern@kernel.org>
Date: Tue,  9 Apr 2019 14:41:09 -0700

> From: David Ahern <dsahern@gmail.com>
> 
> IPv6 has a fib6_nh embedded within each fib6_info and a separate
> fib6_info for each path in a multipath route. A side effect is that
> a fib6_info is passed all the way down the stack when selecting a path
> on a fib lookup. Refactor the fib lookup functions and associated
> helper functions to take a fib6_nh when appropriate to enable IPv6
> to work with nexthop objects where the fib6_nh is not directly part
> of a fib entry.

Series applied, thanks David.