Skip to contentSkip to conversation

rt6i_genid

DISCUSSION Superseded Fan Du 4 participants 13 messages Jul 18, 2013 – Jul 22, 2013

Subsystems (4)
Networking Network drivers IPv4/IPv6 Networking IPsec

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Fan Du

author
#0 lore raw

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to check dst validity" makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid,
As a matter of fact, struct net->rt_genid could only be modified by two places, first is adding/delete IPv4 address, second is inserting new XFRM policy.

Is there any other considerations that adding/deleting IPv4 address would invalid all IPv6 dst as well? because I'm working a patch which actually depends on the result of this question.

Thanks

Signature · 3 lines
浮沉随浪只记今朝笑

--fan
#1 lore raw
Quoted 8 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, As a matter of fact, struct net->rt_genid could only be modified by two places, first is adding/delete IPv4 address, second is inserting new XFRM policy.

Quoted 4 lines

Is there any other considerations that adding/deleting IPv4 address would invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of this question.

No, the goal was to cover the IPsec case, ie invalidate dst entries when an xfrm policy is inserted/deleted.

Regards,
Nicolas

author
#2 lore raw

Thanks for replying :)

Quoted 9 lines · Nicolas Dichtel
Quoted 8 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, As a matter of fact, struct net->rt_genid could only be modified by two places, first is adding/delete IPv4 address, second is inserting new XFRM policy.

Quoted 5 lines
Quoted 4 lines

Is there any other considerations that adding/deleting IPv4 address would invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of this question.

No, the goal was to cover the IPsec case, ie invalidate dst entries when an xfrm policy is inserted/deleted.

Ok, then how about we only checking rt6i_genid against rt_genid *only* when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6 ip6_dst_check for rt_genid is really not necessary.

So what do you think of below modifications?

include/net/ip6_fib.h +2 -0
index 2a601e7..4ae35fd 100644
@@ -119,7 +119,9 @@ struct rt6_info {
        struct inet6_dev                *rt6i_idev;
        unsigned long                   _rt6i_peer;
 
+#ifdef CONFIG_XFRM
        u32                             rt6i_genid;
+#endif
 
        /* more non-fragment space at head required */
        unsigned short                  rt6i_nfheader_len;
net/ipv6/route.c +5 -0
index bd5fd70..1b64406 100644
@@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
                memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
                rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
+#ifdef CONFIG_XFRM
                rt->rt6i_genid = rt_genid(net);
+#endif
                INIT_LIST_HEAD(&rt->rt6i_siblings);
                rt->rt6i_nsiblings = 0;
        }

@@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 
        rt = (struct rt6_info *) dst;
 
+#ifdef CONFIG_XFRM
        /* All IPV6 dsts are created with ->obsolete set to the value
         * DST_OBSOLETE_FORCE_CHK which forces validation calls down
         * into this function always.
+        * Note: for IPv6, rt6i_genid is noly used when XFRM enabled.
         */
        if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
                return NULL;
+#endif
 
        if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
                return dst;
Quoted 4 lines

Regards,
Nicolas

Signature · 3 lines
浮沉随浪只记今朝笑

--fan
#3 lore raw
Quoted 17 lines · Fan Du

Thanks for replying :)

Quoted 13 lines · Nicolas Dichtel
Quoted 12 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, As a matter of fact, struct net->rt_genid could only be modified by two places, first is adding/delete IPv4 address, second is inserting new XFRM policy.

Is there any other considerations that adding/deleting IPv4 address would invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of this question.

Quoted 8 lines
Quoted 2 lines

No, the goal was to cover the IPsec case, ie invalidate dst entries when an xfrm policy is inserted/deleted.

Ok, then how about we only checking rt6i_genid against rt_genid *only* when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6 ip6_dst_check for rt_genid is really not necessary.

So what do you think of below modifications?

Seems good. Just a small comment below.

