Vlad Yasevich wrote:
> A few comments on the patch.
>
>
>> ---
>> net/sctp/ipv6.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>> 1 files changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 9fb5d37..6047b57 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -77,6 +77,8 @@
>> #include <net/sctp/sctp.h>
>>
>> #include <asm/uaccess.h>
>> +static inline int sctp_v6_addr_match_len(union sctp_addr *s1,
>> + union sctp_addr *s2);
>>
>> /* Event handler for inet6 address addition/deletion events.
>> * The sctp_local_addr_list needs to be protocted by a spin lock since
>> @@ -242,8 +244,14 @@ static struct dst_entry *sctp_v6_get_dst(struct sctp_association *asoc,
>> union sctp_addr *daddr,
>> union sctp_addr *saddr)
>> {
>> - struct dst_entry *dst;
>> + struct dst_entry *dst = NULL;
>> struct flowi fl;
>> + struct sctp_bind_addr *bp;
>> + struct sctp_sockaddr_entry *laddr;
>> + union sctp_addr *baddr = NULL;
>> + __u8 matchlen = 0;
>> + __u8 bmatchlen;
>> + sctp_scope_t scope;
>>
>> memset(&fl, 0, sizeof(fl));
>> ipv6_addr_copy(&fl.fl6_dst, &daddr->v6.sin6_addr);
>> @@ -259,6 +267,39 @@ static struct dst_entry *sctp_v6_get_dst(struct sctp_association *asoc,
>> }
>>
>> dst = ip6_route_output(&init_net, NULL, &fl);
>> + if (!asoc || saddr)
>> + goto out;
>> +
>
> If the route lookup fails and you fulfill the if statement, you will return
> without a route.
This appears to be expected behavior, so I'll retract that.
>
>> + if (dst->error) {
>> + dst_release(dst);
>> + dst = NULL;
>> + bp = &asoc->base.bind_addr;
>> + scope = sctp_scope(daddr);
>> + /* Walk through the bind address list and try to get a dst that
>> + * matches a bind address as the source address.
>> + */
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
>> + if (!laddr->valid)
>> + continue;
>> + if ((laddr->state == SCTP_ADDR_SRC) &&
>> + (laddr->a.sa.sa_family == AF_INET6) &&
>> + (scope <= sctp_scope(&laddr->a))) {
>> + bmatchlen = sctp_v6_addr_match_len(daddr, &laddr->a);
>> + if (!baddr || (matchlen < bmatchlen)) {
>> + baddr = &laddr->a;
>> + matchlen = bmatchlen;
>> + }
>
> This is not quite right since routing lookup code now can do proper source
> address selection. The trouble is that in IPv6, a reference to the actual route
> is returned, while in IPv4, a cached route is created and returned. Thus IPv4
> can fill in the source address in the cache route, while IPv6 can not do that
> since it would be then be modifying the route in the actual table. To solve
> this issue, IPv6 returns the source address in the flowi. So, now we need to
> carry around a flowi so that we can properly check the source.
>
> It's possible that original lookup may return the correct source and we can skip
> the whole lookup up routes for every entry in the bind list.
>
> I am going to rework this and merge some of the things IPsec patch tried to do
> here. First, we should call ip6_dst_lookup as that actually fills the flowi
> properly. Next, we need to pass the flowi into this function fill it in here,
> so that sctp_transport_route() can than take that and pass it to get_saddr().
>
> IPv6 version of get_saddr() will take the source address out of the flowi.
>
> The way you have you code is that we end walking the bind list multiple times
> looking for the source address. That's very wasteful, especially since we are
> most likely in softirq context and we want to make that as fast as possible.
>
I think all of the above can be a follow-on patch actually. The thing I really
don't like though is the source address selection. It's just as half-assed as
the original code, only taking into consideration scope and longest prefix
match. There is more to it then that.
So, I am going to take this patch, but there will be follow-ons to fix all the
broken stuff.
-vlad
> -vlad
>
>> + }
>> + }
>> + rcu_read_unlock();
>> + if (baddr) {
>> + ipv6_addr_copy(&fl.fl6_src, &baddr->v6.sin6_addr);
>> + dst = ip6_route_output(&init_net, NULL, &fl);
>> + }
>> + }
>> +
>> +out:
>> if (!dst->error) {
>> struct rt6_info *rt;
>> rt = (struct rt6_info *)dst;
>> @@ -267,7 +308,8 @@ static struct dst_entry *sctp_v6_get_dst(struct sctp_association *asoc,
>> return dst;
>> }
>> SCTP_DEBUG_PRINTK("NO ROUTE\n");
>> - dst_release(dst);
>> + if (dst)
>> + dst_release(dst);
>> return NULL;
>> }
>>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
