Message-ID: <20150716140642.GD1855@localhost.localdomain>
Date:	Thu, 16 Jul 2015 11:06:42 -0300
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
	Michael Tuexen <tuexen@...muenster.de>, netdev@...r.kernel.org,
	linux-sctp@...r.kernel.org, Neil Horman <nhorman@...driver.com>
Subject: Re: [RFC PATCH net-next] sctp: fix src address selection if using
 secondary addresses

On Thu, Jul 16, 2015 at 09:09:57AM -0400, Vlad Yasevich wrote:
> On 07/15/2015 03:03 PM, Marcelo Ricardo Leitner wrote:
> > On Fri, Jul 10, 2015 at 03:27:02PM -0300, Marcelo Ricardo Leitner wrote:
> >> On Fri, Jul 10, 2015 at 01:14:21PM -0400, Vlad Yasevich wrote:
> >>> On 07/10/2015 12:17 PM, Marcelo Ricardo Leitner wrote:
> >>>> On Fri, Jul 10, 2015 at 11:35:28AM -0400, Vlad Yasevich wrote:
> > ...
> >>>>> have been numerous times where I've seen weak host model in use on the wire
> >>>>> even with a BSD peer.
> >>>>>
> >>>>> This also puts a very big nail through many suggestions we've had over the years
> >>>>> to allow source based path multihoming in addition to destination based multihoming
> >>>>> we currently support.
> >>>>>
> >>>>> It might be a good idea to make rp-filter like behavior best effort, and have
> >>>>> the old behavior as fallback.  I am still trying to think up different scenarios
> >>>>> where rp-filter behavior will cause things to fail prematurely...
> >>>>
> >>>> The old behavior is like "if we don't have a src yet and can't find a
> >>>> preferred src for this dst, use the 1st bound address". We can add it
> >>>> but as I said, I'm afraid it is just doing wrong and not worth. If such
> >>>> randomly src addressed packet is meant to be routed, the router will
> >>>> likely drop it as it is seen as a spoof. And if it reaches the peer, it
> >>>> will probably come back through a different path.
> >>>>
> >>>> I'm tempted to say that current usual use cases are handled by the first
> >>>> check on this function, which returns the preferred/primary address for
> >>>> the interface and checks against bound addresses. Whenever you reach the
> >>>> second check, it just allows you to use that 1st bound address that is
> >>>> checked. I mean, I can't see use cases that we would be breaking with
> >>>> this change. 
> >>>
> >>> Yes,  the secondary check didn't amount to much, but we've kept it since 2.5
> >>> days (when sctp was introduced).  I've made attempts over the years to
> >>> try to make it stricter, but that never amounted to anything that worked well.
> >>>
> >>>>
> >>>> But yeah, it impacts source based routing, and I'm not aware of previous
> >>>> discussions on it. I'll try to dig some up but if possible, please share
> >>>> some pointers on it.
> >>>
> >>> It's been suggested a few times that we should support source based multihoming
> >>> particularly for the case where one peer has only 1 address.
> >>> We've always punted on this, but people still ask every now and then.
> >>
> >> Ah okay, now I see it.
> >>  
> >>> I do have a question about the code though.. Have you tried with mutlipath routing
> >>> enabled.  I see rp_filter checks have special code to handle that.  Seem like we
> >>> might get false negatives in sctp.
> >>
> >> In the sense of CONFIG_IP_ROUTE_MULTIPATH=y, yes, but just that. My
> >> routes were simple ones, either 2 peers attaches to 2 local subnets, or
> >> with a gateway in the middle (with 2 subnets on each side, but mapped
> >> 1-1, no crossing. Aka subnet1<->subnet2 and subnet3<->subnet4 while not
> >> (subnet1<->subnet4 or subnet3<->subnet2).
> >>
> >> Note that this is not rp_filter strictly speaking, as it's mirrored.
> >> rp_filter needs to calculate all possible output routes (actually until
> >> it finds a valid one) for finding one that would match the one used for
> >> incoming. 
> >>
> >> This check already has an output path, and it's calculating if such
> >> input would be acceptable. We can't really expect/check for other hits
> >> because it invalidates the chosen output path.
> >>
> >> Hmmm... but we could support multipath in the output selection, ie in
> >> the outputs of ip_route_output_key(), probably in another patch then?
> > 
> > Thinking further.. we could just compare it with the addresses assigned to the
> > interface instead of doing a whole new routing. Cheaper/faster, provides the
> > results I'm looking for and the consequences are easier to see.
> > 
> > Something like (not tested, just illustrating the idea):
> > 
> > --- a/net/sctp/protocol.c
> > +++ b/net/sctp/protocol.c
> > @@ -489,22 +489,33 @@ static void sctp_v4_get_dst(struct sctp_transport *t, union sctp_addr *saddr,
> >         list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> >                 if (!laddr->valid)
> >                         continue;
> >                 if ((laddr->state == SCTP_ADDR_SRC) &&
> >                     (AF_INET == laddr->a.sa.sa_family)) {
> > +                       struct net_device *odev;
> > +
> >                         fl4->fl4_sport = laddr->a.v4.sin_port;
> >                         flowi4_update_output(fl4,
> >                                              asoc->base.sk->sk_bound_dev_if,
> >                                              RT_CONN_FLAGS(asoc->base.sk),
> >                                              daddr->v4.sin_addr.s_addr,
> >                                              laddr->a.v4.sin_addr.s_addr);
> >  
> >                         rt = ip_route_output_key(sock_net(sk), fl4);
> > -                       if (!IS_ERR(rt)) {
> > -                               dst = &rt->dst;
> > -                               goto out_unlock;
> > -                       }
> > +                       if (IS_ERR(rt))
> > +                               continue;
> > +
> > +                       /* Ensure the src address belongs to the output
> > +                        * interface.
> > +                        */
> > +                       odev = __ip_dev_find(net, laddr->a.v4.sin_addr.s_addr,
> > +                                            false);
> > +                       if (odev->if_index != fl4->flowi4_oif)
> > +                               continue;
> > +
> > +                       dst = &rt->dst;
> > +                       goto out_unlock;
> >                 }
> >         }
> >  
> >  out_unlock:
> >         rcu_read_unlock();
> > 
> > 
> > I like this better than my 1st attempt. What do you think?
> 
> Looks better.  Have to drop the ref on the dev since __ip_dev_find takes one.

Cool. I'll go that way then.

Regarding the ref, not really, because above code is under
rcu_read_lock() already and thne I passed false on its 3rd argument,
avoiding that ref.

Thanks,
Marcelo

> > 
> > I'll split the refactoring from this fix on v2, so it's easier to review.
> > 
> 
> Sounds good.
> 
> -vlad
> 
> >   Marcelo
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
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