Message-ID: <20130709113815.GA955@hmsreliant.think-freely.org>
Date:	Tue, 9 Jul 2013 07:38:15 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Fan Du <fan.du@...driver.com>
Cc:	Vlad Yasevich <vyasevich@...il.com>, nicolas.dichtel@...nd.com,
	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [RFC PATCH] sctp: Don't lookup dst if transport dst is still
 valid

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@...driver.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
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists