[PATCH] sctp: check dst validity after IPsec operations

[PATCH] sctp: check dst validity after IPsec operations

[Nicolas Dichtel]

dst stored in struct sctp_transport needs to be recalculated when ipsec policy
are updated. We use flow_cache_genid for that.

For example, if a SCTP connection is established and then an IPsec policy is
set, the old SCTP flow will not be updated and thus will not use the new
IPsec policy.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/sctp/sctp.h    | 3 ++-
 include/net/sctp/structs.h | 3 +++
 net/sctp/ipv6.c            | 1 +
 net/sctp/protocol.c        | 1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 9c6414f..3267246 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
  */
 static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
 {
-	if (t->dst && !dst_check(t->dst, 0)) {
+	if ((t->dst && !dst_check(t->dst, 0)) ||
+	    (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
 		dst_release(t->dst);
 		t->dst = NULL;
 	}
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 0fef00f..9dab882 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -948,6 +948,9 @@ struct sctp_transport {
 
 	/* 64-bit random number sent with heartbeat. */
 	__u64 hb_nonce;
+
+	/* used to check validity of dst */
+	__u32 flow_cache_genid;
 };
 
 struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *,
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index ea14cb4..2ed7410 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -349,6 +349,7 @@ out:
 		struct rt6_info *rt;
 		rt = (struct rt6_info *)dst;
 		t->dst = dst;
+		t->flow_cache_genid = atomic_read(&flow_cache_genid);
 		SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
 			&rt->rt6i_dst.addr, &fl6->saddr);
 	} else {
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 2d51842..4795a1a 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -512,6 +512,7 @@ out_unlock:
 	rcu_read_unlock();
 out:
 	t->dst = dst;
+	t->flow_cache_genid = atomic_read(&flow_cache_genid);
 	if (dst)
 		SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
 				  &fl4->daddr, &fl4->saddr);
-- 
1.7.12

Re: [PATCH] sctp: check dst validity after IPsec operations

[Vlad Yasevich]

On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
> are updated. We use flow_cache_genid for that.
>
> For example, if a SCTP connection is established and then an IPsec policy is
> set, the old SCTP flow will not be updated and thus will not use the new
> IPsec policy.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

why doesn't this need to be done for TCP?  What makes SCTP special in 
this case?

ip_queue_xmit does an __sk_dst_check() which is essentially what 
sctp_transport_dst_check() does.  That should determine if the currently 
cached route is valid or not.

Looks like sctp may need to change to using ip_route_output_ports() call
as ip_route_output_key may not do all that is necessary

-vlad

> ---
>   include/net/sctp/sctp.h    | 3 ++-
>   include/net/sctp/structs.h | 3 +++
>   net/sctp/ipv6.c            | 1 +
>   net/sctp/protocol.c        | 1 +
>   4 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9c6414f..3267246 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>    */
>   static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
>   {
> -	if (t->dst && !dst_check(t->dst, 0)) {
> +	if ((t->dst && !dst_check(t->dst, 0)) ||
> +	    (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>   		dst_release(t->dst);
>   		t->dst = NULL;
>   	}
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 0fef00f..9dab882 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -948,6 +948,9 @@ struct sctp_transport {
>
>   	/* 64-bit random number sent with heartbeat. */
>   	__u64 hb_nonce;
> +
> +	/* used to check validity of dst */
> +	__u32 flow_cache_genid;
>   };
>
>   struct sctp_transport *sctp_transport_new(struct net *, const union sctp_addr *,
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index ea14cb4..2ed7410 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -349,6 +349,7 @@ out:
>   		struct rt6_info *rt;
>   		rt = (struct rt6_info *)dst;
>   		t->dst = dst;
> +		t->flow_cache_genid = atomic_read(&flow_cache_genid);
>   		SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>   			&rt->rt6i_dst.addr, &fl6->saddr);
>   	} else {
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 2d51842..4795a1a 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -512,6 +512,7 @@ out_unlock:
>   	rcu_read_unlock();
>   out:
>   	t->dst = dst;
> +	t->flow_cache_genid = atomic_read(&flow_cache_genid);
>   	if (dst)
>   		SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>   				  &fl4->daddr, &fl4->saddr);
>

Re: [PATCH] sctp: check dst validity after IPsec operations

[Nicolas Dichtel]

Le 06/09/2012 18:04, Vlad Yasevich a écrit :
> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
>> are updated. We use flow_cache_genid for that.
>>
>> For example, if a SCTP connection is established and then an IPsec policy is
>> set, the old SCTP flow will not be updated and thus will not use the new
>> IPsec policy.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> why doesn't this need to be done for TCP?  What makes SCTP special in this case?
Tests prove that the pb does not exist with TCP. I made the patch some times 
ago, I will look again deeply to find the difference.

>
> ip_queue_xmit does an __sk_dst_check() which is essentially what
> sctp_transport_dst_check() does.  That should determine if the currently cached
> route is valid or not.
The problem is that route will not be invalidated, because dst->check() has no 
xfrm path so xfrm_dst_check() will never be called.


Regards,
Nicolas

>
> Looks like sctp may need to change to using ip_route_output_ports() call
> as ip_route_output_key may not do all that is necessary
>
> -vlad
>
>> ---
>>   include/net/sctp/sctp.h    | 3 ++-
>>   include/net/sctp/structs.h | 3 +++
>>   net/sctp/ipv6.c            | 1 +
>>   net/sctp/protocol.c        | 1 +
>>   4 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index 9c6414f..3267246 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>>    */
>>   static inline struct dst_entry *sctp_transport_dst_check(struct
>> sctp_transport *t)
>>   {
>> -    if (t->dst && !dst_check(t->dst, 0)) {
>> +    if ((t->dst && !dst_check(t->dst, 0)) ||
>> +        (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>>           dst_release(t->dst);
>>           t->dst = NULL;
>>       }
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 0fef00f..9dab882 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -948,6 +948,9 @@ struct sctp_transport {
>>
>>       /* 64-bit random number sent with heartbeat. */
>>       __u64 hb_nonce;
>> +
>> +    /* used to check validity of dst */
>> +    __u32 flow_cache_genid;
>>   };
>>
>>   struct sctp_transport *sctp_transport_new(struct net *, const union
>> sctp_addr *,
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index ea14cb4..2ed7410 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -349,6 +349,7 @@ out:
>>           struct rt6_info *rt;
>>           rt = (struct rt6_info *)dst;
>>           t->dst = dst;
>> +        t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>           SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>               &rt->rt6i_dst.addr, &fl6->saddr);
>>       } else {
>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>> index 2d51842..4795a1a 100644
>> --- a/net/sctp/protocol.c
>> +++ b/net/sctp/protocol.c
>> @@ -512,6 +512,7 @@ out_unlock:
>>       rcu_read_unlock();
>>   out:
>>       t->dst = dst;
>> +    t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>       if (dst)
>>           SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>>                     &fl4->daddr, &fl4->saddr);
>>
>

Re: [PATCH] sctp: check dst validity after IPsec operations

[Vlad Yasevich]

On 09/06/2012 12:40 PM, Nicolas Dichtel wrote:
> Le 06/09/2012 18:04, Vlad Yasevich a écrit :
>> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>>> dst stored in struct sctp_transport needs to be recalculated when
>>> ipsec policy
>>> are updated. We use flow_cache_genid for that.
>>>
>>> For example, if a SCTP connection is established and then an IPsec
>>> policy is
>>> set, the old SCTP flow will not be updated and thus will not use the new
>>> IPsec policy.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>
>> why doesn't this need to be done for TCP?  What makes SCTP special in
>> this case?
> Tests prove that the pb does not exist with TCP. I made the patch some
> times ago, I will look again deeply to find the difference.
>

TCP appears to cache the flowi and uses that to re-route the packet.
However, re-route still seems predicated on dst_check()...

>>
>> ip_queue_xmit does an __sk_dst_check() which is essentially what
>> sctp_transport_dst_check() does.  That should determine if the
>> currently cached
>> route is valid or not.
> The problem is that route will not be invalidated, because dst->check()
> has no xfrm path so xfrm_dst_check() will never be called.
>

Shouldn't the cache be invalidated in this case?  If the cache is 
invalidated, that should cause a new lookup.  If the cache isn't 
invalidated, then any established connections that may now be impacted 
by the policy will not pick it up.

-vlad

>
> Regards,
> Nicolas
>
>>
>> Looks like sctp may need to change to using ip_route_output_ports() call
>> as ip_route_output_key may not do all that is necessary
>>
>> -vlad
>>
>>> ---
>>>   include/net/sctp/sctp.h    | 3 ++-
>>>   include/net/sctp/structs.h | 3 +++
>>>   net/sctp/ipv6.c            | 1 +
>>>   net/sctp/protocol.c        | 1 +
>>>   4 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index 9c6414f..3267246 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr
>>> *addr)
>>>    */
>>>   static inline struct dst_entry *sctp_transport_dst_check(struct
>>> sctp_transport *t)
>>>   {
>>> -    if (t->dst && !dst_check(t->dst, 0)) {
>>> +    if ((t->dst && !dst_check(t->dst, 0)) ||
>>> +        (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>>>           dst_release(t->dst);
>>>           t->dst = NULL;
>>>       }
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 0fef00f..9dab882 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -948,6 +948,9 @@ struct sctp_transport {
>>>
>>>       /* 64-bit random number sent with heartbeat. */
>>>       __u64 hb_nonce;
>>> +
>>> +    /* used to check validity of dst */
>>> +    __u32 flow_cache_genid;
>>>   };
>>>
>>>   struct sctp_transport *sctp_transport_new(struct net *, const union
>>> sctp_addr *,
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index ea14cb4..2ed7410 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -349,6 +349,7 @@ out:
>>>           struct rt6_info *rt;
>>>           rt = (struct rt6_info *)dst;
>>>           t->dst = dst;
>>> +        t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>>           SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>               &rt->rt6i_dst.addr, &fl6->saddr);
>>>       } else {
>>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>>> index 2d51842..4795a1a 100644
>>> --- a/net/sctp/protocol.c
>>> +++ b/net/sctp/protocol.c
>>> @@ -512,6 +512,7 @@ out_unlock:
>>>       rcu_read_unlock();
>>>   out:
>>>       t->dst = dst;
>>> +    t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>>       if (dst)
>>>           SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>>>                     &fl4->daddr, &fl4->saddr);
>>>
>>

Re: [PATCH] sctp: check dst validity after IPsec operations

[Nicolas Dichtel]

Le 06/09/2012 19:03, Vlad Yasevich a écrit :
> On 09/06/2012 12:40 PM, Nicolas Dichtel wrote:
>> Le 06/09/2012 18:04, Vlad Yasevich a écrit :
>>> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>>>> dst stored in struct sctp_transport needs to be recalculated when
>>>> ipsec policy
>>>> are updated. We use flow_cache_genid for that.
>>>>
>>>> For example, if a SCTP connection is established and then an IPsec
>>>> policy is
>>>> set, the old SCTP flow will not be updated and thus will not use the new
>>>> IPsec policy.
>>>>
>>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>>
>>> why doesn't this need to be done for TCP?  What makes SCTP special in
>>> this case?
>> Tests prove that the pb does not exist with TCP. I made the patch some
>> times ago, I will look again deeply to find the difference.
>>
>
> TCP appears to cache the flowi and uses that to re-route the packet.
> However, re-route still seems predicated on dst_check()...
Yes ... but I don't find the difference. Re-route is not done immediately in 
TCP, it takes few seconds.

>
>>>
>>> ip_queue_xmit does an __sk_dst_check() which is essentially what
>>> sctp_transport_dst_check() does.  That should determine if the
>>> currently cached
>>> route is valid or not.
>> The problem is that route will not be invalidated, because dst->check()
>> has no xfrm path so xfrm_dst_check() will never be called.
>>
>
> Shouldn't the cache be invalidated in this case?  If the cache is invalidated,
> that should cause a new lookup.  If the cache isn't invalidated, then any
> established connections that may now be impacted by the policy will not pick it up.
Yes, you're right. If I flush the cache manually (with the sysctl), route are 
correctly updated.
I will send a new proposal.


Regards,
Nicolas

Re: [PATCH] sctp: check dst validity after IPsec operations

[Nicolas Dichtel]

Le 06/09/2012 18:04, Vlad Yasevich a écrit :
> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
>> are updated. We use flow_cache_genid for that.
>>
>> For example, if a SCTP connection is established and then an IPsec policy is
>> set, the old SCTP flow will not be updated and thus will not use the new
>> IPsec policy.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> why doesn't this need to be done for TCP?  What makes SCTP special in this case?
>
> ip_queue_xmit does an __sk_dst_check() which is essentially what
> sctp_transport_dst_check() does.  That should determine if the currently cached
> route is valid or not.
>
> Looks like sctp may need to change to using ip_route_output_ports() call
> as ip_route_output_key may not do all that is necessary
I try, but it doesn't solve the problem. In fact, it seems better to use 
ip_route_output_ports(), would you like me to send a patch?


Regards,
Nicolas

Re: [PATCH] sctp: check dst validity after IPsec operations

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu,  6 Sep 2012 13:40:29 -0400

> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
> are updated. We use flow_cache_genid for that.
> 
> For example, if a SCTP connection is established and then an IPsec policy is
> set, the old SCTP flow will not be updated and thus will not use the new
> IPsec policy.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

I don't like that SCTP need to perform special DST validation.

The normal DST validation mechanism already in place should be
sufficient.

Otherwise this problem must exist in other protocols too, and
fixing a tree wide issue privately inside of one protocol is
not acceptable.

Re: [PATCH] sctp: check dst validity after IPsec operations

[Nicolas Dichtel]

Le 06/09/2012 20:10, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Thu,  6 Sep 2012 13:40:29 -0400
>
>> dst stored in struct sctp_transport needs to be recalculated when ipsec policy
>> are updated. We use flow_cache_genid for that.
>>
>> For example, if a SCTP connection is established and then an IPsec policy is
>> set, the old SCTP flow will not be updated and thus will not use the new
>> IPsec policy.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> I don't like that SCTP need to perform special DST validation.
Ipv6 do the same:

inet6_csk_xmit()->inet6_csk_route_socket()->__inet6_csk_dst_check()
-> compare flow_cache_genid and rt6i_flow_cache_genid.

>
> The normal DST validation mechanism already in place should be
> sufficient.
I don't find why TCP recalculate the route, but it's not immediate, we should 
wait a little.

>
> Otherwise this problem must exist in other protocols too, and
> fixing a tree wide issue privately inside of one protocol is
> not acceptable.
I will propose another patch.


Regards,
Nicolas

[PATCH RFC net-next 0/2] Take care of xfrm policy when checking dst entries

[Nicolas Dichtel]

The goal of these patches is to fix the following problem: a session is
established (TCP, SCTP) and after a new policy is inserted. The current
code does not recalculate the route, thus the traffic is not encrypted.

The patch propose to check flow_cache_genid value when checking a dst
entry, which is incremented each time a policy is inserted or deleted.

Patches are tested with TCP and SCTP.

Comments are welcome.

Regards,
Nicolas

[PATCH 1/2] dst: take into account policy update on check()

[Nicolas Dichtel]

When a xfrm policy is inserted or deleted, we must invalidate
all dst and recalculate the route.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/dst.h     | 7 +++++++
 net/core/dst.c        | 1 +
 net/core/sock.c       | 4 ++--
 net/ipv6/ip6_tunnel.c | 3 +--
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index 9a78810..478e55a 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -101,6 +101,9 @@ struct dst_entry {
 	atomic_t		__refcnt;	/* client references	*/
 	int			__use;
 	unsigned long		lastuse;
+#ifdef CONFIG_XFRM
+	u32			flow_cache_genid;
+#endif
 	union {
 		struct dst_entry	*next;
 		struct rtable __rcu	*rt_next;
@@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb)
 
 static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
 {
+#ifdef CONFIG_XFRM
+	if (dst->flow_cache_genid != atomic_read(&flow_cache_genid))
+		return NULL;
+#endif
 	if (dst->obsolete)
 		dst = dst->ops->check(dst, cookie);
 	return dst;
diff --git a/net/core/dst.c b/net/core/dst.c
index f6593d2..19899d3 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -181,6 +181,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
 	dst->path = dst;
 #ifdef CONFIG_XFRM
 	dst->xfrm = NULL;
+	dst->flow_cache_genid = atomic_read(&flow_cache_genid);
 #endif
 	dst->input = dst_discard;
 	dst->output = dst_discard;
diff --git a/net/core/sock.c b/net/core/sock.c
index d765156..1d06d4e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -491,7 +491,7 @@ struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie)
 {
 	struct dst_entry *dst = __sk_dst_get(sk);
 
-	if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) {
+	if (dst && dst_check(dst, cookie) == NULL) {
 		sk_tx_queue_clear(sk);
 		RCU_INIT_POINTER(sk->sk_dst_cache, NULL);
 		dst_release(dst);
@@ -506,7 +506,7 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie)
 {
 	struct dst_entry *dst = sk_dst_get(sk);
 
-	if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) {
+	if (dst && dst_check(dst, cookie) == NULL) {
 		sk_dst_reset(sk);
 		dst_release(dst);
 		return NULL;
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index cb7e2de..e96431a 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -130,8 +130,7 @@ struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t)
 {
 	struct dst_entry *dst = t->dst_cache;
 
-	if (dst && dst->obsolete &&
-	    dst->ops->check(dst, t->dst_cookie) == NULL) {
+	if (dst && dst_check(dst, t->dst_cookie) == NULL) {
 		t->dst_cache = NULL;
 		dst_release(dst);
 		return NULL;
-- 
1.7.12

Re: [PATCH 1/2] dst: take into account policy update on check()

[Vlad Yasevich]

On 09/07/2012 11:57 AM, Nicolas Dichtel wrote:
> When a xfrm policy is inserted or deleted, we must invalidate
> all dst and recalculate the route.
>

One suggestion may be to add an inline to get flow_cache_genid and then 
you can get rid of #ifdefs in the code

Otherwise, this extends what IPv6 is doing to all protocols.  Seem like 
the right thing to do, but I'll let DaveM and others make the final 
determination.

-vlad

> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>   include/net/dst.h     | 7 +++++++
>   net/core/dst.c        | 1 +
>   net/core/sock.c       | 4 ++--
>   net/ipv6/ip6_tunnel.c | 3 +--
>   4 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 9a78810..478e55a 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -101,6 +101,9 @@ struct dst_entry {
>   	atomic_t		__refcnt;	/* client references	*/
>   	int			__use;
>   	unsigned long		lastuse;
> +#ifdef CONFIG_XFRM
> +	u32			flow_cache_genid;
> +#endif
>   	union {
>   		struct dst_entry	*next;
>   		struct rtable __rcu	*rt_next;
> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb)
>
>   static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>   {
> +#ifdef CONFIG_XFRM
> +	if (dst->flow_cache_genid != atomic_read(&flow_cache_genid))
> +		return NULL;
> +#endif
>   	if (dst->obsolete)
>   		dst = dst->ops->check(dst, cookie);
>   	return dst;
> diff --git a/net/core/dst.c b/net/core/dst.c
> index f6593d2..19899d3 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -181,6 +181,7 @@ void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
>   	dst->path = dst;
>   #ifdef CONFIG_XFRM
>   	dst->xfrm = NULL;
> +	dst->flow_cache_genid = atomic_read(&flow_cache_genid);
>   #endif
>   	dst->input = dst_discard;
>   	dst->output = dst_discard;
> diff --git a/net/core/sock.c b/net/core/sock.c
> index d765156..1d06d4e 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -491,7 +491,7 @@ struct dst_entry *__sk_dst_check(struct sock *sk, u32 cookie)
>   {
>   	struct dst_entry *dst = __sk_dst_get(sk);
>
> -	if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) {
> +	if (dst && dst_check(dst, cookie) == NULL) {
>   		sk_tx_queue_clear(sk);
>   		RCU_INIT_POINTER(sk->sk_dst_cache, NULL);
>   		dst_release(dst);
> @@ -506,7 +506,7 @@ struct dst_entry *sk_dst_check(struct sock *sk, u32 cookie)
>   {
>   	struct dst_entry *dst = sk_dst_get(sk);
>
> -	if (dst && dst->obsolete && dst->ops->check(dst, cookie) == NULL) {
> +	if (dst && dst_check(dst, cookie) == NULL) {
>   		sk_dst_reset(sk);
>   		dst_release(dst);
>   		return NULL;
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index cb7e2de..e96431a 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -130,8 +130,7 @@ struct dst_entry *ip6_tnl_dst_check(struct ip6_tnl *t)
>   {
>   	struct dst_entry *dst = t->dst_cache;
>
> -	if (dst && dst->obsolete &&
> -	    dst->ops->check(dst, t->dst_cookie) == NULL) {
> +	if (dst && dst_check(dst, t->dst_cookie) == NULL) {
>   		t->dst_cache = NULL;
>   		dst_release(dst);
>   		return NULL;
>

Re: [PATCH 1/2] dst: take into account policy update on check()

[Eric Dumazet]

On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote:
> When a xfrm policy is inserted or deleted, we must invalidate
> all dst and recalculate the route.
> 
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  include/net/dst.h     | 7 +++++++
>  net/core/dst.c        | 1 +
>  net/core/sock.c       | 4 ++--
>  net/ipv6/ip6_tunnel.c | 3 +--
>  4 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 9a78810..478e55a 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -101,6 +101,9 @@ struct dst_entry {
>  	atomic_t		__refcnt;	/* client references	*/
>  	int			__use;
>  	unsigned long		lastuse;
> +#ifdef CONFIG_XFRM
> +	u32			flow_cache_genid;
> +#endif
>  	union {
>  		struct dst_entry	*next;
>  		struct rtable __rcu	*rt_next;
> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb)
>  
>  static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>  {
> +#ifdef CONFIG_XFRM
> +	if (dst->flow_cache_genid != atomic_read(&flow_cache_genid))
> +		return NULL;
> +#endif

Hmm... cant we reuse rt_genid ?

(When changing flow_cache_genid, change &net->ipv4.rt_genid)

Re: [PATCH 1/2] dst: take into account policy update on check()

[Nicolas Dichtel]

Le 07/09/2012 16:35, Eric Dumazet a écrit :
> On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote:
>> When a xfrm policy is inserted or deleted, we must invalidate
>> all dst and recalculate the route.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   include/net/dst.h     | 7 +++++++
>>   net/core/dst.c        | 1 +
>>   net/core/sock.c       | 4 ++--
>>   net/ipv6/ip6_tunnel.c | 3 +--
>>   4 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 9a78810..478e55a 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -101,6 +101,9 @@ struct dst_entry {
>>   	atomic_t		__refcnt;	/* client references	*/
>>   	int			__use;
>>   	unsigned long		lastuse;
>> +#ifdef CONFIG_XFRM
>> +	u32			flow_cache_genid;
>> +#endif
>>   	union {
>>   		struct dst_entry	*next;
>>   		struct rtable __rcu	*rt_next;
>> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb)
>>
>>   static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>>   {
>> +#ifdef CONFIG_XFRM
>> +	if (dst->flow_cache_genid != atomic_read(&flow_cache_genid))
>> +		return NULL;
>> +#endif
>
> Hmm... cant we reuse rt_genid ?
>
> (When changing flow_cache_genid, change &net->ipv4.rt_genid)

And so adding a new field in net->ipv6?


Regards,
Nicolas

Re: [PATCH 1/2] dst: take into account policy update on check()

[Eric Dumazet]

On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
> Le 07/09/2012 16:35, Eric Dumazet a écrit :
> >
> > Hmm... cant we reuse rt_genid ?
> >
> > (When changing flow_cache_genid, change &net->ipv4.rt_genid)
> 
> And so adding a new field in net->ipv6?

or move net->ipv4.rt_genid to net->rt_genid

Having separate field for IPv4/IPv6 is of little interest IMHO

Re: [PATCH 1/2] dst: take into account policy update on check()

[Nicolas Dichtel]

Le 07/09/2012 17:09, Eric Dumazet a écrit :
> On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
>> Le 07/09/2012 16:35, Eric Dumazet a écrit :
>>>
>>> Hmm... cant we reuse rt_genid ?
>>>
>>> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
>>
>> And so adding a new field in net->ipv6?
>
> or move net->ipv4.rt_genid to net->rt_genid
>
> Having separate field for IPv4/IPv6 is of little interest IMHO
>
Ok, I will wait feedback from other people and repost a patch after.

Re: [PATCH 1/2] dst: take into account policy update on check()

[Eric Dumazet]

On Fri, 2012-09-07 at 17:13 +0200, Nicolas Dichtel wrote:
> Le 07/09/2012 17:09, Eric Dumazet a écrit :
> > On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
> >> Le 07/09/2012 16:35, Eric Dumazet a écrit :
> >>>
> >>> Hmm... cant we reuse rt_genid ?
> >>>
> >>> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
> >>
> >> And so adding a new field in net->ipv6?
> >
> > or move net->ipv4.rt_genid to net->rt_genid
> >
> > Having separate field for IPv4/IPv6 is of little interest IMHO
> >
> Ok, I will wait feedback from other people and repost a patch after.

By the way, the get_random_bytes() calls in rt_cache_invalidate() and in
rt_genid_init() are no longer needed, since we dont use jhash anymore
(no more route cache)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index dc9549b..d6b2b1c 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -448,26 +448,12 @@ static inline bool rt_is_expired(const struct rtable *rth)
 }
 
 /*
- * Perturbation of rt_genid by a small quantity [1..256]
- * Using 8 bits of shuffling ensure we can call rt_cache_invalidate()
- * many times (2^24) without giving recent rt_genid.
- * Jenkins hash is strong enough that litle changes of rt_genid are OK.
- */
-static void rt_cache_invalidate(struct net *net)
-{
-	unsigned char shuffle;
-
-	get_random_bytes(&shuffle, sizeof(shuffle));
-	atomic_add(shuffle + 1U, &net->ipv4.rt_genid);
-}
-
-/*
  * delay < 0  : invalidate cache (fast : entries will be deleted later)
  * delay >= 0 : invalidate & flush cache (can be long)
  */
 void rt_cache_flush(struct net *net, int delay)
 {
-	rt_cache_invalidate(net);
+	atomic_inc(&net->ipv4.rt_genid);
 }
 
 static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
@@ -2533,8 +2519,7 @@ static __net_initdata struct pernet_operations sysctl_route_ops = {
 
 static __net_init int rt_genid_init(struct net *net)
 {
-	get_random_bytes(&net->ipv4.rt_genid,
-			 sizeof(net->ipv4.rt_genid));
+	atomic_set(&net->ipv4.rt_genid, 0);
 	get_random_bytes(&net->ipv4.dev_addr_genid,
 			 sizeof(net->ipv4.dev_addr_genid));
 	return 0;

Re: [PATCH 1/2] dst: take into account policy update on check()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 07 Sep 2012 17:21:52 +0200

> On Fri, 2012-09-07 at 17:13 +0200, Nicolas Dichtel wrote:
>> Le 07/09/2012 17:09, Eric Dumazet a écrit :
>> > On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
>> >> Le 07/09/2012 16:35, Eric Dumazet a écrit :
>> >>>
>> >>> Hmm... cant we reuse rt_genid ?
>> >>>
>> >>> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
>> >>
>> >> And so adding a new field in net->ipv6?
>> >
>> > or move net->ipv4.rt_genid to net->rt_genid
>> >
>> > Having separate field for IPv4/IPv6 is of little interest IMHO
>> >
>> Ok, I will wait feedback from other people and repost a patch after.
> 
> By the way, the get_random_bytes() calls in rt_cache_invalidate() and in
> rt_genid_init() are no longer needed, since we dont use jhash anymore
> (no more route cache)

I like it, please submit this formally for net-next :-)

Re: [PATCH 1/2] dst: take into account policy update on check()

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Fri, 07 Sep 2012 17:13:35 +0200

> Le 07/09/2012 17:09, Eric Dumazet a écrit :
>> On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
>>> Le 07/09/2012 16:35, Eric Dumazet a écrit :
>>>>
>>>> Hmm... cant we reuse rt_genid ?
>>>>
>>>> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
>>>
>>> And so adding a new field in net->ipv6?
>>
>> or move net->ipv4.rt_genid to net->rt_genid
>>
>> Having separate field for IPv4/IPv6 is of little interest IMHO
>>
> Ok, I will wait feedback from other people and repost a patch after.

A global net->rt_genid is definitely the way to go.

Re: [PATCH 1/2] dst: take into account policy update on check()

[Nicolas Dichtel]

Le 07/09/2012 20:48, David Miller a écrit :
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Fri, 07 Sep 2012 17:13:35 +0200
>
>> Le 07/09/2012 17:09, Eric Dumazet a écrit :
>>> On Fri, 2012-09-07 at 16:47 +0200, Nicolas Dichtel wrote:
>>>> Le 07/09/2012 16:35, Eric Dumazet a écrit :
>>>>>
>>>>> Hmm... cant we reuse rt_genid ?
>>>>>
>>>>> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
>>>>
>>>> And so adding a new field in net->ipv6?
>>>
>>> or move net->ipv4.rt_genid to net->rt_genid
>>>
>>> Having separate field for IPv4/IPv6 is of little interest IMHO
>>>
>> Ok, I will wait feedback from other people and repost a patch after.
>
> A global net->rt_genid is definitely the way to go.
>
So it means that IPv6 dst entries will be invalidated by IPv4 route management. 
For example, calling rt_cache_flush() will flush IPv6 dst too. Is this acceptable?

I will send a new version.

Re: [PATCH 1/2] dst: take into account policy update on check()

[Eric Dumazet]

On Mon, 2012-09-10 at 14:47 +0200, Nicolas Dichtel wrote:

> So it means that IPv6 dst entries will be invalidated by IPv4 route management. 
> For example, calling rt_cache_flush() will flush IPv6 dst too. Is this acceptable?

If it can save another test in fast path, this is acceptable, yes.

[PATCH net-next v2] Take care of xfrm policy when checking dst entries

[Nicolas Dichtel]

The goal of these patches is to fix the following problem: a session is
established (TCP, SCTP) and after a new policy is inserted. The current
code does not recalculate the route, thus the traffic is not encrypted.

The patch propose to check flow_cache_genid value when checking a dst
entry, which is incremented each time a policy is inserted or deleted.

v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
    in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
    too. Note that IPv6 will have one more test in fast path.

Patches are tested with TCP and SCTP, IPv4 and IPv6.

Comments are welcome.

Regards,
Nicolas

[PATCH net-next v2 1/4] netns: move net->ipv4.rt_genid to net->rt_genid

[Nicolas Dichtel]

This commit prepares the use of rt_genid by both IPv4 and IPv6.
Initialization is left in IPv4 part.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/net_namespace.h | 10 ++++++++++
 include/net/netns/ipv4.h    |  1 -
 net/ipv4/route.c            |  9 ++-------
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 5ae57f1..037190a 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -107,6 +107,7 @@ struct net {
 #endif
 	struct netns_ipvs	*ipvs;
 	struct sock		*diag_nlsk;
+	atomic_t		rt_genid;
 };
 
 /*
@@ -312,5 +313,14 @@ static inline void unregister_net_sysctl_table(struct ctl_table_header *header)
 }
 #endif
 
+static inline int rt_genid(struct net *net)
+{
+	return atomic_read(&net->rt_genid);
+}
+
+static inline void rt_genid_bump(struct net *net)
+{
+	atomic_inc(&net->rt_genid);
+}
 
 #endif /* __NET_NET_NAMESPACE_H */
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 7d00583..2ae2b83 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -64,7 +64,6 @@ struct netns_ipv4 {
 	kgid_t sysctl_ping_group_range[2];
 	long sysctl_tcp_mem[3];
 
-	atomic_t rt_genid;
 	atomic_t dev_addr_genid;
 
 #ifdef CONFIG_IP_MROUTE
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d39edf1..8ff984b 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -202,11 +202,6 @@ EXPORT_SYMBOL(ip_tos2prio);
 static DEFINE_PER_CPU(struct rt_cache_stat, rt_cache_stat);
 #define RT_CACHE_STAT_INC(field) __this_cpu_inc(rt_cache_stat.field)
 
-static inline int rt_genid(struct net *net)
-{
-	return atomic_read(&net->ipv4.rt_genid);
-}
-
 #ifdef CONFIG_PROC_FS
 static void *rt_cache_seq_start(struct seq_file *seq, loff_t *pos)
 {
@@ -449,7 +444,7 @@ static inline bool rt_is_expired(const struct rtable *rth)
 
 void rt_cache_flush(struct net *net)
 {
-	atomic_inc(&net->ipv4.rt_genid);
+	rt_genid_bump(net);
 }
 
 static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
@@ -2506,7 +2501,7 @@ static __net_initdata struct pernet_operations sysctl_route_ops = {
 
 static __net_init int rt_genid_init(struct net *net)
 {
-	atomic_set(&net->ipv4.rt_genid, 0);
+	atomic_set(&net->rt_genid, 0);
 	get_random_bytes(&net->ipv4.dev_addr_genid,
 			 sizeof(net->ipv4.dev_addr_genid));
 	return 0;
-- 
1.7.12

[PATCH net-next v2 2/4] xfrm: invalidate dst on policy insertion/deletion

[Nicolas Dichtel]

When a policy is inserted or deleted, all dst should be recalculated.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/xfrm/xfrm_policy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 741a32a..67f456d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -602,6 +602,7 @@ 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);
 	if (delpol)
 		__xfrm_policy_unlink(delpol, dir);
 	policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir);
-- 
1.7.12

Re: [PATCH net-next v2 2/4] xfrm: invalidate dst on policy insertion/deletion

[Vlad Yasevich]

On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
> When a policy is inserted or deleted, all dst should be recalculated.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>   net/xfrm/xfrm_policy.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 741a32a..67f456d 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -602,6 +602,7 @@ 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);
>   	if (delpol)
>   		__xfrm_policy_unlink(delpol, dir);
>   	policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir);
>

What about security_load_policy() and security_set_bools(). They also 
bumps the flow_cache_genid by way of selinux_xfrm_notify_policyload().

-vlad

Re: [PATCH net-next v2 2/4] xfrm: invalidate dst on policy insertion/deletion

[Nicolas Dichtel]

Le 10/09/2012 16:21, Vlad Yasevich a écrit :
> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>> When a policy is inserted or deleted, all dst should be recalculated.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   net/xfrm/xfrm_policy.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
>> index 741a32a..67f456d 100644
>> --- a/net/xfrm/xfrm_policy.c
>> +++ b/net/xfrm/xfrm_policy.c
>> @@ -602,6 +602,7 @@ 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);
>>       if (delpol)
>>           __xfrm_policy_unlink(delpol, dir);
>>       policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir);
>>
>
> What about security_load_policy() and security_set_bools(). They also bumps the
> flow_cache_genid by way of selinux_xfrm_notify_policyload().
Right. I'm not familiar with this part, but it seems you're right, rt_genid 
should be bumped too.

[PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[Nicolas Dichtel]

The goal of these patches is to fix the following problem: a session is
established (TCP, SCTP) and after a new policy is inserted. The current
code does not recalculate the route, thus the traffic is not encrypted.

The patch propose to check flow_cache_genid value when checking a dst
entry, which is incremented each time a policy is inserted or deleted.

v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
    in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
    too. Note that IPv6 will have one more test in fast path.

v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part
    bump rt_genid in selinux code (same place than flow_cache_genid)

Patches are tested with TCP and SCTP, IPv4 and IPv6.

Comments are welcome.

Regards,
Nicolas

[PATCH net-next v3 1/4] netns: move net->ipv4.rt_genid to net->rt_genid

[Nicolas Dichtel]

This commit prepares the use of rt_genid by both IPv4 and IPv6.
Initialization is left in IPv4 part.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/net_namespace.h | 10 ++++++++++
 include/net/netns/ipv4.h    |  1 -
 net/ipv4/route.c            |  9 ++-------
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 5ae57f1..037190a 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -107,6 +107,7 @@ struct net {
 #endif
 	struct netns_ipvs	*ipvs;
 	struct sock		*diag_nlsk;
+	atomic_t		rt_genid;
 };
 
 /*
@@ -312,5 +313,14 @@ static inline void unregister_net_sysctl_table(struct ctl_table_header *header)
 }
 #endif
 
+static inline int rt_genid(struct net *net)
+{
+	return atomic_read(&net->rt_genid);
+}
+
+static inline void rt_genid_bump(struct net *net)
+{
+	atomic_inc(&net->rt_genid);
+}
 
 #endif /* __NET_NET_NAMESPACE_H */
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 7d00583..2ae2b83 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -64,7 +64,6 @@ struct netns_ipv4 {
 	kgid_t sysctl_ping_group_range[2];
 	long sysctl_tcp_mem[3];
 
-	atomic_t rt_genid;
 	atomic_t dev_addr_genid;
 
 #ifdef CONFIG_IP_MROUTE
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d39edf1..8ff984b 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -202,11 +202,6 @@ EXPORT_SYMBOL(ip_tos2prio);
 static DEFINE_PER_CPU(struct rt_cache_stat, rt_cache_stat);
 #define RT_CACHE_STAT_INC(field) __this_cpu_inc(rt_cache_stat.field)
 
-static inline int rt_genid(struct net *net)
-{
-	return atomic_read(&net->ipv4.rt_genid);
-}
-
 #ifdef CONFIG_PROC_FS
 static void *rt_cache_seq_start(struct seq_file *seq, loff_t *pos)
 {
@@ -449,7 +444,7 @@ static inline bool rt_is_expired(const struct rtable *rth)
 
 void rt_cache_flush(struct net *net)
 {
-	atomic_inc(&net->ipv4.rt_genid);
+	rt_genid_bump(net);
 }
 
 static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
@@ -2506,7 +2501,7 @@ static __net_initdata struct pernet_operations sysctl_route_ops = {
 
 static __net_init int rt_genid_init(struct net *net)
 {
-	atomic_set(&net->ipv4.rt_genid, 0);
+	atomic_set(&net->rt_genid, 0);
 	get_random_bytes(&net->ipv4.dev_addr_genid,
 			 sizeof(net->ipv4.dev_addr_genid));
 	return 0;
-- 
1.7.12

[PATCH net-next v3 2/4] xfrm: invalidate dst on policy insertion/deletion

[Nicolas Dichtel]

When a policy is inserted or deleted, all dst should be recalculated.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/xfrm/xfrm_policy.c          | 1 +
 security/selinux/include/xfrm.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 741a32a..67f456d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -602,6 +602,7 @@ 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);
 	if (delpol)
 		__xfrm_policy_unlink(delpol, dir);
 	policy->index = delpol ? delpol->index : xfrm_gen_index(net, dir);
diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
index c220f31..65f67cb 100644
--- a/security/selinux/include/xfrm.h
+++ b/security/selinux/include/xfrm.h
@@ -51,6 +51,7 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall);
 static inline void selinux_xfrm_notify_policyload(void)
 {
 	atomic_inc(&flow_cache_genid);
+	rt_genid_bump(&init_net);
 }
 #else
 static inline int selinux_xfrm_enabled(void)
-- 
1.7.12

[PATCH net-next v3 3/4] ipv6: use net->rt_genid to check dst validity

[Nicolas Dichtel]

IPv6 dst should take care of rt_genid too. When a xfrm policy is inserted or
deleted, all dst should be invalidated.
To force the validation, dst entries should be created with ->obsolete set to
DST_OBSOLETE_FORCE_CHK. This was already the case for all functions calling
ip6_dst_alloc(), except for ip6_rt_copy().

As a consequence, we can remove the specific code in inet6_connection_sock.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/ip6_fib.h            |  5 ++---
 net/ipv6/inet6_connection_sock.c | 23 +----------------------
 net/ipv6/route.c                 | 13 +++++++++----
 3 files changed, 12 insertions(+), 29 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index cd64cf3..8a2a203 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -112,9 +112,8 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-#ifdef CONFIG_XFRM
-	u32				rt6i_flow_cache_genid;
-#endif
+	u32				rt6i_genid;
+
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 0251a60..c4f9341 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct dst_entry *dst,
 			   const struct in6_addr *saddr)
 {
 	__ip6_dst_store(sk, dst, daddr, saddr);
-
-#ifdef CONFIG_XFRM
-	{
-		struct rt6_info *rt = (struct rt6_info  *)dst;
-		rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
-	}
-#endif
 }
 
 static inline
 struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
 {
-	struct dst_entry *dst;
-
-	dst = __sk_dst_check(sk, cookie);
-
-#ifdef CONFIG_XFRM
-	if (dst) {
-		struct rt6_info *rt = (struct rt6_info *)dst;
-		if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) {
-			__sk_dst_reset(sk);
-			dst = NULL;
-		}
-	}
-#endif
-
-	return dst;
+	return __sk_dst_check(sk, cookie);
 }
 
 static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 339d921..561f249 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -281,13 +281,14 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 					     struct fib6_table *table)
 {
 	struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
-					0, DST_OBSOLETE_NONE, flags);
+					0, DST_OBSOLETE_FORCE_CHK, flags);
 
 	if (rt) {
 		struct dst_entry *dst = &rt->dst;
 
 		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);
 	}
 	return rt;
 }
@@ -1031,6 +1032,13 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 
 	rt = (struct rt6_info *) dst;
 
+	/* All IPV6 dsts are created with ->obsolete set to the value
+	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
+	 * into this function always.
+	 */
+	if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
+		return NULL;
+
 	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) {
 		if (rt->rt6i_peer_genid != rt6_peer_genid()) {
 			if (!rt6_has_peer(rt))
@@ -1397,8 +1405,6 @@ int ip6_route_add(struct fib6_config *cfg)
 		goto out;
 	}
 
-	rt->dst.obsolete = -1;
-
 	if (cfg->fc_flags & RTF_EXPIRES)
 		rt6_set_expires(rt, jiffies +
 				clock_t_to_jiffies(cfg->fc_expires));
@@ -2093,7 +2099,6 @@ struct rt6_info *addrconf_dst_alloc(struct inet6_dev *idev,
 	rt->dst.input = ip6_input;
 	rt->dst.output = ip6_output;
 	rt->rt6i_idev = idev;
-	rt->dst.obsolete = -1;
 
 	rt->rt6i_flags = RTF_UP | RTF_NONEXTHOP;
 	if (anycast)
-- 
1.7.12

[PATCH net-next v3 4/4] ipv6: use DST_* macro to set obselete field

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/route.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 561f249..0c6f132 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -226,7 +226,7 @@ static struct rt6_info ip6_null_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -ENETUNREACH,
 		.input		= ip6_pkt_discard,
 		.output		= ip6_pkt_discard_out,
@@ -246,7 +246,7 @@ static struct rt6_info ip6_prohibit_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -EACCES,
 		.input		= ip6_pkt_prohibit,
 		.output		= ip6_pkt_prohibit_out,
@@ -261,7 +261,7 @@ static struct rt6_info ip6_blk_hole_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -EINVAL,
 		.input		= dst_discard,
 		.output		= dst_discard,
-- 
1.7.12

Re: [PATCH net-next v3 4/4] ipv6: use DST_* macro to set obselete field

[Eric Dumazet]

On Tue, 2012-09-11 at 10:09 +0200, Nicolas Dichtel wrote:
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>  net/ipv6/route.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 561f249..0c6f132 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -226,7 +226,7 @@ static struct rt6_info ip6_null_entry_template = {
>  	.dst = {
>  		.__refcnt	= ATOMIC_INIT(1),
>  		.__use		= 1,
> -		.obsolete	= -1,
> +		.obsolete	= DST_OBSOLETE_FORCE_CHK,
>  		.error		= -ENETUNREACH,
>  		.input		= ip6_pkt_discard,
>  		.output		= ip6_pkt_discard_out,
> @@ -246,7 +246,7 @@ static struct rt6_info ip6_prohibit_entry_template = {
>  	.dst = {
>  		.__refcnt	= ATOMIC_INIT(1),
>  		.__use		= 1,
> -		.obsolete	= -1,
> +		.obsolete	= DST_OBSOLETE_FORCE_CHK,
>  		.error		= -EACCES,
>  		.input		= ip6_pkt_prohibit,
>  		.output		= ip6_pkt_prohibit_out,
> @@ -261,7 +261,7 @@ static struct rt6_info ip6_blk_hole_entry_template = {
>  	.dst = {
>  		.__refcnt	= ATOMIC_INIT(1),
>  		.__use		= 1,
> -		.obsolete	= -1,
> +		.obsolete	= DST_OBSOLETE_FORCE_CHK,
>  		.error		= -EINVAL,
>  		.input		= dst_discard,
>  		.output		= dst_discard,

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Tue, 11 Sep 2012 10:09:43 +0200

> The goal of these patches is to fix the following problem: a session is
> established (TCP, SCTP) and after a new policy is inserted. The current
> code does not recalculate the route, thus the traffic is not encrypted.
> 
> The patch propose to check flow_cache_genid value when checking a dst
> entry, which is incremented each time a policy is inserted or deleted.
> 
> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
>     in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
>     too. Note that IPv6 will have one more test in fast path.
> 
> v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part
>     bump rt_genid in selinux code (same place than flow_cache_genid)
> 
> Patches are tested with TCP and SCTP, IPv4 and IPv6.

These patches don't apply cleanly at all.

In the net/ipv4/route.c code we don't initialize the genid to zero,
we stick a random value there.

And we don't increment it by one on flushes, instead we increment
it by a random amount.

I wonder what tree these were even against, the differences were
so great.

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[Vlad Yasevich]

On 09/17/2012 12:49 PM, David Miller wrote:
> From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Date: Tue, 11 Sep 2012 10:09:43 +0200
>
>> The goal of these patches is to fix the following problem: a session is
>> established (TCP, SCTP) and after a new policy is inserted. The current
>> code does not recalculate the route, thus the traffic is not encrypted.
>>
>> The patch propose to check flow_cache_genid value when checking a dst
>> entry, which is incremented each time a policy is inserted or deleted.
>>
>> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
>>      in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
>>      too. Note that IPv6 will have one more test in fast path.
>>
>> v3: remove unrelated "#ifdef CONFIG_XFRM" in IPv6 part
>>      bump rt_genid in selinux code (same place than flow_cache_genid)
>>
>> Patches are tested with TCP and SCTP, IPv4 and IPv6.
>
> These patches don't apply cleanly at all.
>
> In the net/ipv4/route.c code we don't initialize the genid to zero,
> we stick a random value there.
>
> And we don't increment it by one on flushes, instead we increment
> it by a random amount.
>
> I wonder what tree these were even against, the differences were
> so great.
>

I think he expected you to take Eric's patch that removed those pieces.

-vlad

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Mon, 17 Sep 2012 14:14:35 -0400

> I think he expected you to take Eric's patch that removed those
> pieces.

Eric's patch was a cleanup, so it went into 'net-next'.

Nicolas's patch is a bonafide bug fix so needs to be
targetted at 'net'

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[Nicolas Dichtel]

Le 17/09/2012 20:25, David Miller a écrit :
> From: Vlad Yasevich <vyasevich@gmail.com>
> Date: Mon, 17 Sep 2012 14:14:35 -0400
>
>> I think he expected you to take Eric's patch that removed those
>> pieces.
>
> Eric's patch was a cleanup, so it went into 'net-next'.
>
> Nicolas's patch is a bonafide bug fix so needs to be
> targetted at 'net'
My fault, it was 'net-next'. I will rebase them against 'net'.

Should I post the last one (4/4) separately, against net-next? Because without 
the '3/4', the cleanup is a bit larger and will cause a conflict during the merge.

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[David Miller]

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 17 Sep 2012 21:52:20 +0200

> Le 17/09/2012 20:25, David Miller a écrit :
>> From: Vlad Yasevich <vyasevich@gmail.com>
>> Date: Mon, 17 Sep 2012 14:14:35 -0400
>>
>>> I think he expected you to take Eric's patch that removed those
>>> pieces.
>>
>> Eric's patch was a cleanup, so it went into 'net-next'.
>>
>> Nicolas's patch is a bonafide bug fix so needs to be
>> targetted at 'net'
> My fault, it was 'net-next'. I will rebase them against 'net'.
> 
> Should I post the last one (4/4) separately, against net-next? Because
> without the '3/4', the cleanup is a bit larger and will cause a
> conflict during the merge.

Actually Nicolas, hold off on this for a bit.

I'm reconsidering putting Eric's patch into 'net' and that would
allow me to use your v3 patches as-is.

Thanks.

Re: [PATCH net-next v3 0/4] Take care of xfrm policy when checking dst entries

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 17 Sep 2012 15:54:58 -0400 (EDT)

> I'm reconsidering putting Eric's patch into 'net' and that would
> allow me to use your v3 patches as-is.

And that's what I've done just now, thanks.

[PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Nicolas Dichtel]

IPv6 dst should take care of rt_genid too. When a xfrm policy is inserted or
deleted, all dst should be invalidated.
To force the validation, dst entries should be created with ->obsolete set to
DST_OBSOLETE_FORCE_CHK. This was already the case for all functions calling
ip6_dst_alloc(), except for ip6_rt_copy().

As a consequence, we can remove the specific code in inet6_connection_sock.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/ip6_fib.h            |  2 +-
 net/ipv6/inet6_connection_sock.c | 23 +----------------------
 net/ipv6/route.c                 | 17 +++++++++++++----
 3 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index cd64cf3..5eb93f4 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -113,7 +113,7 @@ struct rt6_info {
 	unsigned long			_rt6i_peer;
 
 #ifdef CONFIG_XFRM
-	u32				rt6i_flow_cache_genid;
+	u32				rt6i_genid;
 #endif
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 0251a60..c4f9341 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct dst_entry *dst,
 			   const struct in6_addr *saddr)
 {
 	__ip6_dst_store(sk, dst, daddr, saddr);
-
-#ifdef CONFIG_XFRM
-	{
-		struct rt6_info *rt = (struct rt6_info  *)dst;
-		rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
-	}
-#endif
 }
 
 static inline
 struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
 {
-	struct dst_entry *dst;
-
-	dst = __sk_dst_check(sk, cookie);
-
-#ifdef CONFIG_XFRM
-	if (dst) {
-		struct rt6_info *rt = (struct rt6_info *)dst;
-		if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) {
-			__sk_dst_reset(sk);
-			dst = NULL;
-		}
-	}
-#endif
-
-	return dst;
+	return __sk_dst_check(sk, cookie);
 }
 
 static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 339d921..db7b78f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -281,13 +281,16 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
 					     struct fib6_table *table)
 {
 	struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
-					0, DST_OBSOLETE_NONE, flags);
+					0, DST_OBSOLETE_FORCE_CHK, flags);
 
 	if (rt) {
 		struct dst_entry *dst = &rt->dst;
 
 		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
 	}
 	return rt;
 }
@@ -1031,6 +1034,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
 
 	rt = (struct rt6_info *) dst;
 
+	/* All IPV6 dsts are created with ->obsolete set to the value
+	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
+	 * into this function always.
+	 */
+#ifdef CONFIG_XFRM
+	if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
+		return NULL;
+#endif
+
 	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) {
 		if (rt->rt6i_peer_genid != rt6_peer_genid()) {
 			if (!rt6_has_peer(rt))
@@ -1397,8 +1409,6 @@ int ip6_route_add(struct fib6_config *cfg)
 		goto out;
 	}
 
-	rt->dst.obsolete = -1;
-
 	if (cfg->fc_flags & RTF_EXPIRES)
 		rt6_set_expires(rt, jiffies +
 				clock_t_to_jiffies(cfg->fc_expires));
@@ -2093,7 +2103,6 @@ struct rt6_info *addrconf_dst_alloc(struct inet6_dev *idev,
 	rt->dst.input = ip6_input;
 	rt->dst.output = ip6_output;
 	rt->rt6i_idev = idev;
-	rt->dst.obsolete = -1;
 
 	rt->rt6i_flags = RTF_UP | RTF_NONEXTHOP;
 	if (anycast)
-- 
1.7.12

Re: [PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Vlad Yasevich]

On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
> IPv6 dst should take care of rt_genid too. When a xfrm policy is inserted or
> deleted, all dst should be invalidated.
> To force the validation, dst entries should be created with ->obsolete set to
> DST_OBSOLETE_FORCE_CHK. This was already the case for all functions calling
> ip6_dst_alloc(), except for ip6_rt_copy().
>
> As a consequence, we can remove the specific code in inet6_connection_sock.
>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> ---
>   include/net/ip6_fib.h            |  2 +-
>   net/ipv6/inet6_connection_sock.c | 23 +----------------------
>   net/ipv6/route.c                 | 17 +++++++++++++----
>   3 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index cd64cf3..5eb93f4 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -113,7 +113,7 @@ struct rt6_info {
>   	unsigned long			_rt6i_peer;
>
>   #ifdef CONFIG_XFRM
> -	u32				rt6i_flow_cache_genid;
> +	u32				rt6i_genid;
>   #endif
>   	/* more non-fragment space at head required */
>   	unsigned short			rt6i_nfheader_len;
> diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
> index 0251a60..c4f9341 100644
> --- a/net/ipv6/inet6_connection_sock.c
> +++ b/net/ipv6/inet6_connection_sock.c
> @@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct dst_entry *dst,
>   			   const struct in6_addr *saddr)
>   {
>   	__ip6_dst_store(sk, dst, daddr, saddr);
> -
> -#ifdef CONFIG_XFRM
> -	{
> -		struct rt6_info *rt = (struct rt6_info  *)dst;
> -		rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
> -	}
> -#endif
>   }
>
>   static inline
>   struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
>   {
> -	struct dst_entry *dst;
> -
> -	dst = __sk_dst_check(sk, cookie);
> -
> -#ifdef CONFIG_XFRM
> -	if (dst) {
> -		struct rt6_info *rt = (struct rt6_info *)dst;
> -		if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) {
> -			__sk_dst_reset(sk);
> -			dst = NULL;
> -		}
> -	}
> -#endif
> -
> -	return dst;
> +	return __sk_dst_check(sk, cookie);
>   }
>
>   static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 339d921..db7b78f 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -281,13 +281,16 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net,
>   					     struct fib6_table *table)
>   {
>   	struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
> -					0, DST_OBSOLETE_NONE, flags);
> +					0, DST_OBSOLETE_FORCE_CHK, flags);
>
>   	if (rt) {
>   		struct dst_entry *dst = &rt->dst;
>
>   		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

This isn't XFRM dependent any more, is it?

-vlad

>   	}
>   	return rt;
>   }
> @@ -1031,6 +1034,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
>
>   	rt = (struct rt6_info *) dst;
>
> +	/* All IPV6 dsts are created with ->obsolete set to the value
> +	 * DST_OBSOLETE_FORCE_CHK which forces validation calls down
> +	 * into this function always.
> +	 */
> +#ifdef CONFIG_XFRM
> +	if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev)))
> +		return NULL;
> +#endif
> +
>   	if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) {
>   		if (rt->rt6i_peer_genid != rt6_peer_genid()) {
>   			if (!rt6_has_peer(rt))
> @@ -1397,8 +1409,6 @@ int ip6_route_add(struct fib6_config *cfg)
>   		goto out;
>   	}
>
> -	rt->dst.obsolete = -1;
> -
>   	if (cfg->fc_flags & RTF_EXPIRES)
>   		rt6_set_expires(rt, jiffies +
>   				clock_t_to_jiffies(cfg->fc_expires));
> @@ -2093,7 +2103,6 @@ struct rt6_info *addrconf_dst_alloc(struct inet6_dev *idev,
>   	rt->dst.input = ip6_input;
>   	rt->dst.output = ip6_output;
>   	rt->rt6i_idev = idev;
> -	rt->dst.obsolete = -1;
>
>   	rt->rt6i_flags = RTF_UP | RTF_NONEXTHOP;
>   	if (anycast)
>

Re: [PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Nicolas Dichtel]

Le 10/09/2012 16:29, Vlad Yasevich a écrit :
> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>> IPv6 dst should take care of rt_genid too. When a xfrm policy is inserted or
>> deleted, all dst should be invalidated.
>> To force the validation, dst entries should be created with ->obsolete set to
>> DST_OBSOLETE_FORCE_CHK. This was already the case for all functions calling
>> ip6_dst_alloc(), except for ip6_rt_copy().
>>
>> As a consequence, we can remove the specific code in inet6_connection_sock.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   include/net/ip6_fib.h            |  2 +-
>>   net/ipv6/inet6_connection_sock.c | 23 +----------------------
>>   net/ipv6/route.c                 | 17 +++++++++++++----
>>   3 files changed, 15 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>> index cd64cf3..5eb93f4 100644
>> --- a/include/net/ip6_fib.h
>> +++ b/include/net/ip6_fib.h
>> @@ -113,7 +113,7 @@ struct rt6_info {
>>       unsigned long            _rt6i_peer;
>>
>>   #ifdef CONFIG_XFRM
>> -    u32                rt6i_flow_cache_genid;
>> +    u32                rt6i_genid;
>>   #endif
>>       /* more non-fragment space at head required */
>>       unsigned short            rt6i_nfheader_len;
>> diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
>> index 0251a60..c4f9341 100644
>> --- a/net/ipv6/inet6_connection_sock.c
>> +++ b/net/ipv6/inet6_connection_sock.c
>> @@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct
>> dst_entry *dst,
>>                  const struct in6_addr *saddr)
>>   {
>>       __ip6_dst_store(sk, dst, daddr, saddr);
>> -
>> -#ifdef CONFIG_XFRM
>> -    {
>> -        struct rt6_info *rt = (struct rt6_info  *)dst;
>> -        rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
>> -    }
>> -#endif
>>   }
>>
>>   static inline
>>   struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
>>   {
>> -    struct dst_entry *dst;
>> -
>> -    dst = __sk_dst_check(sk, cookie);
>> -
>> -#ifdef CONFIG_XFRM
>> -    if (dst) {
>> -        struct rt6_info *rt = (struct rt6_info *)dst;
>> -        if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) {
>> -            __sk_dst_reset(sk);
>> -            dst = NULL;
>> -        }
>> -    }
>> -#endif
>> -
>> -    return dst;
>> +    return __sk_dst_check(sk, cookie);
>>   }
>>
>>   static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index 339d921..db7b78f 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -281,13 +281,16 @@ static inline struct rt6_info *ip6_dst_alloc(struct net
>> *net,
>>                            struct fib6_table *table)
>>   {
>>       struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
>> -                    0, DST_OBSOLETE_NONE, flags);
>> +                    0, DST_OBSOLETE_FORCE_CHK, flags);
>>
>>       if (rt) {
>>           struct dst_entry *dst = &rt->dst;
>>
>>           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
>
> This isn't XFRM dependent any more, is it?
Not dependent, but for IPv6, it's only usefull when xfrm is set. Goal of this 
ifdef was to avoid the test if xfrm is not used.

Re: [PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Vlad Yasevich]

On 09/10/2012 10:34 AM, Nicolas Dichtel wrote:
> Le 10/09/2012 16:29, Vlad Yasevich a écrit :
>> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>>> IPv6 dst should take care of rt_genid too. When a xfrm policy is
>>> inserted or
>>> deleted, all dst should be invalidated.
>>> To force the validation, dst entries should be created with
>>> ->obsolete set to
>>> DST_OBSOLETE_FORCE_CHK. This was already the case for all functions
>>> calling
>>> ip6_dst_alloc(), except for ip6_rt_copy().
>>>
>>> As a consequence, we can remove the specific code in
>>> inet6_connection_sock.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>> ---
>>>   include/net/ip6_fib.h            |  2 +-
>>>   net/ipv6/inet6_connection_sock.c | 23 +----------------------
>>>   net/ipv6/route.c                 | 17 +++++++++++++----
>>>   3 files changed, 15 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>>> index cd64cf3..5eb93f4 100644
>>> --- a/include/net/ip6_fib.h
>>> +++ b/include/net/ip6_fib.h
>>> @@ -113,7 +113,7 @@ struct rt6_info {
>>>       unsigned long            _rt6i_peer;
>>>
>>>   #ifdef CONFIG_XFRM
>>> -    u32                rt6i_flow_cache_genid;
>>> +    u32                rt6i_genid;
>>>   #endif
>>>       /* more non-fragment space at head required */
>>>       unsigned short            rt6i_nfheader_len;
>>> diff --git a/net/ipv6/inet6_connection_sock.c
>>> b/net/ipv6/inet6_connection_sock.c
>>> index 0251a60..c4f9341 100644
>>> --- a/net/ipv6/inet6_connection_sock.c
>>> +++ b/net/ipv6/inet6_connection_sock.c
>>> @@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct
>>> dst_entry *dst,
>>>                  const struct in6_addr *saddr)
>>>   {
>>>       __ip6_dst_store(sk, dst, daddr, saddr);
>>> -
>>> -#ifdef CONFIG_XFRM
>>> -    {
>>> -        struct rt6_info *rt = (struct rt6_info  *)dst;
>>> -        rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
>>> -    }
>>> -#endif
>>>   }
>>>
>>>   static inline
>>>   struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
>>>   {
>>> -    struct dst_entry *dst;
>>> -
>>> -    dst = __sk_dst_check(sk, cookie);
>>> -
>>> -#ifdef CONFIG_XFRM
>>> -    if (dst) {
>>> -        struct rt6_info *rt = (struct rt6_info *)dst;
>>> -        if (rt->rt6i_flow_cache_genid !=
>>> atomic_read(&flow_cache_genid)) {
>>> -            __sk_dst_reset(sk);
>>> -            dst = NULL;
>>> -        }
>>> -    }
>>> -#endif
>>> -
>>> -    return dst;
>>> +    return __sk_dst_check(sk, cookie);
>>>   }
>>>
>>>   static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 339d921..db7b78f 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -281,13 +281,16 @@ static inline struct rt6_info
>>> *ip6_dst_alloc(struct net
>>> *net,
>>>                            struct fib6_table *table)
>>>   {
>>>       struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
>>> -                    0, DST_OBSOLETE_NONE, flags);
>>> +                    0, DST_OBSOLETE_FORCE_CHK, flags);
>>>
>>>       if (rt) {
>>>           struct dst_entry *dst = &rt->dst;
>>>
>>>           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
>>
>> This isn't XFRM dependent any more, is it?
> Not dependent, but for IPv6, it's only usefull when xfrm is set. Goal of
> this ifdef was to avoid the test if xfrm is not used.

It's not the usage,  it's enable at build time and that's almost always 
on.  Now the cache behavior is different when XFRM is excluded from the 
kernel build.

Before the ifdef was needed since you were actually looking at xfrm 
variable.  Not anymore.   The ifdef doesn't make sense.

-vlad

Re: [PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Nicolas Dichtel]

Le 10/09/2012 16:43, Vlad Yasevich a écrit :
> On 09/10/2012 10:34 AM, Nicolas Dichtel wrote:
>> Le 10/09/2012 16:29, Vlad Yasevich a écrit :
>>> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>>>> IPv6 dst should take care of rt_genid too. When a xfrm policy is
>>>> inserted or
>>>> deleted, all dst should be invalidated.
>>>> To force the validation, dst entries should be created with
>>>> ->obsolete set to
>>>> DST_OBSOLETE_FORCE_CHK. This was already the case for all functions
>>>> calling
>>>> ip6_dst_alloc(), except for ip6_rt_copy().
>>>>
>>>> As a consequence, we can remove the specific code in
>>>> inet6_connection_sock.
>>>>
>>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>>> ---
>>>>   include/net/ip6_fib.h            |  2 +-
>>>>   net/ipv6/inet6_connection_sock.c | 23 +----------------------
>>>>   net/ipv6/route.c                 | 17 +++++++++++++----
>>>>   3 files changed, 15 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>>>> index cd64cf3..5eb93f4 100644
>>>> --- a/include/net/ip6_fib.h
>>>> +++ b/include/net/ip6_fib.h
>>>> @@ -113,7 +113,7 @@ struct rt6_info {
>>>>       unsigned long            _rt6i_peer;
>>>>
>>>>   #ifdef CONFIG_XFRM
>>>> -    u32                rt6i_flow_cache_genid;
>>>> +    u32                rt6i_genid;
>>>>   #endif
>>>>       /* more non-fragment space at head required */
>>>>       unsigned short            rt6i_nfheader_len;
>>>> diff --git a/net/ipv6/inet6_connection_sock.c
>>>> b/net/ipv6/inet6_connection_sock.c
>>>> index 0251a60..c4f9341 100644
>>>> --- a/net/ipv6/inet6_connection_sock.c
>>>> +++ b/net/ipv6/inet6_connection_sock.c
>>>> @@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct
>>>> dst_entry *dst,
>>>>                  const struct in6_addr *saddr)
>>>>   {
>>>>       __ip6_dst_store(sk, dst, daddr, saddr);
>>>> -
>>>> -#ifdef CONFIG_XFRM
>>>> -    {
>>>> -        struct rt6_info *rt = (struct rt6_info  *)dst;
>>>> -        rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
>>>> -    }
>>>> -#endif
>>>>   }
>>>>
>>>>   static inline
>>>>   struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
>>>>   {
>>>> -    struct dst_entry *dst;
>>>> -
>>>> -    dst = __sk_dst_check(sk, cookie);
>>>> -
>>>> -#ifdef CONFIG_XFRM
>>>> -    if (dst) {
>>>> -        struct rt6_info *rt = (struct rt6_info *)dst;
>>>> -        if (rt->rt6i_flow_cache_genid !=
>>>> atomic_read(&flow_cache_genid)) {
>>>> -            __sk_dst_reset(sk);
>>>> -            dst = NULL;
>>>> -        }
>>>> -    }
>>>> -#endif
>>>> -
>>>> -    return dst;
>>>> +    return __sk_dst_check(sk, cookie);
>>>>   }
>>>>
>>>>   static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
>>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>>> index 339d921..db7b78f 100644
>>>> --- a/net/ipv6/route.c
>>>> +++ b/net/ipv6/route.c
>>>> @@ -281,13 +281,16 @@ static inline struct rt6_info
>>>> *ip6_dst_alloc(struct net
>>>> *net,
>>>>                            struct fib6_table *table)
>>>>   {
>>>>       struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
>>>> -                    0, DST_OBSOLETE_NONE, flags);
>>>> +                    0, DST_OBSOLETE_FORCE_CHK, flags);
>>>>
>>>>       if (rt) {
>>>>           struct dst_entry *dst = &rt->dst;
>>>>
>>>>           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
>>>
>>> This isn't XFRM dependent any more, is it?
>> Not dependent, but for IPv6, it's only usefull when xfrm is set. Goal of
>> this ifdef was to avoid the test if xfrm is not used.
>
> It's not the usage,  it's enable at build time and that's almost always on.  Now
> the cache behavior is different when XFRM is excluded from the kernel build.
>
> Before the ifdef was needed since you were actually looking at xfrm variable.
> Not anymore.   The ifdef doesn't make sense.
Ok, I will remove it.

[PATCH net-next v2 4/4] ipv6: use DST_* macro to set obselete field

[Nicolas Dichtel]

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 net/ipv6/route.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index db7b78f..99b41cc 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -226,7 +226,7 @@ static struct rt6_info ip6_null_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -ENETUNREACH,
 		.input		= ip6_pkt_discard,
 		.output		= ip6_pkt_discard_out,
@@ -246,7 +246,7 @@ static struct rt6_info ip6_prohibit_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -EACCES,
 		.input		= ip6_pkt_prohibit,
 		.output		= ip6_pkt_prohibit_out,
@@ -261,7 +261,7 @@ static struct rt6_info ip6_blk_hole_entry_template = {
 	.dst = {
 		.__refcnt	= ATOMIC_INIT(1),
 		.__use		= 1,
-		.obsolete	= -1,
+		.obsolete	= DST_OBSOLETE_FORCE_CHK,
 		.error		= -EINVAL,
 		.input		= dst_discard,
 		.output		= dst_discard,
-- 
1.7.12

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[Vlad Yasevich]

On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
> The goal of these patches is to fix the following problem: a session is
> established (TCP, SCTP) and after a new policy is inserted. The current
> code does not recalculate the route, thus the traffic is not encrypted.
>
> The patch propose to check flow_cache_genid value when checking a dst
> entry, which is incremented each time a policy is inserted or deleted.
>
> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
>      in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
>      too. Note that IPv6 will have one more test in fast path.
>
> Patches are tested with TCP and SCTP, IPv4 and IPv6.
>
> Comments are welcome.
>
> Regards,
> Nicolas
>

I am not sure this is right...  This has a side-effect that when an 
rt_cache_flush() is called, it invalidates IPv6 routes a well....

Its all fine and good do this when a new policy is added, but not when 
IPv4 routing table changes.

-vlad

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[Nicolas Dichtel]

Le 10/09/2012 16:35, Vlad Yasevich a écrit :
> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>> The goal of these patches is to fix the following problem: a session is
>> established (TCP, SCTP) and after a new policy is inserted. The current
>> code does not recalculate the route, thus the traffic is not encrypted.
>>
>> The patch propose to check flow_cache_genid value when checking a dst
>> entry, which is incremented each time a policy is inserted or deleted.
>>
>> v2: use net->ipv4.rt_genid instead of flow_cache_genid (and thus save a test
>>      in fast path). Also move it to net->rt_genid, to be able to use it for IPv6
>>      too. Note that IPv6 will have one more test in fast path.
>>
>> Patches are tested with TCP and SCTP, IPv4 and IPv6.
>>
>> Comments are welcome.
>>
>> Regards,
>> Nicolas
>>
>
> I am not sure this is right...  This has a side-effect that when an
> rt_cache_flush() is called, it invalidates IPv6 routes a well....
>
> Its all fine and good do this when a new policy is added, but not when IPv4
> routing table changes.
I already ask for this side effect, Eric answers me:
http://marc.info/?l=linux-netdev&m=134728265000776&w=2

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Mon, 10 Sep 2012 10:35:03 -0400

> I am not sure this is right...  This has a side-effect that when an
> rt_cache_flush() is called, it invalidates IPv6 routes a well....
> 
> Its all fine and good do this when a new policy is added, but not when
> IPv4 routing table changes.

I disagree.

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[Vlad Yasevich]

On 09/10/2012 01:18 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevich@gmail.com>
> Date: Mon, 10 Sep 2012 10:35:03 -0400
>
>> I am not sure this is right...  This has a side-effect that when an
>> rt_cache_flush() is called, it invalidates IPv6 routes a well....
>>
>> Its all fine and good do this when a new policy is added, but not when
>> IPv4 routing table changes.
>
> I disagree.
>

So you are perfectly ok with invalidating IPv6 cache when IPv4 table 
changes, but not invalidating IPv4 cache if IPv6 table changes?

-vlad

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Mon, 10 Sep 2012 13:59:22 -0400

> On 09/10/2012 01:18 PM, David Miller wrote:
>> From: Vlad Yasevich <vyasevich@gmail.com>
>> Date: Mon, 10 Sep 2012 10:35:03 -0400
>>
>>> I am not sure this is right...  This has a side-effect that when an
>>> rt_cache_flush() is called, it invalidates IPv6 routes a well....
>>>
>>> Its all fine and good do this when a new policy is added, but not when
>>> IPv4 routing table changes.
>>
>> I disagree.
>>
> 
> So you are perfectly ok with invalidating IPv6 cache when IPv4 table
> changes, but not invalidating IPv4 cache if IPv6 table changes?

Due to tunneling I can't see how this is avoidable?

We do ipv6 over ipv4, but not vice-versa.

Re: [PATCH net-next v2] Take care of xfrm policy when checking dst entries

[Jan Engelhardt]

On Monday 2012-09-10 20:01, David Miller wrote:
>> 
>> So you are perfectly ok with invalidating IPv6 cache when IPv4 table
>> changes, but not invalidating IPv4 cache if IPv6 table changes?
>
>Due to tunneling I can't see how this is avoidable?
>
>We do ipv6 over ipv4, but not vice-versa.

I have a setup here where 6 machines are connected with one another 
(most of them) to form 9 IPsec sessions, all of which are ESP6 links - 
since native IPv6 is provided - that also handle the site-to-site IPv4 
traffic. Does that count?

Re: [PATCH 1/2] dst: take into account policy update on check()

[Vlad Yasevich]

On 09/07/2012 10:35 AM, Eric Dumazet wrote:
> On Fri, 2012-09-07 at 17:57 +0200, Nicolas Dichtel wrote:
>> When a xfrm policy is inserted or deleted, we must invalidate
>> all dst and recalculate the route.
>>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>> ---
>>   include/net/dst.h     | 7 +++++++
>>   net/core/dst.c        | 1 +
>>   net/core/sock.c       | 4 ++--
>>   net/ipv6/ip6_tunnel.c | 3 +--
>>   4 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 9a78810..478e55a 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -101,6 +101,9 @@ struct dst_entry {
>>   	atomic_t		__refcnt;	/* client references	*/
>>   	int			__use;
>>   	unsigned long		lastuse;
>> +#ifdef CONFIG_XFRM
>> +	u32			flow_cache_genid;
>> +#endif
>>   	union {
>>   		struct dst_entry	*next;
>>   		struct rtable __rcu	*rt_next;
>> @@ -457,6 +460,10 @@ static inline int dst_input(struct sk_buff *skb)
>>
>>   static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
>>   {
>> +#ifdef CONFIG_XFRM
>> +	if (dst->flow_cache_genid != atomic_read(&flow_cache_genid))
>> +		return NULL;
>> +#endif
>
> Hmm... cant we reuse rt_genid ?
>
> (When changing flow_cache_genid, change &net->ipv4.rt_genid)
>

I thought of that too, but that requires hacking xfrm to know the 
namespace at the time it changes the flow_cache_genid.  This one seems 
simpler to do.

-vlad

Re: [PATCH 1/2] dst: take into account policy update on check()

[Eric Dumazet]

On Fri, 2012-09-07 at 10:51 -0400, Vlad Yasevich wrote:

> I thought of that too, but that requires hacking xfrm to know the 
> namespace at the time it changes the flow_cache_genid.  This one seems 
> simpler to do.

Simpler maybe, but its yet another test done in fast path.

[PATCH 2/2] ipv6: remove rt6i_flow_cache_genid field in rt6_info

[Nicolas Dichtel]

Now that flow_cache_genid is stored in dst, there is no need to add
specific code here.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/net/ip6_fib.h            |  3 ---
 net/ipv6/inet6_connection_sock.c | 23 +----------------------
 2 files changed, 1 insertion(+), 25 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index cd64cf3..af11e95 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -112,9 +112,6 @@ struct rt6_info {
 	struct inet6_dev		*rt6i_idev;
 	unsigned long			_rt6i_peer;
 
-#ifdef CONFIG_XFRM
-	u32				rt6i_flow_cache_genid;
-#endif
 	/* more non-fragment space at head required */
 	unsigned short			rt6i_nfheader_len;
 
diff --git a/net/ipv6/inet6_connection_sock.c b/net/ipv6/inet6_connection_sock.c
index 0251a60..c4f9341 100644
--- a/net/ipv6/inet6_connection_sock.c
+++ b/net/ipv6/inet6_connection_sock.c
@@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct dst_entry *dst,
 			   const struct in6_addr *saddr)
 {
 	__ip6_dst_store(sk, dst, daddr, saddr);
-
-#ifdef CONFIG_XFRM
-	{
-		struct rt6_info *rt = (struct rt6_info  *)dst;
-		rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
-	}
-#endif
 }
 
 static inline
 struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
 {
-	struct dst_entry *dst;
-
-	dst = __sk_dst_check(sk, cookie);
-
-#ifdef CONFIG_XFRM
-	if (dst) {
-		struct rt6_info *rt = (struct rt6_info *)dst;
-		if (rt->rt6i_flow_cache_genid != atomic_read(&flow_cache_genid)) {
-			__sk_dst_reset(sk);
-			dst = NULL;
-		}
-	}
-#endif
-
-	return dst;
+	return __sk_dst_check(sk, cookie);
 }
 
 static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
-- 
1.7.12