All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
@ 2013-07-02  6:39 Fan Du
  2013-07-02 14:29 ` Vlad Yasevich
  0 siblings, 1 reply; 24+ messages in thread
From: Fan Du @ 2013-07-02  6:39 UTC (permalink / raw)
  To: vyasevich, nhorman, nicolas.dichtel; +Cc: davem, netdev

When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
as a result ip6_dst_check always fail out. This behaviour makes
transport->dst useless, because every sctp_packet_transmit must look
for valid dst(Is this what supposed to be?)

One aggressive way is to call rt_genid_bump which invalid all dst to
make new dst for transport, apparently it also hurts others. 
I'm sure this may not be the best for all, so any commnets?

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/net/sctp/sctp.h |   18 ++++++++++++------
 net/sctp/ipv6.c         |    2 ++
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index cd89510..f05af01 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
 	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
 }
 
-/* The cookie is always 0 since this is how it's used in the
- * pmtu code.
- */
+/* Set cookie with the right one for IPv6 and zero for others */
 static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
 {
-	if (t->dst && !dst_check(t->dst, 0)) {
-		dst_release(t->dst);
-		t->dst = NULL;
+
+	if (t->dst) {
+		struct rt6_info *rt = (struct rt6_info *)t->dst;
+		u32 cookie = 0;
+
+		if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)	
+			cookie = rt->rt6i_node->fn_sernum;	
+		if (!dst_check(t->dst, cookie)) {
+			dst_release(t->dst);
+			t->dst = NULL;
+		}
 	}
 
 	return t->dst;
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 8ee553b..cfae77e 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
 		break;
 	}
 
+	/* invalid all transport dst forcing to look up new dst */
+	rt_genid_bump(net);
 	return NOTIFY_DONE;
 }
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-02  6:39 [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid Fan Du
@ 2013-07-02 14:29 ` Vlad Yasevich
  2013-07-02 15:55   ` Neil Horman
                     ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-02 14:29 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/02/2013 02:39 AM, Fan Du wrote:
> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
> as a result ip6_dst_check always fail out. This behaviour makes
> transport->dst useless, because every sctp_packet_transmit must look
> for valid dst(Is this what supposed to be?)
>
> One aggressive way is to call rt_genid_bump which invalid all dst to
> make new dst for transport, apparently it also hurts others.
> I'm sure this may not be the best for all, so any commnets?
>
> Signed-off-by: Fan Du <fan.du@windriver.com>
> ---
>   include/net/sctp/sctp.h |   18 ++++++++++++------
>   net/sctp/ipv6.c         |    2 ++
>   2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index cd89510..f05af01 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>   	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>   }
>
> -/* The cookie is always 0 since this is how it's used in the
> - * pmtu code.
> - */
> +/* Set cookie with the right one for IPv6 and zero for others */
>   static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
>   {
> -	if (t->dst && !dst_check(t->dst, 0)) {
> -		dst_release(t->dst);
> -		t->dst = NULL;
> +
> +	if (t->dst) {
> +		struct rt6_info *rt = (struct rt6_info *)t->dst;
> +		u32 cookie = 0;
> +
> +		if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)	
> +			cookie = rt->rt6i_node->fn_sernum;	
> +		if (!dst_check(t->dst, cookie)) {
> +			dst_release(t->dst);
> +			t->dst = NULL;
> +		}
>   	}

I think it would be better if we stored the dst_cookie in the transport 
structure and initialized it at lookup time.   If you do that, then if 
the route table changes, we'd correctly detect it without artificially 
bumping rt_genid (and hurting ipv4).

-vlad

>
>   	return t->dst;
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 8ee553b..cfae77e 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
>   		break;
>   	}
>
> +	/* invalid all transport dst forcing to look up new dst */
> +	rt_genid_bump(net);
>   	return NOTIFY_DONE;
>   }
>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-02 14:29 ` Vlad Yasevich
@ 2013-07-02 15:55   ` Neil Horman
  2013-07-03  2:39     ` Fan Du
  2013-07-02 19:47   ` David Miller
  2013-07-03  2:18   ` Fan Du
  2 siblings, 1 reply; 24+ messages in thread
From: Neil Horman @ 2013-07-02 15:55 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: Fan Du, nicolas.dichtel, davem, netdev

On Tue, Jul 02, 2013 at 10:29:28AM -0400, Vlad Yasevich wrote:
> On 07/02/2013 02:39 AM, Fan Du wrote:
> >When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
> >as a result ip6_dst_check always fail out. This behaviour makes
> >transport->dst useless, because every sctp_packet_transmit must look
> >for valid dst(Is this what supposed to be?)
> >
> >One aggressive way is to call rt_genid_bump which invalid all dst to
> >make new dst for transport, apparently it also hurts others.
> >I'm sure this may not be the best for all, so any commnets?
> >
> >Signed-off-by: Fan Du <fan.du@windriver.com>
> >---
> >  include/net/sctp/sctp.h |   18 ++++++++++++------
> >  net/sctp/ipv6.c         |    2 ++
> >  2 files changed, 14 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >index cd89510..f05af01 100644
> >--- a/include/net/sctp/sctp.h
> >+++ b/include/net/sctp/sctp.h
> >@@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
> >  	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
> >  }
> >
> >-/* The cookie is always 0 since this is how it's used in the
> >- * pmtu code.
> >- */
> >+/* Set cookie with the right one for IPv6 and zero for others */
> >  static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
> >  {
> >-	if (t->dst && !dst_check(t->dst, 0)) {
> >-		dst_release(t->dst);
> >-		t->dst = NULL;
> >+
> >+	if (t->dst) {
> >+		struct rt6_info *rt = (struct rt6_info *)t->dst;
> >+		u32 cookie = 0;
> >+
> >+		if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)	
> >+			cookie = rt->rt6i_node->fn_sernum;	
> >+		if (!dst_check(t->dst, cookie)) {
> >+			dst_release(t->dst);
> >+			t->dst = NULL;
> >+		}
> >  	}
> 
> I think it would be better if we stored the dst_cookie in the
> transport structure and initialized it at lookup time.   If you do
> that, then if the route table changes, we'd correctly detect it
> without artificially bumping rt_genid (and hurting ipv4).
> 
Agreed, thats how we do it for ipv4, IIRC
Neil

> -vlad
> 
> >
> >  	return t->dst;
> >diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> >index 8ee553b..cfae77e 100644
> >--- a/net/sctp/ipv6.c
> >+++ b/net/sctp/ipv6.c
> >@@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
> >  		break;
> >  	}
> >
> >+	/* invalid all transport dst forcing to look up new dst */
> >+	rt_genid_bump(net);
> >  	return NOTIFY_DONE;
> >  }
> >
> >
> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-02 14:29 ` Vlad Yasevich
  2013-07-02 15:55   ` Neil Horman
@ 2013-07-02 19:47   ` David Miller
  2013-07-03  2:18   ` Fan Du
  2 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2013-07-02 19:47 UTC (permalink / raw)
  To: vyasevich; +Cc: fan.du, nhorman, nicolas.dichtel, netdev

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Tue, 02 Jul 2013 10:29:28 -0400

> I think it would be better if we stored the dst_cookie in the
> transport structure and initialized it at lookup time.

Agreed, that is the proper fix for this problem.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-02 14:29 ` Vlad Yasevich
  2013-07-02 15:55   ` Neil Horman
  2013-07-02 19:47   ` David Miller
@ 2013-07-03  2:18   ` Fan Du
  2013-07-03 13:23     ` Vlad Yasevich
  2 siblings, 1 reply; 24+ messages in thread
From: Fan Du @ 2013-07-03  2:18 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: nhorman, nicolas.dichtel, davem, netdev



On 2013年07月02日 22:29, Vlad Yasevich wrote:
> On 07/02/2013 02:39 AM, Fan Du wrote:
>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>> as a result ip6_dst_check always fail out. This behaviour makes
>> transport->dst useless, because every sctp_packet_transmit must look
>> for valid dst(Is this what supposed to be?)
>>
>> One aggressive way is to call rt_genid_bump which invalid all dst to
>> make new dst for transport, apparently it also hurts others.
>> I'm sure this may not be the best for all, so any commnets?
>>
>> Signed-off-by: Fan Du <fan.du@windriver.com>
>> ---
>> include/net/sctp/sctp.h | 18 ++++++++++++------
>> net/sctp/ipv6.c | 2 ++
>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index cd89510..f05af01 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>> }
>>
>> -/* The cookie is always 0 since this is how it's used in the
>> - * pmtu code.
>> - */
>> +/* Set cookie with the right one for IPv6 and zero for others */
>> static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
>> {
>> - if (t->dst && !dst_check(t->dst, 0)) {
>> - dst_release(t->dst);
>> - t->dst = NULL;
>> +
>> + if (t->dst) {
>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>> + u32 cookie = 0;
>> +
>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>> + cookie = rt->rt6i_node->fn_sernum;
>> + if (!dst_check(t->dst, cookie)) {
>> + dst_release(t->dst);
>> + t->dst = NULL;
>> + }
>> }
>
> I think it would be better if we stored the dst_cookie in the transport structure and initialized it at lookup time. If you do that, then if the route table changes, we'd correctly detect it without artificially bumping rt_genid (and hurting ipv4).

Hi Vlad/Neil

Is this what you mean?

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index cd89510..0a646a5 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
                 dst_release(t->dst);
                 t->dst = NULL;
         }
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 1bd4c41..cafdd19 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -946,6 +946,8 @@ struct sctp_transport {
         __u64 hb_nonce;

         struct rcu_head rcu;
+
+       u32 dst_cookie;
  };

  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 8ee553b..82a420f 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -350,6 +350,7 @@ out:
                 struct rt6_info *rt;
                 rt = (struct rt6_info *)dst;
                 t->dst = dst;
+               t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum : 0;
                 SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
                         &rt->rt6i_dst.addr, &fl6->saddr);
         } else {





> -vlad
>
>>
>> return t->dst;
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 8ee553b..cfae77e 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
>> break;
>> }
>>
>> + /* invalid all transport dst forcing to look up new dst */
>> + rt_genid_bump(net);
>> return NOTIFY_DONE;
>> }
>>
>>
>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-02 15:55   ` Neil Horman
@ 2013-07-03  2:39     ` Fan Du
  2013-07-03 13:48       ` Vlad Yasevich
  0 siblings, 1 reply; 24+ messages in thread
From: Fan Du @ 2013-07-03  2:39 UTC (permalink / raw)
  To: Neil Horman; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev



On 2013年07月02日 23:55, Neil Horman wrote:
> On Tue, Jul 02, 2013 at 10:29:28AM -0400, Vlad Yasevich wrote:
>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>> as a result ip6_dst_check always fail out. This behaviour makes
>>> transport->dst useless, because every sctp_packet_transmit must look
>>> for valid dst(Is this what supposed to be?)
>>>
>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>> make new dst for transport, apparently it also hurts others.
>>> I'm sure this may not be the best for all, so any commnets?
>>>
>>> Signed-off-by: Fan Du<fan.du@windriver.com>
>>> ---
>>>   include/net/sctp/sctp.h |   18 ++++++++++++------
>>>   net/sctp/ipv6.c         |    2 ++
>>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index cd89510..f05af01 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union sctp_addr *addr)
>>>   	addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>   }
>>>
>>> -/* The cookie is always 0 since this is how it's used in the
>>> - * pmtu code.
>>> - */
>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>   static inline struct dst_entry *sctp_transport_dst_check(struct sctp_transport *t)
>>>   {
>>> -	if (t->dst&&  !dst_check(t->dst, 0)) {
>>> -		dst_release(t->dst);
>>> -		t->dst = NULL;
>>> +
>>> +	if (t->dst) {
>>> +		struct rt6_info *rt = (struct rt6_info *)t->dst;
>>> +		u32 cookie = 0;
>>> +
>>> +		if ((t->af_specific->sa_family == AF_INET6)&&  rt->rt6i_node)	
>>> +			cookie = rt->rt6i_node->fn_sernum;	
>>> +		if (!dst_check(t->dst, cookie)) {
>>> +			dst_release(t->dst);
>>> +			t->dst = NULL;
>>> +		}
>>>   	}
>>
>> I think it would be better if we stored the dst_cookie in the
>> transport structure and initialized it at lookup time.   If you do
>> that, then if the route table changes, we'd correctly detect it
>> without artificially bumping rt_genid (and hurting ipv4).
>>
> Agreed, thats how we do it for ipv4, IIRC
                                  ^^^^
Hello Neil

I'm sorry I didn't find such things about IPv4.

sctp_transport_dst_check
  -> ipv4_dst_check
    -> rt_is_expired
            ^^^^^^^^^
By "expired", currently we *do* check rt->rt_genid against net->rt_genid,
and for IPv4, every adding/deleting an IPv4 address will bump net->rt_genid,
(this does impact IPv6 dst checking as well) so when I delete an IPv4 address,
ipv4_dst_check failed as expected to look for new dst, basically that's what
I find after checking the code for IPv4 cookie things as mentioned above.
Am I missing something here?


> Neil
>
>> -vlad
>>
>>>
>>>   	return t->dst;
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index 8ee553b..cfae77e 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
>>>   		break;
>>>   	}
>>>
>>> +	/* invalid all transport dst forcing to look up new dst */
>>> +	rt_genid_bump(net);
>>>   	return NOTIFY_DONE;
>>>   }
>>>
>>>
>>
>>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-03  2:18   ` Fan Du
@ 2013-07-03 13:23     ` Vlad Yasevich
  2013-07-03 14:11       ` Vlad Yasevich
  2013-07-04  2:33       ` Fan Du
  0 siblings, 2 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-03 13:23 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/02/2013 10:18 PM, Fan Du wrote:
>
>
> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>> as a result ip6_dst_check always fail out. This behaviour makes
>>> transport->dst useless, because every sctp_packet_transmit must look
>>> for valid dst(Is this what supposed to be?)
>>>
>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>> make new dst for transport, apparently it also hurts others.
>>> I'm sure this may not be the best for all, so any commnets?
>>>
>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>> ---
>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>> net/sctp/ipv6.c | 2 ++
>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index cd89510..f05af01 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>> sctp_addr *addr)
>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>> }
>>>
>>> -/* The cookie is always 0 since this is how it's used in the
>>> - * pmtu code.
>>> - */
>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>> sctp_transport *t)
>>> {
>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>> - dst_release(t->dst);
>>> - t->dst = NULL;
>>> +
>>> + if (t->dst) {
>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>> + u32 cookie = 0;
>>> +
>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>> + cookie = rt->rt6i_node->fn_sernum;
>>> + if (!dst_check(t->dst, cookie)) {
>>> + dst_release(t->dst);
>>> + t->dst = NULL;
>>> + }
>>> }
>>
>> I think it would be better if we stored the dst_cookie in the
>> transport structure and initialized it at lookup time. If you do that,
>> then if the route table changes, we'd correctly detect it without
>> artificially bumping rt_genid (and hurting ipv4).
>
> Hi Vlad/Neil
>
> Is this what you mean?

Yes, exactly.

-vlad

>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index cd89510..0a646a5 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>                  dst_release(t->dst);
>                  t->dst = NULL;
>          }
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 1bd4c41..cafdd19 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -946,6 +946,8 @@ struct sctp_transport {
>          __u64 hb_nonce;
>
>          struct rcu_head rcu;
> +
> +       u32 dst_cookie;
>   };
>
>   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 8ee553b..82a420f 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -350,6 +350,7 @@ out:
>                  struct rt6_info *rt;
>                  rt = (struct rt6_info *)dst;
>                  t->dst = dst;
> +               t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
> : 0;
>                  SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>                          &rt->rt6i_dst.addr, &fl6->saddr);
>          } else {
>
>
>
>
>
>> -vlad
>>
>>>
>>> return t->dst;
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index 8ee553b..cfae77e 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>> notifier_block *this, unsigned long ev,
>>> break;
>>> }
>>>
>>> + /* invalid all transport dst forcing to look up new dst */
>>> + rt_genid_bump(net);
>>> return NOTIFY_DONE;
>>> }
>>>
>>>
>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-03  2:39     ` Fan Du
@ 2013-07-03 13:48       ` Vlad Yasevich
  0 siblings, 0 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-03 13:48 UTC (permalink / raw)
  To: Fan Du; +Cc: Neil Horman, nicolas.dichtel, davem, netdev

