[PATCH v2 net-next 0/5] ipv6: avoid taking refcnt on dst during route lookup

[PATCH v2 net-next 0/5] ipv6: avoid taking refcnt on dst during route lookup

[Wei Wang]

From: Wei Wang <weiwan@google.com>

Ipv6 route lookup code always grabs refcnt on the dst for the caller.
But for certain cases, grabbing refcnt is not always necessary if the
call path is rcu protected and the caller does not cache the dst.
Another issue in the route lookup logic is:
When there are multiple custom rules, we have to do the lookup into
each table associated to each rule individually. And when we can't
find the route in one table, we grab and release refcnt on
net->ipv6.ip6_null_entry before going to the next table.
This operation is completely redundant, and causes false issue because
net->ipv6.ip6_null_entry is a shared object.

This patch set introduces a new flag RT6_LOOKUP_F_DST_NOREF for route
lookup callers to set, to avoid any manipulation on the dst refcnt. And
it converts the major input and output path to use it.

The performance gain is noticable.
I ran synflood tests between 2 hosts under the same switch. Both hosts
have 20G mlx NIC, and 8 tx/rx queues.
Sender sends pure SYN flood with random src IPs and ports using trafgen.
Receiver has a simple TCP listener on the target port.
Both hosts have multiple custom rules:
- For incoming packets, only local table is traversed.
- For outgoing packets, 3 tables are traversed to find the route.
The packet processing rate on the receiver is as follows:
- Before the fix: 3.78Mpps
- After the fix:  5.50Mpps

v1->v2:
- Added a helper ip6_rt_put_flags() in patch 3 suggested by David Miller

Wei Wang (5):
  ipv6: introduce RT6_LOOKUP_F_DST_NOREF flag in ip6_pol_route()
  ipv6: initialize rt6->rt6i_uncached in all pre-allocated dst entries
  ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic
  ipv6: convert rx data path to not take refcnt on dst
  ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

 drivers/net/vrf.c       | 11 ++---
 include/net/ip6_route.h | 36 +++++++++++++++-
 include/net/l3mdev.h    | 11 +++--
 net/ipv6/fib6_rules.c   | 12 +++---
 net/ipv6/route.c        | 93 +++++++++++++++++++----------------------
 net/l3mdev/l3mdev.c     | 22 +++++-----
 6 files changed, 108 insertions(+), 77 deletions(-)

-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 net-next 1/5] ipv6: introduce RT6_LOOKUP_F_DST_NOREF flag in ip6_pol_route()

[Wei Wang]

From: Wei Wang <weiwan@google.com>

This new flag is to instruct the route lookup function to not take
refcnt on the dst entry. The user which does route lookup with this flag
must properly use rcu protection.
ip6_pol_route() is the major route lookup function for both tx and rx
path.
In this function:
Do not take refcnt on dst if RT6_LOOKUP_F_DST_NOREF flag is set, and
directly return the route entry. The caller should be holding rcu lock
when using this flag, and decide whether to take refcnt or not.

