Re: [PATCH 9/13] [SCTP] Merge IPv4 and IPv6 versions of get_saddr() with their corresponding get_dst().

2006-11-02 Thread Ville Nuorvala 
On 10/27/06 20:47, Sridhar Samudrala wrote:
 On Tue, 2006-10-17 at 03:19 +0300, Ville Nuorvala wrote:
 As the IPv6 route lookup now also returns the selected source address
 there is no need for a separate source address lookup. In fact, the
 source address selection needs to be moved to get_dst() because the
 selected IPv6 source address isn't always stored in the route.
 Sometimes this makes it impossible to guess the correct address later on.

 
 Ville,
 
 Overall the patch looks pretty good. I found only 1 issue in 
 sctp_v6_get_dst(). See below.
 
 
 snip
 
 
 +/* Returns the dst cache entry for the given source and destination ip
 + * addresses.
 + */
 +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 flowi fl;
 +struct sctp_bind_addr *bp;
 +rwlock_t *addr_lock;
 +struct sctp_sockaddr_entry *laddr;
 +struct list_head *pos;
 +struct rt6_info *rt;
 +union sctp_addr baddr;
 +sctp_scope_t scope;
 +__u8 matchlen = 0;
 +__u8 bmatchlen;
 +
 +memset(fl, 0, sizeof(fl));
 +ipv6_addr_copy(fl.fl6_dst, daddr-v6.sin6_addr);
 +if (ipv6_addr_type(daddr-v6.sin6_addr)  IPV6_ADDR_LINKLOCAL)
 +fl.oif = daddr-v6.sin6_scope_id;
 +
 +ipv6_addr_copy(fl.fl6_src, saddr-v6.sin6_addr);
 +SCTP_DEBUG_PRINTK(%s: DST= NIP6_FMT  SRC= NIP6_FMT  ,
 +  __FUNCTION__, NIP6(fl.fl6_dst), NIP6(fl.fl6_src));
 +
 +dst = ip6_route_output(NULL, fl);
 +if (dst-error) {
 +dst_release(dst);
 +dst = NULL;
 +}
 +if (!ipv6_addr_any(saddr-v6.sin6_addr))
 +goto out;
 +if (!asoc) {
 +if (dst)
 +ipv6_addr_copy(saddr-v6.sin6_addr, fl.fl6_src);
 +goto out;
 +}
 +bp = asoc-base.bind_addr;
 +addr_lock = asoc-base.addr_lock;
 +
 +if (dst) {
 +/* Walk through the bind address list and look for a bind
 + * address that matches the source address of the returned rt.
 + */
 +sctp_v6_fl_saddr(baddr, fl, bp-port);
 Here we are checking if the source address returned in the dst matches one of
 the address in the bind address list of the association. Not the source 
 address
 that is passed to this routine(it could be INADDRY_ANY).
 So this should be changed back to sctp_v6_dst_saddr().

No, see that's the problem. The source address isn't always stored in
the  rt6_info. Therefore I copy the address into the flowi, so after
ip6_route_output() fl does indeed contain the selected source address.
Sorry if I didn't cc you all relevant patches from the patch set.

Anyway there are still some unresolved issues with my code, but it's
nice to know I didn't at least mess up SCTP in a big way ;-)

Thanks,
Ville
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/13] [SCTP] Merge IPv4 and IPv6 versions of get_saddr() with their corresponding get_dst().