On 07/02/2013 10:39 PM, Fan Du wrote:
>
>
> On 2013年07月02日 23:55, Neil Horman wrote:
>> On Tue, Jul 02, 2013 at 10:29:28AM -0400, Vlad Yasevich wrote:
>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>> for valid dst(Is this what supposed to be?)
>>>>
>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>> make new dst for transport, apparently it also hurts others.
>>>> I'm sure this may not be the best for all, so any commnets?
>>>>
>>>> Signed-off-by: Fan Du<fan.du@windriver.com>
>>>> ---
>>>>   include/net/sctp/sctp.h |   18 ++++++++++++------
>>>>   net/sctp/ipv6.c         |    2 ++
>>>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>> index cd89510..f05af01 100644
>>>> --- a/include/net/sctp/sctp.h
>>>> +++ b/include/net/sctp/sctp.h
>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>> sctp_addr *addr)
>>>>       addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>   }
>>>>
>>>> -/* The cookie is always 0 since this is how it's used in the
>>>> - * pmtu code.
>>>> - */
>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>   static inline struct dst_entry *sctp_transport_dst_check(struct
>>>> sctp_transport *t)
>>>>   {
>>>> -    if (t->dst&&  !dst_check(t->dst, 0)) {
>>>> -        dst_release(t->dst);
>>>> -        t->dst = NULL;
>>>> +
>>>> +    if (t->dst) {
>>>> +        struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>> +        u32 cookie = 0;
>>>> +
>>>> +        if ((t->af_specific->sa_family == AF_INET6)&&  rt->rt6i_node)
>>>> +            cookie = rt->rt6i_node->fn_sernum;
>>>> +        if (!dst_check(t->dst, cookie)) {
>>>> +            dst_release(t->dst);
>>>> +            t->dst = NULL;
>>>> +        }
>>>>       }
>>>
>>> I think it would be better if we stored the dst_cookie in the
>>> transport structure and initialized it at lookup time.   If you do
>>> that, then if the route table changes, we'd correctly detect it
>>> without artificially bumping rt_genid (and hurting ipv4).
>>>
>> Agreed, thats how we do it for ipv4, IIRC
>                                   ^^^^
> Hello Neil
>
> I'm sorry I didn't find such things about IPv4.
>
> sctp_transport_dst_check
>   -> ipv4_dst_check
>     -> rt_is_expired
>             ^^^^^^^^^
> By "expired", currently we *do* check rt->rt_genid against net->rt_genid,
> and for IPv4, every adding/deleting an IPv4 address will bump
> net->rt_genid,
> (this does impact IPv6 dst checking as well) so when I delete an IPv4
> address,
> ipv4_dst_check failed as expected to look for new dst, basically that's
> what
> I find after checking the code for IPv4 cookie things as mentioned above.
> Am I missing something here?

No, you are not missing anything.  IPv4 doesn't use the cookie and 
always seems to pass it as 0.

Yes, ipv4 will bump the gen_id thus invalidating all routes (there has 
been disagreement about it).  IPv6 doesn't do that.  In ipv6, when the 
addresses are added or removed, routes are also added or removed and any 
time the route is added it will have a new serial number.  So, you don't 
have to disturb ipv4 cache when ipv6 routing info changes.

-vlad

>
>
>> Neil
>>
>>> -vlad
>>>
>>>>
>>>>       return t->dst;
>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>> index 8ee553b..cfae77e 100644
>>>> --- a/net/sctp/ipv6.c
>>>> +++ b/net/sctp/ipv6.c
>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>> notifier_block *this, unsigned long ev,
>>>>           break;
>>>>       }
>>>>
>>>> +    /* invalid all transport dst forcing to look up new dst */
>>>> +    rt_genid_bump(net);
>>>>       return NOTIFY_DONE;
>>>>   }
>>>>
>>>>
>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-03 13:23     ` Vlad Yasevich
@ 2013-07-03 14:11       ` Vlad Yasevich
  2013-07-04  2:33       ` Fan Du
  1 sibling, 0 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-03 14:11 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/03/2013 09:23 AM, Vlad Yasevich wrote:
> On 07/02/2013 10:18 PM, Fan Du wrote:
>>
>>
>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>> for valid dst(Is this what supposed to be?)
>>>>
>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>> make new dst for transport, apparently it also hurts others.
>>>> I'm sure this may not be the best for all, so any commnets?
>>>>
>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>> ---
>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>> net/sctp/ipv6.c | 2 ++
>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>> index cd89510..f05af01 100644
>>>> --- a/include/net/sctp/sctp.h
>>>> +++ b/include/net/sctp/sctp.h
>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>> sctp_addr *addr)
>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>> }
>>>>
>>>> -/* The cookie is always 0 since this is how it's used in the
>>>> - * pmtu code.
>>>> - */
>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>> sctp_transport *t)
>>>> {
>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>> - dst_release(t->dst);
>>>> - t->dst = NULL;
>>>> +
>>>> + if (t->dst) {
>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>> + u32 cookie = 0;
>>>> +
>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>> + if (!dst_check(t->dst, cookie)) {
>>>> + dst_release(t->dst);
>>>> + t->dst = NULL;
>>>> + }
>>>> }
>>>
>>> I think it would be better if we stored the dst_cookie in the
>>> transport structure and initialized it at lookup time. If you do that,
>>> then if the route table changes, we'd correctly detect it without
>>> artificially bumping rt_genid (and hurting ipv4).
>>
>> Hi Vlad/Neil
>>
>> Is this what you mean?
>
> Yes, exactly.
>
> -vlad
>
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index cd89510..0a646a5 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>                  dst_release(t->dst);
>>                  t->dst = NULL;
>>          }
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 1bd4c41..cafdd19 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>          __u64 hb_nonce;
>>
>>          struct rcu_head rcu;
>> +
>> +       u32 dst_cookie;
>>   };

One comment is that you might try to find a way to put the dst_cookie 
closer to the cached dst.  See if there is a hole the you can fill.

-vlad

>>
>>   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 8ee553b..82a420f 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -350,6 +350,7 @@ out:
>>                  struct rt6_info *rt;
>>                  rt = (struct rt6_info *)dst;
>>                  t->dst = dst;
>> +               t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>> : 0;
>>                  SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>                          &rt->rt6i_dst.addr, &fl6->saddr);
>>          } else {
>>
>>
>>
>>
>>
>>> -vlad
>>>
>>>>
>>>> return t->dst;
>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>> index 8ee553b..cfae77e 100644
>>>> --- a/net/sctp/ipv6.c
>>>> +++ b/net/sctp/ipv6.c
>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>> notifier_block *this, unsigned long ev,
>>>> break;
>>>> }
>>>>
>>>> + /* invalid all transport dst forcing to look up new dst */
>>>> + rt_genid_bump(net);
>>>> return NOTIFY_DONE;
>>>> }
>>>>
>>>>
>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-03 13:23     ` Vlad Yasevich
  2013-07-03 14:11       ` Vlad Yasevich
@ 2013-07-04  2:33       ` Fan Du
  2013-07-05 13:03         ` Neil Horman
  2013-07-05 14:09         ` Vlad Yasevich
  1 sibling, 2 replies; 24+ messages in thread
From: Fan Du @ 2013-07-04  2:33 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: nhorman, nicolas.dichtel, davem, netdev



On 2013年07月03日 21:23, Vlad Yasevich wrote:
> On 07/02/2013 10:18 PM, Fan Du wrote:
>>
>>
>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>> for valid dst(Is this what supposed to be?)
>>>>
>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>> make new dst for transport, apparently it also hurts others.
>>>> I'm sure this may not be the best for all, so any commnets?
>>>>
>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>> ---
>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>> net/sctp/ipv6.c | 2 ++
>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>> index cd89510..f05af01 100644
>>>> --- a/include/net/sctp/sctp.h
>>>> +++ b/include/net/sctp/sctp.h
>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>> sctp_addr *addr)
>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>> }
>>>>
>>>> -/* The cookie is always 0 since this is how it's used in the
>>>> - * pmtu code.
>>>> - */
>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>> sctp_transport *t)
>>>> {
>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>> - dst_release(t->dst);
>>>> - t->dst = NULL;
>>>> +
>>>> + if (t->dst) {
>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>> + u32 cookie = 0;
>>>> +
>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>> + if (!dst_check(t->dst, cookie)) {
>>>> + dst_release(t->dst);
>>>> + t->dst = NULL;
>>>> + }
>>>> }
>>>
>>> I think it would be better if we stored the dst_cookie in the
>>> transport structure and initialized it at lookup time. If you do that,
>>> then if the route table changes, we'd correctly detect it without
>>> artificially bumping rt_genid (and hurting ipv4).
>>
>> Hi Vlad/Neil
>>
>> Is this what you mean?
>
> Yes, exactly.
>

Hi Vlad

I thinks twice about below patch, this is actually a chicken-egg issue.
Look below scenario:
  (1) The first time we push packet through a transport, dst_cookie is 0,
      so sctp_transport_dst_check also pass cookie as 0, then return dst as NULL.
      Then we lookup dst by sctp_transport_route, and in there we initiate dst_cookie
      with rt->rt6i_node->fn_sernum

  (2) Then the next time we push packet through this transport again,
      we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
      return valid dst without bothering to lookup dst again.

BUT, suppose when deleting the source address of this dst after transport->dst_cookie
has been well initialized. transport->dst_cookie still holds rt->rt6i_node->fn_sernum,
meaning ip6_dst_check will return valid dst, which it shouldn't in this case, the
result will be association ABORT.

Other way is invalid all transport->dst which using the deleting address as source address
without bumping gen_id, problem is the traverse times depends heavily on transport number,
and also need to take account locking issue it will introduce.

 >
 > No, you are not missing anything. IPv4 doesn't use the cookie and always seems to pass it as 0.
 >
 > Yes, ipv4 will bump the gen_id thus invalidating all routes (there has been disagreement about it).
 > IPv6 doesn't do that. In ipv6, when the addresses are added or removed, routes are also added or removed and
 > any time the route is added it will have a new serial number. So, you don't have to disturb ipv4 cache when ipv6 routing info changes.

Thank you very much for your explanation!

IPv6 don't bump gen_id, when adding/deleting address, and tag an serial number with each route.
Doing this way loose the semantic of dst_check, because SCTP depends no dst_check fulfill its
duty to actually check whether the holding dst is still valid, well most other Layer 4 protocol
simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every time sending data out.

So please pronounce a final judgment.

> -vlad
>
>>
>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>> index cd89510..0a646a5 100644
>> --- a/include/net/sctp/sctp.h
>> +++ b/include/net/sctp/sctp.h
>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>> dst_release(t->dst);
>> t->dst = NULL;
>> }
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index 1bd4c41..cafdd19 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -946,6 +946,8 @@ struct sctp_transport {
>> __u64 hb_nonce;
>>
>> struct rcu_head rcu;
>> +
>> + u32 dst_cookie;
>> };
>>
>> 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 8ee553b..82a420f 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -350,6 +350,7 @@ out:
>> struct rt6_info *rt;
>> rt = (struct rt6_info *)dst;
>> t->dst = dst;
>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>> : 0;
>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>> &rt->rt6i_dst.addr, &fl6->saddr);
>> } else {
>>
>>
>>
>>
>>
>>> -vlad
>>>
>>>>
>>>> return t->dst;
>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>> index 8ee553b..cfae77e 100644
>>>> --- a/net/sctp/ipv6.c
>>>> +++ b/net/sctp/ipv6.c
>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>> notifier_block *this, unsigned long ev,
>>>> break;
>>>> }
>>>>
>>>> + /* invalid all transport dst forcing to look up new dst */
>>>> + rt_genid_bump(net);
>>>> return NOTIFY_DONE;
>>>> }
>>>>
>>>>
>>>
>>>
>>
>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-04  2:33       ` Fan Du
@ 2013-07-05 13:03         ` Neil Horman
  2013-07-09  7:11           ` Fan Du
  2013-07-05 14:09         ` Vlad Yasevich
  1 sibling, 1 reply; 24+ messages in thread
From: Neil Horman @ 2013-07-05 13:03 UTC (permalink / raw)
  To: Fan Du; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev

On Thu, Jul 04, 2013 at 10:33:35AM +0800, Fan Du wrote:
> 
> 
> On 2013年07月03日 21:23, Vlad Yasevich wrote:
> >On 07/02/2013 10:18 PM, Fan Du wrote:
> >>
> >>
> >>On 2013年07月02日 22:29, Vlad Yasevich wrote:
> >>>On 07/02/2013 02:39 AM, Fan Du wrote:
> >>>>When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
> >>>>as a result ip6_dst_check always fail out. This behaviour makes
> >>>>transport->dst useless, because every sctp_packet_transmit must look
> >>>>for valid dst(Is this what supposed to be?)
> >>>>
> >>>>One aggressive way is to call rt_genid_bump which invalid all dst to
> >>>>make new dst for transport, apparently it also hurts others.
> >>>>I'm sure this may not be the best for all, so any commnets?
> >>>>
> >>>>Signed-off-by: Fan Du <fan.du@windriver.com>
> >>>>---
> >>>>include/net/sctp/sctp.h | 18 ++++++++++++------
> >>>>net/sctp/ipv6.c | 2 ++
> >>>>2 files changed, 14 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >>>>index cd89510..f05af01 100644
> >>>>--- a/include/net/sctp/sctp.h
> >>>>+++ b/include/net/sctp/sctp.h
> >>>>@@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
> >>>>sctp_addr *addr)
> >>>>addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
> >>>>}
> >>>>
> >>>>-/* The cookie is always 0 since this is how it's used in the
> >>>>- * pmtu code.
> >>>>- */
> >>>>+/* Set cookie with the right one for IPv6 and zero for others */
> >>>>static inline struct dst_entry *sctp_transport_dst_check(struct
> >>>>sctp_transport *t)
> >>>>{
> >>>>- if (t->dst && !dst_check(t->dst, 0)) {
> >>>>- dst_release(t->dst);
> >>>>- t->dst = NULL;
> >>>>+
> >>>>+ if (t->dst) {
> >>>>+ struct rt6_info *rt = (struct rt6_info *)t->dst;
> >>>>+ u32 cookie = 0;
> >>>>+
> >>>>+ if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
> >>>>+ cookie = rt->rt6i_node->fn_sernum;
> >>>>+ if (!dst_check(t->dst, cookie)) {
> >>>>+ dst_release(t->dst);
> >>>>+ t->dst = NULL;
> >>>>+ }
> >>>>}
> >>>
> >>>I think it would be better if we stored the dst_cookie in the
> >>>transport structure and initialized it at lookup time. If you do that,
> >>>then if the route table changes, we'd correctly detect it without
> >>>artificially bumping rt_genid (and hurting ipv4).
> >>
> >>Hi Vlad/Neil
> >>
> >>Is this what you mean?
> >
> >Yes, exactly.
> >
> 
> Hi Vlad
> 
> I thinks twice about below patch, this is actually a chicken-egg issue.
> Look below scenario:
>  (1) The first time we push packet through a transport, dst_cookie is 0,
>      so sctp_transport_dst_check also pass cookie as 0, then return dst as NULL.
>      Then we lookup dst by sctp_transport_route, and in there we initiate dst_cookie
>      with rt->rt6i_node->fn_sernum
> 
>  (2) Then the next time we push packet through this transport again,
>      we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>      return valid dst without bothering to lookup dst again.
> 
> BUT, suppose when deleting the source address of this dst after transport->dst_cookie
> has been well initialized. transport->dst_cookie still holds rt->rt6i_node->fn_sernum,
> meaning ip6_dst_check will return valid dst, which it shouldn't in this case, the
> result will be association ABORT.
> 
Have you tried this?  It seems to me in the situation you describe, deleting a
source address will result in fib_inetaddr_event getting called, which will call
rt_cache_flush, bumping the rt_genid to get bumped for that network namespace.
That will cause any subsequent calls to dst_check->ip6_dst_check to return NULL
rather than the cached dst_entry for that transport.

Neil

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-04  2:33       ` Fan Du
  2013-07-05 13:03         ` Neil Horman
@ 2013-07-05 14:09         ` Vlad Yasevich
  2013-07-09 15:11           ` Vlad Yasevich
  1 sibling, 1 reply; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-05 14:09 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/03/2013 10:33 PM, Fan Du wrote:
>
>
> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>
>>>
>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>> for valid dst(Is this what supposed to be?)
>>>>>
>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>> make new dst for transport, apparently it also hurts others.
>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>
>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>> ---
>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>> net/sctp/ipv6.c | 2 ++
>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>> index cd89510..f05af01 100644
>>>>> --- a/include/net/sctp/sctp.h
>>>>> +++ b/include/net/sctp/sctp.h
>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>> sctp_addr *addr)
>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>> }
>>>>>
>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>> - * pmtu code.
>>>>> - */
>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>> sctp_transport *t)
>>>>> {
>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>> - dst_release(t->dst);
>>>>> - t->dst = NULL;
>>>>> +
>>>>> + if (t->dst) {
>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>> + u32 cookie = 0;
>>>>> +
>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>> + dst_release(t->dst);
>>>>> + t->dst = NULL;
>>>>> + }
>>>>> }
>>>>
>>>> I think it would be better if we stored the dst_cookie in the
>>>> transport structure and initialized it at lookup time. If you do that,
>>>> then if the route table changes, we'd correctly detect it without
>>>> artificially bumping rt_genid (and hurting ipv4).
>>>
>>> Hi Vlad/Neil
>>>
>>> Is this what you mean?
>>
>> Yes, exactly.
>>
>
> Hi Vlad
>
> I thinks twice about below patch, this is actually a chicken-egg issue.
> Look below scenario:
>   (1) The first time we push packet through a transport, dst_cookie is 0,
>       so sctp_transport_dst_check also pass cookie as 0, then return dst
> as NULL.
>       Then we lookup dst by sctp_transport_route, and in there we
> initiate dst_cookie
>       with rt->rt6i_node->fn_sernum
>
>   (2) Then the next time we push packet through this transport again,
>       we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>       return valid dst without bothering to lookup dst again.

No, if the route was removed rt6i_node will be NULL, and NULL will be 
returned from ip6_dst_check().  If the route still exists then we'll 
compare the serial number with a cookie.

>
> BUT, suppose when deleting the source address of this dst after
> transport->dst_cookie
> has been well initialized. transport->dst_cookie still holds
> rt->rt6i_node->fn_sernum,
> meaning ip6_dst_check will return valid dst, which it shouldn't in this
> case, the
> result will be association ABORT.

No, removing the address cause the route for that prefix to be removed 
as well.  This will set rt6i_node to NULL.

>
> Other way is invalid all transport->dst which using the deleting address
> as source address
> without bumping gen_id, problem is the traverse times depends heavily on
> transport number,
> and also need to take account locking issue it will introduce.
>
>  >
>  > No, you are not missing anything. IPv4 doesn't use the cookie and
> always seems to pass it as 0.
>  >
>  > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
> has been disagreement about it).
>  > IPv6 doesn't do that. In ipv6, when the addresses are added or
> removed, routes are also added or removed and
>  > any time the route is added it will have a new serial number. So, you
> don't have to disturb ipv4 cache when ipv6 routing info changes.
>
> Thank you very much for your explanation!
>
> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
> number with each route.
> Doing this way loose the semantic of dst_check, because SCTP depends no
> dst_check fulfill its
> duty to actually check whether the holding dst is still valid, well most
> other Layer 4 protocol
> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
> time sending data out.

Look at how other protocols (tcp, dccp) do this.  It is sufficient to 
cache the route serial number into the dst_cookie at the time the route
was lookup-up and cached.  Then the cookie is passed to dst_check to 
validate the route.

-vlad
>
> So please pronounce a final judgment.
>
>> -vlad
>>
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index cd89510..0a646a5 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>> dst_release(t->dst);
>>> t->dst = NULL;
>>> }
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 1bd4c41..cafdd19 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>> __u64 hb_nonce;
>>>
>>> struct rcu_head rcu;
>>> +
>>> + u32 dst_cookie;
>>> };
>>>
>>> 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 8ee553b..82a420f 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -350,6 +350,7 @@ out:
>>> struct rt6_info *rt;
>>> rt = (struct rt6_info *)dst;
>>> t->dst = dst;
>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>> : 0;
>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>> } else {
>>>
>>>
>>>
>>>
>>>
>>>> -vlad
>>>>
>>>>>
>>>>> return t->dst;
>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>> index 8ee553b..cfae77e 100644
>>>>> --- a/net/sctp/ipv6.c
>>>>> +++ b/net/sctp/ipv6.c
>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>> notifier_block *this, unsigned long ev,
>>>>> break;
>>>>> }
>>>>>
>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>> + rt_genid_bump(net);
>>>>> return NOTIFY_DONE;
>>>>> }
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-05 13:03         ` Neil Horman
@ 2013-07-09  7:11           ` Fan Du
  2013-07-09 11:38             ` Neil Horman
  0 siblings, 1 reply; 24+ messages in thread
From: Fan Du @ 2013-07-09  7:11 UTC (permalink / raw)
  To: Neil Horman; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev

Hello Neil

On 2013年07月05日 21:03, Neil Horman wrote:
> On Thu, Jul 04, 2013 at 10:33:35AM +0800, Fan Du wrote:
>>
>>
>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>
>>>>
>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>
>>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>
>>>>>> Signed-off-by: Fan Du<fan.du@windriver.com>
>>>>>> ---
>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>> index cd89510..f05af01 100644
>>>>>> --- a/include/net/sctp/sctp.h
>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>> sctp_addr *addr)
>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>> }
>>>>>>
>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>> - * pmtu code.
>>>>>> - */
>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>> sctp_transport *t)
>>>>>> {
>>>>>> - if (t->dst&&  !dst_check(t->dst, 0)) {
>>>>>> - dst_release(t->dst);
>>>>>> - t->dst = NULL;
>>>>>> +
>>>>>> + if (t->dst) {
>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>> + u32 cookie = 0;
>>>>>> +
>>>>>> + if ((t->af_specific->sa_family == AF_INET6)&&  rt->rt6i_node)
>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>> + dst_release(t->dst);
>>>>>> + t->dst = NULL;
>>>>>> + }
>>>>>> }
>>>>>
>>>>> I think it would be better if we stored the dst_cookie in the
>>>>> transport structure and initialized it at lookup time. If you do that,
>>>>> then if the route table changes, we'd correctly detect it without
>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>
>>>> Hi Vlad/Neil
>>>>
>>>> Is this what you mean?
>>>
>>> Yes, exactly.
>>>
>>
>> Hi Vlad
>>
>> I thinks twice about below patch, this is actually a chicken-egg issue.
>> Look below scenario:
>>   (1) The first time we push packet through a transport, dst_cookie is 0,
>>       so sctp_transport_dst_check also pass cookie as 0, then return dst as NULL.
>>       Then we lookup dst by sctp_transport_route, and in there we initiate dst_cookie
>>       with rt->rt6i_node->fn_sernum
>>
>>   (2) Then the next time we push packet through this transport again,
>>       we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>       return valid dst without bothering to lookup dst again.
>>
>> BUT, suppose when deleting the source address of this dst after transport->dst_cookie
>> has been well initialized. transport->dst_cookie still holds rt->rt6i_node->fn_sernum,
>> meaning ip6_dst_check will return valid dst, which it shouldn't in this case, the
>> result will be association ABORT.
>>
> Have you tried this?  It seems to me in the situation you describe, deleting a
> source address will result in fib_inetaddr_event getting called, which will call
                                  ^^^^^^^^^^
                                  This is IPv4 specific.

> rt_cache_flush, bumping the rt_genid to get bumped for that network namespace.
> That will cause any subsequent calls to dst_check->ip6_dst_check to return NULL
                                            ^^^^^^^^^^^^^^^^^
                                            This is IPv6 specific
> rather than the cached dst_entry for that transport.

The phenomenon(transport->dst got lookup every time for IPv6) I described before
could be easily observed by turning on SCTP debug.

> Neil
>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-09  7:11           ` Fan Du
@ 2013-07-09 11:38             ` Neil Horman
  0 siblings, 0 replies; 24+ messages in thread
From: Neil Horman @ 2013-07-09 11:38 UTC (permalink / raw)
  To: Fan Du; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev

On Tue, Jul 09, 2013 at 03:11:55PM +0800, Fan Du wrote:
> Hello Neil
> 
> On 2013年07月05日 21:03, Neil Horman wrote:
> >On Thu, Jul 04, 2013 at 10:33:35AM +0800, Fan Du wrote:
> >>
> >>
> >>On 2013年07月03日 21:23, Vlad Yasevich wrote:
> >>>On 07/02/2013 10:18 PM, Fan Du wrote:
> >>>>
> >>>>
> >>>>On 2013年07月02日 22:29, Vlad Yasevich wrote:
> >>>>>On 07/02/2013 02:39 AM, Fan Du wrote:
> >>>>>>When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
> >>>>>>as a result ip6_dst_check always fail out. This behaviour makes
> >>>>>>transport->dst useless, because every sctp_packet_transmit must look
> >>>>>>for valid dst(Is this what supposed to be?)
> >>>>>>
> >>>>>>One aggressive way is to call rt_genid_bump which invalid all dst to
> >>>>>>make new dst for transport, apparently it also hurts others.
> >>>>>>I'm sure this may not be the best for all, so any commnets?
> >>>>>>
> >>>>>>Signed-off-by: Fan Du<fan.du@windriver.com>
> >>>>>>---
> >>>>>>include/net/sctp/sctp.h | 18 ++++++++++++------
> >>>>>>net/sctp/ipv6.c | 2 ++
> >>>>>>2 files changed, 14 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>>diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >>>>>>index cd89510..f05af01 100644
> >>>>>>--- a/include/net/sctp/sctp.h
> >>>>>>+++ b/include/net/sctp/sctp.h
> >>>>>>@@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
> >>>>>>sctp_addr *addr)
> >>>>>>addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
> >>>>>>}
> >>>>>>
> >>>>>>-/* The cookie is always 0 since this is how it's used in the
> >>>>>>- * pmtu code.
> >>>>>>- */
> >>>>>>+/* Set cookie with the right one for IPv6 and zero for others */
> >>>>>>static inline struct dst_entry *sctp_transport_dst_check(struct
> >>>>>>sctp_transport *t)
> >>>>>>{
> >>>>>>- if (t->dst&&  !dst_check(t->dst, 0)) {
> >>>>>>- dst_release(t->dst);
> >>>>>>- t->dst = NULL;
> >>>>>>+
> >>>>>>+ if (t->dst) {
> >>>>>>+ struct rt6_info *rt = (struct rt6_info *)t->dst;
> >>>>>>+ u32 cookie = 0;
> >>>>>>+
> >>>>>>+ if ((t->af_specific->sa_family == AF_INET6)&&  rt->rt6i_node)
> >>>>>>+ cookie = rt->rt6i_node->fn_sernum;
> >>>>>>+ if (!dst_check(t->dst, cookie)) {
> >>>>>>+ dst_release(t->dst);
> >>>>>>+ t->dst = NULL;
> >>>>>>+ }
> >>>>>>}
> >>>>>
> >>>>>I think it would be better if we stored the dst_cookie in the
> >>>>>transport structure and initialized it at lookup time. If you do that,
> >>>>>then if the route table changes, we'd correctly detect it without
> >>>>>artificially bumping rt_genid (and hurting ipv4).
> >>>>
> >>>>Hi Vlad/Neil
> >>>>
> >>>>Is this what you mean?
> >>>
> >>>Yes, exactly.
> >>>
> >>
> >>Hi Vlad
> >>
> >>I thinks twice about below patch, this is actually a chicken-egg issue.
> >>Look below scenario:
> >>  (1) The first time we push packet through a transport, dst_cookie is 0,
> >>      so sctp_transport_dst_check also pass cookie as 0, then return dst as NULL.
> >>      Then we lookup dst by sctp_transport_route, and in there we initiate dst_cookie
> >>      with rt->rt6i_node->fn_sernum
> >>
> >>  (2) Then the next time we push packet through this transport again,
> >>      we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
> >>      return valid dst without bothering to lookup dst again.
> >>
> >>BUT, suppose when deleting the source address of this dst after transport->dst_cookie
> >>has been well initialized. transport->dst_cookie still holds rt->rt6i_node->fn_sernum,
> >>meaning ip6_dst_check will return valid dst, which it shouldn't in this case, the
> >>result will be association ABORT.
> >>
> >Have you tried this?  It seems to me in the situation you describe, deleting a
> >source address will result in fib_inetaddr_event getting called, which will call
>                                  ^^^^^^^^^^
>                                  This is IPv4 specific.
> 
Sorry, meant to be looking at fib6_run_gc here, as called from
ndisc_event_netdev.  That should age all the ipv6 router entries appropriately.


> >rt_cache_flush, bumping the rt_genid to get bumped for that network namespace.
> >That will cause any subsequent calls to dst_check->ip6_dst_check to return NULL
>                                            ^^^^^^^^^^^^^^^^^
>                                            This is IPv6 specific
> >rather than the cached dst_entry for that transport.
> 
> The phenomenon(transport->dst got lookup every time for IPv6) I described before
> could be easily observed by turning on SCTP debug.
> 
I'm not talking about the origional behavior, I'm talking about the concern you
have above with your patch.  Nothing messes with sernum or genid in the ipv6
garbage collector, but cached routes, when expunged by fib6_del_route, will set
rt6i_node to NULL, which should cause both if clauses in ip6_dst_chk to be
skipped, returning NULL, forcing a new lookup.

Neil

> >Neil
> >
> >
> 
> -- 
> 浮沉随浪只记今朝笑
> 
> --fan
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-05 14:09         ` Vlad Yasevich
@ 2013-07-09 15:11           ` Vlad Yasevich
  2013-07-10  5:26             ` Fan Du
  2013-07-12  3:15             ` Fan Du
  0 siblings, 2 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-09 15:11 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
> On 07/03/2013 10:33 PM, Fan Du wrote:
>>
>>
>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>
>>>>
>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>
>>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>
>>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>>> ---
>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>> index cd89510..f05af01 100644
>>>>>> --- a/include/net/sctp/sctp.h
>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>> sctp_addr *addr)
>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>> }
>>>>>>
>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>> - * pmtu code.
>>>>>> - */
>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>> sctp_transport *t)
>>>>>> {
>>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>>> - dst_release(t->dst);
>>>>>> - t->dst = NULL;
>>>>>> +
>>>>>> + if (t->dst) {
>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>> + u32 cookie = 0;
>>>>>> +
>>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>> + dst_release(t->dst);
>>>>>> + t->dst = NULL;
>>>>>> + }
>>>>>> }
>>>>>
>>>>> I think it would be better if we stored the dst_cookie in the
>>>>> transport structure and initialized it at lookup time. If you do that,
>>>>> then if the route table changes, we'd correctly detect it without
>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>
>>>> Hi Vlad/Neil
>>>>
>>>> Is this what you mean?
>>>
>>> Yes, exactly.
>>>
>>
>> Hi Vlad
>>
>> I thinks twice about below patch, this is actually a chicken-egg issue.
>> Look below scenario:
>>   (1) The first time we push packet through a transport, dst_cookie is 0,
>>       so sctp_transport_dst_check also pass cookie as 0, then return dst
>> as NULL.
>>       Then we lookup dst by sctp_transport_route, and in there we
>> initiate dst_cookie
>>       with rt->rt6i_node->fn_sernum
>>
>>   (2) Then the next time we push packet through this transport again,
>>       we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>       return valid dst without bothering to lookup dst again.
>
> No, if the route was removed rt6i_node will be NULL, and NULL will be
> returned from ip6_dst_check().  If the route still exists then we'll
> compare the serial number with a cookie.
>
>>
>> BUT, suppose when deleting the source address of this dst after
>> transport->dst_cookie
>> has been well initialized. transport->dst_cookie still holds
>> rt->rt6i_node->fn_sernum,
>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>> case, the
>> result will be association ABORT.
>
> No, removing the address cause the route for that prefix to be removed
> as well.  This will set rt6i_node to NULL.
>
>>
>> Other way is invalid all transport->dst which using the deleting address
>> as source address
>> without bumping gen_id, problem is the traverse times depends heavily on
>> transport number,
>> and also need to take account locking issue it will introduce.
>>
>>  >
>>  > No, you are not missing anything. IPv4 doesn't use the cookie and
>> always seems to pass it as 0.
>>  >
>>  > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>> has been disagreement about it).
>>  > IPv6 doesn't do that. In ipv6, when the addresses are added or
>> removed, routes are also added or removed and
>>  > any time the route is added it will have a new serial number. So, you
>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>
>> Thank you very much for your explanation!
>>
>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>> number with each route.
>> Doing this way loose the semantic of dst_check, because SCTP depends no
>> dst_check fulfill its
>> duty to actually check whether the holding dst is still valid, well most
>> other Layer 4 protocol
>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>> time sending data out.
>
> Look at how other protocols (tcp, dccp) do this.  It is sufficient to
> cache the route serial number into the dst_cookie at the time the route
> was lookup-up and cached.  Then the cookie is passed to dst_check to
> validate the route.


Hi Fan

Have you tried the updated patch you sent?  Based on what the tcp/udp 
code is doing, the updated patch should work correctly.  If it does, can 
you re-post with attribution/sign-off

Thanks
-vlad

>
> -vlad
>>
>> So please pronounce a final judgment.
>>
>>> -vlad
>>>
>>>>
>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>> index cd89510..0a646a5 100644
>>>> --- a/include/net/sctp/sctp.h
>>>> +++ b/include/net/sctp/sctp.h
>>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>>> dst_release(t->dst);
>>>> t->dst = NULL;
>>>> }
>>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>>> index 1bd4c41..cafdd19 100644
>>>> --- a/include/net/sctp/structs.h
>>>> +++ b/include/net/sctp/structs.h
>>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>>> __u64 hb_nonce;
>>>>
>>>> struct rcu_head rcu;
>>>> +
>>>> + u32 dst_cookie;
>>>> };
>>>>
>>>> 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 8ee553b..82a420f 100644
>>>> --- a/net/sctp/ipv6.c
>>>> +++ b/net/sctp/ipv6.c
>>>> @@ -350,6 +350,7 @@ out:
>>>> struct rt6_info *rt;
>>>> rt = (struct rt6_info *)dst;
>>>> t->dst = dst;
>>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>>> : 0;
>>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>>> } else {
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> -vlad
>>>>>
>>>>>>
>>>>>> return t->dst;
>>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>>> index 8ee553b..cfae77e 100644
>>>>>> --- a/net/sctp/ipv6.c
>>>>>> +++ b/net/sctp/ipv6.c
>>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>>> notifier_block *this, unsigned long ev,
>>>>>> break;
>>>>>> }
>>>>>>
>>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>>> + rt_genid_bump(net);
>>>>>> return NOTIFY_DONE;
>>>>>> }
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-09 15:11           ` Vlad Yasevich
@ 2013-07-10  5:26             ` Fan Du
  2013-07-12 11:19               ` Neil Horman
  2013-07-13 12:21               ` Vlad Yasevich
  2013-07-12  3:15             ` Fan Du
  1 sibling, 2 replies; 24+ messages in thread
From: Fan Du @ 2013-07-10  5:26 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: nhorman, nicolas.dichtel, davem, netdev



On 2013年07月09日 23:11, Vlad Yasevich wrote:
> On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
>> On 07/03/2013 10:33 PM, Fan Du wrote:
>>>
>>>
>>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>>
>>>>>
>>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>>
>>>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>>
>>>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>>>> ---
>>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>> index cd89510..f05af01 100644
>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>>> sctp_addr *addr)
>>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>>> }
>>>>>>>
>>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>>> - * pmtu code.
>>>>>>> - */
>>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>>> sctp_transport *t)
>>>>>>> {
>>>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>>>> - dst_release(t->dst);
>>>>>>> - t->dst = NULL;
>>>>>>> +
>>>>>>> + if (t->dst) {
>>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>>> + u32 cookie = 0;
>>>>>>> +
>>>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>>> + dst_release(t->dst);
>>>>>>> + t->dst = NULL;
>>>>>>> + }
>>>>>>> }
>>>>>>
>>>>>> I think it would be better if we stored the dst_cookie in the
>>>>>> transport structure and initialized it at lookup time. If you do that,
>>>>>> then if the route table changes, we'd correctly detect it without
>>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>>
>>>>> Hi Vlad/Neil
>>>>>
>>>>> Is this what you mean?
>>>>
>>>> Yes, exactly.
>>>>
>>>
>>> Hi Vlad
>>>
>>> I thinks twice about below patch, this is actually a chicken-egg issue.
>>> Look below scenario:
>>> (1) The first time we push packet through a transport, dst_cookie is 0,
>>> so sctp_transport_dst_check also pass cookie as 0, then return dst
>>> as NULL.
>>> Then we lookup dst by sctp_transport_route, and in there we
>>> initiate dst_cookie
>>> with rt->rt6i_node->fn_sernum
>>>
>>> (2) Then the next time we push packet through this transport again,
>>> we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>> return valid dst without bothering to lookup dst again.
>>
>> No, if the route was removed rt6i_node will be NULL, and NULL will be
>> returned from ip6_dst_check(). If the route still exists then we'll
>> compare the serial number with a cookie.
>>
>>>
>>> BUT, suppose when deleting the source address of this dst after
>>> transport->dst_cookie
>>> has been well initialized. transport->dst_cookie still holds
>>> rt->rt6i_node->fn_sernum,
>>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>>> case, the
>>> result will be association ABORT.
>>
>> No, removing the address cause the route for that prefix to be removed
>> as well. This will set rt6i_node to NULL.
>>
>>>
>>> Other way is invalid all transport->dst which using the deleting address
>>> as source address
>>> without bumping gen_id, problem is the traverse times depends heavily on
>>> transport number,
>>> and also need to take account locking issue it will introduce.
>>>
>>> >
>>> > No, you are not missing anything. IPv4 doesn't use the cookie and
>>> always seems to pass it as 0.
>>> >
>>> > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>>> has been disagreement about it).
>>> > IPv6 doesn't do that. In ipv6, when the addresses are added or
>>> removed, routes are also added or removed and
>>> > any time the route is added it will have a new serial number. So, you
>>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>>
>>> Thank you very much for your explanation!
>>>
>>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>>> number with each route.
>>> Doing this way loose the semantic of dst_check, because SCTP depends no
>>> dst_check fulfill its
>>> duty to actually check whether the holding dst is still valid, well most
>>> other Layer 4 protocol
>>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>>> time sending data out.
>>
>> Look at how other protocols (tcp, dccp) do this. It is sufficient to
>> cache the route serial number into the dst_cookie at the time the route
>> was lookup-up and cached. Then the cookie is passed to dst_check to
>> validate the route.
>
>
> Hi Fan
>
> Have you tried the updated patch you sent? Based on what the tcp/udp code is doing, the updated patch should work correctly. If it does, can you re-post with attribution/sign-off
>

Hello Vlad and Neil

I don't know whether my test procedure has drawbacks or something else.
It seems the updated patch does not works well, but my first patch is ok under below configuration.

Host A:
	ifconfig eth1 inet6 add 2001::803/64
	ifconfig eth1 inet6 add 2001::804/64
	sctp_darn -H 2001::803 -B 2001::804 -P 5001 -l

Host B:
	ifconfig eth1 inet6 add 2001::805/64
	ifconfig eth1 inet6 add 2001::806/64
         sctp_darn -H 2001::805 -B 2001::806 -P 5002 -h 2001::803 -p 5001 -s

hbinterval set to 2 seconds, after setup the association, primary address: 2001::804 <--> 2001::806


Step 1:

After adding in IPv6 address to an interface, we populate
struct inet6_ifaddr *ifp->rt->rt6i_node, this rt6i_node is stored in a radix tree.

addrconf_add_ifaddr
  ->inet6_addr_add
     ->ipv6_add_addr  <-- Do DAD checking afterward.
     ->addrconf_prefix_route
       ->ip6_route_add
         ->__ip6_ins_rt
           ->fib6_add
             ->fib6_add_1  <-- Create struct fib6_node *fn
               ->fib6_add_rt2node
                 ->rt->rt6i_node = fn; (*1*)

Step 2:

In host A ,for the packet destinate for 2001::806 using source address 2001::804,
In sctp_v6_get_dst, let's print the rt, rt->rt6i_node, and rt->rt6i_node->fn_sernum
after ip6_dst_lookup_flow got valid dst. The problem is the transport->dst->rt6i_node
we have looked for is *not* the rt6i_node in (*1*), I'm not drunk here.....
So in Step 3, let's be naughty: ifconfig eth1 inet6 del 2001::804/64


Step 3:

Then we delete the IPv6 address, certainly this struct inet6_ifaddr *ifp->rt
will be delete as well.

addrconf_del_ifaddr
   ->inet6_addr_del
     ->ipv6_del_addr
       ->ipv6_ifa_notify /RTM_DELADDR
         ->ip6_del_rt
           ->__ip6_del_rt
             ->fib6_del
               ->fib6_del_route
                -> rt->rt6i_node = NULL; (*2*) here we undo the operation in (*1*),
                                               but rt6i_node in Step2 is untouched!

Step 4:
Then we do the dst checking, for sctp_tranport_dst_check it looks like nothing in Step3
has consequence on it. So transport->dst is still valid, let's ship the packet out
using 2001::804. The real world test result is ASSOCIATION ABORT after a while.
As far as I can tell, rt in Step 2 got deleted *after* the ASSOCIATION ABORT.

    sctp_tranport_dst_check
       ->dst_check
         -> ip6_dst_check


Root node
    *
   / \
      * <-- fn node we are using.
     / \
        *
       / \
      *
     / \
        * <--- Add a new fn node, on the path from the root node down to here, each fn
               now has new fn_sernum, which force dst_check return NULL for lookup again.
               Then previously save fn_sernum will take effect now. But when delete address
               only the ifp->rt->rt6i_node is set to NULL.