One note on the dst cache in the uncached_list:
As uncached_list does not consume refcnt, one refcnt is always returned
back to the caller even if RT6_LOOKUP_F_DST_NOREF flag is set.
Uncached dst is only possible in the output path. So in such call path,
caller MUST check if the dst is in the uncached_list before assuming
that there is no refcnt taken on the returned dst.

Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 include/net/ip6_route.h |  1 +
 net/ipv6/route.c        | 73 +++++++++++++++++------------------------
 2 files changed, 31 insertions(+), 43 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 7375a165fd98..82bced2fc1e3 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -36,6 +36,7 @@ struct route_info {
 #define RT6_LOOKUP_F_SRCPREF_PUBLIC	0x00000010
 #define RT6_LOOKUP_F_SRCPREF_COA	0x00000020
 #define RT6_LOOKUP_F_IGNORE_LINKSTATE	0x00000040
+#define RT6_LOOKUP_F_DST_NOREF		0x00000080
 
 /* We do not (yet ?) support IPv6 jumbograms (RFC 2675)
  * Unlike IPv4, hdr->seg_len doesn't include the IPv6 header
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index c4d285fe0adc..9dcbc56e4151 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1391,9 +1391,6 @@ static struct rt6_info *rt6_get_pcpu_route(const struct fib6_result *res)
 
 	pcpu_rt = this_cpu_read(*res->nh->rt6i_pcpu);
 
-	if (pcpu_rt)
-		ip6_hold_safe(NULL, &pcpu_rt);
-
 	return pcpu_rt;
 }
 
@@ -1403,12 +1400,9 @@ static struct rt6_info *rt6_make_pcpu_route(struct net *net,
 	struct rt6_info *pcpu_rt, *prev, **p;
 
 	pcpu_rt = ip6_rt_pcpu_alloc(res);
-	if (!pcpu_rt) {
-		dst_hold(&net->ipv6.ip6_null_entry->dst);
-		return net->ipv6.ip6_null_entry;
-	}
+	if (!pcpu_rt)
+		return NULL;
 
-	dst_hold(&pcpu_rt->dst);
 	p = this_cpu_ptr(res->nh->rt6i_pcpu);
 	prev = cmpxchg(p, NULL, pcpu_rt);
 	BUG_ON(prev);
@@ -2189,9 +2183,12 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 			       const struct sk_buff *skb, int flags)
 {
 	struct fib6_result res = {};
-	struct rt6_info *rt;
+	struct rt6_info *rt = NULL;
 	int strict = 0;
 
+	WARN_ON_ONCE((flags & RT6_LOOKUP_F_DST_NOREF) &&
+		     !rcu_read_lock_held());
+
 	strict |= flags & RT6_LOOKUP_F_IFACE;
 	strict |= flags & RT6_LOOKUP_F_IGNORE_LINKSTATE;
 	if (net->ipv6.devconf_all->forwarding == 0)
@@ -2200,23 +2197,15 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 	rcu_read_lock();
 
 	fib6_table_lookup(net, table, oif, fl6, &res, strict);
-	if (res.f6i == net->ipv6.fib6_null_entry) {
-		rt = net->ipv6.ip6_null_entry;
-		rcu_read_unlock();
-		dst_hold(&rt->dst);
-		return rt;
-	}
+	if (res.f6i == net->ipv6.fib6_null_entry)
+		goto out;
 
 	fib6_select_path(net, &res, fl6, oif, false, skb, strict);
 
 	/*Search through exception table */
 	rt = rt6_find_cached_rt(&res, &fl6->daddr, &fl6->saddr);
 	if (rt) {
-		if (ip6_hold_safe(net, &rt))
-			dst_use_noref(&rt->dst, jiffies);
-
-		rcu_read_unlock();
-		return rt;
+		goto out;
 	} else if (unlikely((fl6->flowi6_flags & FLOWI_FLAG_KNOWN_NH) &&
 			    !res.nh->fib_nh_gw_family)) {
 		/* Create a RTF_CACHE clone which will not be
@@ -2224,40 +2213,38 @@ struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
 		 * the daddr in the skb during the neighbor look-up is different
 		 * from the fl6->daddr used to look-up route here.
 		 */
-		struct rt6_info *uncached_rt;
+		rt = ip6_rt_cache_alloc(&res, &fl6->daddr, NULL);
 
-		uncached_rt = ip6_rt_cache_alloc(&res, &fl6->daddr, NULL);
-
-		rcu_read_unlock();
-
-		if (uncached_rt) {
-			/* Uncached_rt's refcnt is taken during ip6_rt_cache_alloc()
-			 * No need for another dst_hold()
+		if (rt) {
+			/* 1 refcnt is taken during ip6_rt_cache_alloc().
+			 * As rt6_uncached_list_add() does not consume refcnt,
+			 * this refcnt is always returned to the caller even
+			 * if caller sets RT6_LOOKUP_F_DST_NOREF flag.
 			 */
-			rt6_uncached_list_add(uncached_rt);
+			rt6_uncached_list_add(rt);
 			atomic_inc(&net->ipv6.rt6_stats->fib_rt_uncache);
-		} else {
-			uncached_rt = net->ipv6.ip6_null_entry;
-			dst_hold(&uncached_rt->dst);
-		}
+			rcu_read_unlock();
 
-		return uncached_rt;
+			return rt;
+		}
 	} else {
 		/* Get a percpu copy */
-
-		struct rt6_info *pcpu_rt;
-
 		local_bh_disable();
-		pcpu_rt = rt6_get_pcpu_route(&res);
+		rt = rt6_get_pcpu_route(&res);
 
-		if (!pcpu_rt)
-			pcpu_rt = rt6_make_pcpu_route(net, &res);
+		if (!rt)
+			rt = rt6_make_pcpu_route(net, &res);
 
 		local_bh_enable();
-		rcu_read_unlock();
-
-		return pcpu_rt;
 	}