2006-11-02 Thread Sridhar Samudrala 
On Thu, 2006-11-02 at 14:32 +0200, Ville Nuorvala wrote:
 On 10/27/06 20:47, Sridhar Samudrala wrote:
  On Tue, 2006-10-17 at 03:19 +0300, Ville Nuorvala wrote:
  As the IPv6 route lookup now also returns the selected source address
  there is no need for a separate source address lookup. In fact, the
  source address selection needs to be moved to get_dst() because the
  selected IPv6 source address isn't always stored in the route.
  Sometimes this makes it impossible to guess the correct address later on.
 
  
  Ville,
  
  Overall the patch looks pretty good. I found only 1 issue in 
  sctp_v6_get_dst(). See below.
  
  
  snip
  
  
  +/* Returns the dst cache entry for the given source and destination ip
  + * addresses.
  + */
  +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 flowi fl;
  +  struct sctp_bind_addr *bp;
  +  rwlock_t *addr_lock;
  +  struct sctp_sockaddr_entry *laddr;
  +  struct list_head *pos;
  +  struct rt6_info *rt;
  +  union sctp_addr baddr;
  +  sctp_scope_t scope;
  +  __u8 matchlen = 0;
  +  __u8 bmatchlen;
  +
  +  memset(fl, 0, sizeof(fl));
  +  ipv6_addr_copy(fl.fl6_dst, daddr-v6.sin6_addr);
  +  if (ipv6_addr_type(daddr-v6.sin6_addr)  IPV6_ADDR_LINKLOCAL)
  +  fl.oif = daddr-v6.sin6_scope_id;
  +
  +  ipv6_addr_copy(fl.fl6_src, saddr-v6.sin6_addr);
  +  SCTP_DEBUG_PRINTK(%s: DST= NIP6_FMT  SRC= NIP6_FMT  ,
  +__FUNCTION__, NIP6(fl.fl6_dst), NIP6(fl.fl6_src));
  +
  +  dst = ip6_route_output(NULL, fl);
  +  if (dst-error) {
  +  dst_release(dst);
  +  dst = NULL;
  +  }
  +  if (!ipv6_addr_any(saddr-v6.sin6_addr))
  +  goto out;
  +  if (!asoc) {
  +  if (dst)
  +  ipv6_addr_copy(saddr-v6.sin6_addr, fl.fl6_src);
  +  goto out;
  +  }
  +  bp = asoc-base.bind_addr;
  +  addr_lock = asoc-base.addr_lock;
  +
  +  if (dst) {
  +  /* Walk through the bind address list and look for a bind
  +   * address that matches the source address of the returned rt.
  +   */
  +  sctp_v6_fl_saddr(baddr, fl, bp-port);
  Here we are checking if the source address returned in the dst matches one 
  of
  the address in the bind address list of the association. Not the source 
  address
  that is passed to this routine(it could be INADDRY_ANY).
  So this should be changed back to sctp_v6_dst_saddr().
 
 No, see that's the problem. The source address isn't always stored in
 the  rt6_info. Therefore I copy the address into the flowi, so after
 ip6_route_output() fl does indeed contain the selected source address.
 Sorry if I didn't cc you all relevant patches from the patch set.

When you said that IPV6 route lookup returns the selected source
address, i assumed it will be returned via rt6_info-rt6i_src.  But
looks like it is returned in fl-fl6_src. If so, there is no problem.
Why is rt6i_src not filled in some cases?

I noticed this problem when i ran SCTP frametests that use a IP 
simulator in userspace. We have our own ip6_route_output that i 
modified to set source in rt6_info and not fl. 

Thanks
Sridhar

 
 Anyway there are still some unresolved issues with my code, but it's
 nice to know I didn't at least mess up SCTP in a big way ;-)
 
 Thanks,
 Ville

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/13] [SCTP] Merge IPv4 and IPv6 versions of get_saddr() with their corresponding get_dst().

2006-10-27 Thread Sridhar Samudrala 
On Tue, 2006-10-17 at 03:19 +0300, Ville Nuorvala wrote:
 As the IPv6 route lookup now also returns the selected source address
 there is no need for a separate source address lookup. In fact, the
 source address selection needs to be moved to get_dst() because the
 selected IPv6 source address isn't always stored in the route.
 Sometimes this makes it impossible to guess the correct address later on.
 

Ville,

Overall the patch looks pretty good. I found only 1 issue in 
sctp_v6_get_dst(). See below.