Quoted 30 lines
include/net/ip6_fib.h +2 -0
index 2a601e7..4ae35fd 100644
@@ -119,7 +119,9 @@ struct rt6_info {
         struct inet6_dev                *rt6i_idev;
         unsigned long                   _rt6i_peer;
 
+#ifdef CONFIG_XFRM
         u32                             rt6i_genid;
+#endif
 
         /* more non-fragment space at head required */
         unsigned short                  rt6i_nfheader_len;
net/ipv6/route.c +2 -0
index bd5fd70..1b64406 100644
@@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
                 memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
                 rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
+#ifdef CONFIG_XFRM
                 rt->rt6i_genid = rt_genid(net);
+#endif
                 INIT_LIST_HEAD(&rt->rt6i_siblings);
                 rt->rt6i_nsiblings = 0;
         }

net/ipv6/route.c static struct dst_entry *ip6_dst_check(struct dst_entry · line 1051

Quoted 9 lines
net/ipv6/route.c +2 -0
@@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry
 *dst, u32 cookie)
 
         rt = (struct rt6_info *) dst;
 
+#ifdef CONFIG_XFRM
         /* All IPV6 dsts are created with ->obsolete set to the value
          * DST_OBSOLETE_FORCE_CHK which forces validation calls down
          * into this function always.
+        * Note: for IPv6, rt6i_genid is noly used when XFRM enabled.

indentation is wrong, but I'm not sure this comment is really needed ... you've put the ifdef 4 lines above.

Quoted 7 lines
net/ipv6/route.c +1 -0
          */
         if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
                 return NULL;
+#endif
 
         if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))

                 return dst;
author
#4 lore raw
Quoted 21 lines · Nicolas Dichtel
Quoted 20 lines · Fan Du

Thanks for replying :)

Quoted 15 lines · Nicolas Dichtel
Quoted 8 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, As a matter of fact, struct net->rt_genid could only be modified by two places, first is adding/delete IPv4 address, second is inserting new XFRM policy.

Quoted 4 lines

Is there any other considerations that adding/deleting IPv4 address would invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of this question.

No, the goal was to cover the IPsec case, ie invalidate dst entries when an xfrm policy is inserted/deleted.

Quoted 6 lines
Quoted 5 lines

Ok, then how about we only checking rt6i_genid against rt_genid *only* when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6 ip6_dst_check for rt_genid is really not necessary.

So what do you think of below modifications?

Seems good. Just a small comment below.

Will send v2 for your reviewing when net-next is reopen.

Thanks

Quoted 49 lines
Quoted 30 lines
include/net/ip6_fib.h +2 -0
index 2a601e7..4ae35fd 100644
@@ -119,7 +119,9 @@ struct rt6_info {
 struct inet6_dev *rt6i_idev;
 unsigned long _rt6i_peer;
 
+#ifdef CONFIG_XFRM
 u32 rt6i_genid;
+#endif
 
 /* more non-fragment space at head required */
 unsigned short rt6i_nfheader_len;
net/ipv6/route.c +2 -0
index bd5fd70..1b64406 100644
@@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
+#ifdef CONFIG_XFRM
 rt->rt6i_genid = rt_genid(net);
+#endif
 INIT_LIST_HEAD(&rt->rt6i_siblings);
 rt->rt6i_nsiblings = 0;
 }

net/ipv6/route.c static struct dst_entry *ip6_dst_check(struct dst_entry · line 1051

Quoted 9 lines
net/ipv6/route.c +2 -0
@@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry
 *dst, u32 cookie)
 
 rt = (struct rt6_info *) dst;
 
+#ifdef CONFIG_XFRM
 /* All IPV6 dsts are created with ->obsolete set to the value
 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 * into this function always.
+ * Note: for IPv6, rt6i_genid is noly used when XFRM enabled.

indentation is wrong, but I'm not sure this comment is really needed ... you've put the ifdef 4 lines above.

Quoted 7 lines
net/ipv6/route.c +1 -0
 */
 if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
 return NULL;
+#endif
 
 if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))

return dst;

Signature · 3 lines
浮沉随浪只记今朝笑

--fan
maintainer
#5 lore raw

From: Fan Du <[email protected]>
Date: Fri, 19 Jul 2013 08:01:47 +0800

Quoted 36 lines · Nicolas Dichtel
Quoted 33 lines · Fan Du
Quoted 32 lines

Thanks for replying :)

Quoted 19 lines · Nicolas Dichtel
Quoted 12 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to
check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid,
As a matter of fact, struct net->rt_genid could only be modified by two places,
first is adding/delete IPv4 address, second is inserting new XFRM policy.

Quoted 6 lines

Is there any other considerations that adding/deleting IPv4 address would
invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of
this question.

Quoted 3 lines

No, the goal was to cover the IPsec case, ie invalidate dst entries when an
xfrm policy is inserted/deleted.

Ok, then how about we only checking rt6i_genid against rt_genid *only* when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6
ip6_dst_check for rt_genid is really not necessary.

Quoted 4 lines
Quoted 2 lines
Quoted 1 line

So what do you think of below modifications?

Seems good. Just a small comment below.

Will send v2 for your reviewing when net-next is reopen.

Although it's a correct change, it is of almost no value. %99.9999999 of users will be running kernels with CONFIG_XFRM enabled.

So your savings are essentially for no-one.

author
#6 lore raw
Quoted 39 lines · David Miller

From: Fan Du<[email protected]>
Date: Fri, 19 Jul 2013 08:01:47 +0800

Quoted 36 lines · Nicolas Dichtel
Quoted 33 lines · Fan Du
Quoted 32 lines

Thanks for replying :)

Quoted 19 lines · Nicolas Dichtel
Quoted 12 lines · Fan Du

Hello Nicolas

Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to
check dst validity"
makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid,
As a matter of fact, struct net->rt_genid could only be modified by two places,
first is adding/delete IPv4 address, second is inserting new XFRM policy.

Quoted 6 lines

Is there any other considerations that adding/deleting IPv4 address would
invalid all IPv6 dst
as well? because I'm working a patch which actually depends on the result of
this question.

Quoted 3 lines

No, the goal was to cover the IPsec case, ie invalidate dst entries when an
xfrm policy is inserted/deleted.

Ok, then how about we only checking rt6i_genid against rt_genid *only* when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6
ip6_dst_check for rt_genid is really not necessary.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line

So what do you think of below modifications?

Seems good. Just a small comment below.

Will send v2 for your reviewing when net-next is reopen.

Although it's a correct change, it is of almost no value. %99.9999999 of users will be running kernels with CONFIG_XFRM enabled.

Thanks. Good to know %99.99999999 users protect their networking with IPsec.

Quoted 1 line

So your savings are essentially for no-one.

Signature · 3 lines
浮沉随浪只记今朝笑

--fan
maintainer
#7 lore raw

From: Fan Du <[email protected]>
Date: Fri, 19 Jul 2013 11:28:51 +0800

Quoted 6 lines · David Miller
Quoted 2 lines

Although it's a correct change, it is of almost no value. %99.9999999 of users will be running kernels with CONFIG_XFRM enabled.

Thanks. Good to know %99.99999999 users protect their networking with IPsec.

That is not what I said.

I said that nearly every user will be running a kernel with that config option enabled, I did not say that they will actually be using IPSEC.

Distributions enable all options, so that users may use any facility that they want.

So optimizing for things like this are almost pointless.

author
#8 lore raw
Quoted 12 lines · David Miller

From: Fan Du<[email protected]>
Date: Fri, 19 Jul 2013 11:28:51 +0800

Quoted 6 lines · David Miller
Quoted 2 lines

Although it's a correct change, it is of almost no value. %99.9999999 of users will be running kernels with CONFIG_XFRM enabled.

Thanks. Good to know %99.99999999 users protect their networking with IPsec.

That is not what I said.

Quoted 9 lines

I said that nearly every user will be running a kernel with that config option enabled, I did not say that they will actually be using IPSEC.

Distributions enable all options, so that users may use any facility that they want.

So optimizing for things like this are almost pointless.

I've understood the situation/point you're trying to describe. No problem, I will drop this almost-pointless patch :)

The original commit is targeted for XFRM policy inserting/removing, but it uses net genid shared by both IPv4 and IPv6, the side effect is add/delete IPv4 address will invalidate IPv6 dst in all.

We *do* need to bump genid when add/delete IPv6 address in scenario I described here: http://www.spinics.net/lists/netdev/msg243398.html,
but definitely not from add/delete IPv4 address. Moreover test shows that DCCP still push thousands of packets on wire after delete its IPv6 address in the same scenario I describe before.

The impulse to bump genid for IPv6 is much more stronger after this commit even do it unintentionally.

So am I missing some thing more important inside IPv6, Dave?

Signature · 3 lines
浮沉随浪只记今朝笑

--fan
maintainer
#9 lore raw

From: Fan Du <[email protected]>
Date: Fri, 19 Jul 2013 15:50:20 +0800

Quoted 11 lines

The original commit is targeted for XFRM policy inserting/removing, but it uses net genid shared by both IPv4 and IPv6, the side effect is add/delete IPv4 address will invalidate IPv6 dst in all.

We *do* need to bump genid when add/delete IPv6 address in scenario I described here: http://www.spinics.net/lists/netdev/msg243398.html,
but definitely not from add/delete IPv4 address. Moreover test shows that DCCP still push thousands of packets on wire after delete its IPv6
address in the same scenario I describe before.

Quoted 2 lines

The impulse to bump genid for IPv6 is much more stronger after this commit even do it unintentionally.

If you really think it will help, and it will still handle the IPSEC case, you can make a seperate genid for ipv4 and ipv6 but that might not work out so cleanly.

author
#10 lore raw

RFC PATCH 1/1 net: split rt_genid for ipv4 and ipv6

Adding IPsec and other IPv4/IPv6 maintainers in the list.

Quoted 14 lines · David Miller

From: Fan Du<[email protected]>
Date: Fri, 19 Jul 2013 15:50:20 +0800

Quoted 11 lines

The original commit is targeted for XFRM policy inserting/removing, but it uses net genid shared by both IPv4 and IPv6, the side effect is add/delete IPv4 address will invalidate IPv6 dst in all.

We *do* need to bump genid when add/delete IPv6 address in scenario I described here: http://www.spinics.net/lists/netdev/msg243398.html,
but definitely not from add/delete IPv4 address. Moreover test shows that DCCP still push thousands of packets on wire after delete its IPv6
address in the same scenario I describe before.

Quoted 7 lines
Quoted 2 lines

The impulse to bump genid for IPv6 is much more stronger after this commit even do it unintentionally.

If you really think it will help, and it will still handle the IPSEC case, you can make a seperate genid for ipv4 and ipv6 but that might not work out so cleanly.

At least let me give it a try. Any comments would be truly welcome.

From c79215d64038d62340d77c6ac070d8bb479b2f89 Mon Sep 17 00:00:00 2001
From: Fan Du <[email protected]>
Date: Mon, 22 Jul 2013 11:31:56 +0800
Subject: [RFC PATCH net-next] net: split rt_genid for ipv4 and ipv6

Current net name space has only one genid for both IPv4 and IPv6, it has below drawbacks:

- Add/delete an IPv4 address will invalidate all IPv6 routing table entries.
- Insert/remove XFRM policy will also invalidate both IPv4/IPv6 routing table entries even when the policy is only applied for one address family.

Thus, this patch attempt to split one genid for two to cater for IPv4 and IPv6 separately in a fine granularity.

Signed-off-by Fan Du <[email protected]>
---
 include/net/net_namespace.h |   33 ++++++++++++++++++++++++++++-----
 net/ipv4/route.c            |   16 ++++++++--------
 net/ipv6/af_inet6.c         |    1 +
 net/ipv6/route.c            |    4 ++--
 net/xfrm/xfrm_policy.c      |    8 +++++++-
 5 files changed, 46 insertions(+), 16 deletions(-)
include/net/net_namespace.h +28 -5
index 84e37b1..a08e312 100644
@@ -119,8 +119,11 @@ struct net {
 	struct netns_ipvs	*ipvs;
 #endif
 	struct sock		*diag_nlsk;
-	atomic_t		rt_genid;
 	atomic_t		fnhe_genid;
+	atomic_t                rt_genid_ipv4;
+#if IS_ENABLED(CONFIG_IPV6)
+	atomic_t                rt_genid_ipv6;
+#endif
 };
 
 /*

@@ -333,14 +336,34 @@ static inline void unregister_net_sysctl_table(struct ctl_table_header *header)
 }
 #endif
 
-static inline int rt_genid(struct net *net)
+static inline int rt_genid_ipv4(struct net *net)
+{
+	return atomic_read(&net->rt_genid_ipv4);
+}
+
+static inline void rt_genid_bump_ipv4(struct net *net)
 {
-	return atomic_read(&net->rt_genid);
+	atomic_inc(&net->rt_genid_ipv4);
 }
 
-static inline void rt_genid_bump(struct net *net)
+#if IS_ENABLED(CONFIG_IPV6)
+static inline int rt_genid_ipv6(struct net *net)
 {
-	atomic_inc(&net->rt_genid);
+	return atomic_read(&net->rt_genid_ipv6);
+}
+
+static inline void rt_genid_bump_ipv6(struct net *net)
+{
+	atomic_inc(&net->rt_genid_ipv6);
+}
+#endif
+
+static inline void rt_genid_bump_all(struct net *net)
+{
+	atomic_inc(&net->rt_genid_ipv4);
+#if IS_ENABLED(CONFIG_IPV6)
+	atomic_inc(&net->rt_genid_ipv6);
+#endif
 }
 
 static inline int fnhe_genid(struct net *net)
net/ipv4/route.c +8 -8
index a9a54a2..df6095d 100644
@@ -435,12 +435,12 @@ static inline int ip_rt_proc_init(void)
 
 static inline bool rt_is_expired(const struct rtable *rth)
 {
-	return rth->rt_genid != rt_genid(dev_net(rth->dst.dev));
+	return rth->rt_genid != rt_genid_ipv4(dev_net(rth->dst.dev));
 }
 
 void rt_cache_flush(struct net *net)
 {
-	rt_genid_bump(net);
+	rt_genid_bump_ipv4(net);
 }
 
 static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,

@@ -1458,7 +1458,7 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 #endif
 	rth->dst.output = ip_rt_bug;
 
-	rth->rt_genid	= rt_genid(dev_net(dev));
+	rth->rt_genid	= rt_genid_ipv4(dev_net(dev));
 	rth->rt_flags	= RTCF_MULTICAST;
 	rth->rt_type	= RTN_MULTICAST;
 	rth->rt_is_input= 1;

@@ -1589,7 +1589,7 @@ static int __mkroute_input(struct sk_buff *skb,
 		goto cleanup;
 	}
 
-	rth->rt_genid = rt_genid(dev_net(rth->dst.dev));
+	rth->rt_genid = rt_genid_ipv4(dev_net(rth->dst.dev));
 	rth->rt_flags = flags;
 	rth->rt_type = res->type;
 	rth->rt_is_input = 1;

@@ -1760,7 +1760,7 @@ local_input:
 	rth->dst.tclassid = itag;
 #endif
 
-	rth->rt_genid = rt_genid(net);
+	rth->rt_genid = rt_genid_ipv4(net);
 	rth->rt_flags 	= flags|RTCF_LOCAL;
 	rth->rt_type	= res.type;
 	rth->rt_is_input = 1;

@@ -1945,7 +1945,7 @@ add:
 
 	rth->dst.output = ip_output;
 
-	rth->rt_genid = rt_genid(dev_net(dev_out));
+	rth->rt_genid = rt_genid_ipv4(dev_net(dev_out));
 	rth->rt_flags	= flags;
 	rth->rt_type	= type;
 	rth->rt_is_input = 0;

@@ -2227,7 +2227,7 @@ struct dst_entry *ipv4_blackhole_route(struct net *net, struct dst_entry *dst_or
 		rt->rt_iif = ort->rt_iif;
 		rt->rt_pmtu = ort->rt_pmtu;
 
-		rt->rt_genid = rt_genid(net);
+		rt->rt_genid = rt_genid_ipv4(net);
 		rt->rt_flags = ort->rt_flags;
 		rt->rt_type = ort->rt_type;
 		rt->rt_gateway = ort->rt_gateway;

@@ -2665,7 +2665,7 @@ static __net_initdata struct pernet_operations sysctl_route_ops = {
 
 static __net_init int rt_genid_init(struct net *net)
 {
-	atomic_set(&net->rt_genid, 0);
+	atomic_set(&net->rt_genid_ipv4, 0);
 	atomic_set(&net->fnhe_genid, 0);
 	get_random_bytes(&net->ipv4.dev_addr_genid,
 			 sizeof(net->ipv4.dev_addr_genid));
net/ipv6/af_inet6.c +1 -0
index a5ac969..af6855c 100644
@@ -766,6 +766,7 @@ static int __net_init inet6_net_init(struct net *net)
 
 	net->ipv6.sysctl.bindv6only = 0;
 	net->ipv6.sysctl.icmpv6_time = 1*HZ;
+	atomic_set(&net->rt_genid_ipv6, 0);
 
 	err = ipv6_init_mibs(net);
 	if (err)
net/ipv6/route.c +2 -2
index a8c891a..45ca9af 100644
@@ -283,7 +283,7 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 
 		memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
 		rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers);
-		rt->rt6i_genid = rt_genid(net);
+		rt->rt6i_genid = rt_genid_ipv6(net);
 		INIT_LIST_HEAD(&rt->rt6i_siblings);
 		rt->rt6i_nsiblings = 0;
 	}

@@ -1062,7 +1062,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
 	 * into this function always.
 	 */