+out:
+	if (!rt)
+		rt = net->ipv6.ip6_null_entry;
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		ip6_hold_safe(net, &rt);
+	rcu_read_unlock();
+
+	return rt;
 }
 EXPORT_SYMBOL_GPL(ip6_pol_route);
 
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 net-next 2/5] ipv6: initialize rt6->rt6i_uncached in all pre-allocated dst entries

[Wei Wang]

From: Wei Wang <weiwan@google.com>

Initialize rt6->rt6i_uncached on the following pre-allocated dsts:
net->ipv6.ip6_null_entry
net->ipv6.ip6_prohibit_entry
net->ipv6.ip6_blk_hole_entry

This is a preparation patch for later commits to be able to distinguish
dst entries in uncached list by doing:
!list_empty(rt6->rt6i_uncached)

Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 net/ipv6/route.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 9dcbc56e4151..33dc8af9a4bf 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -6010,6 +6010,7 @@ static int __net_init ip6_route_net_init(struct net *net)
 	net->ipv6.ip6_null_entry->dst.ops = &net->ipv6.ip6_dst_ops;
 	dst_init_metrics(&net->ipv6.ip6_null_entry->dst,
 			 ip6_template_metrics, true);
+	INIT_LIST_HEAD(&net->ipv6.ip6_null_entry->rt6i_uncached);
 
 #ifdef CONFIG_IPV6_MULTIPLE_TABLES
 	net->ipv6.fib6_has_custom_rules = false;
@@ -6021,6 +6022,7 @@ static int __net_init ip6_route_net_init(struct net *net)
 	net->ipv6.ip6_prohibit_entry->dst.ops = &net->ipv6.ip6_dst_ops;
 	dst_init_metrics(&net->ipv6.ip6_prohibit_entry->dst,
 			 ip6_template_metrics, true);
+	INIT_LIST_HEAD(&net->ipv6.ip6_prohibit_entry->rt6i_uncached);
 
 	net->ipv6.ip6_blk_hole_entry = kmemdup(&ip6_blk_hole_entry_template,
 					       sizeof(*net->ipv6.ip6_blk_hole_entry),
@@ -6030,6 +6032,7 @@ static int __net_init ip6_route_net_init(struct net *net)
 	net->ipv6.ip6_blk_hole_entry->dst.ops = &net->ipv6.ip6_dst_ops;
 	dst_init_metrics(&net->ipv6.ip6_blk_hole_entry->dst,
 			 ip6_template_metrics, true);
+	INIT_LIST_HEAD(&net->ipv6.ip6_blk_hole_entry->rt6i_uncached);
 #endif
 
 	net->ipv6.sysctl.flush_delay = 0;
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 net-next 3/5] ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic

[Wei Wang]

From: Wei Wang <weiwan@google.com>

This patch specifically converts the rule lookup logic to honor this
flag and not release refcnt when traversing each rule and calling
lookup() on each routing table.
Similar to previous patch, we also need some special handling of dst
entries in uncached list because there is always 1 refcnt taken for them
even if RT6_LOOKUP_F_DST_NOREF flag is set.

Signed-off-by: Wei Wang <weiwan@google.com>
---
 include/net/ip6_route.h | 10 ++++++++++
 net/ipv6/fib6_rules.c   | 12 +++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 82bced2fc1e3..0709835c01ad 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -94,6 +94,16 @@ static inline struct dst_entry *ip6_route_output(struct net *net,
 	return ip6_route_output_flags(net, sk, fl6, 0);
 }
 
+/* Only conditionally release dst if flags indicates
+ * !RT6_LOOKUP_F_DST_NOREF or dst is in uncached_list.
+ */
+static inline void ip6_rt_put_flags(struct rt6_info *rt, int flags)
+{
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF) ||
+	    !list_empty(&rt->rt6i_uncached))
+		ip6_rt_put(rt);
+}
+
 struct dst_entry *ip6_route_lookup(struct net *net, struct flowi6 *fl6,
 				   const struct sk_buff *skb, int flags);
 struct rt6_info *ip6_pol_route(struct net *net, struct fib6_table *table,
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index bcfae13409b5..d22b6c140f23 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -113,14 +113,15 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
 		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
 		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
 			return &rt->dst;
-		ip6_rt_put(rt);
+		ip6_rt_put_flags(rt, flags);
 		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
 		if (rt->dst.error != -EAGAIN)
 			return &rt->dst;
-		ip6_rt_put(rt);
+		ip6_rt_put_flags(rt, flags);
 	}
 
