[PATCH net 0/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[PATCH net 0/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

This patch series is to fix the dst refcnt bugs in ip6_tunnel.

Patch 1 and 2 are the prep works.  Patch 3 is the fix.

I can reproduce the bug by adding and removing the ip6gre tunnel
while running a super_netperf TCP_CRR test.  I get the following
trace by adding WARN_ON_ONCE(newrefcnt < 0) to dst_release():

[  312.760432] ------------[ cut here ]------------
[  312.774664] WARNING: CPU: 2 PID: 10263 at net/core/dst.c:288 dst_release+0xf3/0x100()
[  312.776041] Modules linked in: k10temp coretemp hwmon ip6_gre ip6_tunnel tunnel6 ipmi_devintf ipmi_ms\
ghandler ip6table_filter ip6_tables xt_NFLOG nfnetlink_log nfnetlink xt_comment xt_statistic iptable_fil\
ter ip_tables x_tables nfsv3 nfs_acl nfs fscache lockd grace mptctl netconsole autofs4 rpcsec_gss_krb5 a\
uth_rpcgss oid_registry sunrpc ipv6 dm_mod loop iTCO_wdt iTCO_vendor_support serio_raw rtc_cmos pcspkr i\
2c_i801 i2c_core lpc_ich mfd_core ehci_pci ehci_hcd e1000e mlx4_en ptp pps_core vxlan udp_tunnel ip6_udp\
_tunnel mlx4_core sg button ext3 jbd mpt2sas raid_class
[  312.785302] CPU: 2 PID: 10263 Comm: netperf Not tainted 4.2.0-rc8-00046-g4db9b63-dirty #15
[  312.791695] Hardware name: Quanta Freedom /Windmill-EP, BIOS F03_3B04 09/12/2013
[  312.792965]  ffffffff819dca2c ffff8811dfbdf6f8 ffffffff816537de ffff88123788fdb8
[  312.794263]  0000000000000000 ffff8811dfbdf738 ffffffff81052646 ffff8811dfbdf768
[  312.795593]  ffff881203a98180 00000000ffffffff ffff88242927a000 ffff88120a2532e0
[  312.796946] Call Trace:
[  312.797380]  [<ffffffff816537de>] dump_stack+0x45/0x57
[  312.798288]  [<ffffffff81052646>] warn_slowpath_common+0x86/0xc0
[  312.799699]  [<ffffffff8105273a>] warn_slowpath_null+0x1a/0x20
[  312.800852]  [<ffffffff8159f9b3>] dst_release+0xf3/0x100
[  312.801834]  [<ffffffffa03f1308>] ip6_tnl_dst_store+0x48/0x70 [ip6_tunnel]
[  312.803738]  [<ffffffffa03fd0b6>] ip6gre_xmit2+0x536/0x720 [ip6_gre]
[  312.804774]  [<ffffffffa03fd40a>] ip6gre_tunnel_xmit+0x16a/0x410 [ip6_gre]
[  312.805986]  [<ffffffff8159934b>] dev_hard_start_xmit+0x23b/0x390
[  312.808810]  [<ffffffff815a2f5f>] ? neigh_destroy+0xef/0x140
[  312.809843]  [<ffffffff81599a6c>] __dev_queue_xmit+0x48c/0x4f0
[  312.813931]  [<ffffffff81599ae3>] dev_queue_xmit_sk+0x13/0x20
[  312.814993]  [<ffffffff815a0832>] neigh_direct_output+0x12/0x20
[  312.817448]  [<ffffffffa021d633>] ip6_finish_output2+0x183/0x460 [ipv6]
[  312.818762]  [<ffffffff81306fc5>] ? find_next_bit+0x15/0x20
[  312.819671]  [<ffffffffa021fd79>] ip6_finish_output+0x89/0xe0 [ipv6]
[  312.820720]  [<ffffffffa021fe14>] ip6_output+0x44/0xe0 [ipv6]
[  312.821762]  [<ffffffff815c8809>] ? nf_hook_slow+0x69/0xc0
[  312.823123]  [<ffffffffa021d232>] ip6_xmit+0x242/0x4c0 [ipv6]
[  312.824073]  [<ffffffffa021c9f0>] ? ac6_proc_exit+0x20/0x20 [ipv6]
[  312.825116]  [<ffffffffa024c751>] inet6_csk_xmit+0x61/0xa0 [ipv6]
[  312.826127]  [<ffffffff815eb590>] tcp_transmit_skb+0x4f0/0x9b0
[  312.827441]  [<ffffffff815ed267>] tcp_connect+0x637/0x7a0
[  312.828327]  [<ffffffffa0245906>] tcp_v6_connect+0x2d6/0x550 [ipv6]
[  312.829581]  [<ffffffff81606f05>] __inet_stream_connect+0x95/0x2f0
[  312.830600]  [<ffffffff810ae13a>] ? hrtimer_try_to_cancel+0x1a/0xf0
[  312.833456]  [<ffffffff812fba19>] ? timerqueue_add+0x59/0xb0
[  312.834407]  [<ffffffff81607198>] inet_stream_connect+0x38/0x50
[  312.835886]  [<ffffffff8157cb17>] SYSC_connect+0xb7/0xf0
[  312.840035]  [<ffffffff810af6d3>] ? do_setitimer+0x1b3/0x200
[  312.840983]  [<ffffffff810af75a>] ? alarm_setitimer+0x3a/0x70
[  312.841941]  [<ffffffff8157d7ae>] SyS_connect+0xe/0x10
[  312.842818]  [<ffffffff81659297>] entry_SYSCALL_64_fastpath+0x12/0x6a
[  312.844206] ---[ end trace 43f3ecd86c3b1313 ]---

[PATCH net 1/3] ipv6: Refactor common ip6gre_tunnel_init codes

[Martin KaFai Lau]

It is a prep work to fix the dst_entry refcnt bugs in ip6_tunnel.

This patch refactors some common init codes used by both
ip6gre_tunnel_init and ip6gre_tap_init.

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 net/ipv6/ip6_gre.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 69f4f68..495eab3 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -1244,7 +1244,7 @@ static void ip6gre_tunnel_setup(struct net_device *dev)
 	netif_keep_dst(dev);
 }
 
-static int ip6gre_tunnel_init(struct net_device *dev)
+static int ip6gre_tunnel_init_common(struct net_device *dev)
 {
 	struct ip6_tnl *tunnel;
 
@@ -1254,16 +1254,30 @@ static int ip6gre_tunnel_init(struct net_device *dev)
 	tunnel->net = dev_net(dev);
 	strcpy(tunnel->parms.name, dev->name);
 
+	dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
+	if (!dev->tstats)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int ip6gre_tunnel_init(struct net_device *dev)
+{
+	struct ip6_tnl *tunnel;
+	int ret;
+
+	ret = ip6gre_tunnel_init_common(dev);
+	if (ret)
+		return ret;
+
+	tunnel = netdev_priv(dev);
+
 	memcpy(dev->dev_addr, &tunnel->parms.laddr, sizeof(struct in6_addr));
 	memcpy(dev->broadcast, &tunnel->parms.raddr, sizeof(struct in6_addr));
 
 	if (ipv6_addr_any(&tunnel->parms.raddr))
 		dev->header_ops = &ip6gre_header_ops;
 
-	dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
-	if (!dev->tstats)
-		return -ENOMEM;
-
 	return 0;
 }
 
@@ -1459,19 +1473,16 @@ static void ip6gre_netlink_parms(struct nlattr *data[],
 static int ip6gre_tap_init(struct net_device *dev)
 {
 	struct ip6_tnl *tunnel;
+	int ret;
 
-	tunnel = netdev_priv(dev);
+	ret = ip6gre_tunnel_init_common(dev);
+	if (ret)
+		return ret;
 
-	tunnel->dev = dev;
-	tunnel->net = dev_net(dev);
-	strcpy(tunnel->parms.name, dev->name);
+	tunnel = netdev_priv(dev);
 
 	ip6gre_tnl_link_config(tunnel, 1);
 
-	dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
-	if (!dev->tstats)
-		return -ENOMEM;
-
 	return 0;
 }
 
-- 
1.8.1

[PATCH net 2/3] ipv6: Rename the dst_cache helper functions in ip6_tunnel

[Martin KaFai Lau]

It is a prep work to fix the dst_entry refcnt bugs in
ip6_tunnel.

This patch rename:
1. ip6_tnl_dst_check() to ip6_tnl_dst_get() to better
   reflect that it will take a dst refcnt in the next patch.
2. ip6_tnl_dst_store() to ip6_tnl_dst_set() to have a more
   conventional name matching with ip6_tnl_dst_get().

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/ip6_tunnel.h |  4 ++--
 net/ipv6/ip6_gre.c       |  4 ++--
 net/ipv6/ip6_tunnel.c    | 12 ++++++------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index b8529aa..979b081 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -60,9 +60,9 @@ struct ipv6_tlv_tnl_enc_lim {
 	__u8 encap_limit;	/* tunnel encapsulation limit   */
 } __packed;
 
-struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t);
+struct dst_entry *ip6_tnl_dst_get(struct ip6_tnl *t);
 void ip6_tnl_dst_reset(struct ip6_tnl *t);
-void ip6_tnl_dst_store(struct ip6_tnl *t, struct dst_entry *dst);
+void ip6_tnl_dst_set(struct ip6_tnl *t, struct dst_entry *dst);
 int ip6_tnl_rcv_ctl(struct ip6_tnl *t, const struct in6_addr *laddr,
 		const struct in6_addr *raddr);
 int ip6_tnl_xmit_ctl(struct ip6_tnl *t, const struct in6_addr *laddr,
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 495eab3..2f2cff0 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -634,7 +634,7 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 	}
 
 	if (!fl6->flowi6_mark)
-		dst = ip6_tnl_dst_check(tunnel);
+		dst = ip6_tnl_dst_get(tunnel);
 
 	if (!dst) {
 		ndst = ip6_route_output(net, NULL, fl6);
@@ -763,7 +763,7 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 
 	ip6tunnel_xmit(NULL, skb, dev);
 	if (ndst)
-		ip6_tnl_dst_store(tunnel, ndst);
+		ip6_tnl_dst_set(tunnel, ndst);
 	return 0;
 tx_err_link_failure:
 	stats->tx_carrier_errors++;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 2e67b66..d2dc9e7 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -126,7 +126,7 @@ static struct net_device_stats *ip6_get_stats(struct net_device *dev)
  * Locking : hash tables are protected by RCU and RTNL
  */
 
-struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t)
+struct dst_entry *ip6_tnl_dst_get(struct ip6_tnl *t)
 {
 	struct dst_entry *dst = t->dst_cache;
 
@@ -139,7 +139,7 @@ struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t)
 
 	return dst;
 }
-EXPORT_SYMBOL_GPL(ip6_tnl_dst_check);
+EXPORT_SYMBOL_GPL(ip6_tnl_dst_get);
 
 void ip6_tnl_dst_reset(struct ip6_tnl *t)
 {
@@ -148,14 +148,14 @@ void ip6_tnl_dst_reset(struct ip6_tnl *t)
 }
 EXPORT_SYMBOL_GPL(ip6_tnl_dst_reset);
 
-void ip6_tnl_dst_store(struct ip6_tnl *t, struct dst_entry *dst)
+void ip6_tnl_dst_set(struct ip6_tnl *t, struct dst_entry *dst)
 {
 	struct rt6_info *rt = (struct rt6_info *) dst;
 	t->dst_cookie = rt6_get_cookie(rt);
 	dst_release(t->dst_cache);
 	t->dst_cache = dst;
 }
-EXPORT_SYMBOL_GPL(ip6_tnl_dst_store);
+EXPORT_SYMBOL_GPL(ip6_tnl_dst_set);
 
 /**
  * ip6_tnl_lookup - fetch tunnel matching the end-point addresses
@@ -1010,7 +1010,7 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 		memcpy(&fl6->daddr, addr6, sizeof(fl6->daddr));
 		neigh_release(neigh);
 	} else if (!fl6->flowi6_mark)
-		dst = ip6_tnl_dst_check(t);
+		dst = ip6_tnl_dst_get(t);
 
 	if (!ip6_tnl_xmit_ctl(t, &fl6->saddr, &fl6->daddr))
 		goto tx_err_link_failure;
@@ -1102,7 +1102,7 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 	ipv6h->daddr = fl6->daddr;
 	ip6tunnel_xmit(NULL, skb, dev);
 	if (ndst)
-		ip6_tnl_dst_store(t, ndst);
+		ip6_tnl_dst_set(t, ndst);
 	return 0;
 tx_err_link_failure:
 	stats->tx_carrier_errors++;
-- 
1.8.1

[PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

Problems in the current dst_entry cache in the ip6_tunnel:

1. ip6_tnl_dst_set is racy.  There is no lock to protect it:
   - One major problem is that the dst refcnt gets messed up. F.e.
     the same dst_cache can be released multiple times and then
     triggering the infamous dst refcnt < 0 warning message.
   - Another issue is the inconsistency between dst_cache and
     dst_cookie.

   It can be reproduced by adding and removing the ip6gre tunnel
   while running a super_netperf TCP_CRR test.

2. In ip6_tnl_xmit2() and ip6gre_xmit2(), the outgoing skb does
   not hold a dst_entry's refcnt.

This patch:
1. Create a percpu dst_entry cache in ip6_tnl
2. Use a spinlock to protect the dst_cache operations
3. The outgoing skb always holds the dst_entry's refcnt

Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 include/net/ip6_tunnel.h |  11 ++++-
 net/ipv6/ip6_gre.c       |  38 ++++++++-------
 net/ipv6/ip6_tunnel.c    | 122 +++++++++++++++++++++++++++++++++++------------
 3 files changed, 123 insertions(+), 48 deletions(-)

diff --git a/include/net/ip6_tunnel.h b/include/net/ip6_tunnel.h
index 979b081..60b4f40 100644
--- a/include/net/ip6_tunnel.h
+++ b/include/net/ip6_tunnel.h
@@ -32,6 +32,12 @@ struct __ip6_tnl_parm {
 	__be32			o_key;
 };
 
+struct ip6_tnl_dst {
+	spinlock_t lock;
+	struct dst_entry *dst;
+	u32 cookie;
+};
+
 /* IPv6 tunnel */
 struct ip6_tnl {
 	struct ip6_tnl __rcu *next;	/* next tunnel in list */
@@ -39,8 +45,7 @@ struct ip6_tnl {
 	struct net *net;	/* netns for packet i/o */
 	struct __ip6_tnl_parm parms;	/* tunnel configuration parameters */
 	struct flowi fl;	/* flowi template for xmit */
-	struct dst_entry *dst_cache;    /* cached dst */
-	u32 dst_cookie;
+	struct ip6_tnl_dst __percpu *dst_cache;	/* cached dst */
 
 	int err_count;
 	unsigned long err_time;
@@ -61,6 +66,8 @@ struct ipv6_tlv_tnl_enc_lim {
 } __packed;
 
 struct dst_entry *ip6_tnl_dst_get(struct ip6_tnl *t);
+int ip6_tnl_dst_init(struct ip6_tnl *t);
+void ip6_tnl_dst_destroy(struct ip6_tnl *t);
 void ip6_tnl_dst_reset(struct ip6_tnl *t);
 void ip6_tnl_dst_set(struct ip6_tnl *t, struct dst_entry *dst);
 int ip6_tnl_rcv_ctl(struct ip6_tnl *t, const struct in6_addr *laddr,
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 2f2cff0..76fb4ac 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -637,17 +637,17 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 		dst = ip6_tnl_dst_get(tunnel);
 
 	if (!dst) {
-		ndst = ip6_route_output(net, NULL, fl6);
+		dst = ip6_route_output(net, NULL, fl6);
 
-		if (ndst->error)
+		if (dst->error)
 			goto tx_err_link_failure;
-		ndst = xfrm_lookup(net, ndst, flowi6_to_flowi(fl6), NULL, 0);
-		if (IS_ERR(ndst)) {
-			err = PTR_ERR(ndst);
-			ndst = NULL;
+		dst = xfrm_lookup(net, dst, flowi6_to_flowi(fl6), NULL, 0);
+		if (IS_ERR(dst)) {
+			err = PTR_ERR(dst);
+			dst = NULL;
 			goto tx_err_link_failure;
 		}
-		dst = ndst;
+		ndst = dst;
 	}
 
 	tdev = dst->dev;
@@ -702,12 +702,9 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 		skb = new_skb;
 	}
 
-	if (fl6->flowi6_mark) {
-		skb_dst_set(skb, dst);
-		ndst = NULL;
-	} else {
-		skb_dst_set_noref(skb, dst);
-	}
+	if (!fl6->flowi6_mark && ndst)
+		ip6_tnl_dst_set(tunnel, ndst);
+	skb_dst_set(skb, dst);
 
 	proto = NEXTHDR_GRE;
 	if (encap_limit >= 0) {
@@ -762,14 +759,12 @@ static netdev_tx_t ip6gre_xmit2(struct sk_buff *skb,
 	skb_set_inner_protocol(skb, protocol);
 
 	ip6tunnel_xmit(NULL, skb, dev);
-	if (ndst)
-		ip6_tnl_dst_set(tunnel, ndst);
 	return 0;
 tx_err_link_failure:
 	stats->tx_carrier_errors++;
 	dst_link_failure(skb);
 tx_err_dst_release:
-	dst_release(ndst);
+	dst_release(dst);
 	return err;
 }
 
@@ -1222,6 +1217,9 @@ static const struct net_device_ops ip6gre_netdev_ops = {
 
 static void ip6gre_dev_free(struct net_device *dev)
 {
+	struct ip6_tnl *t = netdev_priv(dev);
+
+	ip6_tnl_dst_destroy(t);
 	free_percpu(dev->tstats);
 	free_netdev(dev);
 }
@@ -1247,6 +1245,7 @@ static void ip6gre_tunnel_setup(struct net_device *dev)
 static int ip6gre_tunnel_init_common(struct net_device *dev)
 {
 	struct ip6_tnl *tunnel;
+	int ret;
 
 	tunnel = netdev_priv(dev);
 
@@ -1258,6 +1257,13 @@ static int ip6gre_tunnel_init_common(struct net_device *dev)
 	if (!dev->tstats)
 		return -ENOMEM;
 
+	ret = ip6_tnl_dst_init(tunnel);
+	if (ret) {
+		free_percpu(dev->tstats);
+		dev->tstats = NULL;
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index d2dc9e7..97b3e781 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -126,37 +126,90 @@ static struct net_device_stats *ip6_get_stats(struct net_device *dev)
  * Locking : hash tables are protected by RCU and RTNL
  */
 
-struct dst_entry *ip6_tnl_dst_get(struct ip6_tnl *t)
+static void __ip6_tnl_per_cpu_dst_set(struct ip6_tnl_dst *idst,
+				      struct dst_entry *dst)
 {
-	struct dst_entry *dst = t->dst_cache;
-
-	if (dst && dst->obsolete &&
-	    !dst->ops->check(dst, t->dst_cookie)) {
-		t->dst_cache = NULL;
-		dst_release(dst);
-		return NULL;
+	dst_release(idst->dst);
+	idst->dst = dst;
+	if (dst) {
+		dst_hold(dst);
+		idst->cookie = rt6_get_cookie((struct rt6_info *)dst);
+	} else {
+		idst->cookie = 0;
 	}
+}
+
+static void ip6_tnl_per_cpu_dst_set(struct ip6_tnl_dst *idst,
+				    struct dst_entry *dst)
+{
 
+	spin_lock_bh(&idst->lock);
+	__ip6_tnl_per_cpu_dst_set(idst, dst);
+	spin_unlock_bh(&idst->lock);
+}
+
+struct dst_entry *ip6_tnl_dst_get(struct ip6_tnl *t)
+{
+	struct dst_entry *dst;
+	struct ip6_tnl_dst *idst;
+
+	idst = raw_cpu_ptr(t->dst_cache);
+	spin_lock_bh(&idst->lock);
+	dst = idst->dst;
+	if (dst) {
+		if (!dst->obsolete || dst->ops->check(dst, idst->cookie)) {
+			dst_hold(idst->dst);
+		} else {
+			__ip6_tnl_per_cpu_dst_set(idst, NULL);
+			dst = NULL;
+		}
+	}
+	spin_unlock_bh(&idst->lock);
 	return dst;
 }
 EXPORT_SYMBOL_GPL(ip6_tnl_dst_get);
 
 void ip6_tnl_dst_reset(struct ip6_tnl *t)
 {
-	dst_release(t->dst_cache);
-	t->dst_cache = NULL;
+	int i;
+
+	for_each_possible_cpu(i)
+		ip6_tnl_per_cpu_dst_set(raw_cpu_ptr(t->dst_cache), NULL);
 }
 EXPORT_SYMBOL_GPL(ip6_tnl_dst_reset);
 
 void ip6_tnl_dst_set(struct ip6_tnl *t, struct dst_entry *dst)
 {
-	struct rt6_info *rt = (struct rt6_info *) dst;
-	t->dst_cookie = rt6_get_cookie(rt);
-	dst_release(t->dst_cache);
-	t->dst_cache = dst;
+	ip6_tnl_per_cpu_dst_set(raw_cpu_ptr(t->dst_cache), dst);
+
 }
 EXPORT_SYMBOL_GPL(ip6_tnl_dst_set);
 
+void ip6_tnl_dst_destroy(struct ip6_tnl *t)
+{
+	if (!t->dst_cache)
+		return;
+
+	ip6_tnl_dst_reset(t);
+	free_percpu(t->dst_cache);
+}
+EXPORT_SYMBOL_GPL(ip6_tnl_dst_destroy);
+
+int ip6_tnl_dst_init(struct ip6_tnl *t)
+{
+	int i;
+
+	t->dst_cache = alloc_percpu(struct ip6_tnl_dst);
+	if (!t->dst_cache)
+		return -ENOMEM;
+
+	for_each_possible_cpu(i)
+		spin_lock_init(&per_cpu_ptr(t->dst_cache, i)->lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ip6_tnl_dst_init);
+
 /**
  * ip6_tnl_lookup - fetch tunnel matching the end-point addresses
  *   @remote: the address of the tunnel exit-point
@@ -271,6 +324,9 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t)
 
 static void ip6_dev_free(struct net_device *dev)
 {
+	struct ip6_tnl *t = netdev_priv(dev);
+
+	ip6_tnl_dst_destroy(t);
 	free_percpu(dev->tstats);
 	free_netdev(dev);
 }
@@ -1016,17 +1072,17 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 		goto tx_err_link_failure;
 
 	if (!dst) {
-		ndst = ip6_route_output(net, NULL, fl6);
+		dst = ip6_route_output(net, NULL, fl6);
 
-		if (ndst->error)
+		if (dst->error)
 			goto tx_err_link_failure;
-		ndst = xfrm_lookup(net, ndst, flowi6_to_flowi(fl6), NULL, 0);
-		if (IS_ERR(ndst)) {
-			err = PTR_ERR(ndst);
-			ndst = NULL;
+		dst = xfrm_lookup(net, dst, flowi6_to_flowi(fl6), NULL, 0);
+		if (IS_ERR(dst)) {
+			err = PTR_ERR(dst);
+			dst = NULL;
 			goto tx_err_link_failure;
 		}
-		dst = ndst;
+		ndst = dst;
 	}
 
 	tdev = dst->dev;
@@ -1072,12 +1128,11 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 		consume_skb(skb);
 		skb = new_skb;
 	}
-	if (fl6->flowi6_mark) {
-		skb_dst_set(skb, dst);
-		ndst = NULL;
-	} else {
-		skb_dst_set_noref(skb, dst);
-	}
+
+	if (!fl6->flowi6_mark && ndst)
+		ip6_tnl_dst_set(t, ndst);
+	skb_dst_set(skb, dst);
+
 	skb->transport_header = skb->network_header;
 
 	proto = fl6->flowi6_proto;
@@ -1101,14 +1156,12 @@ static int ip6_tnl_xmit2(struct sk_buff *skb,
 	ipv6h->saddr = fl6->saddr;
 	ipv6h->daddr = fl6->daddr;
 	ip6tunnel_xmit(NULL, skb, dev);
-	if (ndst)
-		ip6_tnl_dst_set(t, ndst);
 	return 0;
 tx_err_link_failure:
 	stats->tx_carrier_errors++;
 	dst_link_failure(skb);
 tx_err_dst_release:
-	dst_release(ndst);
+	dst_release(dst);
 	return err;
 }
 
@@ -1573,12 +1626,21 @@ static inline int
 ip6_tnl_dev_init_gen(struct net_device *dev)
 {
 	struct ip6_tnl *t = netdev_priv(dev);
+	int ret;
 
 	t->dev = dev;
 	t->net = dev_net(dev);
 	dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
 	if (!dev->tstats)
 		return -ENOMEM;
+
+	ret = ip6_tnl_dst_init(t);
+	if (ret) {
+		free_percpu(dev->tstats);
+		dev->tstats = NULL;
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
1.8.1

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Tue, 2015-09-01 at 11:55 -0700, Martin KaFai Lau wrote:
> Problems in the current dst_entry cache in the ip6_tunnel:
> 
> 1. ip6_tnl_dst_set is racy.  There is no lock to protect it:
>    - One major problem is that the dst refcnt gets messed up. F.e.
>      the same dst_cache can be released multiple times and then
>      triggering the infamous dst refcnt < 0 warning message.
>    - Another issue is the inconsistency between dst_cache and
>      dst_cookie.
> 
>    It can be reproduced by adding and removing the ip6gre tunnel
>    while running a super_netperf TCP_CRR test.
> 
> 2. In ip6_tnl_xmit2() and ip6gre_xmit2(), the outgoing skb does
>    not hold a dst_entry's refcnt.

It should not be a problem. refcnt is taken when/if necessary (skb
queued on a qdisc for example)

We have other uses of skb_dst_set_noref()

Please describe the problem ?

> 
> This patch:
> 1. Create a percpu dst_entry cache in ip6_tnl
OK
> 2. Use a spinlock to protect the dst_cache operations

Well, a seqlock would be better : No need for an atomic operation in
fast path.

> 3. The outgoing skb always holds the dst_entry's refcnt
> 

Not clear why it is needed. Inexpensive thanks to per cpu dst, but
still...

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> It should not be a problem. refcnt is taken when/if necessary (skb
> queued on a qdisc for example)
>
> We have other uses of skb_dst_set_noref()
>
> Please describe the problem ?
The current ip6_tnl_dst_get() does not take the dst refcnt.

If the dst is released after ip6_tnl_dst_get() and before
skb_dst_set_noref(), would it cause an issue?

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote:
> On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > It should not be a problem. refcnt is taken when/if necessary (skb
> > queued on a qdisc for example)
> >
> > We have other uses of skb_dst_set_noref()
> >
> > Please describe the problem ?
> The current ip6_tnl_dst_get() does not take the dst refcnt.
> 
> If the dst is released after ip6_tnl_dst_get() and before
> skb_dst_set_noref(), would it cause an issue?

We are under rcu here, and a dst in a cache is protected by RCU by
definition.

skb_dst_set_noref() has following debugging clause, does it trigger for
you ?

WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote:
> On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote:
> > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > > It should not be a problem. refcnt is taken when/if necessary (skb
> > > queued on a qdisc for example)
> > >
> > > We have other uses of skb_dst_set_noref()
> > >
> > > Please describe the problem ?
> > The current ip6_tnl_dst_get() does not take the dst refcnt.
> >
> > If the dst is released after ip6_tnl_dst_get() and before
> > skb_dst_set_noref(), would it cause an issue?
>
> We are under rcu here, and a dst in a cache is protected by RCU by
> definition.
>
> skb_dst_set_noref() has following debugging clause, does it trigger for
> you ?
>
> WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
No. I did not see this.

I am probably missing something.  Do you mean the rcu can
protect the followings:


ip6_tnl_dst_get()
			dst_release()
						dst_free() /* refcnt is 0 */
skb_dst_set_noref()

--------

or for DST_NOCACHE

ip6_tnl_dst_get()
			dst_release() /* refcnt is 0 */
skb_dst_set_noref()
/* ip6tunnel_xmit()
 * or its callees
 * take a dst refcnt
 */

--------

or the above situation does not matter/will not happen?

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote:
> On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote:
> > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote:
> > > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > > > It should not be a problem. refcnt is taken when/if necessary (skb
> > > > queued on a qdisc for example)
> > > >
> > > > We have other uses of skb_dst_set_noref()
> > > >
> > > > Please describe the problem ?
> > > The current ip6_tnl_dst_get() does not take the dst refcnt.
> > >
> > > If the dst is released after ip6_tnl_dst_get() and before
> > > skb_dst_set_noref(), would it cause an issue?
> >
> > We are under rcu here, and a dst in a cache is protected by RCU by
> > definition.
> >
> > skb_dst_set_noref() has following debugging clause, does it trigger for
> > you ?
> >
> > WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> No. I did not see this.
> 
> I am probably missing something.  Do you mean the rcu can
> protect the followings:
> 
> 
> ip6_tnl_dst_get()
> 			dst_release()
> 						dst_free() /* refcnt is 0 */
> skb_dst_set_noref()
> 

Yes, this is protected by normal rcu rules.

dst wont be freed until all cpus exit their rcu read sections.

> --------
> 
> or for DST_NOCACHE
> 
> ip6_tnl_dst_get()
> 			dst_release() /* refcnt is 0 */
> skb_dst_set_noref()
> /* ip6tunnel_xmit()
>  * or its callees
>  * take a dst refcnt
>  */
> 



> --------
> 
> or the above situation does not matter/will not happen?
> --

You should take a look at following commits for a bit of history

10e2eb878f3ca07ac2f05fa5ca5e6c4c9174a27a
dbfc4fb7d578d4f224faa6b60deb40804dfdc1b1
f88649721268999bdff09777847080a52004f691
6c7e7610ff6888ea15a901fbcb30c5d461816b34

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 03:38:36PM -0700, Eric Dumazet wrote:
> On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote:
> > On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote:
> > > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote:
> > > > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > > > > It should not be a problem. refcnt is taken when/if necessary (skb
> > > > > queued on a qdisc for example)
> > > > >
> > > > > We have other uses of skb_dst_set_noref()
> > > > >
> > > > > Please describe the problem ?
> > > > The current ip6_tnl_dst_get() does not take the dst refcnt.
> > > >
> > > > If the dst is released after ip6_tnl_dst_get() and before
> > > > skb_dst_set_noref(), would it cause an issue?
> > >
> > > We are under rcu here, and a dst in a cache is protected by RCU by
> > > definition.
> > >
> > > skb_dst_set_noref() has following debugging clause, does it trigger for
> > > you ?
> > >
> > > WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> > No. I did not see this.
> >
> > I am probably missing something.  Do you mean the rcu can
> > protect the followings:
> >
> >
> > ip6_tnl_dst_get()
> > 			dst_release()
> > 						dst_free() /* refcnt is 0 */
> > skb_dst_set_noref()
> >
>
> Yes, this is protected by normal rcu rules.
>
> dst wont be freed until all cpus exit their rcu read sections.
I can see skb_dst_set_noref() is safe at this point
but what happen after rcu_read_unlock()?

hmmm... Also, I don't see dst_free() is under the rcu contract, like
call_rcu() or synchronize_rcu().

Even it is (like dst_release for DST_NOCACHE dst_entry),  what happen
after the rcu_read_unlock()? Would someone (like qdisc) holds a dst refcnt
to an already/to-be destroyed dst? For DST_NOCACHE, like:

rcu_read_lock()

ip6_tnl_dst_get()
 			dst_release()	/* refcnt is 0 */
			=>call_rcu(dst_destroy)
skb_dst_set_noref()
__dev_queue_xmit()
=>skb_dst_force()
=>__dev_xmit_skb()
=>q->enqueue()

rcu_read_unlock()
/* Here, I am holding a dst refcnt but
 * the dst is already in the next
 * rcu destroy cycle?
 */

> You should take a look at following commits for a bit of history
>
> 10e2eb878f3ca07ac2f05fa5ca5e6c4c9174a27a
> dbfc4fb7d578d4f224faa6b60deb40804dfdc1b1
> f88649721268999bdff09777847080a52004f691
> 6c7e7610ff6888ea15a901fbcb30c5d461816b34
>
Thanks for the pointers.  It seems the ip_tunnel.c and ip_tunnel_core.c is
always doing skb_dst_set() also.  It is something that can also be optimized
in IPv4 or the situation is different in IPv4?

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 05:31:44PM -0700, Martin KaFai Lau wrote:
> On Tue, Sep 01, 2015 at 03:38:36PM -0700, Eric Dumazet wrote:
> > On Tue, 2015-09-01 at 15:25 -0700, Martin KaFai Lau wrote:
> > > On Tue, Sep 01, 2015 at 02:26:58PM -0700, Eric Dumazet wrote:
> > > > On Tue, 2015-09-01 at 13:55 -0700, Martin KaFai Lau wrote:
> > > > > On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > > > > > It should not be a problem. refcnt is taken when/if necessary (skb
> > > > > > queued on a qdisc for example)
> > > > > >
> > > > > > We have other uses of skb_dst_set_noref()
> > > > > >
> > > > > > Please describe the problem ?
> > > > > The current ip6_tnl_dst_get() does not take the dst refcnt.
> > > > >
> > > > > If the dst is released after ip6_tnl_dst_get() and before
> > > > > skb_dst_set_noref(), would it cause an issue?
> > > >
> > > > We are under rcu here, and a dst in a cache is protected by RCU by
> > > > definition.
> > > >
> > > > skb_dst_set_noref() has following debugging clause, does it trigger for
> > > > you ?
> > > >
> > > > WARN_ON(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> > > No. I did not see this.
> > >
> > > I am probably missing something.  Do you mean the rcu can
> > > protect the followings:
> > >
> > >
> > > ip6_tnl_dst_get()
> > > 			dst_release()
> > > 						dst_free() /* refcnt is 0 */
> > > skb_dst_set_noref()
> > >
> >
> > Yes, this is protected by normal rcu rules.
> >
> > dst wont be freed until all cpus exit their rcu read sections.
> For DST_NOCACHE, like:
>
> rcu_read_lock()
>
> ip6_tnl_dst_get()
>  			dst_release()	/* refcnt is 0 */
> 			=>call_rcu(dst_destroy)
> skb_dst_set_noref()
> __dev_queue_xmit()
> =>skb_dst_force()
> =>__dev_xmit_skb()
> =>q->enqueue()
>
> rcu_read_unlock()
> /* Here, I am holding a dst refcnt but
>  * the dst is already in the next
>  * rcu destroy cycle?
>  */
I look a closer look at dst_rcu_free() and your commit pointers.  I can see your point
for DST_NOCACHE.

However, dst_free() for not DST_NOCACHE is still an issue, I think.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 05:42:00PM -0700, Martin KaFai Lau wrote:
> I look a closer look at dst_rcu_free() and your commit pointers.  I can see your point
> for DST_NOCACHE.
>
> However, dst_free() for not DST_NOCACHE is still an issue, I think.
oops. Ignore this email and continue the discussion in my last email instead.

For DST_NOCACHE, dst_release() is doing call_rcu(dst_destroy_rcu)
and dst_destroy_rcu() is directly calling dst_destroy().   I was confused dst_rcu_free()
was used instead.... time for a break towards the end of the day :p

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> On Tue, 2015-09-01 at 11:55 -0700, Martin KaFai Lau wrote:
> > Problems in the current dst_entry cache in the ip6_tunnel:
> >
> > 1. ip6_tnl_dst_set is racy.  There is no lock to protect it:
> >    - One major problem is that the dst refcnt gets messed up. F.e.
> >      the same dst_cache can be released multiple times and then
> >      triggering the infamous dst refcnt < 0 warning message.
> >    - Another issue is the inconsistency between dst_cache and
> >      dst_cookie.
> >
> >    It can be reproduced by adding and removing the ip6gre tunnel
> >    while running a super_netperf TCP_CRR test.
> >
> > 2. In ip6_tnl_xmit2() and ip6gre_xmit2(), the outgoing skb does
> >    not hold a dst_entry's refcnt.
>
> It should not be a problem. refcnt is taken when/if necessary (skb
> queued on a qdisc for example)
>
> We have other uses of skb_dst_set_noref()
>
> Please describe the problem ?
After some more thoughts, I think it could be that the commit description is
inaccurate/confusing.  skb_dst_set_noref() is not the _source_ of the
problem per se.  Instead, the ip6_tnl_dst_get() should always bump the
dst refcnt before returning the dst. Using skb_dst_set() instead
of skb_dst_set_noref() here is just a follow-through effect.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > 2. Use a spinlock to protect the dst_cache operations
>
> Well, a seqlock would be better : No need for an atomic operation in
> fast path.
>
seqlock can ensure consistency between idst->dst and idst->cookie.
However, IPv6 dst destruction is not protected by rcu.  dst_free() is
directly called, like in ip6_fib.c and a few other places.
Hence, atomic_inc_not_zero() cannot be used here because the dst may
have already been kmem_cache_free() when refcnt is 0.  A spinlock is
needed to stop the ip6_tnl_dst_set() side from removing the refcnt.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Wed, 2015-09-02 at 13:58 -0700, Martin KaFai Lau wrote:
> On Tue, Sep 01, 2015 at 01:14:20PM -0700, Eric Dumazet wrote:
> > > 2. Use a spinlock to protect the dst_cache operations
> >
> > Well, a seqlock would be better : No need for an atomic operation in
> > fast path.
> >
> seqlock can ensure consistency between idst->dst and idst->cookie.
> However, IPv6 dst destruction is not protected by rcu.  dst_free() is
> directly called, like in ip6_fib.c and a few other places.
> Hence, atomic_inc_not_zero() cannot be used here because the dst may
> have already been kmem_cache_free() when refcnt is 0.

Really ? What about basic rcu rules ?

Object cannot be freed until all cpus have exited their RCU sections.

>   A spinlock is
> needed to stop the ip6_tnl_dst_set() side from removing the refcnt.

Are you telling me RCU should be banished from the kernel ? ;)

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote:
> Object cannot be freed until all cpus have exited their RCU sections.
You meant the dst_destroy() here will wait for all cpus exited their RCU sections?

static inline void dst_free(struct dst_entry *dst)
{
	if (dst->obsolete > 0)
		return;
	if (!atomic_read(&dst->__refcnt)) {
		dst = dst_destroy(dst);
		if (!dst)
			return;
	}
	__dst_free(dst);
}

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote:
> On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote:
> > Object cannot be freed until all cpus have exited their RCU sections.
> You meant the dst_destroy() here will wait for all cpus exited their RCU sections?
> 
> static inline void dst_free(struct dst_entry *dst)
> {
> 	if (dst->obsolete > 0)
> 		return;
> 	if (!atomic_read(&dst->__refcnt)) {
> 		dst = dst_destroy(dst);
> 		if (!dst)
> 			return;
> 	}
> 	__dst_free(dst);
> }

dst_free() is called after RCU grace period, in the case you are
interested in.

Look at dst_rcu_free() and rt_free()

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 02 Sep 2015 15:48:57 -0700

> On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote:
>> On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote:
>> > Object cannot be freed until all cpus have exited their RCU sections.
>> You meant the dst_destroy() here will wait for all cpus exited their RCU sections?
>> 
>> static inline void dst_free(struct dst_entry *dst)
>> {
>> 	if (dst->obsolete > 0)
>> 		return;
>> 	if (!atomic_read(&dst->__refcnt)) {
>> 		dst = dst_destroy(dst);
>> 		if (!dst)
>> 			return;
>> 	}
>> 	__dst_free(dst);
>> }
> 
> dst_free() is called after RCU grace period, in the case you are
> interested in.
> 
> Look at dst_rcu_free() and rt_free()

For ipv4, this is true, but in ipv6, it is not necessarily done in
this way.  And I think that is the point Martin is trying to make.

If you look, the dst_free() calls in ipv6 are basically synchronous,
it does not use dst_rcu_free().

And thus, the fix is to make ipv6 properly RCU free route entries.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Martin KaFai Lau]

On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote:
> On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote:
> > On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote:
> > > Object cannot be freed until all cpus have exited their RCU sections.
> > You meant the dst_destroy() here will wait for all cpus exited their RCU sections?
> >
> > static inline void dst_free(struct dst_entry *dst)
> > {
> > 	if (dst->obsolete > 0)
> > 		return;
> > 	if (!atomic_read(&dst->__refcnt)) {
> > 		dst = dst_destroy(dst);
> > 		if (!dst)
> > 			return;
> > 	}
> > 	__dst_free(dst);
> > }
>
> dst_free() is called after RCU grace period, in the case you are
> interested in.
>
> Look at dst_rcu_free() and rt_free()
Yes for IPv4 FIB

Not for IPv6 FIB. F.e. rt6_release()
The IPv6 FIB is protected by rwlock now.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[David Miller]

From: Martin KaFai Lau <kafai@fb.com>
Date: Wed, 2 Sep 2015 16:10:31 -0700

> On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote:
>> dst_free() is called after RCU grace period, in the case you are
>> interested in.
>>
>> Look at dst_rcu_free() and rt_free()
> Yes for IPv4 FIB
> 
> Not for IPv6 FIB. F.e. rt6_release()
> The IPv6 FIB is protected by rwlock now.

The FIB tree can use whatever locking scheme it wants, but the
actual route objects need to be released via RCU to fix the
problems you are seeing.

Converting the entire ipv6 FIB tree handling to RCU is not a
prerequisite for this.

Re: [PATCH net 3/3] ipv6: Fix dst_entry refcnt bugs in ip6_tunnel

[Eric Dumazet]

On Wed, 2015-09-02 at 16:10 -0700, Martin KaFai Lau wrote:
> On Wed, Sep 02, 2015 at 03:48:57PM -0700, Eric Dumazet wrote:
> > On Wed, 2015-09-02 at 14:52 -0700, Martin KaFai Lau wrote:
> > > On Wed, Sep 02, 2015 at 02:30:45PM -0700, Eric Dumazet wrote:
> > > > Object cannot be freed until all cpus have exited their RCU sections.
> > > You meant the dst_destroy() here will wait for all cpus exited their RCU sections?
> > >
> > > static inline void dst_free(struct dst_entry *dst)
> > > {
> > > 	if (dst->obsolete > 0)
> > > 		return;
> > > 	if (!atomic_read(&dst->__refcnt)) {
> > > 		dst = dst_destroy(dst);
> > > 		if (!dst)
> > > 			return;
> > > 	}
> > > 	__dst_free(dst);
> > > }
> >
> > dst_free() is called after RCU grace period, in the case you are
> > interested in.
> >
> > Look at dst_rcu_free() and rt_free()
> Yes for IPv4 FIB
> 
> Not for IPv6 FIB. F.e. rt6_release()
> The IPv6 FIB is protected by rwlock now.

Oh well. I gave you a hint. I was not saying that it was currently used
in IPv6.

Are you telling me that IPv6 needs to continue to use techniques from
1990 ?

Surely we can use modern stuff, like proper RCU and/or seqlocks.

Since you are fixing a day-0 bug, I do not believe there is a particular
hurry to be conservative.