> Thanks
> -vlad
>
>>
>> -vlad
>>>
>>> So please pronounce a final judgment.
>>>
>>>> -vlad
>>>>
>>>>>
>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>> index cd89510..0a646a5 100644
>>>>> --- a/include/net/sctp/sctp.h
>>>>> +++ b/include/net/sctp/sctp.h
>>>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>>>> dst_release(t->dst);
>>>>> t->dst = NULL;
>>>>> }
>>>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>>>> index 1bd4c41..cafdd19 100644
>>>>> --- a/include/net/sctp/structs.h
>>>>> +++ b/include/net/sctp/structs.h
>>>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>>>> __u64 hb_nonce;
>>>>>
>>>>> struct rcu_head rcu;
>>>>> +
>>>>> + u32 dst_cookie;
>>>>> };
>>>>>
>>>>> 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 8ee553b..82a420f 100644
>>>>> --- a/net/sctp/ipv6.c
>>>>> +++ b/net/sctp/ipv6.c
>>>>> @@ -350,6 +350,7 @@ out:
>>>>> struct rt6_info *rt;
>>>>> rt = (struct rt6_info *)dst;
>>>>> t->dst = dst;
>>>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>>>> : 0;
>>>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>>>> } else {
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>>
>>>>>>> return t->dst;
>>>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>>>> index 8ee553b..cfae77e 100644
>>>>>>> --- a/net/sctp/ipv6.c
>>>>>>> +++ b/net/sctp/ipv6.c
>>>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>>>> notifier_block *this, unsigned long ev,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>>>> + rt_genid_bump(net);
>>>>>>> return NOTIFY_DONE;
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-09 15:11           ` Vlad Yasevich
  2013-07-10  5:26             ` Fan Du
@ 2013-07-12  3:15             ` Fan Du
  2013-07-12 22:58               ` David Miller
  1 sibling, 1 reply; 24+ messages in thread
From: Fan Du @ 2013-07-12  3:15 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: nhorman, nicolas.dichtel, davem, netdev



On 2013年07月09日 23:11, Vlad Yasevich wrote:
> On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
>> On 07/03/2013 10:33 PM, Fan Du wrote:
>>>
>>>
>>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>>
>>>>>
>>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>>
>>>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>>
>>>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>>>> ---
>>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>> index cd89510..f05af01 100644
>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>>> sctp_addr *addr)
>>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>>> }
>>>>>>>
>>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>>> - * pmtu code.
>>>>>>> - */
>>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>>> sctp_transport *t)
>>>>>>> {
>>>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>>>> - dst_release(t->dst);
>>>>>>> - t->dst = NULL;
>>>>>>> +
>>>>>>> + if (t->dst) {
>>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>>> + u32 cookie = 0;
>>>>>>> +
>>>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>>> + dst_release(t->dst);
>>>>>>> + t->dst = NULL;
>>>>>>> + }
>>>>>>> }
>>>>>>
>>>>>> I think it would be better if we stored the dst_cookie in the
>>>>>> transport structure and initialized it at lookup time. If you do that,
>>>>>> then if the route table changes, we'd correctly detect it without
>>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>>
>>>>> Hi Vlad/Neil
>>>>>
>>>>> Is this what you mean?
>>>>
>>>> Yes, exactly.
>>>>
>>>
>>> Hi Vlad
>>>
>>> I thinks twice about below patch, this is actually a chicken-egg issue.
>>> Look below scenario:
>>> (1) The first time we push packet through a transport, dst_cookie is 0,
>>> so sctp_transport_dst_check also pass cookie as 0, then return dst
>>> as NULL.
>>> Then we lookup dst by sctp_transport_route, and in there we
>>> initiate dst_cookie
>>> with rt->rt6i_node->fn_sernum
>>>
>>> (2) Then the next time we push packet through this transport again,
>>> we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>> return valid dst without bothering to lookup dst again.
>>
>> No, if the route was removed rt6i_node will be NULL, and NULL will be
>> returned from ip6_dst_check(). If the route still exists then we'll
>> compare the serial number with a cookie.
>>
>>>
>>> BUT, suppose when deleting the source address of this dst after
>>> transport->dst_cookie
>>> has been well initialized. transport->dst_cookie still holds
>>> rt->rt6i_node->fn_sernum,
>>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>>> case, the
>>> result will be association ABORT.
>>
>> No, removing the address cause the route for that prefix to be removed
>> as well. This will set rt6i_node to NULL.
>>
>>>
>>> Other way is invalid all transport->dst which using the deleting address
>>> as source address
>>> without bumping gen_id, problem is the traverse times depends heavily on
>>> transport number,
>>> and also need to take account locking issue it will introduce.
>>>
>>> >
>>> > No, you are not missing anything. IPv4 doesn't use the cookie and
>>> always seems to pass it as 0.
>>> >
>>> > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>>> has been disagreement about it).
>>> > IPv6 doesn't do that. In ipv6, when the addresses are added or
>>> removed, routes are also added or removed and
>>> > any time the route is added it will have a new serial number. So, you
>>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>>
>>> Thank you very much for your explanation!
>>>
>>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>>> number with each route.
>>> Doing this way loose the semantic of dst_check, because SCTP depends no
>>> dst_check fulfill its
>>> duty to actually check whether the holding dst is still valid, well most
>>> other Layer 4 protocol
>>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>>> time sending data out.
>>
>> Look at how other protocols (tcp, dccp) do this. It is sufficient to
>> cache the route serial number into the dst_cookie at the time the route
>> was lookup-up and cached. Then the cookie is passed to dst_check to
>> validate the route.
>
>
> Hi Fan
>
> Have you tried the updated patch you sent? Based on what the tcp/udp code is doing, the updated patch should work correctly. If it does, can you re-post with attribution/sign-off

Hello Vlad and Neil

Could you please kindly review below patch?

Thanks


 From ec43c8ec27e16a72d87d6d4b07a1f494083261a2 Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Fri, 12 Jul 2013 10:22:35 +0800
Subject: [PATCH] sctp: Don't lookup dst if transport dst is still valid

When sctp sits on IPv6, current sctp_transport_dst_check pass ZERO
ip6_dst_check, which result in dst lookup every time for IPv6.
We use dst_cookie to check against fn_sernum to avoid unnecessary
lookup.

But problem still arise when we attempt to delete address
in multi-home mode, deleting an IPv6 address does not invalidate
any dst which source address is the same at the deleted one.
Which means sctp cannot rely on ip6_dst_check in this scenario.

To enforce multi-home functionality, introducing a sctp_genid similar
with rt genid, but only in sctp territory, so we don't hurt any other
rt validness against other layer 4 protocol.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
  include/net/netns/sctp.h   |    5 +++++
  include/net/sctp/sctp.h    |   11 +++++++++--
  include/net/sctp/structs.h |    3 +++
  net/sctp/ipv6.c            |   24 +++++++++++++++++++++---
  net/sctp/protocol.c        |    7 +++++++
  5 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
index 3573a81..83204db 100644
--- a/include/net/netns/sctp.h
+++ b/include/net/netns/sctp.h
@@ -129,6 +129,11 @@ struct netns_sctp {

  	/* Threshold for autoclose timeout, in seconds. */
  	unsigned long max_autoclose;
+
+#if IS_ENABLED(CONFIG_IPV6)	
+	/* sctp IPv6 specific */
+	atomic_t	addr_genid;
+#endif
  };

  #endif /* __NETNS_SCTP_H__ */
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index d8e37ec..c344ea8 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -613,11 +613,18 @@ 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)) {
+	struct sctp_af *af = t->af_specific;
+
+	/* We handle two sceanario here:
+	 * One: using dst_cookie to check against any routing information change
+	 * Two: using sctp_cookie to check against address deletion
+	 * Either of them force a new dst lookup for transport
+	 */
+	if ((t->dst && !dst_check(t->dst, t->dst_cookie)) ||
+		af->check_addr_change(t)) {
  		dst_release(t->dst);
  		t->dst = NULL;
  	}
-
  	return t->dst;
  }

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e745c92..9849915 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -492,6 +492,7 @@ struct sctp_af {
  	void		(*seq_dump_addr)(struct seq_file *seq,
  					 union sctp_addr *addr);
  	void		(*ecn_capable)(struct sock *sk);
+	int 		(*check_addr_change)(struct sctp_transport *t);
  	__u16		net_header_len;
  	int		sockaddr_len;
  	sa_family_t	sa_family;
@@ -946,6 +947,8 @@ struct sctp_transport {
  	__u64 hb_nonce;

  	struct rcu_head rcu;
+	u32 dst_cookie;
+	u32 addr_cookie;
  };

  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 09ffcc9..343cd10 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct notifier_block *this, unsigned long ev,
  		break;
  	}

+	/* force a lookup on all transport->dst for stcp IPv6 ONLY */	
+	atomic_inc(&net->sctp.addr_genid);
  	return NOTIFY_DONE;
  }

@@ -348,12 +350,20 @@ static void sctp_v6_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
  out:
  	if (!IS_ERR_OR_NULL(dst)) {
  		struct rt6_info *rt;
+		struct net *net;

  		rt = (struct rt6_info *)dst;
  		t->dst = dst;
-
+		net = dev_net(rt->rt6i_idev->dev);		
+	
  		pr_debug("rt6_dst:%pI6 rt6_src:%pI6\n", &rt->rt6i_dst.addr,
-			 &fl6->saddr);
+			&fl6->saddr);
+	
+		/* use fn_sernum to detect routing change */
+		t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum : 0;
+
+		/* record addr_genid to check against address delete */
+		t->addr_cookie = atomic_read(&net->sctp.addr_genid);
  	} else {
  		t->dst = NULL;

@@ -724,6 +734,14 @@ static void sctp_v6_ecn_capable(struct sock *sk)
  	inet6_sk(sk)->tclass |= INET_ECN_ECT_0;
  }

+static int sctp_v6_check_addr_change(struct sctp_transport *t)
+{
+	struct rt6_info *rt = (struct rt6_info *)t->dst;
+	struct net *net = dev_net(rt->rt6i_idev->dev);
+	
+	return (t->addr_cookie != atomic_read(&net->sctp.addr_genid) ? 1 : 0);
+}
+
  /* Initialize a PF_INET6 socket msg_name. */
  static void sctp_inet6_msgname(char *msgname, int *addr_len)
  {
@@ -1008,6 +1026,7 @@ static struct sctp_af sctp_af_inet6 = {
  	.is_ce		   = sctp_v6_is_ce,
  	.seq_dump_addr	   = sctp_v6_seq_dump_addr,
  	.ecn_capable	   = sctp_v6_ecn_capable,
+	.check_addr_change = sctp_v6_check_addr_change,
  	.net_header_len	   = sizeof(struct ipv6hdr),
  	.sockaddr_len	   = sizeof(struct sockaddr_in6),
  #ifdef CONFIG_COMPAT
@@ -1076,7 +1095,6 @@ int sctp_v6_add_protocol(void)

  	if (inet6_add_protocol(&sctpv6_protocol, IPPROTO_SCTP) < 0)
  		return -EAGAIN;
-
  	return 0;
  }

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 4a17494d..b9335d7 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -595,6 +595,12 @@ static void sctp_v4_ecn_capable(struct sock *sk)
  	INET_ECN_xmit(sk);
  }

+static int sctp_v4_check_addr_change(struct sctp_transport *t)
+{
+	/* Always return 0, as IPv4 bump rt genid when address changes */
+	return 0;
+}
+
  static void sctp_addr_wq_timeout_handler(unsigned long arg)
  {
  	struct net *net = (struct net *)arg;
@@ -1064,6 +1070,7 @@ static struct sctp_af sctp_af_inet = {
  	.is_ce		   = sctp_v4_is_ce,
  	.seq_dump_addr	   = sctp_v4_seq_dump_addr,
  	.ecn_capable	   = sctp_v4_ecn_capable,
+	.check_addr_change = sctp_v4_check_addr_change,
  	.net_header_len	   = sizeof(struct iphdr),
  	.sockaddr_len	   = sizeof(struct sockaddr_in),
  #ifdef CONFIG_COMPAT



> Thanks
> -vlad
>
>>
>> -vlad
>>>
>>> So please pronounce a final judgment.
>>>
>>>> -vlad
>>>>
>>>>>
>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>> index cd89510..0a646a5 100644
>>>>> --- a/include/net/sctp/sctp.h
>>>>> +++ b/include/net/sctp/sctp.h
>>>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>>>> dst_release(t->dst);
>>>>> t->dst = NULL;
>>>>> }
>>>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>>>> index 1bd4c41..cafdd19 100644
>>>>> --- a/include/net/sctp/structs.h
>>>>> +++ b/include/net/sctp/structs.h
>>>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>>>> __u64 hb_nonce;
>>>>>
>>>>> struct rcu_head rcu;
>>>>> +
>>>>> + u32 dst_cookie;
>>>>> };
>>>>>
>>>>> 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 8ee553b..82a420f 100644
>>>>> --- a/net/sctp/ipv6.c
>>>>> +++ b/net/sctp/ipv6.c
>>>>> @@ -350,6 +350,7 @@ out:
>>>>> struct rt6_info *rt;
>>>>> rt = (struct rt6_info *)dst;
>>>>> t->dst = dst;
>>>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>>>> : 0;
>>>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>>>> } else {
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>>
>>>>>>> return t->dst;
>>>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>>>> index 8ee553b..cfae77e 100644
>>>>>>> --- a/net/sctp/ipv6.c
>>>>>>> +++ b/net/sctp/ipv6.c
>>>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>>>> notifier_block *this, unsigned long ev,
>>>>>>> break;
>>>>>>> }
>>>>>>>
>>>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>>>> + rt_genid_bump(net);
>>>>>>> return NOTIFY_DONE;
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-10  5:26             ` Fan Du
@ 2013-07-12 11:19               ` Neil Horman
  2013-07-16  9:13                 ` Fan Du
  2013-07-13 12:21               ` Vlad Yasevich
  1 sibling, 1 reply; 24+ messages in thread
From: Neil Horman @ 2013-07-12 11:19 UTC (permalink / raw)
  To: Fan Du; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev

On Wed, Jul 10, 2013 at 01:26:51PM +0800, Fan Du wrote:
> 
> 
> On 2013年07月09日 23:11, Vlad Yasevich wrote:
> >On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
> >>On 07/03/2013 10:33 PM, Fan Du wrote:
> >>>
> >>>
> >>>On 2013年07月03日 21:23, Vlad Yasevich wrote:
> >>>>On 07/02/2013 10:18 PM, Fan Du wrote:
> >>>>>
> >>>>>
> >>>>>On 2013年07月02日 22:29, Vlad Yasevich wrote:
> >>>>>>On 07/02/2013 02:39 AM, Fan Du wrote:
> >>>>>>>When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
> >>>>>>>as a result ip6_dst_check always fail out. This behaviour makes
> >>>>>>>transport->dst useless, because every sctp_packet_transmit must look
> >>>>>>>for valid dst(Is this what supposed to be?)
> >>>>>>>
> >>>>>>>One aggressive way is to call rt_genid_bump which invalid all dst to
> >>>>>>>make new dst for transport, apparently it also hurts others.
> >>>>>>>I'm sure this may not be the best for all, so any commnets?
> >>>>>>>
> >>>>>>>Signed-off-by: Fan Du <fan.du@windriver.com>
> >>>>>>>---
> >>>>>>>include/net/sctp/sctp.h | 18 ++++++++++++------
> >>>>>>>net/sctp/ipv6.c | 2 ++
> >>>>>>>2 files changed, 14 insertions(+), 6 deletions(-)
> >>>>>>>
> >>>>>>>diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> >>>>>>>index cd89510..f05af01 100644
> >>>>>>>--- a/include/net/sctp/sctp.h
> >>>>>>>+++ b/include/net/sctp/sctp.h
> >>>>>>>@@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
> >>>>>>>sctp_addr *addr)
> >>>>>>>addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
> >>>>>>>}
> >>>>>>>
> >>>>>>>-/* The cookie is always 0 since this is how it's used in the
> >>>>>>>- * pmtu code.
> >>>>>>>- */
> >>>>>>>+/* Set cookie with the right one for IPv6 and zero for others */
> >>>>>>>static inline struct dst_entry *sctp_transport_dst_check(struct
> >>>>>>>sctp_transport *t)
> >>>>>>>{
> >>>>>>>- if (t->dst && !dst_check(t->dst, 0)) {
> >>>>>>>- dst_release(t->dst);
> >>>>>>>- t->dst = NULL;
> >>>>>>>+
> >>>>>>>+ if (t->dst) {
> >>>>>>>+ struct rt6_info *rt = (struct rt6_info *)t->dst;
> >>>>>>>+ u32 cookie = 0;
> >>>>>>>+
> >>>>>>>+ if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
> >>>>>>>+ cookie = rt->rt6i_node->fn_sernum;
> >>>>>>>+ if (!dst_check(t->dst, cookie)) {
> >>>>>>>+ dst_release(t->dst);
> >>>>>>>+ t->dst = NULL;
> >>>>>>>+ }
> >>>>>>>}
> >>>>>>
> >>>>>>I think it would be better if we stored the dst_cookie in the
> >>>>>>transport structure and initialized it at lookup time. If you do that,
> >>>>>>then if the route table changes, we'd correctly detect it without
> >>>>>>artificially bumping rt_genid (and hurting ipv4).
> >>>>>
> >>>>>Hi Vlad/Neil
> >>>>>
> >>>>>Is this what you mean?
> >>>>
> >>>>Yes, exactly.
> >>>>
> >>>
> >>>Hi Vlad
> >>>
> >>>I thinks twice about below patch, this is actually a chicken-egg issue.
> >>>Look below scenario:
> >>>(1) The first time we push packet through a transport, dst_cookie is 0,
> >>>so sctp_transport_dst_check also pass cookie as 0, then return dst
> >>>as NULL.
> >>>Then we lookup dst by sctp_transport_route, and in there we
> >>>initiate dst_cookie
> >>>with rt->rt6i_node->fn_sernum
> >>>
> >>>(2) Then the next time we push packet through this transport again,
> >>>we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
> >>>return valid dst without bothering to lookup dst again.
> >>
> >>No, if the route was removed rt6i_node will be NULL, and NULL will be
> >>returned from ip6_dst_check(). If the route still exists then we'll
> >>compare the serial number with a cookie.
> >>
> >>>
> >>>BUT, suppose when deleting the source address of this dst after
> >>>transport->dst_cookie
> >>>has been well initialized. transport->dst_cookie still holds
> >>>rt->rt6i_node->fn_sernum,
> >>>meaning ip6_dst_check will return valid dst, which it shouldn't in this
> >>>case, the
> >>>result will be association ABORT.
> >>
> >>No, removing the address cause the route for that prefix to be removed
> >>as well. This will set rt6i_node to NULL.
> >>
> >>>
> >>>Other way is invalid all transport->dst which using the deleting address
> >>>as source address
> >>>without bumping gen_id, problem is the traverse times depends heavily on
> >>>transport number,
> >>>and also need to take account locking issue it will introduce.
> >>>
> >>>>
> >>>> No, you are not missing anything. IPv4 doesn't use the cookie and
> >>>always seems to pass it as 0.
> >>>>
> >>>> Yes, ipv4 will bump the gen_id thus invalidating all routes (there
> >>>has been disagreement about it).
> >>>> IPv6 doesn't do that. In ipv6, when the addresses are added or
> >>>removed, routes are also added or removed and
> >>>> any time the route is added it will have a new serial number. So, you
> >>>don't have to disturb ipv4 cache when ipv6 routing info changes.
> >>>
> >>>Thank you very much for your explanation!
> >>>
> >>>IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
> >>>number with each route.
> >>>Doing this way loose the semantic of dst_check, because SCTP depends no
> >>>dst_check fulfill its
> >>>duty to actually check whether the holding dst is still valid, well most
> >>>other Layer 4 protocol
> >>>simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
> >>>time sending data out.
> >>
> >>Look at how other protocols (tcp, dccp) do this. It is sufficient to
> >>cache the route serial number into the dst_cookie at the time the route
> >>was lookup-up and cached. Then the cookie is passed to dst_check to
> >>validate the route.
> >
> >
> >Hi Fan
> >
> >Have you tried the updated patch you sent? Based on what the tcp/udp code is doing, the updated patch should work correctly. If it does, can you re-post with attribution/sign-off
> >
> 
> Hello Vlad and Neil
> 
> I don't know whether my test procedure has drawbacks or something else.
> It seems the updated patch does not works well, but my first patch is ok under below configuration.
> 
> Host A:
> 	ifconfig eth1 inet6 add 2001::803/64
> 	ifconfig eth1 inet6 add 2001::804/64
> 	sctp_darn -H 2001::803 -B 2001::804 -P 5001 -l
> 
> Host B:
> 	ifconfig eth1 inet6 add 2001::805/64
> 	ifconfig eth1 inet6 add 2001::806/64
>         sctp_darn -H 2001::805 -B 2001::806 -P 5002 -h 2001::803 -p 5001 -s
> 
> hbinterval set to 2 seconds, after setup the association, primary address: 2001::804 <--> 2001::806
> 
> 
> Step 1:
> 
> After adding in IPv6 address to an interface, we populate
> struct inet6_ifaddr *ifp->rt->rt6i_node, this rt6i_node is stored in a radix tree.
> 
> addrconf_add_ifaddr
>  ->inet6_addr_add
>     ->ipv6_add_addr  <-- Do DAD checking afterward.
>     ->addrconf_prefix_route
>       ->ip6_route_add
>         ->__ip6_ins_rt
>           ->fib6_add
>             ->fib6_add_1  <-- Create struct fib6_node *fn
>               ->fib6_add_rt2node
>                 ->rt->rt6i_node = fn; (*1*)
> 
> Step 2:
> 
> In host A ,for the packet destinate for 2001::806 using source address 2001::804,
> In sctp_v6_get_dst, let's print the rt, rt->rt6i_node, and rt->rt6i_node->fn_sernum
> after ip6_dst_lookup_flow got valid dst. The problem is the transport->dst->rt6i_node
> we have looked for is *not* the rt6i_node in (*1*), I'm not drunk here.....
> So in Step 3, let's be naughty: ifconfig eth1 inet6 del 2001::804/64
> 
Wait, don't just skip past here.  As far as I know the dst_entry that gets
cached in transport->dst really needs to point at the same rt6_info and
rt6i_node as the one created in step 1.  If it doesn't then that second check in
ip6_dst_check is pointless and will never work.  This really needs to be the
question here.  What is the result of your call to sctp_transport_route
returning if not the route matching the one added for the source interface?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-12  3:15             ` Fan Du
@ 2013-07-12 22:58               ` David Miller
  2013-07-13 12:18                 ` Vlad Yasevich
  2013-07-16  9:58                 ` Fan Du
  0 siblings, 2 replies; 24+ messages in thread
From: David Miller @ 2013-07-12 22:58 UTC (permalink / raw)
  To: fan.du; +Cc: vyasevich, nhorman, nicolas.dichtel, netdev

From: Fan Du <fan.du@windriver.com>
Date: Fri, 12 Jul 2013 11:15:10 +0800

> But problem still arise when we attempt to delete address
> in multi-home mode, deleting an IPv6 address does not invalidate
> any dst which source address is the same at the deleted one.
> Which means sctp cannot rely on ip6_dst_check in this scenario.

I still cannot understand why this is an SCTP specific issue.

Specifically, I cannot see why address addition/deletion doesn't
cause problems for cached ipv6 routes in UDP and TCP sockets too.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-12 22:58               ` David Miller
@ 2013-07-13 12:18                 ` Vlad Yasevich
  2013-07-16  9:58                 ` Fan Du
  1 sibling, 0 replies; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-13 12:18 UTC (permalink / raw)
  To: David Miller; +Cc: fan.du, nhorman, nicolas.dichtel, netdev

On 07/12/2013 06:58 PM, David Miller wrote:
> From: Fan Du <fan.du@windriver.com>
> Date: Fri, 12 Jul 2013 11:15:10 +0800
>
>> But problem still arise when we attempt to delete address
>> in multi-home mode, deleting an IPv6 address does not invalidate
>> any dst which source address is the same at the deleted one.
>> Which means sctp cannot rely on ip6_dst_check in this scenario.
>
> I still cannot understand why this is an SCTP specific issue.
>
> Specifically, I cannot see why address addition/deletion doesn't
> cause problems for cached ipv6 routes in UDP and TCP sockets too.
>

Trying to figure this out.

-vlad

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-10  5:26             ` Fan Du
  2013-07-12 11:19               ` Neil Horman
@ 2013-07-13 12:21               ` Vlad Yasevich
  2013-07-17  7:04                 ` Fan Du
  1 sibling, 1 reply; 24+ messages in thread
From: Vlad Yasevich @ 2013-07-13 12:21 UTC (permalink / raw)
  To: Fan Du; +Cc: nhorman, nicolas.dichtel, davem, netdev

On 07/10/2013 01:26 AM, Fan Du wrote:
>
>
> On 2013年07月09日 23:11, Vlad Yasevich wrote:
>> On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
>>> On 07/03/2013 10:33 PM, Fan Du wrote:
>>>>
>>>>
>>>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>>>
>>>>>>
>>>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as
>>>>>>>> ZERO,
>>>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>>>> transport->dst useless, because every sctp_packet_transmit must
>>>>>>>> look
>>>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>>>
>>>>>>>> One aggressive way is to call rt_genid_bump which invalid all
>>>>>>>> dst to
>>>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>>>
>>>>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>>>>> ---
>>>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>>> index cd89510..f05af01 100644
>>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>>>> sctp_addr *addr)
>>>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>>>> }
>>>>>>>>
>>>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>>>> - * pmtu code.
>>>>>>>> - */
>>>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>>>> sctp_transport *t)
>>>>>>>> {
>>>>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>>>>> - dst_release(t->dst);
>>>>>>>> - t->dst = NULL;
>>>>>>>> +
>>>>>>>> + if (t->dst) {
>>>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>>>> + u32 cookie = 0;
>>>>>>>> +
>>>>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>>>> + dst_release(t->dst);
>>>>>>>> + t->dst = NULL;
>>>>>>>> + }
>>>>>>>> }
>>>>>>>
>>>>>>> I think it would be better if we stored the dst_cookie in the
>>>>>>> transport structure and initialized it at lookup time. If you do
>>>>>>> that,
>>>>>>> then if the route table changes, we'd correctly detect it without
>>>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>>>
>>>>>> Hi Vlad/Neil
>>>>>>
>>>>>> Is this what you mean?
>>>>>
>>>>> Yes, exactly.
>>>>>
>>>>
>>>> Hi Vlad
>>>>
>>>> I thinks twice about below patch, this is actually a chicken-egg issue.
>>>> Look below scenario:
>>>> (1) The first time we push packet through a transport, dst_cookie is 0,
>>>> so sctp_transport_dst_check also pass cookie as 0, then return dst
>>>> as NULL.
>>>> Then we lookup dst by sctp_transport_route, and in there we
>>>> initiate dst_cookie
>>>> with rt->rt6i_node->fn_sernum
>>>>
>>>> (2) Then the next time we push packet through this transport again,
>>>> we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>>> return valid dst without bothering to lookup dst again.
>>>
>>> No, if the route was removed rt6i_node will be NULL, and NULL will be
>>> returned from ip6_dst_check(). If the route still exists then we'll
>>> compare the serial number with a cookie.
>>>
>>>>
>>>> BUT, suppose when deleting the source address of this dst after
>>>> transport->dst_cookie
>>>> has been well initialized. transport->dst_cookie still holds
>>>> rt->rt6i_node->fn_sernum,
>>>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>>>> case, the
>>>> result will be association ABORT.
>>>
>>> No, removing the address cause the route for that prefix to be removed
>>> as well. This will set rt6i_node to NULL.
>>>
>>>>
>>>> Other way is invalid all transport->dst which using the deleting
>>>> address
>>>> as source address
>>>> without bumping gen_id, problem is the traverse times depends
>>>> heavily on
>>>> transport number,
>>>> and also need to take account locking issue it will introduce.
>>>>
>>>> >
>>>> > No, you are not missing anything. IPv4 doesn't use the cookie and
>>>> always seems to pass it as 0.
>>>> >
>>>> > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>>>> has been disagreement about it).
>>>> > IPv6 doesn't do that. In ipv6, when the addresses are added or
>>>> removed, routes are also added or removed and
>>>> > any time the route is added it will have a new serial number. So, you
>>>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>>>
>>>> Thank you very much for your explanation!
>>>>
>>>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>>>> number with each route.
>>>> Doing this way loose the semantic of dst_check, because SCTP depends no
>>>> dst_check fulfill its
>>>> duty to actually check whether the holding dst is still valid, well
>>>> most
>>>> other Layer 4 protocol
>>>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>>>> time sending data out.
>>>
>>> Look at how other protocols (tcp, dccp) do this. It is sufficient to
>>> cache the route serial number into the dst_cookie at the time the route
>>> was lookup-up and cached. Then the cookie is passed to dst_check to
>>> validate the route.
>>
>>
>> Hi Fan
>>
>> Have you tried the updated patch you sent? Based on what the tcp/udp
>> code is doing, the updated patch should work correctly. If it does,
>> can you re-post with attribution/sign-off
>>
>
> Hello Vlad and Neil
>
> I don't know whether my test procedure has drawbacks or something else.
> It seems the updated patch does not works well, but my first patch is ok
> under below configuration.
>
> Host A:
>      ifconfig eth1 inet6 add 2001::803/64
>      ifconfig eth1 inet6 add 2001::804/64
>      sctp_darn -H 2001::803 -B 2001::804 -P 5001 -l
>
> Host B:
>      ifconfig eth1 inet6 add 2001::805/64
>      ifconfig eth1 inet6 add 2001::806/64
>          sctp_darn -H 2001::805 -B 2001::806 -P 5002 -h 2001::803 -p

Hi Fan

Could you try using different prefixes.  I think we are running into 
some routing issue.

For example, configure 2001:1::803/64 and 2001:2::804/64.  I think 
that's going to make this work (it seems to for me).

-vlad
> 5001 -s
>
> hbinterval set to 2 seconds, after setup the association, primary
> address: 2001::804 <--> 2001::806
>
>
> Step 1:
>
> After adding in IPv6 address to an interface, we populate
> struct inet6_ifaddr *ifp->rt->rt6i_node, this rt6i_node is stored in a
> radix tree.
>
> addrconf_add_ifaddr
>   ->inet6_addr_add
>      ->ipv6_add_addr  <-- Do DAD checking afterward.
>      ->addrconf_prefix_route
>        ->ip6_route_add
>          ->__ip6_ins_rt
>            ->fib6_add
>              ->fib6_add_1  <-- Create struct fib6_node *fn
>                ->fib6_add_rt2node
>                  ->rt->rt6i_node = fn; (*1*)
>
> Step 2:
>
> In host A ,for the packet destinate for 2001::806 using source address
> 2001::804,
> In sctp_v6_get_dst, let's print the rt, rt->rt6i_node, and
> rt->rt6i_node->fn_sernum
> after ip6_dst_lookup_flow got valid dst. The problem is the
> transport->dst->rt6i_node
> we have looked for is *not* the rt6i_node in (*1*), I'm not drunk here.....
> So in Step 3, let's be naughty: ifconfig eth1 inet6 del 2001::804/64
>
>
> Step 3:
>
> Then we delete the IPv6 address, certainly this struct inet6_ifaddr
> *ifp->rt
> will be delete as well.
>
> addrconf_del_ifaddr
>    ->inet6_addr_del
>      ->ipv6_del_addr
>        ->ipv6_ifa_notify /RTM_DELADDR
>          ->ip6_del_rt
>            ->__ip6_del_rt
>              ->fib6_del
>                ->fib6_del_route
>                 -> rt->rt6i_node = NULL; (*2*) here we undo the
> operation in (*1*),
>                                                but rt6i_node in Step2 is
> untouched!
>
> Step 4:
> Then we do the dst checking, for sctp_tranport_dst_check it looks like
> nothing in Step3
> has consequence on it. So transport->dst is still valid, let's ship the
> packet out
> using 2001::804. The real world test result is ASSOCIATION ABORT after a
> while.
> As far as I can tell, rt in Step 2 got deleted *after* the ASSOCIATION
> ABORT.
>
>     sctp_tranport_dst_check
>        ->dst_check
>          -> ip6_dst_check
>
>
> Root node
>     *
>    / \
>       * <-- fn node we are using.
>      / \
>         *
>        / \
>       *
>      / \
>         * <--- Add a new fn node, on the path from the root node down to
> here, each fn
>                now has new fn_sernum, which force dst_check return NULL
> for lookup again.
>                Then previously save fn_sernum will take effect now. But
> when delete address
>                only the ifp->rt->rt6i_node is set to NULL.
>
>
>> Thanks
>> -vlad
>>
>>>
>>> -vlad
>>>>
>>>> So please pronounce a final judgment.
>>>>
>>>>> -vlad
>>>>>
>>>>>>
>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>> index cd89510..0a646a5 100644
>>>>>> --- a/include/net/sctp/sctp.h
>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>>>>> dst_release(t->dst);
>>>>>> t->dst = NULL;
>>>>>> }
>>>>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>>>>> index 1bd4c41..cafdd19 100644
>>>>>> --- a/include/net/sctp/structs.h
>>>>>> +++ b/include/net/sctp/structs.h
>>>>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>>>>> __u64 hb_nonce;
>>>>>>
>>>>>> struct rcu_head rcu;
>>>>>> +
>>>>>> + u32 dst_cookie;
>>>>>> };
>>>>>>
>>>>>> 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 8ee553b..82a420f 100644
>>>>>> --- a/net/sctp/ipv6.c
>>>>>> +++ b/net/sctp/ipv6.c
>>>>>> @@ -350,6 +350,7 @@ out:
>>>>>> struct rt6_info *rt;
>>>>>> rt = (struct rt6_info *)dst;
>>>>>> t->dst = dst;
>>>>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>>>>> : 0;
>>>>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>>>>> } else {
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -vlad
>>>>>>>
>>>>>>>>
>>>>>>>> return t->dst;
>>>>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>>>>> index 8ee553b..cfae77e 100644
>>>>>>>> --- a/net/sctp/ipv6.c
>>>>>>>> +++ b/net/sctp/ipv6.c
>>>>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>>>>> notifier_block *this, unsigned long ev,
>>>>>>>> break;
>>>>>>>> }
>>>>>>>>
>>>>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>>>>> + rt_genid_bump(net);
>>>>>>>> return NOTIFY_DONE;
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-12 11:19               ` Neil Horman
@ 2013-07-16  9:13                 ` Fan Du
  0 siblings, 0 replies; 24+ messages in thread
From: Fan Du @ 2013-07-16  9:13 UTC (permalink / raw)
  To: Neil Horman; +Cc: Vlad Yasevich, nicolas.dichtel, davem, netdev


On 2013年07月12日 19:19, Neil Horman wrote:
> On Wed, Jul 10, 2013 at 01:26:51PM +0800, Fan Du wrote:
>>
>>
>> On 2013年07月09日 23:11, Vlad Yasevich wrote:
>>> On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
>>>> On 07/03/2013 10:33 PM, Fan Du wrote:
>>>>>
>>>>>
>>>>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>>>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as ZERO,
>>>>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>>>>> transport->dst useless, because every sctp_packet_transmit must look
>>>>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>>>>
>>>>>>>>> One aggressive way is to call rt_genid_bump which invalid all dst to
>>>>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>>>>
>>>>>>>>> Signed-off-by: Fan Du<fan.du@windriver.com>
>>>>>>>>> ---
>>>>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>>>> index cd89510..f05af01 100644
>>>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>>>>> sctp_addr *addr)
>>>>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>>>>> - * pmtu code.
>>>>>>>>> - */
>>>>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>>>>> sctp_transport *t)
>>>>>>>>> {
>>>>>>>>> - if (t->dst&&  !dst_check(t->dst, 0)) {
>>>>>>>>> - dst_release(t->dst);
>>>>>>>>> - t->dst = NULL;
>>>>>>>>> +
>>>>>>>>> + if (t->dst) {
>>>>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>>>>> + u32 cookie = 0;
>>>>>>>>> +
>>>>>>>>> + if ((t->af_specific->sa_family == AF_INET6)&&  rt->rt6i_node)
>>>>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>>>>> + dst_release(t->dst);
>>>>>>>>> + t->dst = NULL;
>>>>>>>>> + }
>>>>>>>>> }
>>>>>>>>
>>>>>>>> I think it would be better if we stored the dst_cookie in the
>>>>>>>> transport structure and initialized it at lookup time. If you do that,
>>>>>>>> then if the route table changes, we'd correctly detect it without
>>>>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>>>>
>>>>>>> Hi Vlad/Neil
>>>>>>>
>>>>>>> Is this what you mean?
>>>>>>
>>>>>> Yes, exactly.
>>>>>>
>>>>>
>>>>> Hi Vlad
>>>>>
>>>>> I thinks twice about below patch, this is actually a chicken-egg issue.
>>>>> Look below scenario:
>>>>> (1) The first time we push packet through a transport, dst_cookie is 0,
>>>>> so sctp_transport_dst_check also pass cookie as 0, then return dst
>>>>> as NULL.
>>>>> Then we lookup dst by sctp_transport_route, and in there we
>>>>> initiate dst_cookie
>>>>> with rt->rt6i_node->fn_sernum
>>>>>
>>>>> (2) Then the next time we push packet through this transport again,
>>>>> we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>>>> return valid dst without bothering to lookup dst again.
>>>>
>>>> No, if the route was removed rt6i_node will be NULL, and NULL will be
>>>> returned from ip6_dst_check(). If the route still exists then we'll
>>>> compare the serial number with a cookie.
>>>>
>>>>>
>>>>> BUT, suppose when deleting the source address of this dst after
>>>>> transport->dst_cookie
>>>>> has been well initialized. transport->dst_cookie still holds
>>>>> rt->rt6i_node->fn_sernum,
>>>>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>>>>> case, the
>>>>> result will be association ABORT.
>>>>
>>>> No, removing the address cause the route for that prefix to be removed
>>>> as well. This will set rt6i_node to NULL.
>>>>
>>>>>
>>>>> Other way is invalid all transport->dst which using the deleting address
>>>>> as source address
>>>>> without bumping gen_id, problem is the traverse times depends heavily on
>>>>> transport number,
>>>>> and also need to take account locking issue it will introduce.
>>>>>
>>>>>>
>>>>>> No, you are not missing anything. IPv4 doesn't use the cookie and
>>>>> always seems to pass it as 0.
>>>>>>
>>>>>> Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>>>>> has been disagreement about it).
>>>>>> IPv6 doesn't do that. In ipv6, when the addresses are added or
>>>>> removed, routes are also added or removed and
>>>>>> any time the route is added it will have a new serial number. So, you
>>>>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>>>>
>>>>> Thank you very much for your explanation!
>>>>>
>>>>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>>>>> number with each route.
>>>>> Doing this way loose the semantic of dst_check, because SCTP depends no
>>>>> dst_check fulfill its
>>>>> duty to actually check whether the holding dst is still valid, well most
>>>>> other Layer 4 protocol
>>>>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>>>>> time sending data out.
>>>>
>>>> Look at how other protocols (tcp, dccp) do this. It is sufficient to
>>>> cache the route serial number into the dst_cookie at the time the route
>>>> was lookup-up and cached. Then the cookie is passed to dst_check to
>>>> validate the route.
>>>
>>>
>>> Hi Fan
>>>
>>> Have you tried the updated patch you sent? Based on what the tcp/udp code is doing, the updated patch should work correctly. If it does, can you re-post with attribution/sign-off
>>>
>>
>> Hello Vlad and Neil
>>
>> I don't know whether my test procedure has drawbacks or something else.
>> It seems the updated patch does not works well, but my first patch is ok under below configuration.
>>
>> Host A:
>> 	ifconfig eth1 inet6 add 2001::803/64
>> 	ifconfig eth1 inet6 add 2001::804/64
>> 	sctp_darn -H 2001::803 -B 2001::804 -P 5001 -l
>>
>> Host B:
>> 	ifconfig eth1 inet6 add 2001::805/64
>> 	ifconfig eth1 inet6 add 2001::806/64
>>          sctp_darn -H 2001::805 -B 2001::806 -P 5002 -h 2001::803 -p 5001 -s
>>
>> hbinterval set to 2 seconds, after setup the association, primary address: 2001::804<-->  2001::806
>>
>>
>> Step 1:
>>
>> After adding in IPv6 address to an interface, we populate
>> struct inet6_ifaddr *ifp->rt->rt6i_node, this rt6i_node is stored in a radix tree.
>>
>> addrconf_add_ifaddr
>>   ->inet6_addr_add
>>      ->ipv6_add_addr<-- Do DAD checking afterward.
>>      ->addrconf_prefix_route
>>        ->ip6_route_add
>>          ->__ip6_ins_rt
>>            ->fib6_add
>>              ->fib6_add_1<-- Create struct fib6_node *fn
>>                ->fib6_add_rt2node
>>                  ->rt->rt6i_node = fn; (*1*)
>>
>> Step 2:
>>
>> In host A ,for the packet destinate for 2001::806 using source address 2001::804,
>> In sctp_v6_get_dst, let's print the rt, rt->rt6i_node, and rt->rt6i_node->fn_sernum
>> after ip6_dst_lookup_flow got valid dst. The problem is the transport->dst->rt6i_node
>> we have looked for is *not* the rt6i_node in (*1*), I'm not drunk here.....
>> So in Step 3, let's be naughty: ifconfig eth1 inet6 del 2001::804/64
>>
> Wait, don't just skip past here.  As far as I know the dst_entry that gets
> cached in transport->dst really needs to point at the same rt6_info and
> rt6i_node as the one created in step 1.  If it doesn't then that second check in
> ip6_dst_check is pointless and will never work.  This really needs to be the
> question here.  What is the result of your call to sctp_transport_route
> returning if not the route matching the one added for the source interface?

Hello Neil

First, sorry for not respond quickly, busy with debugging。。。

 > Wait, don't just skip past here.  As far as I know the dst_entry that gets
 > cached in transport->dst really needs to point at the same rt6_info and
 > rt6i_node as the one created in step 1.

I'm afraid I can not agree with above statement.
Below is the IPv6 binary tree route table with small notes of which step generate which node.
Take a look at (4) when host respond with INIT_ACK, destination address is 2001::806 with rt
different than that of rt in (1), that's because rt6_alloc_cow/rt6_alloc_clone may replicate
new rt for the node. And another point is this tree is created using destination address,
which means if I delete node(1), node(4) is intact, and the source address is still 2001::804
there to reach 2001::806/128 for node(4).


     (1) 2001::804 DAD complete
     (2) 2001::803 DAD complete
     (3) 2001::806 -> 2001::803 SCTP_INIT
     (4) 2001::804 -> 2001::806 INIT_ACK
                                                       root:ffff880037914560
                                                      /
                                                     /
                                      ffff88002103f000
                                     /                 \
                                    /                   \
        rt:ffff88002d47f600 (::1/128)                   (2001:/64)
                     ffff880037a0a4c0                   ffff88002103ffc0
                                                       /
                                                      /
                                       ffff88002103c380
                                     /                  \
                         (2) (3)    /                    \
rt:ffff880021091a80 (2001::803/128)                      ffff880037ac07c0
                    ffff88002103c240                     /               \
                                                        /                 \
                                                       /                   \  (4)
                                                      /                    (2001::806/128)rt:ffff8800243ce900
                                            ffff880037ac0640               ffff880037ac0480
                                            /             \
                                 (1)       /               \
        rt:ffff8800210af780 (2001::804/128)                (2001::805/128) rt:ffff88002c980000
                           ffff88002103c080                ffff880037ac0f80


-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-12 22:58               ` David Miller
  2013-07-13 12:18                 ` Vlad Yasevich
@ 2013-07-16  9:58                 ` Fan Du
  1 sibling, 0 replies; 24+ messages in thread
From: Fan Du @ 2013-07-16  9:58 UTC (permalink / raw)
  To: David Miller; +Cc: vyasevich, nhorman, nicolas.dichtel, netdev



On 2013年07月13日 06:58, David Miller wrote:
> From: Fan Du<fan.du@windriver.com>
> Date: Fri, 12 Jul 2013 11:15:10 +0800
>
>> But problem still arise when we attempt to delete address
>> in multi-home mode, deleting an IPv6 address does not invalidate
>> any dst which source address is the same at the deleted one.
>> Which means sctp cannot rely on ip6_dst_check in this scenario.
>
> I still cannot understand why this is an SCTP specific issue.

It's not SCTP specific, it's shared by all all layer 4 protocol IMHO.
The issue of SCTP IPv6 doesn't check IPv6 dst validness has been addressed
using *only* dst_cookie as other layer 4 protocol does for its sock.
But this scheme cannot cover scenario when delete primary address to support
SCTP multi-home feature, this is where the concern is.

Use netsend to send a large file using DCCP, considering the sender
host has two IPv6 address, while sending, delete the one netsend currently
using. Wireshark could catch the sender is still transmit packet out using
the deleted address in a slowly manner.

All of those boils down to one question that I cannot resist to ask:
If delete an IPv6 address(*1*), whether the original rt/dst destinate for a
remote address(*2*) using the deleted address as source address is still legal
for subsequent usage in current kernel IPv6 routing implementation???

btw, (*1*) and (*2*) are on quite different node in the binary tree as well
as different leaf.

> Specifically, I cannot see why address addition/deletion doesn't
> cause problems for cached ipv6 routes in UDP and TCP sockets too.
>

-- 
浮沉随浪只记今朝笑

--fan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid
  2013-07-13 12:21               ` Vlad Yasevich
@ 2013-07-17  7:04                 ` Fan Du
  0 siblings, 0 replies; 24+ messages in thread
From: Fan Du @ 2013-07-17  7:04 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: nhorman, nicolas.dichtel, davem, netdev

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



On 2013年07月13日 20:21, Vlad Yasevich wrote:
> On 07/10/2013 01:26 AM, Fan Du wrote:
>>
>>
>> On 2013年07月09日 23:11, Vlad Yasevich wrote:
>>> On 07/05/2013 10:09 AM, Vlad Yasevich wrote:
>>>> On 07/03/2013 10:33 PM, Fan Du wrote:
>>>>>
>>>>>
>>>>> On 2013年07月03日 21:23, Vlad Yasevich wrote:
>>>>>> On 07/02/2013 10:18 PM, Fan Du wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2013年07月02日 22:29, Vlad Yasevich wrote:
>>>>>>>> On 07/02/2013 02:39 AM, Fan Du wrote:
>>>>>>>>> When sctp sits on IPv6, sctp_transport_dst_check pass cookie as
>>>>>>>>> ZERO,
>>>>>>>>> as a result ip6_dst_check always fail out. This behaviour makes
>>>>>>>>> transport->dst useless, because every sctp_packet_transmit must
>>>>>>>>> look
>>>>>>>>> for valid dst(Is this what supposed to be?)
>>>>>>>>>
>>>>>>>>> One aggressive way is to call rt_genid_bump which invalid all
>>>>>>>>> dst to
>>>>>>>>> make new dst for transport, apparently it also hurts others.
>>>>>>>>> I'm sure this may not be the best for all, so any commnets?
>>>>>>>>>
>>>>>>>>> Signed-off-by: Fan Du <fan.du@windriver.com>
>>>>>>>>> ---
>>>>>>>>> include/net/sctp/sctp.h | 18 ++++++++++++------
>>>>>>>>> net/sctp/ipv6.c | 2 ++
>>>>>>>>> 2 files changed, 14 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>>>> index cd89510..f05af01 100644
>>>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>>>> @@ -719,14 +719,20 @@ static inline void sctp_v4_map_v6(union
>>>>>>>>> sctp_addr *addr)
>>>>>>>>> addr->v6.sin6_addr.s6_addr32[2] = htonl(0x0000ffff);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> -/* The cookie is always 0 since this is how it's used in the
>>>>>>>>> - * pmtu code.
>>>>>>>>> - */
>>>>>>>>> +/* Set cookie with the right one for IPv6 and zero for others */
>>>>>>>>> static inline struct dst_entry *sctp_transport_dst_check(struct
>>>>>>>>> sctp_transport *t)
>>>>>>>>> {
>>>>>>>>> - if (t->dst && !dst_check(t->dst, 0)) {
>>>>>>>>> - dst_release(t->dst);
>>>>>>>>> - t->dst = NULL;
>>>>>>>>> +
>>>>>>>>> + if (t->dst) {
>>>>>>>>> + struct rt6_info *rt = (struct rt6_info *)t->dst;
>>>>>>>>> + u32 cookie = 0;
>>>>>>>>> +
>>>>>>>>> + if ((t->af_specific->sa_family == AF_INET6) && rt->rt6i_node)
>>>>>>>>> + cookie = rt->rt6i_node->fn_sernum;
>>>>>>>>> + if (!dst_check(t->dst, cookie)) {
>>>>>>>>> + dst_release(t->dst);
>>>>>>>>> + t->dst = NULL;
>>>>>>>>> + }
>>>>>>>>> }
>>>>>>>>
>>>>>>>> I think it would be better if we stored the dst_cookie in the
>>>>>>>> transport structure and initialized it at lookup time. If you do
>>>>>>>> that,
>>>>>>>> then if the route table changes, we'd correctly detect it without
>>>>>>>> artificially bumping rt_genid (and hurting ipv4).
>>>>>>>
>>>>>>> Hi Vlad/Neil
>>>>>>>
>>>>>>> Is this what you mean?
>>>>>>
>>>>>> Yes, exactly.
>>>>>>
>>>>>
>>>>> Hi Vlad
>>>>>
>>>>> I thinks twice about below patch, this is actually a chicken-egg issue.
>>>>> Look below scenario:
>>>>> (1) The first time we push packet through a transport, dst_cookie is 0,
>>>>> so sctp_transport_dst_check also pass cookie as 0, then return dst
>>>>> as NULL.
>>>>> Then we lookup dst by sctp_transport_route, and in there we
>>>>> initiate dst_cookie
>>>>> with rt->rt6i_node->fn_sernum
>>>>>
>>>>> (2) Then the next time we push packet through this transport again,
>>>>> we pass dst_cookie(rt->rt6i_node->fn_sernum) to ip6_dst_check, and
>>>>> return valid dst without bothering to lookup dst again.
>>>>
>>>> No, if the route was removed rt6i_node will be NULL, and NULL will be
>>>> returned from ip6_dst_check(). If the route still exists then we'll
>>>> compare the serial number with a cookie.
>>>>
>>>>>
>>>>> BUT, suppose when deleting the source address of this dst after
>>>>> transport->dst_cookie
>>>>> has been well initialized. transport->dst_cookie still holds
>>>>> rt->rt6i_node->fn_sernum,
>>>>> meaning ip6_dst_check will return valid dst, which it shouldn't in this
>>>>> case, the
>>>>> result will be association ABORT.
>>>>
>>>> No, removing the address cause the route for that prefix to be removed
>>>> as well. This will set rt6i_node to NULL.
>>>>
>>>>>
>>>>> Other way is invalid all transport->dst which using the deleting
>>>>> address
>>>>> as source address
>>>>> without bumping gen_id, problem is the traverse times depends
>>>>> heavily on
>>>>> transport number,
>>>>> and also need to take account locking issue it will introduce.
>>>>>
>>>>> >
>>>>> > No, you are not missing anything. IPv4 doesn't use the cookie and
>>>>> always seems to pass it as 0.
>>>>> >
>>>>> > Yes, ipv4 will bump the gen_id thus invalidating all routes (there
>>>>> has been disagreement about it).
>>>>> > IPv6 doesn't do that. In ipv6, when the addresses are added or
>>>>> removed, routes are also added or removed and
>>>>> > any time the route is added it will have a new serial number. So, you
>>>>> don't have to disturb ipv4 cache when ipv6 routing info changes.
>>>>>
>>>>> Thank you very much for your explanation!
>>>>>
>>>>> IPv6 don't bump gen_id, when adding/deleting address, and tag an serial
>>>>> number with each route.
>>>>> Doing this way loose the semantic of dst_check, because SCTP depends no
>>>>> dst_check fulfill its
>>>>> duty to actually check whether the holding dst is still valid, well
>>>>> most
>>>>> other Layer 4 protocol
>>>>> simply rely on ip6_route_output/ip6_dst_lookup_flow to grab dst every
>>>>> time sending data out.
>>>>
>>>> Look at how other protocols (tcp, dccp) do this. It is sufficient to
>>>> cache the route serial number into the dst_cookie at the time the route
>>>> was lookup-up and cached. Then the cookie is passed to dst_check to
>>>> validate the route.
>>>
>>>
>>> Hi Fan
>>>
>>> Have you tried the updated patch you sent? Based on what the tcp/udp
>>> code is doing, the updated patch should work correctly. If it does,
>>> can you re-post with attribution/sign-off
>>>
>>
>> Hello Vlad and Neil
>>
>> I don't know whether my test procedure has drawbacks or something else.
>> It seems the updated patch does not works well, but my first patch is ok
>> under below configuration.
>>
>> Host A:
>> ifconfig eth1 inet6 add 2001::803/64
>> ifconfig eth1 inet6 add 2001::804/64
>> sctp_darn -H 2001::803 -B 2001::804 -P 5001 -l
>>
>> Host B:
>> ifconfig eth1 inet6 add 2001::805/64
>> ifconfig eth1 inet6 add 2001::806/64
>> sctp_darn -H 2001::805 -B 2001::806 -P 5002 -h 2001::803 -p
>
> Hi Fan
>
> Could you try using different prefixes. I think we are running into some routing issue.
>
> For example, configure 2001:1::803/64 and 2001:2::804/64. I think that's going to make this work (it seems to for me).

Hello Vlad

By using different prefix as below configuration

      Server           Client
2001:1::803/64  <-> 2001:1::805/64
2001:2::804/64  <-> 2001:2::806/64

After association setup, heartbeat path is different than using same prefix
as before. Yes, delete 2001:2::804/64 won't cause association ABORT. That's
because, delete 2001:2::804/64 will also make the prefix route got deleted as
well, which in turn invalidate rt destinate to 2001:2::806/64. So subsequent
HEART_BEAT checks from 2001:2::804/64 -> 2001:2::806/64 will use ip6_null_entry
as rt, which actually discard packet. See sctp_ipv6_route_2prefix.jpeg for
route table in this configuration.

As is shown in sctp_ipv6_route_same_prefix.jpeg for using same prefix configuration,
Delete 2001::804/64 won't cause prefix route deleted as well as rt in (4) destinate
to 2001::806 with source address as 2001::804/64. That's because 2001::803/64 is
still alive, which make onlink=1 in ipv6_del_addr, this is where the substantial
difference between same prefix configuration and different prefix configuration :)
So packet are still transmitted out to 2001::806 with source address as 2001::804/64.

Current IPv6 route table implementation cannot prudently rudely traverse all the node
below the prefix route to mark new fn_sernum, that's is way too much time-consuming,
neither could we delete all the node below the prefix route, same reason as before.
As you putted earlier, bump_genid is not allowed for IPv6, I think there isn't a better
way here to enforce SCTP multi-home so far than the patch I posted here:
http://marc.info/?l=linux-netdev&m=137359898232255&w=2

Or should I only repost dst_cookie style patch in here ?
http://marc.info/?l=linux-netdev&m=137281805012583&w=2

Thanks for reading this long story...


> -vlad
>> 5001 -s
>>
>> hbinterval set to 2 seconds, after setup the association, primary
>> address: 2001::804 <--> 2001::806
>>
>>
>> Step 1:
>>
>> After adding in IPv6 address to an interface, we populate
>> struct inet6_ifaddr *ifp->rt->rt6i_node, this rt6i_node is stored in a
>> radix tree.
>>
>> addrconf_add_ifaddr
>> ->inet6_addr_add
>> ->ipv6_add_addr <-- Do DAD checking afterward.
>> ->addrconf_prefix_route
>> ->ip6_route_add
>> ->__ip6_ins_rt
>> ->fib6_add
>> ->fib6_add_1 <-- Create struct fib6_node *fn
>> ->fib6_add_rt2node
>> ->rt->rt6i_node = fn; (*1*)
>>
>> Step 2:
>>
>> In host A ,for the packet destinate for 2001::806 using source address
>> 2001::804,
>> In sctp_v6_get_dst, let's print the rt, rt->rt6i_node, and
>> rt->rt6i_node->fn_sernum
>> after ip6_dst_lookup_flow got valid dst. The problem is the
>> transport->dst->rt6i_node
>> we have looked for is *not* the rt6i_node in (*1*), I'm not drunk here.....
>> So in Step 3, let's be naughty: ifconfig eth1 inet6 del 2001::804/64
>>
>>
>> Step 3:
>>
>> Then we delete the IPv6 address, certainly this struct inet6_ifaddr
>> *ifp->rt
>> will be delete as well.
>>
>> addrconf_del_ifaddr
>> ->inet6_addr_del
>> ->ipv6_del_addr
>> ->ipv6_ifa_notify /RTM_DELADDR
>> ->ip6_del_rt
>> ->__ip6_del_rt
>> ->fib6_del
>> ->fib6_del_route
>> -> rt->rt6i_node = NULL; (*2*) here we undo the
>> operation in (*1*),
>> but rt6i_node in Step2 is
>> untouched!
>>
>> Step 4:
>> Then we do the dst checking, for sctp_tranport_dst_check it looks like
>> nothing in Step3
>> has consequence on it. So transport->dst is still valid, let's ship the
>> packet out
>> using 2001::804. The real world test result is ASSOCIATION ABORT after a
>> while.
>> As far as I can tell, rt in Step 2 got deleted *after* the ASSOCIATION
>> ABORT.
>>
>> sctp_tranport_dst_check
>> ->dst_check
>> -> ip6_dst_check
>>
>>
>> Root node
>> *
>> / \
>> * <-- fn node we are using.
>> / \
>> *
>> / \
>> *
>> / \
>> * <--- Add a new fn node, on the path from the root node down to
>> here, each fn
>> now has new fn_sernum, which force dst_check return NULL
>> for lookup again.
>> Then previously save fn_sernum will take effect now. But
>> when delete address
>> only the ifp->rt->rt6i_node is set to NULL.
>>
>>
>>> Thanks
>>> -vlad
>>>
>>>>
>>>> -vlad
>>>>>
>>>>> So please pronounce a final judgment.
>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>>
>>>>>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>>>>>> index cd89510..0a646a5 100644
>>>>>>> --- a/include/net/sctp/sctp.h
>>>>>>> +++ b/include/net/sctp/sctp.h
>>>>>>> @@ -724,7 +724,7 @@ 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, t->dst_cookie)) {
>>>>>>> dst_release(t->dst);
>>>>>>> t->dst = NULL;
>>>>>>> }
>>>>>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>>>>>> index 1bd4c41..cafdd19 100644
>>>>>>> --- a/include/net/sctp/structs.h
>>>>>>> +++ b/include/net/sctp/structs.h
>>>>>>> @@ -946,6 +946,8 @@ struct sctp_transport {
>>>>>>> __u64 hb_nonce;
>>>>>>>
>>>>>>> struct rcu_head rcu;
>>>>>>> +
>>>>>>> + u32 dst_cookie;
>>>>>>> };
>>>>>>>
>>>>>>> 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 8ee553b..82a420f 100644
>>>>>>> --- a/net/sctp/ipv6.c
>>>>>>> +++ b/net/sctp/ipv6.c
>>>>>>> @@ -350,6 +350,7 @@ out:
>>>>>>> struct rt6_info *rt;
>>>>>>> rt = (struct rt6_info *)dst;
>>>>>>> t->dst = dst;
>>>>>>> + t->dst_cookie = rt->rt6i_node ? rt->rt6i_node->fn_sernum
>>>>>>> : 0;
>>>>>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>>>>> &rt->rt6i_dst.addr, &fl6->saddr);
>>>>>>> } else {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> -vlad
>>>>>>>>
>>>>>>>>>
>>>>>>>>> return t->dst;
>>>>>>>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>>>>>>>> index 8ee553b..cfae77e 100644
>>>>>>>>> --- a/net/sctp/ipv6.c
>>>>>>>>> +++ b/net/sctp/ipv6.c
>>>>>>>>> @@ -137,6 +137,8 @@ static int sctp_inet6addr_event(struct
>>>>>>>>> notifier_block *this, unsigned long ev,
>>>>>>>>> break;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> + /* invalid all transport dst forcing to look up new dst */
>>>>>>>>> + rt_genid_bump(net);
>>>>>>>>> return NOTIFY_DONE;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

-- 
浮沉随浪只记今朝笑

--fan

[-- Attachment #2: sctp_ipv6_route_same_prefix.jpeg --]
[-- Type: image/jpeg, Size: 71731 bytes --]

[-- Attachment #3: sctp_ipv6_route_2prefix.jpeg --]
[-- Type: image/jpeg, Size: 79537 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2013-07-17  7:04 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-02  6:39 [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid Fan Du
2013-07-02 14:29 ` Vlad Yasevich
2013-07-02 15:55   ` Neil Horman
2013-07-03  2:39     ` Fan Du
2013-07-03 13:48       ` Vlad Yasevich
2013-07-02 19:47   ` David Miller
2013-07-03  2:18   ` Fan Du
2013-07-03 13:23     ` Vlad Yasevich
2013-07-03 14:11       ` Vlad Yasevich
2013-07-04  2:33       ` Fan Du
2013-07-05 13:03         ` Neil Horman
2013-07-09  7:11           ` Fan Du
2013-07-09 11:38             ` Neil Horman
2013-07-05 14:09         ` Vlad Yasevich
2013-07-09 15:11           ` Vlad Yasevich
2013-07-10  5:26             ` Fan Du
2013-07-12 11:19               ` Neil Horman
2013-07-16  9:13                 ` Fan Du
2013-07-13 12:21               ` Vlad Yasevich
2013-07-17  7:04                 ` Fan Du
2013-07-12  3:15             ` Fan Du
2013-07-12 22:58               ` David Miller
2013-07-13 12:18                 ` Vlad Yasevich
2013-07-16  9:58                 ` Fan Du

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.