-	dst_hold(&net->ipv6.ip6_null_entry->dst);
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		dst_hold(&net->ipv6.ip6_null_entry->dst);
 	return &net->ipv6.ip6_null_entry->dst;
 }
 
@@ -237,13 +238,14 @@ static int __fib6_rule_action(struct fib_rule *rule, struct flowi *flp,
 			goto out;
 	}
 again:
-	ip6_rt_put(rt);
+	ip6_rt_put_flags(rt, flags);
 	err = -EAGAIN;
 	rt = NULL;
 	goto out;
 
 discard_pkt:
-	dst_hold(&rt->dst);
+	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
+		dst_hold(&rt->dst);
 out:
 	res->rt6 = rt;
 	return err;
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 net-next 4/5] ipv6: convert rx data path to not take refcnt on dst

[Wei Wang]

From: Wei Wang <weiwan@google.com>

ip6_route_input() is the key function to do the route lookup in the
rx data path. All the callers to this function are already holding rcu
lock. So it is fairly easy to convert it to not take refcnt on the dst:
We pass in flag RT6_LOOKUP_F_DST_NOREF and do skb_dst_set_noref().
This saves a few atomic inc or dec operations and should boost
performance overall.
This also makes the logic more aligned with v4.

Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 net/ipv6/route.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 33dc8af9a4bf..d2b287635aab 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2375,11 +2375,12 @@ u32 rt6_multipath_hash(const struct net *net, const struct flowi6 *fl6,
 	return mhash >> 1;
 }
 
+/* Called with rcu held */
 void ip6_route_input(struct sk_buff *skb)
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
 	struct net *net = dev_net(skb->dev);
