* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
@ 2010-05-06 8:23 ` Nicolas Dichtel
2010-05-07 8:47 ` Weixing.Shi(Stone)
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Nicolas Dichtel @ 2010-05-06 8:23 UTC (permalink / raw)
To: linux-sctp
Hi Vlad,
I didn't succeed to test this patch. My kernel is not really an head, but a
2.6.33, maybe I miss a patch.
My tesbed:
A -------------- B
2001::102:101 2001::101:101
With this patch, the stack send out wrong packets. This is a capture on B:
10:15:00.879812 ::.39713 > 2001::101:101.1234: sctp [class 0x2]
10:15:03.877103 ::.39713 > 2001::101:101.1234: sctp [class 0x2]
10:15:09.875920 ::.39713 > 2001::101:101.1234: sctp [class 0x2]
10:15:21.874233 ::.39713 > 2001::101:101.1234: sctp [class 0x2]
Regards,
Nicolas
Le 05.05.2010 14:30, Vlad Yasevich a écrit :
> From: Weixing Shi <Weixing.Shi@windriver.com>
>
> <vlad>
> Ok, updated to be a bit more correct. Only leave the function early if the
> first lookup succeeds.
> </vlad>
>
> in the below test case, using the source address routing,
> sctp can not work.
> Node-A
> 1)ifconfig eth0 inet6 add 2001:1::1/64
> 2)ip -6 rule add from 2001:1::1 table 100 pref 100
> 3)ip -6 route add 2001:2::1 dev eth0 table 100
> 4)sctp_darn -H 2001:1::1 -P 250 -l &
> Node-B
> 1)ifconfig eth0 inet6 add 2001:2::1/64
> 2)ip -6 rule add from 2001:2::1 table 100 pref 100
> 3)ip -6 route add 2001:1::1 dev eth0 table 100
> 4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
>
> root cause:
> Node-A and Node-B use the source address routing, and
> at begining, source address will be NULL,sctp will
> search the routing table by the destination address,
> because using the source address routing table, and
> the result dst_entry will be NULL.
>
> solution:
> walk through the bind address list to get the source
> address and then lookup the routing table again to get
> the correct dst_entry.
>
> Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
> net/sctp/ipv6.c | 113 ++++++++++++++++++++++++++++++++++---------------------
> 1 files changed, 70 insertions(+), 43 deletions(-)
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 7326891..f75e2f8 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -77,6 +77,10 @@
> #include <net/sctp/sctp.h>
>
> #include <asm/uaccess.h>
> +static void sctp_v6_dst_saddr(union sctp_addr *addr, struct dst_entry *dst,
> + __be16 port);
> +static int sctp_v6_cmp_addr(const union sctp_addr *addr1,
> + const union sctp_addr *addr2);
>
> /* Event handler for inet6 address addition/deletion events.
> * The sctp_local_addr_list needs to be protocted by a spin lock since
> @@ -242,8 +246,12 @@ 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 dst_saddr;
> + sctp_scope_t scope;
>
> memset(&fl, 0, sizeof(fl));
> ipv6_addr_copy(&fl.fl6_dst, &daddr->v6.sin6_addr);
> @@ -259,16 +267,71 @@ static struct dst_entry *sctp_v6_get_dst(struct sctp_association *asoc,
> }
>
> dst = ip6_route_output(&init_net, NULL, &fl);
> +
> + bp = &asoc->base.bind_addr;
> + scope = sctp_scope(daddr);
> +
> if (!dst->error) {
> + if (!asoc || saddr)
> + goto out;
> +
> + /* Walk through the bind address list and look for a bind
> + * address that matches the source address of the returned dst.
> + */
> + sctp_v6_dst_saddr(&dst_saddr, dst, htons(bp->port));
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid || (laddr->state != SCTP_ADDR_SRC))
> + continue;
> +
> + /* Do not compare against v4 addrs */
> + if ((laddr->a.sa.sa_family = AF_INET6) &&
> + (sctp_v6_cmp_addr(&dst_saddr, &laddr->a)))
> + goto out_unlock;
> + }
> + rcu_read_unlock();
> +
> + /* None of the bound addresses match the source address of the
> + * dst. So release it.
> + */
> + dst_release(dst);
> + dst = NULL;
> + }
> +
> + /* 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 || (laddr->state != SCTP_ADDR_SRC))
> + continue;
> +
> + if ((laddr->a.sa.sa_family = AF_INET6) &&
> + (scope <= sctp_scope(&laddr->a))) {
> + ipv6_addr_copy(&fl.fl6_src, &laddr->a.v6.sin6_addr);
> + dst = ip6_route_output(&init_net, NULL, &fl);
> + if (dst->error) {
> + dst_release(dst);
> + dst = NULL;
> + } else
> + break;
> + }
> + }
> +
> +out_unlock:
> + rcu_read_unlock();
> +
> +out:
> + if (dst) {
> struct rt6_info *rt;
> rt = (struct rt6_info *)dst;
> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
> &rt->rt6i_dst.addr, &rt->rt6i_src.addr);
> return dst;
> - }
> - SCTP_DEBUG_PRINTK("NO ROUTE\n");
> - dst_release(dst);
> - return NULL;
> + } else
> + SCTP_DEBUG_PRINTK("NO ROUTE\n");
> +
> + return dst;
> }
>
> /* Returns the number of consecutive initial bits that match in the 2 ipv6
> @@ -289,13 +352,6 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
> union sctp_addr *daddr,
> union sctp_addr *saddr)
> {
> - struct sctp_bind_addr *bp;
> - struct sctp_sockaddr_entry *laddr;
> - sctp_scope_t scope;
> - union sctp_addr *baddr = NULL;
> - __u8 matchlen = 0;
> - __u8 bmatchlen;
> -
> SCTP_DEBUG_PRINTK("%s: asoc:%p dst:%p daddr:%pI6 ",
> __func__, asoc, dst, &daddr->v6.sin6_addr);
>
> @@ -310,38 +366,9 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
> return;
> }
>
> - scope = sctp_scope(daddr);
> -
> - bp = &asoc->base.bind_addr;
> -
> - /* Go through the bind address list and find the best source address
> - * that matches the scope of the destination 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;
> - }
> - }
> - }
> -
> - if (baddr) {
> - memcpy(saddr, baddr, sizeof(union sctp_addr));
> - SCTP_DEBUG_PRINTK("saddr: %pI6\n", &saddr->v6.sin6_addr);
> - } else {
> - printk(KERN_ERR "%s: asoc:%p Could not find a valid source "
> - "address for the dest:%pI6\n",
> - __func__, asoc, &daddr->v6.sin6_addr);
> - }
> + if (dst)
> + sctp_v6_dst_saddr(saddr, dst, htons(asoc->base.bind_addr.port));
>
> - rcu_read_unlock();
> }
>
> /* Make a copy of all potential local addresses. */
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
2010-05-06 8:23 ` [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address Nicolas Dichtel
@ 2010-05-07 8:47 ` Weixing.Shi(Stone)
2010-05-07 9:12 ` Nicolas Dichtel
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Weixing.Shi(Stone) @ 2010-05-07 8:47 UTC (permalink / raw)
To: linux-sctp
[-- Attachment #1: Type: text/plain, Size: 7639 bytes --]
Vlad Yasevich wrote:
> From: Weixing Shi <Weixing.Shi@windriver.com>
>
> <vlad>
> Ok, updated to be a bit more correct. Only leave the function early if the
> first lookup succeeds.
> </vlad>
>
> in the below test case, using the source address routing,
> sctp can not work.
> Node-A
> 1)ifconfig eth0 inet6 add 2001:1::1/64
> 2)ip -6 rule add from 2001:1::1 table 100 pref 100
> 3)ip -6 route add 2001:2::1 dev eth0 table 100
> 4)sctp_darn -H 2001:1::1 -P 250 -l &
> Node-B
> 1)ifconfig eth0 inet6 add 2001:2::1/64
> 2)ip -6 rule add from 2001:2::1 table 100 pref 100
> 3)ip -6 route add 2001:1::1 dev eth0 table 100
> 4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
>
> root cause:
> Node-A and Node-B use the source address routing, and
> at begining, source address will be NULL,sctp will
> search the routing table by the destination address,
> because using the source address routing table, and
> the result dst_entry will be NULL.
>
> solution:
> walk through the bind address list to get the source
> address and then lookup the routing table again to get
> the correct dst_entry.
>
> Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
> net/sctp/ipv6.c | 113 ++++++++++++++++++++++++++++++++++---------------------
> 1 files changed, 70 insertions(+), 43 deletions(-)
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index 7326891..f75e2f8 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -77,6 +77,10 @@
> #include <net/sctp/sctp.h>
>
> #include <asm/uaccess.h>
> +static void sctp_v6_dst_saddr(union sctp_addr *addr, struct dst_entry *dst,
> + __be16 port);
> +static int sctp_v6_cmp_addr(const union sctp_addr *addr1,
> + const union sctp_addr *addr2);
>
> /* Event handler for inet6 address addition/deletion events.
> * The sctp_local_addr_list needs to be protocted by a spin lock since
> @@ -242,8 +246,12 @@ 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 dst_saddr;
> + sctp_scope_t scope;
>
> memset(&fl, 0, sizeof(fl));
> ipv6_addr_copy(&fl.fl6_dst, &daddr->v6.sin6_addr);
> @@ -259,16 +267,71 @@ static struct dst_entry *sctp_v6_get_dst(struct sctp_association *asoc,
> }
>
> dst = ip6_route_output(&init_net, NULL, &fl);
> +
> + bp = &asoc->base.bind_addr;
> + scope = sctp_scope(daddr);
> +
>
if assoc = NULL, will cause kernel crash!
> if (!dst->error) {
> + if (!asoc || saddr)
> + goto out;
> +
> + /* Walk through the bind address list and look for a bind
> + * address that matches the source address of the returned dst.
> + */
> + sctp_v6_dst_saddr(&dst_saddr, dst, htons(bp->port));
> + rcu_read_lock();
> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
> + if (!laddr->valid || (laddr->state != SCTP_ADDR_SRC))
> + continue;
> +
> + /* Do not compare against v4 addrs */
> + if ((laddr->a.sa.sa_family == AF_INET6) &&
> + (sctp_v6_cmp_addr(&dst_saddr, &laddr->a)))
>
we do not concern use source address to find the dest address, so these
lines can ignore!
> + goto out_unlock;
> + }
> + rcu_read_unlock();
> +
> + /* None of the bound addresses match the source address of the
> + * dst. So release it.
> + */
> + dst_release(dst);
> + dst = NULL;
> + }
> +
> + /* 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 || (laddr->state != SCTP_ADDR_SRC))
> + continue;
> +
> + if ((laddr->a.sa.sa_family == AF_INET6) &&
> + (scope <= sctp_scope(&laddr->a))) {
>
here should be consider the most matched ipv6 address for the dest
address, so it should be like :
+ 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;
+ }
+ }
> + ipv6_addr_copy(&fl.fl6_src, &laddr->a.v6.sin6_addr);
> + dst = ip6_route_output(&init_net, NULL, &fl);
> + if (dst->error) {
> + dst_release(dst);
> + dst = NULL;
> + } else
> + break;
> + }
> + }
> +
> +out_unlock:
> + rcu_read_unlock();
> +
> +out:
> + if (dst) {
> struct rt6_info *rt;
> rt = (struct rt6_info *)dst;
> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
> &rt->rt6i_dst.addr, &rt->rt6i_src.addr);
> return dst;
> - }
> - SCTP_DEBUG_PRINTK("NO ROUTE\n");
> - dst_release(dst);
> - return NULL;
> + } else
> + SCTP_DEBUG_PRINTK("NO ROUTE\n");
> +
> + return dst;
> }
>
> /* Returns the number of consecutive initial bits that match in the 2 ipv6
> @@ -289,13 +352,6 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
> union sctp_addr *daddr,
> union sctp_addr *saddr)
> {
> - struct sctp_bind_addr *bp;
> - struct sctp_sockaddr_entry *laddr;
> - sctp_scope_t scope;
> - union sctp_addr *baddr = NULL;
> - __u8 matchlen = 0;
> - __u8 bmatchlen;
> -
> SCTP_DEBUG_PRINTK("%s: asoc:%p dst:%p daddr:%pI6 ",
> __func__, asoc, dst, &daddr->v6.sin6_addr);
>
> @@ -310,38 +366,9 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
> return;
> }
>
> - scope = sctp_scope(daddr);
> -
> - bp = &asoc->base.bind_addr;
> -
> - /* Go through the bind address list and find the best source address
> - * that matches the scope of the destination 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;
> - }
> - }
> - }
> -
> - if (baddr) {
> - memcpy(saddr, baddr, sizeof(union sctp_addr));
> - SCTP_DEBUG_PRINTK("saddr: %pI6\n", &saddr->v6.sin6_addr);
> - } else {
> - printk(KERN_ERR "%s: asoc:%p Could not find a valid source "
> - "address for the dest:%pI6\n",
> - __func__, asoc, &daddr->v6.sin6_addr);
> - }
> + if (dst)
> + sctp_v6_dst_saddr(saddr, dst, htons(asoc->base.bind_addr.port));
>
> - rcu_read_unlock();
> }
>
>
i open the sctp debug and and find that the using the source address to find the dst and dst source address does not have correct source address, it is always 0000:0000:0000:0000:0000:0000:0000:0000, i think this is maybe an ipv6 route bug, so if modify the code as yours, sctp can not work!
> /* Make a copy of all potential local addresses. */
>
based on the below consideration, i make a new patch for it! it can work
in my environment!
--
Weixing.Shi(Stone)
BSP team
WIND RIVER | China Development Center
Tel: 86-10-8477-8502 | Fax: 86-10-64790367
(M) : 86-13520312764
[-- Attachment #2: 0001-sctp-fix-sctp-to-work-with-ipv6-source-address-rout.patch --]
[-- Type: text/x-patch, Size: 3554 bytes --]
From 300e10f7e4130829c49e5651b68bcf89f3b8b759 Mon Sep 17 00:00:00 2001
From: Weixing Shi <Weixing.Shi@windriver.com>
Date: Fri, 7 May 2010 16:32:30 +0800
Subject: [PATCH] sctp: fix sctp to work with ipv6 source address routing
in the below test case, using the source address routing,
sctp can not work.
Node-A
1)ifconfig eth0 inet6 add 2001:1::1/64
2)ip -6 rule add from 2001:1::1 table 100 pref 100
3)ip -6 route add 2001:2::1 dev eth0 table 100
4)sctp_darn -H 2001:1::1 -P 250 -l &
Node-B
1)ifconfig eth0 inet6 add 2001:2::1/64
2)ip -6 rule add from 2001:2::1 table 100 pref 100
3)ip -6 route add 2001:1::1 dev eth0 table 100
4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
root cause:
Node-A and Node-B use the source address routing, and
at begining, source address will be NULL,sctp will
search the routing table by the destination address,
because using the source address routing table, and
the result dst_entry will be NULL.
solution:
walk through the bind address list to get the source
address and then lookup the routing table again to get
the correct dst_entry.
Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
---
net/sctp/ipv6.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 9fb5d37..03a2d8d 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,15 @@ 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 dst_saddr;
+ 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 +268,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 (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;
+ }
+ }
+ }
+ 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 +309,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;
}
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
2010-05-06 8:23 ` [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address Nicolas Dichtel
2010-05-07 8:47 ` Weixing.Shi(Stone)
@ 2010-05-07 9:12 ` Nicolas Dichtel
2010-05-07 9:26 ` Weixing.Shi(Stone)
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Nicolas Dichtel @ 2010-05-07 9:12 UTC (permalink / raw)
To: linux-sctp
Now it's ok in my testbed (without IPsec). There is just a little warning:
CC net/sctp/ipv6.o
net/sctp/ipv6.c: In function ?sctp_v6_get_dst?:
net/sctp/ipv6.c:250: warning: unused variable ?dst_saddr?
Regards,
Nicolas
Le 07.05.2010 10:47, Weixing.Shi(Stone) a écrit :
> Vlad Yasevich wrote:
>> From: Weixing Shi <Weixing.Shi@windriver.com>
>>
>> <vlad>
>> Ok, updated to be a bit more correct. Only leave the function early
>> if the
>> first lookup succeeds.
>> </vlad>
>>
>> in the below test case, using the source address routing,
>> sctp can not work.
>> Node-A
>> 1)ifconfig eth0 inet6 add 2001:1::1/64
>> 2)ip -6 rule add from 2001:1::1 table 100 pref 100
>> 3)ip -6 route add 2001:2::1 dev eth0 table 100
>> 4)sctp_darn -H 2001:1::1 -P 250 -l &
>> Node-B
>> 1)ifconfig eth0 inet6 add 2001:2::1/64
>> 2)ip -6 rule add from 2001:2::1 table 100 pref 100
>> 3)ip -6 route add 2001:1::1 dev eth0 table 100
>> 4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
>>
>> root cause:
>> Node-A and Node-B use the source address routing, and
>> at begining, source address will be NULL,sctp will
>> search the routing table by the destination address,
>> because using the source address routing table, and
>> the result dst_entry will be NULL.
>>
>> solution:
>> walk through the bind address list to get the source
>> address and then lookup the routing table again to get
>> the correct dst_entry.
>>
>> Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>> ---
>> net/sctp/ipv6.c | 113
>> ++++++++++++++++++++++++++++++++++---------------------
>> 1 files changed, 70 insertions(+), 43 deletions(-)
>>
>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>> index 7326891..f75e2f8 100644
>> --- a/net/sctp/ipv6.c
>> +++ b/net/sctp/ipv6.c
>> @@ -77,6 +77,10 @@
>> #include <net/sctp/sctp.h>
>>
>> #include <asm/uaccess.h>
>> +static void sctp_v6_dst_saddr(union sctp_addr *addr, struct dst_entry
>> *dst,
>> + __be16 port);
>> +static int sctp_v6_cmp_addr(const union sctp_addr *addr1,
>> + const union sctp_addr *addr2);
>>
>> /* Event handler for inet6 address addition/deletion events.
>> * The sctp_local_addr_list needs to be protocted by a spin lock since
>> @@ -242,8 +246,12 @@ 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 dst_saddr;
>> + sctp_scope_t scope;
>>
>> memset(&fl, 0, sizeof(fl));
>> ipv6_addr_copy(&fl.fl6_dst, &daddr->v6.sin6_addr);
>> @@ -259,16 +267,71 @@ static struct dst_entry *sctp_v6_get_dst(struct
>> sctp_association *asoc,
>> }
>>
>> dst = ip6_route_output(&init_net, NULL, &fl);
>> +
>> + bp = &asoc->base.bind_addr;
>> + scope = sctp_scope(daddr);
>> +
>>
> if assoc = NULL, will cause kernel crash!
>> if (!dst->error) {
>> + if (!asoc || saddr)
>> + goto out;
>> +
>> + /* Walk through the bind address list and look for a bind
>> + * address that matches the source address of the returned dst.
>> + */
>> + sctp_v6_dst_saddr(&dst_saddr, dst, htons(bp->port));
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
>> + if (!laddr->valid || (laddr->state != SCTP_ADDR_SRC))
>> + continue;
>> +
>> + /* Do not compare against v4 addrs */
>> + if ((laddr->a.sa.sa_family = AF_INET6) &&
>> + (sctp_v6_cmp_addr(&dst_saddr, &laddr->a)))
>>
> we do not concern use source address to find the dest address, so these
> lines can ignore!
>> + goto out_unlock;
>> + }
>> + rcu_read_unlock();
>> +
>> + /* None of the bound addresses match the source address of the
>> + * dst. So release it.
>> + */
>> + dst_release(dst);
>> + dst = NULL;
>> + }
>> +
>> + /* 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 || (laddr->state != SCTP_ADDR_SRC))
>> + continue;
>> +
>> + if ((laddr->a.sa.sa_family = AF_INET6) &&
>> + (scope <= sctp_scope(&laddr->a))) {
>>
> here should be consider the most matched ipv6 address for the dest
> address, so it should be like :
> + 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;
> + }
> + }
>
>> + ipv6_addr_copy(&fl.fl6_src, &laddr->a.v6.sin6_addr);
>> + dst = ip6_route_output(&init_net, NULL, &fl);
>> + if (dst->error) {
>> + dst_release(dst);
>> + dst = NULL;
>> + } else
>> + break;
>> + }
>> + }
>> +
>> +out_unlock:
>> + rcu_read_unlock();
>> +
>> +out:
>> + if (dst) {
>> struct rt6_info *rt;
>> rt = (struct rt6_info *)dst;
>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>> &rt->rt6i_dst.addr, &rt->rt6i_src.addr);
>> return dst;
>> - }
>> - SCTP_DEBUG_PRINTK("NO ROUTE\n");
>> - dst_release(dst);
>> - return NULL;
>> + } else + SCTP_DEBUG_PRINTK("NO ROUTE\n");
>> +
>> + return dst;
>> }
>>
>> /* Returns the number of consecutive initial bits that match in the 2
>> ipv6
>> @@ -289,13 +352,6 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
>> union sctp_addr *daddr,
>> union sctp_addr *saddr)
>> {
>> - struct sctp_bind_addr *bp;
>> - struct sctp_sockaddr_entry *laddr;
>> - sctp_scope_t scope;
>> - union sctp_addr *baddr = NULL;
>> - __u8 matchlen = 0;
>> - __u8 bmatchlen;
>> -
>> SCTP_DEBUG_PRINTK("%s: asoc:%p dst:%p daddr:%pI6 ",
>> __func__, asoc, dst, &daddr->v6.sin6_addr);
>>
>> @@ -310,38 +366,9 @@ static void sctp_v6_get_saddr(struct sctp_sock *sk,
>> return;
>> }
>>
>> - scope = sctp_scope(daddr);
>> -
>> - bp = &asoc->base.bind_addr;
>> -
>> - /* Go through the bind address list and find the best source address
>> - * that matches the scope of the destination 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;
>> - }
>> - }
>> - }
>> -
>> - if (baddr) {
>> - memcpy(saddr, baddr, sizeof(union sctp_addr));
>> - SCTP_DEBUG_PRINTK("saddr: %pI6\n", &saddr->v6.sin6_addr);
>> - } else {
>> - printk(KERN_ERR "%s: asoc:%p Could not find a valid source "
>> - "address for the dest:%pI6\n",
>> - __func__, asoc, &daddr->v6.sin6_addr);
>> - }
>> + if (dst)
>> + sctp_v6_dst_saddr(saddr, dst, htons(asoc->base.bind_addr.port));
>>
>> - rcu_read_unlock();
>> }
>>
>>
>
> i open the sctp debug and and find that the using the source address to
> find the dst and dst source address does not have correct source
> address, it is always 0000:0000:0000:0000:0000:0000:0000:0000, i think
> this is maybe an ipv6 route bug, so if modify the code as yours, sctp
> can not work!
>
>> /* Make a copy of all potential local addresses. */
>>
> based on the below consideration, i make a new patch for it! it can work
> in my environment!
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
` (2 preceding siblings ...)
2010-05-07 9:12 ` Nicolas Dichtel
@ 2010-05-07 9:26 ` Weixing.Shi(Stone)
2010-05-07 13:36 ` Vlad Yasevich
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Weixing.Shi(Stone) @ 2010-05-07 9:26 UTC (permalink / raw)
To: linux-sctp
[-- Attachment #1: Type: text/plain, Size: 9429 bytes --]
Nicolas Dichtel wrote:
> Now it's ok in my testbed (without IPsec). There is just a little
> warning:
>
> CC net/sctp/ipv6.o
> net/sctp/ipv6.c: In function ?sctp_v6_get_dst?:
> net/sctp/ipv6.c:250: warning: unused variable ?dst_saddr?
>
>
> Regards,
> Nicolas
>
> Le 07.05.2010 10:47, Weixing.Shi(Stone) a écrit :
>> Vlad Yasevich wrote:
>>> From: Weixing Shi <Weixing.Shi@windriver.com>
>>>
>>> <vlad>
>>> Ok, updated to be a bit more correct. Only leave the function early
>>> if the
>>> first lookup succeeds.
>>> </vlad>
>>>
>>> in the below test case, using the source address routing,
>>> sctp can not work.
>>> Node-A
>>> 1)ifconfig eth0 inet6 add 2001:1::1/64
>>> 2)ip -6 rule add from 2001:1::1 table 100 pref 100
>>> 3)ip -6 route add 2001:2::1 dev eth0 table 100
>>> 4)sctp_darn -H 2001:1::1 -P 250 -l &
>>> Node-B
>>> 1)ifconfig eth0 inet6 add 2001:2::1/64
>>> 2)ip -6 rule add from 2001:2::1 table 100 pref 100
>>> 3)ip -6 route add 2001:1::1 dev eth0 table 100
>>> 4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
>>>
>>> root cause:
>>> Node-A and Node-B use the source address routing, and
>>> at begining, source address will be NULL,sctp will
>>> search the routing table by the destination address,
>>> because using the source address routing table, and
>>> the result dst_entry will be NULL.
>>>
>>> solution:
>>> walk through the bind address list to get the source
>>> address and then lookup the routing table again to get
>>> the correct dst_entry.
>>>
>>> Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
>>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>>> ---
>>> net/sctp/ipv6.c | 113
>>> ++++++++++++++++++++++++++++++++++---------------------
>>> 1 files changed, 70 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index 7326891..f75e2f8 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -77,6 +77,10 @@
>>> #include <net/sctp/sctp.h>
>>>
>>> #include <asm/uaccess.h>
>>> +static void sctp_v6_dst_saddr(union sctp_addr *addr, struct
>>> dst_entry *dst,
>>> + __be16 port);
>>> +static int sctp_v6_cmp_addr(const union sctp_addr *addr1,
>>> + const union sctp_addr *addr2);
>>>
>>> /* Event handler for inet6 address addition/deletion events.
>>> * The sctp_local_addr_list needs to be protocted by a spin lock since
>>> @@ -242,8 +246,12 @@ 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 dst_saddr;
>>> + sctp_scope_t scope;
>>>
>>> memset(&fl, 0, sizeof(fl));
>>> ipv6_addr_copy(&fl.fl6_dst, &daddr->v6.sin6_addr);
>>> @@ -259,16 +267,71 @@ static struct dst_entry
>>> *sctp_v6_get_dst(struct sctp_association *asoc,
>>> }
>>>
>>> dst = ip6_route_output(&init_net, NULL, &fl);
>>> +
>>> + bp = &asoc->base.bind_addr;
>>> + scope = sctp_scope(daddr);
>>> +
>>>
>> if assoc = NULL, will cause kernel crash!
>>> if (!dst->error) {
>>> + if (!asoc || saddr)
>>> + goto out;
>>> +
>>> + /* Walk through the bind address list and look for a bind
>>> + * address that matches the source address of the returned
>>> dst.
>>> + */
>>> + sctp_v6_dst_saddr(&dst_saddr, dst, htons(bp->port));
>>> + rcu_read_lock();
>>> + list_for_each_entry_rcu(laddr, &bp->address_list, list) {
>>> + if (!laddr->valid || (laddr->state != SCTP_ADDR_SRC))
>>> + continue;
>>> +
>>> + /* Do not compare against v4 addrs */
>>> + if ((laddr->a.sa.sa_family == AF_INET6) &&
>>> + (sctp_v6_cmp_addr(&dst_saddr, &laddr->a)))
>>>
>> we do not concern use source address to find the dest address, so
>> these lines can ignore!
>>> + goto out_unlock;
>>> + }
>>> + rcu_read_unlock();
>>> +
>>> + /* None of the bound addresses match the source address of the
>>> + * dst. So release it.
>>> + */
>>> + dst_release(dst);
>>> + dst = NULL;
>>> + }
>>> +
>>> + /* 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 || (laddr->state != SCTP_ADDR_SRC))
>>> + continue;
>>> +
>>> + if ((laddr->a.sa.sa_family == AF_INET6) &&
>>> + (scope <= sctp_scope(&laddr->a))) {
>>>
>> here should be consider the most matched ipv6 address for the dest
>> address, so it should be like :
>> + 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;
>> + }
>> + }
>>
>>> + ipv6_addr_copy(&fl.fl6_src, &laddr->a.v6.sin6_addr);
>>> + dst = ip6_route_output(&init_net, NULL, &fl);
>>> + if (dst->error) {
>>> + dst_release(dst);
>>> + dst = NULL;
>>> + } else
>>> + break;
>>> + }
>>> + }
>>> +
>>> +out_unlock:
>>> + rcu_read_unlock();
>>> +
>>> +out:
>>> + if (dst) {
>>> struct rt6_info *rt;
>>> rt = (struct rt6_info *)dst;
>>> SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>> &rt->rt6i_dst.addr, &rt->rt6i_src.addr);
>>> return dst;
>>> - }
>>> - SCTP_DEBUG_PRINTK("NO ROUTE\n");
>>> - dst_release(dst);
>>> - return NULL;
>>> + } else + SCTP_DEBUG_PRINTK("NO ROUTE\n");
>>> +
>>> + return dst;
>>> }
>>>
>>> /* Returns the number of consecutive initial bits that match in the
>>> 2 ipv6
>>> @@ -289,13 +352,6 @@ static void sctp_v6_get_saddr(struct sctp_sock
>>> *sk,
>>> union sctp_addr *daddr,
>>> union sctp_addr *saddr)
>>> {
>>> - struct sctp_bind_addr *bp;
>>> - struct sctp_sockaddr_entry *laddr;
>>> - sctp_scope_t scope;
>>> - union sctp_addr *baddr = NULL;
>>> - __u8 matchlen = 0;
>>> - __u8 bmatchlen;
>>> -
>>> SCTP_DEBUG_PRINTK("%s: asoc:%p dst:%p daddr:%pI6 ",
>>> __func__, asoc, dst, &daddr->v6.sin6_addr);
>>>
>>> @@ -310,38 +366,9 @@ static void sctp_v6_get_saddr(struct sctp_sock
>>> *sk,
>>> return;
>>> }
>>>
>>> - scope = sctp_scope(daddr);
>>> -
>>> - bp = &asoc->base.bind_addr;
>>> -
>>> - /* Go through the bind address list and find the best source
>>> address
>>> - * that matches the scope of the destination 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;
>>> - }
>>> - }
>>> - }
>>> -
>>> - if (baddr) {
>>> - memcpy(saddr, baddr, sizeof(union sctp_addr));
>>> - SCTP_DEBUG_PRINTK("saddr: %pI6\n", &saddr->v6.sin6_addr);
>>> - } else {
>>> - printk(KERN_ERR "%s: asoc:%p Could not find a valid source "
>>> - "address for the dest:%pI6\n",
>>> - __func__, asoc, &daddr->v6.sin6_addr);
>>> - }
>>> + if (dst)
>>> + sctp_v6_dst_saddr(saddr, dst,
>>> htons(asoc->base.bind_addr.port));
>>>
>>> - rcu_read_unlock();
>>> }
>>>
>>>
>>
>> i open the sctp debug and and find that the using the source address
>> to find the dst and dst source address does not have correct source
>> address, it is always 0000:0000:0000:0000:0000:0000:0000:0000, i
>> think this is maybe an ipv6 route bug, so if modify the code as
>> yours, sctp can not work!
>>
>>> /* Make a copy of all potential local addresses. */
>>>
>> based on the below consideration, i make a new patch for it! it can
>> work in my environment!
ok! new patch done!
--
Weixing.Shi(Stone)
BSP team
WIND RIVER | China Development Center
Tel: 86-10-8477-8502 | Fax: 86-10-64790367
(M) : 86-13520312764
[-- Attachment #2: 0001-sctp-fix-sctp-to-work-with-ipv6-source-address-rout.patch --]
[-- Type: text/x-patch, Size: 3527 bytes --]
From 225fea3ba094b378647865eecebcb471bc3cdc11 Mon Sep 17 00:00:00 2001
From: Weixing Shi <Weixing.Shi@windriver.com>
Date: Fri, 7 May 2010 17:23:18 +0800
Subject: [PATCH] sctp: fix sctp to work with ipv6 source address routing
in the below test case, using the source address routing,
sctp can not work.
Node-A
1)ifconfig eth0 inet6 add 2001:1::1/64
2)ip -6 rule add from 2001:1::1 table 100 pref 100
3)ip -6 route add 2001:2::1 dev eth0 table 100
4)sctp_darn -H 2001:1::1 -P 250 -l &
Node-B
1)ifconfig eth0 inet6 add 2001:2::1/64
2)ip -6 rule add from 2001:2::1 table 100 pref 100
3)ip -6 route add 2001:1::1 dev eth0 table 100
4)sctp_darn -H 2001:2::1 -P 250 -h 2001:1::1 -p 250 -s
root cause:
Node-A and Node-B use the source address routing, and
at begining, source address will be NULL,sctp will
search the routing table by the destination address,
because using the source address routing table, and
the result dst_entry will be NULL.
solution:
walk through the bind address list to get the source
address and then lookup the routing table again to get
the correct dst_entry.
Signed-off-by: Weixing Shi <Weixing.Shi@windriver.com>
---
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 (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;
+ }
+ }
+ }
+ 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;
}
--
1.5.5.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
` (3 preceding siblings ...)
2010-05-07 9:26 ` Weixing.Shi(Stone)
@ 2010-05-07 13:36 ` Vlad Yasevich
2010-05-07 19:05 ` Vlad Yasevich
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Vlad Yasevich @ 2010-05-07 13:36 UTC (permalink / raw)
To: linux-sctp
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.
> + 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.
-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;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
` (4 preceding siblings ...)
2010-05-07 13:36 ` Vlad Yasevich
@ 2010-05-07 19:05 ` Vlad Yasevich
2010-05-10 9:47 ` Weixing.Shi(Stone)
2010-05-10 13:39 ` Vlad Yasevich
7 siblings, 0 replies; 9+ messages in thread
From: Vlad Yasevich @ 2010-05-07 19:05 UTC (permalink / raw)
To: linux-sctp
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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
` (5 preceding siblings ...)
2010-05-07 19:05 ` Vlad Yasevich
@ 2010-05-10 9:47 ` Weixing.Shi(Stone)
2010-05-10 13:39 ` Vlad Yasevich
7 siblings, 0 replies; 9+ messages in thread
From: Weixing.Shi(Stone) @ 2010-05-10 9:47 UTC (permalink / raw)
To: linux-sctp
Vlad Yasevich wrote:
> 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.
hi vlad:
i have a question about the routing cache, why do we use routing
cache to get the source and dest address?because source address has
inserted into bp->address_list when we bind the source address and dest
address is from msghdr when we send message.
> 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.
>
>
if we support multi-home, here are more than one source address
existed, which ip will be selected for a primary source address? i do
not find the algorithm in the sctp RFC(maybe i am mistake here), so i
think the nearest ip for the dest address is best choice!
> 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@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
--
Weixing.Shi(Stone)
BSP team
WIND RIVER | China Development Center
Tel: 86-10-8477-8502 | Fax: 86-10-64790367
(M) : 86-13520312764
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address
2010-05-05 12:30 [RFC PATCH v2] sctp: fix sctp to work with ipv6 source address routing Vlad Yasevich
` (6 preceding siblings ...)
2010-05-10 9:47 ` Weixing.Shi(Stone)
@ 2010-05-10 13:39 ` Vlad Yasevich
7 siblings, 0 replies; 9+ messages in thread
From: Vlad Yasevich @ 2010-05-10 13:39 UTC (permalink / raw)
To: linux-sctp
Weixing.Shi(Stone) wrote:
> Vlad Yasevich wrote:
>> Vlad Yasevich wrote:
>>
>>
>> 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.
> hi vlad:
> i have a question about the routing cache, why do we use routing
> cache to get the source and dest address?because source address has
> inserted into bp->address_list when we bind the source address and dest
> address is from msghdr when we send message.
We don't do anything wit destination addresses other then lookup routes to them.
However, source address is different in that we don't know it a lot of time due
to multihoming nature of SCTP. We know a set of source addresses, but not
what's the best one for this particular destination. That's why we try to use a
route lookup to tell us what the best source address is. Once we know it, we
can determine if we can use it.
When we find the best source, it's cached in the sctp_transport structure for
future use. Also, as routes and interfaces change on the system, the source
address may change as well.
>> 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.
>>
>>
> if we support multi-home, here are more than one source address
> existed, which ip will be selected for a primary source address?
There is no concept of primary source, just a primary destination. The source
is selected based on source address selection algorithms and routes to the
destination.
> i do
> not find the algorithm in the sctp RFC(maybe i am mistake here), so i
> think the nearest ip for the dest address is best choice!
For IPv4 yes, it works fairly well, unless there are tons of aliases.
For IPv6, there is a whole source address selection and preference that has to
be followed. See rfc 3484. So, by us selectiong the closes match is not always
right. Additionally, with the current code, if you have 2 addresses that have
the same number of common bits with destination, only the first found will be
used, but the route may not be the most optimal one or may not even exist.
Thus having on 1 route lookup hurts us in this case.
So, this algorithm needs some work. The source address selection rules already
specify that there must be route for a given source to a given destination for
the source to be considered. So, I'll look into how we can use that algorithm.
-vlad
>
>> 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@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread