Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid

[Vlad Yasevich <vyasevich@gmail.com>]

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;
>>>>   }
>>>>
>>>>
>>>
>>>
>>
>