-	int flags = RT6_LOOKUP_F_HAS_SADDR;
+	int flags = RT6_LOOKUP_F_HAS_SADDR | RT6_LOOKUP_F_DST_NOREF;
 	struct ip_tunnel_info *tun_info;
 	struct flowi6 fl6 = {
 		.flowi6_iif = skb->dev->ifindex,
@@ -2401,8 +2402,8 @@ void ip6_route_input(struct sk_buff *skb)
 	if (unlikely(fl6.flowi6_proto == IPPROTO_ICMPV6))
 		fl6.mp_hash = rt6_multipath_hash(net, &fl6, skb, flkeys);
 	skb_dst_drop(skb);
-	skb_dst_set(skb,
-		    ip6_route_input_lookup(net, skb->dev, &fl6, skb, flags));
+	skb_dst_set_noref(skb, ip6_route_input_lookup(net, skb->dev,
+						      &fl6, skb, flags));
 }
 
 static struct rt6_info *ip6_pol_route_output(struct net *net,
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

[Wei Wang]

From: Wei Wang <weiwan@google.com>

For tx path, in most cases, we still have to take refcnt on the dst
cause the caller is caching the dst somewhere. But it still is
beneficial to make use of RT6_LOOKUP_F_DST_NOREF flag while doing the
route lookup. It is cause this flag prevents manipulating refcnt on
net->ipv6.ip6_null_entry when doing fib6_rule_lookup() to traverse each
routing table. The null_entry is a shared object and constant updates on
it cause false sharing.

We converted the current major lookup function ip6_route_output_flags()
to make use of RT6_LOOKUP_F_DST_NOREF.

Together with the change in the rx path, we see noticable performance
boost:
I ran synflood tests between 2 hosts under the same switch. Both hosts
have 20G mlx NIC, and 8 tx/rx queues.
Sender sends pure SYN flood with random src IPs and ports using trafgen.
Receiver has a simple TCP listener on the target port.
Both hosts have multiple custom rules:
- For incoming packets, only local table is traversed.
- For outgoing packets, 3 tables are traversed to find the route.
The packet processing rate on the receiver is as follows:
- Before the fix: 3.78Mpps
- After the fix:  5.50Mpps

Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 drivers/net/vrf.c       | 11 ++++++-----
 include/net/ip6_route.h | 25 +++++++++++++++++++++++--
 include/net/l3mdev.h    | 11 +++++++----
 net/ipv6/route.c        | 10 ++++++----
 net/l3mdev/l3mdev.c     | 22 +++++++++++-----------
 5 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c
index 11b9525dff27..1d1ac78b167e 100644
--- a/drivers/net/vrf.c
+++ b/drivers/net/vrf.c
@@ -1072,12 +1072,14 @@ static struct sk_buff *vrf_l3_rcv(struct net_device *vrf_dev,
 #if IS_ENABLED(CONFIG_IPV6)
 /* send to link-local or multicast address via interface enslaved to
  * VRF device. Force lookup to VRF table without changing flow struct
+ * No refcnt is taken on the dst.
  */
-static struct dst_entry *vrf_link_scope_lookup(const struct net_device *dev,
-					      struct flowi6 *fl6)
+static struct dst_entry *vrf_link_scope_lookup_noref(
+					    const struct net_device *dev,
+					    struct flowi6 *fl6)
 {
 	struct net *net = dev_net(dev);
-	int flags = RT6_LOOKUP_F_IFACE;
+	int flags = RT6_LOOKUP_F_IFACE | RT6_LOOKUP_F_DST_NOREF;
 	struct dst_entry *dst = NULL;
 	struct rt6_info *rt;
 
@@ -1087,7 +1089,6 @@ static struct dst_entry *vrf_link_scope_lookup(const struct net_device *dev,
 	 */
 	if (fl6->flowi6_oif == dev->ifindex) {
 		dst = &net->ipv6.ip6_null_entry->dst;
-		dst_hold(dst);
 		return dst;
 	}
 
@@ -1107,7 +1108,7 @@ static const struct l3mdev_ops vrf_l3mdev_ops = {
 	.l3mdev_l3_rcv		= vrf_l3_rcv,
 	.l3mdev_l3_out		= vrf_l3_out,
 #if IS_ENABLED(CONFIG_IPV6)
-	.l3mdev_link_scope_lookup = vrf_link_scope_lookup,
+	.l3mdev_link_scope_lookup_noref = vrf_link_scope_lookup_noref,
 #endif
 };
 
diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h
index 0709835c01ad..66802ecd81e5 100644
--- a/include/net/ip6_route.h
+++ b/include/net/ip6_route.h
@@ -84,8 +84,29 @@ struct dst_entry *ip6_route_input_lookup(struct net *net,
 					 struct flowi6 *fl6,
 					 const struct sk_buff *skb, int flags);
 
-struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
-					 struct flowi6 *fl6, int flags);
+struct dst_entry *ip6_route_output_flags_noref(struct net *net,
+					       const struct sock *sk,
+					       struct flowi6 *fl6, int flags);
+
+static inline struct dst_entry *ip6_route_output_flags(struct net *net,
+						       const struct sock *sk,
+						       struct flowi6 *fl6,
+						       int flags) {
+	struct dst_entry *dst;
+	struct rt6_info *rt6;
+
+	rcu_read_lock();
+	dst = ip6_route_output_flags_noref(net, sk, fl6, flags);
+	rt6 = (struct rt6_info *)dst;
+	/* For dst cached in uncached_list, refcnt is already taken. */
+	if (list_empty(&rt6->rt6i_uncached) && !dst_hold_safe(dst)) {
+		dst = &net->ipv6.ip6_null_entry->dst;
+		dst_hold(dst);
+	}
+	rcu_read_unlock();
+
+	return dst;
+}
 
 static inline struct dst_entry *ip6_route_output(struct net *net,
 						 const struct sock *sk,
diff --git a/include/net/l3mdev.h b/include/net/l3mdev.h
index e942372b077b..d8c37317bb86 100644
--- a/include/net/l3mdev.h
+++ b/include/net/l3mdev.h
@@ -31,8 +31,9 @@ struct l3mdev_ops {
 					  u16 proto);
 
 	/* IPv6 ops */
-	struct dst_entry * (*l3mdev_link_scope_lookup)(const struct net_device *dev,
-						 struct flowi6 *fl6);
+	struct dst_entry * (*l3mdev_link_scope_lookup_noref)(
+					    const struct net_device *dev,
+					    struct flowi6 *fl6);
 };
 
 #ifdef CONFIG_NET_L3_MASTER_DEV
@@ -140,7 +141,8 @@ static inline bool netif_index_is_l3_master(struct net *net, int ifindex)
 	return rc;
 }
 
-struct dst_entry *l3mdev_link_scope_lookup(struct net *net, struct flowi6 *fl6);
+struct dst_entry *l3mdev_link_scope_lookup_noref(struct net *net,
+						 struct flowi6 *fl6);
 
 static inline
 struct sk_buff *l3mdev_l3_rcv(struct sk_buff *skb, u16 proto)
@@ -251,7 +253,8 @@ static inline bool netif_index_is_l3_master(struct net *net, int ifindex)
 }
 
 static inline
-struct dst_entry *l3mdev_link_scope_lookup(struct net *net, struct flowi6 *fl6)
+struct dst_entry *l3mdev_link_scope_lookup_noref(struct net *net,
+						 struct flowi6 *fl6)
 {
 	return NULL;
 }
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index d2b287635aab..602d00794b30 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -2415,8 +2415,9 @@ static struct rt6_info *ip6_pol_route_output(struct net *net,
 	return ip6_pol_route(net, table, fl6->flowi6_oif, fl6, skb, flags);
 }
 
-struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
-					 struct flowi6 *fl6, int flags)
+struct dst_entry *ip6_route_output_flags_noref(struct net *net,
+					       const struct sock *sk,
+					       struct flowi6 *fl6, int flags)
 {
 	bool any_src;
 
@@ -2424,13 +2425,14 @@ struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
 	    (IPV6_ADDR_MULTICAST | IPV6_ADDR_LINKLOCAL)) {
 		struct dst_entry *dst;
 
-		dst = l3mdev_link_scope_lookup(net, fl6);
+		dst = l3mdev_link_scope_lookup_noref(net, fl6);
 		if (dst)
 			return dst;
 	}
 
 	fl6->flowi6_iif = LOOPBACK_IFINDEX;
 
+	flags |= RT6_LOOKUP_F_DST_NOREF;
 	any_src = ipv6_addr_any(&fl6->saddr);
 	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl6->daddr) ||
 	    (fl6->flowi6_oif && any_src))
@@ -2443,7 +2445,7 @@ struct dst_entry *ip6_route_output_flags(struct net *net, const struct sock *sk,
 
 	return fib6_rule_lookup(net, fl6, NULL, flags, ip6_pol_route_output);
 }
-EXPORT_SYMBOL_GPL(ip6_route_output_flags);
+EXPORT_SYMBOL_GPL(ip6_route_output_flags_noref);
 
 struct dst_entry *ip6_blackhole_route(struct net *net, struct dst_entry *dst_orig)
 {
diff --git a/net/l3mdev/l3mdev.c b/net/l3mdev/l3mdev.c
index cfc9fcb97465..06133426549b 100644
--- a/net/l3mdev/l3mdev.c
+++ b/net/l3mdev/l3mdev.c
@@ -114,35 +114,35 @@ u32 l3mdev_fib_table_by_index(struct net *net, int ifindex)
 EXPORT_SYMBOL_GPL(l3mdev_fib_table_by_index);
 
 /**
- *	l3mdev_link_scope_lookup - IPv6 route lookup based on flow for link
- *			     local and multicast addresses
+ *	l3mdev_link_scope_lookup_noref - IPv6 route lookup based on flow
+ *			     for link local and multicast addresses
  *	@net: network namespace for device index lookup
  *	@fl6: IPv6 flow struct for lookup
+ *	This function does not hold refcnt on the returned dst.
+ *	Caller must hold rcu_read_lock().
  */
 
-struct dst_entry *l3mdev_link_scope_lookup(struct net *net,
-					   struct flowi6 *fl6)
+struct dst_entry *l3mdev_link_scope_lookup_noref(struct net *net,
+						 struct flowi6 *fl6)
 {
 	struct dst_entry *dst = NULL;
 	struct net_device *dev;
 
+	WARN_ON_ONCE(!rcu_read_lock_held());
 	if (fl6->flowi6_oif) {
-		rcu_read_lock();
-
 		dev = dev_get_by_index_rcu(net, fl6->flowi6_oif);
 		if (dev && netif_is_l3_slave(dev))
 			dev = netdev_master_upper_dev_get_rcu(dev);
 
 		if (dev && netif_is_l3_master(dev) &&
-		    dev->l3mdev_ops->l3mdev_link_scope_lookup)
-			dst = dev->l3mdev_ops->l3mdev_link_scope_lookup(dev, fl6);
-
-		rcu_read_unlock();
+		    dev->l3mdev_ops->l3mdev_link_scope_lookup_noref)
+			dst = dev->l3mdev_ops->
+				l3mdev_link_scope_lookup_noref(dev, fl6);
 	}
 
 	return dst;
 }