snip


 
 +/* Returns the dst cache entry for the given source and destination ip
 + * addresses.
 + */
 +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 flowi fl;
 + struct sctp_bind_addr *bp;
 + rwlock_t *addr_lock;
 + struct sctp_sockaddr_entry *laddr;
 + struct list_head *pos;
 + struct rt6_info *rt;
 + union sctp_addr baddr;
 + sctp_scope_t scope;
 + __u8 matchlen = 0;
 + __u8 bmatchlen;
 +
 + memset(fl, 0, sizeof(fl));
 + ipv6_addr_copy(fl.fl6_dst, daddr-v6.sin6_addr);
 + if (ipv6_addr_type(daddr-v6.sin6_addr)  IPV6_ADDR_LINKLOCAL)
 + fl.oif = daddr-v6.sin6_scope_id;
 +
 + ipv6_addr_copy(fl.fl6_src, saddr-v6.sin6_addr);
 + SCTP_DEBUG_PRINTK(%s: DST= NIP6_FMT  SRC= NIP6_FMT  ,
 +   __FUNCTION__, NIP6(fl.fl6_dst), NIP6(fl.fl6_src));
 +
 + dst = ip6_route_output(NULL, fl);
 + if (dst-error) {
 + dst_release(dst);
 + dst = NULL;
 + }
 + if (!ipv6_addr_any(saddr-v6.sin6_addr))
 + goto out;
 + if (!asoc) {
 + if (dst)
 + ipv6_addr_copy(saddr-v6.sin6_addr, fl.fl6_src);
 + goto out;
 + }
 + bp = asoc-base.bind_addr;
 + addr_lock = asoc-base.addr_lock;
 +
 + if (dst) {
 + /* Walk through the bind address list and look for a bind
 +  * address that matches the source address of the returned rt.
 +  */
 + sctp_v6_fl_saddr(baddr, fl, bp-port);
Here we are checking if the source address returned in the dst matches one of
the address in the bind address list of the association. Not the source address
that is passed to this routine(it could be INADDRY_ANY).
So this should be changed back to sctp_v6_dst_saddr().

Thanks
Sridhar

 + sctp_read_lock(addr_lock);
 + list_for_each(pos, bp-address_list) {
 + laddr = list_entry(pos, struct sctp_sockaddr_entry,
 +list);
 + if (!laddr-use_as_src)
 + continue;
 + if (sctp_v6_cmp_addr(baddr, laddr-a))
 + goto init_saddr;
 + }
 + sctp_read_unlock(addr_lock);
 +
 + /* Invalid rt or none of the bound addresses match the source
 +  * address. So release it.
 +  */
 + dst_release(dst);
 + dst = NULL;
 + }
 +
 + /* Go through the bind address list and find the best source address
 +  * that matches the scope of the destination address.
 +  */
 + memset(baddr, 0, sizeof(union sctp_addr));
 + scope = sctp_scope(daddr);
 + sctp_read_lock(addr_lock);
 + list_for_each(pos, bp-address_list) {
 + laddr = list_entry(pos, struct sctp_sockaddr_entry, list);
 + 
 + if (!laddr-use_as_src ||
 + laddr-a.sa.sa_family != AF_INET6 ||
 + scope  sctp_scope(laddr-a) ||
 + (ipv6_addr_type(laddr-a.v6.sin6_addr) 
 +  IPV6_ADDR_LINKLOCAL 
 +  laddr-a.v6.sin6_scope_id != fl.oif))
 + continue;
 +
 + bmatchlen = sctp_v6_addr_match_len(daddr, laddr-a);
 + if (!dst || (matchlen  bmatchlen)) {
 + struct dst_entry *dst2;
 + ipv6_addr_copy(fl.fl6_src, laddr-a.v6.sin6_addr);
 + dst2 = ip6_route_output(NULL, fl);
 + if (dst2-error) {
 + dst_release(dst2);
 + dst2 = NULL;
 + continue;
 + }
 + dst_release(dst);
 + dst = dst2;
 + memcpy(baddr, laddr-a, sizeof(union sctp_addr));
 + matchlen = bmatchlen;
 + }
 + }
 + if (dst)
 + goto init_saddr;
 +out_unlock:
 + sctp_read_unlock(addr_lock);
 +out:
 + if (dst) {
 + rt = (struct rt6_info *) dst;
 + SCTP_DEBUG_PRINTK(SRC= NIP6_FMT
 +rt6_dst= NIP6_FMT
 +rt6_src= NIP6_FMT \n,
 +

Re: [PATCH 9/13] [SCTP] Merge IPv4 and IPv6 versions of get_saddr() with their corresponding get_dst().

2006-10-17 Thread Sridhar Samudrala 

Ville Nuorvala wrote:

As the IPv6 route lookup now also returns the selected source address
there is no need for a separate source address lookup. In fact, the
source address selection needs to be moved to get_dst() because the
selected IPv6 source address isn't always stored in the route.
Sometimes this makes it impossible to guess the correct address later on.

  
I remember having  to do a separate call to ipv6_get_saddr() 
specifically because ip6_route_output()
didn't fill in the source address. Now if the route lookup also returns 
the source address, it looks logical
to remove the separate source address lookup. So i agree with the idea 
behind the patch.
I will review the patch in detail and try it out next week to see that 
it doesn't break SCTP

and  get back to you.
I guess this is targeted for 2.6.20. Is that right?

Thanks
Sridhar

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/13] [SCTP] Merge IPv4 and IPv6 versions of get_saddr() with their corresponding get_dst().

2006-10-16 Thread Ville Nuorvala 

As the IPv6 route lookup now also returns the selected source address
there is no need for a separate source address lookup. In fact, the
source address selection needs to be moved to get_dst() because the
selected IPv6 source address isn't always stored in the route.
Sometimes this makes it impossible to guess the correct address later on.

Signed-off-by: Ville Nuorvala [EMAIL PROTECTED]
---
 include/net/sctp/structs.h |7 -
 net/sctp/ipv6.c|  235 +++-
 net/sctp/protocol.c|   56 --
 net/sctp/transport.c   |8 +
 4 files changed, 148 insertions(+), 158 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index c6d93bb..e0973a3 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -529,15 +529,8 @@ struct sctp_af {
struct dst_entry *(*get_dst)(struct sctp_association *asoc,
 union sctp_addr *daddr,
 union sctp_addr *saddr);
-   void(*get_saddr)(struct sctp_association *asoc,
-struct dst_entry *dst,
-union sctp_addr *daddr,
-union sctp_addr *saddr);
void(*copy_addrlist) (struct list_head *,
  struct net_device *);
-   void(*dst_saddr)(union sctp_addr *saddr,
-struct dst_entry *dst,
-unsigned short port);
int (*cmp_addr) (const union sctp_addr *addr1,
 const union sctp_addr *addr2);
void(*addr_copy)(union sctp_addr *dst,
diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
index 78071c6..68ead54 100644
--- a/net/sctp/ipv6.c
+++ b/net/sctp/ipv6.c
@@ -188,46 +188,6 @@ static int sctp_v6_xmit(struct sk_buff *
return ip6_xmit(sk, skb, fl, np-opt, ipfragok);
 }

-/* Returns the dst cache entry for the given source and destination ip
- * addresses.
- */
-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 flowi fl;
-
-   memset(fl, 0, sizeof(fl));
-   ipv6_addr_copy(fl.fl6_dst, daddr-v6.sin6_addr);
-   if (ipv6_addr_type(daddr-v6.sin6_addr)  IPV6_ADDR_LINKLOCAL)
-   fl.oif = daddr-v6.sin6_scope_id;
-   
-
-   SCTP_DEBUG_PRINTK(%s: DST= NIP6_FMT  ,
- __FUNCTION__, NIP6(fl.fl6_dst));
-
-   if (saddr) {
-   ipv6_addr_copy(fl.fl6_src, saddr-v6.sin6_addr);
-   SCTP_DEBUG_PRINTK(
-   SRC= NIP6_FMT  - ,
-   NIP6(fl.fl6_src));
-   }
-
-   dst = ip6_route_output(NULL, fl);
-   if (!dst-error) {
-   struct rt6_info *rt;
-   rt = (struct rt6_info *)dst;
-   SCTP_DEBUG_PRINTK(
-   rt6_dst: NIP6_FMT  rt6_src: NIP6_FMT \n,
-   NIP6(rt-rt6i_dst.addr), NIP6(rt-rt6i_src.addr));
-   return dst;
-   }
-   SCTP_DEBUG_PRINTK(NO ROUTE\n);
-   dst_release(dst);
-   return NULL;
-}
-
 /* Returns the number of consecutive initial bits that match in the 2 ipv6
  * addresses.
  */
@@ -250,69 +210,6 @@ static inline int sctp_v6_addr_match_len
return (i*32);
 }

-/* Fills in the source address(saddr) based on the destination address(daddr)
- * and asoc's bind address list.
- */
-static void sctp_v6_get_saddr(struct sctp_association *asoc,
- struct dst_entry *dst,
- union sctp_addr *daddr,
- union sctp_addr *saddr)
-{
-   struct sctp_bind_addr *bp;
-   rwlock_t *addr_lock;
-   struct sctp_sockaddr_entry *laddr;
-   struct list_head *pos;
-   sctp_scope_t scope;
-   union sctp_addr *baddr = NULL;
-   __u8 matchlen = 0;
-   __u8 bmatchlen;
-
-   SCTP_DEBUG_PRINTK(%s: asoc:%p dst:%p 
- daddr: NIP6_FMT  ,
- __FUNCTION__, asoc, dst, NIP6(daddr-v6.sin6_addr));
-
-   if (!asoc) {
-   ipv6_get_saddr(dst, daddr-v6.sin6_addr,saddr-v6.sin6_addr);
-   SCTP_DEBUG_PRINTK(saddr from ipv6_get_saddr:  NIP6_FMT \n,
- NIP6(saddr-v6.sin6_addr));
-   return;
-   }
-
-   scope = sctp_scope(daddr);
-
-   bp = asoc-base.bind_addr;
-   addr_lock = asoc-base.addr_lock;
-
-   /* Go through the bind address list and find the best source address
-* that matches the scope of the destination address.
-*/
-   sctp_read_lock(addr_lock);
-