From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still valid Date: Fri, 05 Jul 2013 10:09:36 -0400 Message-ID: <51D6D3A0.7050106@gmail.com> References: <1372747174-23580-1-git-send-email-fan.du@windriver.com> <51D2E3C8.5050100@gmail.com> <51D389F9.4030804@windriver.com> <51D425B7.20204@gmail.com> <51D4DEFF.8030305@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: nhorman@tuxdriver.com, nicolas.dichtel@6wind.com, davem@davemloft.net, netdev@vger.kernel.org To: Fan Du Return-path: Received: from mail-vc0-f180.google.com ([209.85.220.180]:35130 "EHLO mail-vc0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752356Ab3GEOJq (ORCPT ); Fri, 5 Jul 2013 10:09:46 -0400 Received: by mail-vc0-f180.google.com with SMTP id gf11so1679299vcb.11 for ; Fri, 05 Jul 2013 07:09:45 -0700 (PDT) In-Reply-To: <51D4DEFF.8030305@windriver.com> Sender: netdev-owner@vger.kernel.org List-ID: On 07/03/2013 10:33 PM, Fan Du wrote: > > > On 2013=E5=B9=B407=E6=9C=8803=E6=97=A5 21:23, Vlad Yasevich wrote: >> On 07/02/2013 10:18 PM, Fan Du wrote: >>> >>> >>> On 2013=E5=B9=B407=E6=9C=8802=E6=97=A5 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 Z= ERO, >>>>> as a result ip6_dst_check always fail out. This behaviour makes >>>>> transport->dst useless, because every sctp_packet_transmit must l= ook >>>>> 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 >>>>> --- >>>>> 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] =3D 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 =3D NULL; >>>>> + >>>>> + if (t->dst) { >>>>> + struct rt6_info *rt =3D (struct rt6_info *)t->dst; >>>>> + u32 cookie =3D 0; >>>>> + >>>>> + if ((t->af_specific->sa_family =3D=3D AF_INET6) && rt->rt6i_nod= e) >>>>> + cookie =3D rt->rt6i_node->fn_sernum; >>>>> + if (!dst_check(t->dst, cookie)) { >>>>> + dst_release(t->dst); >>>>> + t->dst =3D 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 t= hat, >>>> 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 issu= e. > Look below scenario: > (1) The first time we push packet through a transport, dst_cookie i= s 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=20 returned from ip6_dst_check(). If the route still exists then we'll=20 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 th= is > case, the > result will be association ABORT. No, removing the address cause the route for that prefix to be removed=20 as well. This will set rt6i_node to NULL. > > Other way is invalid all transport->dst which using the deleting addr= ess > 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 seri= al > 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 m= ost > 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=20 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=20 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_ad= dr >>> *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 =3D 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 =3D (struct rt6_info *)dst; >>> t->dst =3D dst; >>> + t->dst_cookie =3D 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; >>>>> } >>>>> >>>>> >>>> >>>> >>> >> >> >