-EXPORT_SYMBOL_GPL(l3mdev_link_scope_lookup);
+EXPORT_SYMBOL_GPL(l3mdev_link_scope_lookup_noref);
 
 /**
  *	l3mdev_fib_rule_match - Determine if flowi references an
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v2 net-next 3/5] ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic

[David Ahern]

On 6/19/19 4:31 PM, Wei Wang wrote:
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index bcfae13409b5..d22b6c140f23 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -113,14 +113,15 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
>  		rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
>  		if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
>  			return &rt->dst;
> -		ip6_rt_put(rt);
> +		ip6_rt_put_flags(rt, flags);
>  		rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
>  		if (rt->dst.error != -EAGAIN)
>  			return &rt->dst;
> -		ip6_rt_put(rt);
> +		ip6_rt_put_flags(rt, flags);
>  	}
>  
> -	dst_hold(&net->ipv6.ip6_null_entry->dst);
> +	if (!(flags & RT6_LOOKUP_F_DST_NOREF))
> +		dst_hold(&net->ipv6.ip6_null_entry->dst);
>  	return &net->ipv6.ip6_null_entry->dst;
>  }
>  

What about the fib6_rule_lookup when CONFIG_IPV6_MULTIPLE_TABLES is
configured off? If caller passes in RT6_LOOKUP_F_DST_NOREF that version
is still taking a reference in the error path and does a put after the
lookup.

Re: [PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

[David Ahern]

On 6/19/19 4:31 PM, Wei Wang wrote:
> diff --git a/include/net/l3mdev.h b/include/net/l3mdev.h
> index e942372b077b..d8c37317bb86 100644
> --- a/include/net/l3mdev.h
> +++ b/include/net/l3mdev.h
> @@ -31,8 +31,9 @@ struct l3mdev_ops {
>  					  u16 proto);
>  
>  	/* IPv6 ops */
> -	struct dst_entry * (*l3mdev_link_scope_lookup)(const struct net_device *dev,
> -						 struct flowi6 *fl6);
> +	struct dst_entry * (*l3mdev_link_scope_lookup_noref)(
> +					    const struct net_device *dev,
> +					    struct flowi6 *fl6);

I would prefer to add a comment about not taking a references vs adding
the _noref extension. There is only 1 user for 1 context and the name
length is getting out of hand (as evidenced by the line wrapping below).

Re: [PATCH v2 net-next 3/5] ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic

[Wei Wang]

On Wed, Jun 19, 2019 at 4:12 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 6/19/19 4:31 PM, Wei Wang wrote:
> > diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> > index bcfae13409b5..d22b6c140f23 100644
> > --- a/net/ipv6/fib6_rules.c
> > +++ b/net/ipv6/fib6_rules.c
> > @@ -113,14 +113,15 @@ struct dst_entry *fib6_rule_lookup(struct net *net, struct flowi6 *fl6,
> >               rt = lookup(net, net->ipv6.fib6_local_tbl, fl6, skb, flags);
> >               if (rt != net->ipv6.ip6_null_entry && rt->dst.error != -EAGAIN)
> >                       return &rt->dst;
> > -             ip6_rt_put(rt);
> > +             ip6_rt_put_flags(rt, flags);
> >               rt = lookup(net, net->ipv6.fib6_main_tbl, fl6, skb, flags);
> >               if (rt->dst.error != -EAGAIN)
> >                       return &rt->dst;
> > -             ip6_rt_put(rt);
> > +             ip6_rt_put_flags(rt, flags);
> >       }
> >
> > -     dst_hold(&net->ipv6.ip6_null_entry->dst);
> > +     if (!(flags & RT6_LOOKUP_F_DST_NOREF))
> > +             dst_hold(&net->ipv6.ip6_null_entry->dst);
> >       return &net->ipv6.ip6_null_entry->dst;
> >  }
> >
>
> What about the fib6_rule_lookup when CONFIG_IPV6_MULTIPLE_TABLES is
> configured off? If caller passes in RT6_LOOKUP_F_DST_NOREF that version
> is still taking a reference in the error path and does a put after the
> lookup.

True. I missed that part. Thanks for catching it. Will update.