-	if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
+	if (rt->rt6i_genid != rt_genid_ipv6(dev_net(rt->dst.dev)))
 		return NULL;
 
 	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
net/xfrm/xfrm_policy.c +7 -1
index e52cab3..dbd36e0 100644
@@ -660,7 +660,13 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 	xfrm_pol_hold(policy);
 	net->xfrm.policy_count[dir]++;
 	atomic_inc(&flow_cache_genid);
-	rt_genid_bump(net);
+
+	/* After previous checking, p->family can either be AF_INET or AF_INET6 */
+	if (policy->family == AF_INET)	
+		rt_genid_bump_ipv4(net);
+	else
+		rt_genid_bump_ipv6(net);
+
 	if (delpol) {
 		xfrm_policy_requeue(delpol, policy);
 		__xfrm_policy_unlink(delpol, dir);
Signature · 11 lines
1.7.9.5






-- 
浮沉随浪只记今朝笑

--fan
maintainer
#11 lore raw

Re: [RFC PATCH net-next] net: split rt_genid for ipv4 and ipv6

Quoted 5 lines · Fan Du
include/net/net_namespace.h +2 -1
-static inline void rt_genid_bump(struct net *net)
+#if IS_ENABLED(CONFIG_IPV6)
+static inline int rt_genid_ipv6(struct net *net)
 {

include/net/net_namespace.h static inline void unregister_net_sysctl_table(struct ctl_table_header *header) · line 360

Quoted 10 lines
include/net/net_namespace.h +9 -1
-	atomic_inc(&net->rt_genid);
+	return atomic_read(&net->rt_genid_ipv6);
+}
+
+static inline void rt_genid_bump_ipv6(struct net *net)
+{
+	atomic_inc(&net->rt_genid_ipv6);
+}
+#endif
+

I think this will introduce a compile error if ipv6 is not enabled because you use rt_genid_bump_ipv6() in the generic IPsec code.

include/net/net_namespace.h static inline void unregister_net_sysctl_table(struct ctl_table_header *header) · line 367

Quoted 8 lines
include/net/net_namespace.h +6 -0
+static inline void rt_genid_bump_all(struct net *net)
+{
+	atomic_inc(&net->rt_genid_ipv4);
+#if IS_ENABLED(CONFIG_IPV6)
+	atomic_inc(&net->rt_genid_ipv6);
+#endif
 }

Function is unused.

The rest looks ok.

#12 lore raw

Re: [RFC PATCH net-next] net: split rt_genid for ipv4 and ipv6

In reply to Fan Du
Quoted 59 lines · Fan Du

Adding IPsec and other IPv4/IPv6 maintainers in the list.

Quoted 14 lines · David Miller

From: Fan Du<[email protected]>
Date: Fri, 19 Jul 2013 15:50:20 +0800

Quoted 11 lines

The original commit is targeted for XFRM policy inserting/removing, but it uses net genid shared by both IPv4 and IPv6, the side effect is add/delete IPv4 address will invalidate IPv6 dst in all.

We *do* need to bump genid when add/delete IPv6 address in scenario I described here: http://www.spinics.net/lists/netdev/msg243398.html,
but definitely not from add/delete IPv4 address. Moreover test shows that DCCP still push thousands of packets on wire after delete its IPv6
address in the same scenario I describe before.

Quoted 7 lines
Quoted 2 lines

The impulse to bump genid for IPv6 is much more stronger after this commit even do it unintentionally.

If you really think it will help, and it will still handle the IPSEC case, you can make a seperate genid for ipv4 and ipv6 but that might not work out so cleanly.

At least let me give it a try. Any comments would be truly welcome.

From c79215d64038d62340d77c6ac070d8bb479b2f89 Mon Sep 17 00:00:00 2001
From: Fan Du <[email protected]>
Date: Mon, 22 Jul 2013 11:31:56 +0800
Subject: [RFC PATCH net-next] net: split rt_genid for ipv4 and ipv6

Current net name space has only one genid for both IPv4 and IPv6, it has below drawbacks:

- Add/delete an IPv4 address will invalidate all IPv6 routing table entries.
- Insert/remove XFRM policy will also invalidate both IPv4/IPv6 routing table entries
even when the policy is only applied for one address family.

Thus, this patch attempt to split one genid for two to cater for IPv4 and IPv6 separately
in a fine granularity.

Signed-off-by Fan Du <[email protected]>
---
  include/net/net_namespace.h |   33 ++++++++++++++++++++++++++++-----
  net/ipv4/route.c            |   16 ++++++++--------
  net/ipv6/af_inet6.c         |    1 +
  net/ipv6/route.c            |    4 ++--
  net/xfrm/xfrm_policy.c      |    8 +++++++-
  5 files changed, 46 insertions(+), 16 deletions(-)
include/net/net_namespace.h +0 -0
index 84e37b1..a08e312 100644

include/net/net_namespace.h struct net { · line 127

Quoted 10 lines
include/net/net_namespace.h +4 -1
@@ -119,8 +119,11 @@ struct net {
      struct netns_ipvs    *ipvs;
  #endif
      struct sock        *diag_nlsk;
-    atomic_t        rt_genid;
      atomic_t        fnhe_genid;
+    atomic_t                rt_genid_ipv4;
+#if IS_ENABLED(CONFIG_IPV6)
+    atomic_t                rt_genid_ipv6;
+#endif
  };

Why not putting these new fields in 'struct netns_ipv4' and 'struct netns_ipv6'?