Re: [PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

[Wei Wang]

On Wed, Jun 19, 2019 at 4:21 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 6/19/19 4:31 PM, Wei Wang wrote:
> > diff --git a/include/net/l3mdev.h b/include/net/l3mdev.h
> > index e942372b077b..d8c37317bb86 100644
> > --- a/include/net/l3mdev.h
> > +++ b/include/net/l3mdev.h
> > @@ -31,8 +31,9 @@ struct l3mdev_ops {
> >                                         u16 proto);
> >
> >       /* IPv6 ops */
> > -     struct dst_entry * (*l3mdev_link_scope_lookup)(const struct net_device *dev,
> > -                                              struct flowi6 *fl6);
> > +     struct dst_entry * (*l3mdev_link_scope_lookup_noref)(
> > +                                         const struct net_device *dev,
> > +                                         struct flowi6 *fl6);
>
> I would prefer to add a comment about not taking a references vs adding
> the _noref extension. There is only 1 user for 1 context and the name
> length is getting out of hand (as evidenced by the line wrapping below).

Hmm OK. I was trying to make it clear to make sure future callers do
not misuse it.

Re: [PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

Hi Wei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Wei-Wang/ipv6-avoid-taking-refcnt-on-dst-during-route-lookup/20190621-195237
config: x86_64-kexec (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/net/ethernet/broadcom/cnic.c:41:0:
   include/net/ip6_route.h: In function 'ip6_route_output_flags':
>> include/net/ip6_route.h:103:15: error: 'struct net' has no member named 'ipv6'; did you mean 'ipv4'?
      dst = &net->ipv6.ip6_null_entry->dst;
                  ^~~~
                  ipv4

vim +103 include/net/ip6_route.h

    80	
    81	void ip6_route_input(struct sk_buff *skb);
    82	struct dst_entry *ip6_route_input_lookup(struct net *net,
    83						 struct net_device *dev,
    84						 struct flowi6 *fl6,
    85						 const struct sk_buff *skb, int flags);
    86	
    87	struct dst_entry *ip6_route_output_flags_noref(struct net *net,
    88						       const struct sock *sk,
    89						       struct flowi6 *fl6, int flags);
    90	
    91	static inline struct dst_entry *ip6_route_output_flags(struct net *net,
    92							       const struct sock *sk,
    93							       struct flowi6 *fl6,
    94							       int flags) {
    95		struct dst_entry *dst;
    96		struct rt6_info *rt6;
    97	
    98		rcu_read_lock();
    99		dst = ip6_route_output_flags_noref(net, sk, fl6, flags);
   100		rt6 = (struct rt6_info *)dst;
   101		/* For dst cached in uncached_list, refcnt is already taken. */
   102		if (list_empty(&rt6->rt6i_uncached) && !dst_hold_safe(dst)) {
 > 103			dst = &net->ipv6.ip6_null_entry->dst;
   104			dst_hold(dst);
   105		}
   106		rcu_read_unlock();
   107	
   108		return dst;
   109	}
   110	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27295 bytes --]

Re: [PATCH v2 net-next 5/5] ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

Hi Wei,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Wei-Wang/ipv6-avoid-taking-refcnt-on-dst-during-route-lookup/20190621-195237
config: x86_64-randconfig-u0-06220153 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/net//ethernet/broadcom/cnic.c:41:0:
   include/net/ip6_route.h: In function 'ip6_route_output_flags':
>> include/net/ip6_route.h:103:13: error: 'struct net' has no member named 'ipv6'
      dst = &net->ipv6.ip6_null_entry->dst;
                ^

vim +103 include/net/ip6_route.h

    80	
    81	void ip6_route_input(struct sk_buff *skb);
    82	struct dst_entry *ip6_route_input_lookup(struct net *net,
    83						 struct net_device *dev,
    84						 struct flowi6 *fl6,
    85						 const struct sk_buff *skb, int flags);
    86	
    87	struct dst_entry *ip6_route_output_flags_noref(struct net *net,
    88						       const struct sock *sk,
    89						       struct flowi6 *fl6, int flags);
    90	
    91	static inline struct dst_entry *ip6_route_output_flags(struct net *net,
    92							       const struct sock *sk,
    93							       struct flowi6 *fl6,
    94							       int flags) {
    95		struct dst_entry *dst;
    96		struct rt6_info *rt6;
    97	
    98		rcu_read_lock();
    99		dst = ip6_route_output_flags_noref(net, sk, fl6, flags);
   100		rt6 = (struct rt6_info *)dst;
   101		/* For dst cached in uncached_list, refcnt is already taken. */
   102		if (list_empty(&rt6->rt6i_uncached) && !dst_hold_safe(dst)) {
 > 103			dst = &net->ipv6.ip6_null_entry->dst;
   104			dst_hold(dst);
   105		}
   106		rcu_read_unlock();
   107	
   108		return dst;
   109	}
   110	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32662 bytes --]