[PATCH net 0/2] sctp: not copying duplicate addrs to the assoc's bind address list

[PATCH net 0/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

Patch 1/2 is to fix some indent level.

Given that we have kernels out there with this issue, patch 2/2 also
fix sctp_raw_to_bind_addrs.

Xin Long (2):
  sctp: reduce indent level in sctp_copy_local_addr_list
  sctp: not copying duplicate addrs to the assoc's bind address list

 net/sctp/bind_addr.c |  3 +++
 net/sctp/protocol.c  | 40 ++++++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH net 0/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

Patch 1/2 is to fix some indent level.

Given that we have kernels out there with this issue, patch 2/2 also
fix sctp_raw_to_bind_addrs.

Xin Long (2):
  sctp: reduce indent level in sctp_copy_local_addr_list
  sctp: not copying duplicate addrs to the assoc's bind address list

 net/sctp/bind_addr.c |  3 +++
 net/sctp/protocol.c  | 40 ++++++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH net 1/2] sctp: reduce indent level in sctp_copy_local_addr_list

[Xin Long]

This patch is to reduce indent level by using continue when the addr
is not allowed, and also drop end_copy by using break.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/protocol.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 7b523e3..da5d82b 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -205,26 +205,27 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 	list_for_each_entry_rcu(addr, &net->sctp.local_addr_list, list) {
 		if (!addr->valid)
 			continue;
-		if (sctp_in_scope(net, &addr->a, scope)) {
-			/* Now that the address is in scope, check to see if
-			 * the address type is really supported by the local
-			 * sock as well as the remote peer.
-			 */
-			if ((((AF_INET == addr->a.sa.sa_family) &&
-			      (copy_flags & SCTP_ADDR4_PEERSUPP))) ||
-			    (((AF_INET6 == addr->a.sa.sa_family) &&
-			      (copy_flags & SCTP_ADDR6_ALLOWED) &&
-			      (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
-				error = sctp_add_bind_addr(bp, &addr->a,
-						    sizeof(addr->a),
-						    SCTP_ADDR_SRC, GFP_ATOMIC);
-				if (error)
-					goto end_copy;
-			}
-		}
+		if (!sctp_in_scope(net, &addr->a, scope))
+			continue;
+
+		/* Now that the address is in scope, check to see if
+		 * the address type is really supported by the local
+		 * sock as well as the remote peer.
+		 */
+		if (addr->a.sa.sa_family == AF_INET &&
+		    !(copy_flags & SCTP_ADDR4_PEERSUPP))
+			continue;
+		if (addr->a.sa.sa_family == AF_INET6 &&
+		    (!(copy_flags & SCTP_ADDR6_ALLOWED) ||
+		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
+			continue;
+
+		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
+					   SCTP_ADDR_SRC, GFP_ATOMIC);
+		if (error)
+			break;
 	}
 
-end_copy:
 	rcu_read_unlock();
 	return error;
 }
-- 
2.1.0

[PATCH net 1/2] sctp: reduce indent level in sctp_copy_local_addr_list

[Xin Long]

This patch is to reduce indent level by using continue when the addr
is not allowed, and also drop end_copy by using break.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/protocol.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 7b523e3..da5d82b 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -205,26 +205,27 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 	list_for_each_entry_rcu(addr, &net->sctp.local_addr_list, list) {
 		if (!addr->valid)
 			continue;
-		if (sctp_in_scope(net, &addr->a, scope)) {
-			/* Now that the address is in scope, check to see if
-			 * the address type is really supported by the local
-			 * sock as well as the remote peer.
-			 */
-			if ((((AF_INET = addr->a.sa.sa_family) &&
-			      (copy_flags & SCTP_ADDR4_PEERSUPP))) ||
-			    (((AF_INET6 = addr->a.sa.sa_family) &&
-			      (copy_flags & SCTP_ADDR6_ALLOWED) &&
-			      (copy_flags & SCTP_ADDR6_PEERSUPP)))) {
-				error = sctp_add_bind_addr(bp, &addr->a,
-						    sizeof(addr->a),
-						    SCTP_ADDR_SRC, GFP_ATOMIC);
-				if (error)
-					goto end_copy;
-			}
-		}
+		if (!sctp_in_scope(net, &addr->a, scope))
+			continue;
+
+		/* Now that the address is in scope, check to see if
+		 * the address type is really supported by the local
+		 * sock as well as the remote peer.
+		 */
+		if (addr->a.sa.sa_family = AF_INET &&
+		    !(copy_flags & SCTP_ADDR4_PEERSUPP))
+			continue;
+		if (addr->a.sa.sa_family = AF_INET6 &&
+		    (!(copy_flags & SCTP_ADDR6_ALLOWED) ||
+		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
+			continue;
+
+		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
+					   SCTP_ADDR_SRC, GFP_ATOMIC);
+		if (error)
+			break;
 	}
 
-end_copy:
 	rcu_read_unlock();
 	return error;
 }
-- 
2.1.0

[PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

From: Xin Long <lxin@redhat.com>

sctp.local_addr_list is a global address list that is supposed to include
all the local addresses. sctp updates this list according to NETDEV_UP/
NETDEV_DOWN notifications.

However, if multiple NICs have the same address, the global list will
have duplicate addresses. Even if for one NIC, promote secondaries in
__inet_del_ifa can also lead to accumulating duplicate addresses.

When sctp binds address 'ANY' and creates a connection, it copies all
the addresses from global list into asoc's bind addr list, which makes
sctp pack the duplicate addresses into INIT/INIT_ACK packets.

This patch is to filter the duplicate addresses when copying the addrs
from global list in sctp_copy_local_addr_list and unpacking addr_param
from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.

Signed-off-by: Xin Long <lxin@redhat.com>
---
 net/sctp/bind_addr.c | 3 +++
 net/sctp/protocol.c  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 401c607..1ebc184 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 		}
 
 		af->from_addr_param(&addr, rawaddr, htons(port), 0);
+		if (sctp_bind_addr_state(bp, &addr) != -1)
+			goto next;
 		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
 					    SCTP_ADDR_SRC, gfp);
 		if (retval) {
@@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 			break;
 		}
 
+next:
 		len = ntohs(param->length);
 		addrs_len -= len;
 		raw_addr_list += len;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index da5d82b..616a942 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
 			continue;
 
+		if (sctp_bind_addr_state(bp, &addr->a) != -1)
+			continue;
+
 		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
 					   SCTP_ADDR_SRC, GFP_ATOMIC);
 		if (error)
-- 
2.1.0

[PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

From: Xin Long <lxin@redhat.com>

sctp.local_addr_list is a global address list that is supposed to include
all the local addresses. sctp updates this list according to NETDEV_UP/
NETDEV_DOWN notifications.

However, if multiple NICs have the same address, the global list will
have duplicate addresses. Even if for one NIC, promote secondaries in
__inet_del_ifa can also lead to accumulating duplicate addresses.

When sctp binds address 'ANY' and creates a connection, it copies all
the addresses from global list into asoc's bind addr list, which makes
sctp pack the duplicate addresses into INIT/INIT_ACK packets.

This patch is to filter the duplicate addresses when copying the addrs
from global list in sctp_copy_local_addr_list and unpacking addr_param
from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.

Signed-off-by: Xin Long <lxin@redhat.com>
---
 net/sctp/bind_addr.c | 3 +++
 net/sctp/protocol.c  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index 401c607..1ebc184 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 		}
 
 		af->from_addr_param(&addr, rawaddr, htons(port), 0);
+		if (sctp_bind_addr_state(bp, &addr) != -1)
+			goto next;
 		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
 					    SCTP_ADDR_SRC, gfp);
 		if (retval) {
@@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
 			break;
 		}
 
+next:
 		len = ntohs(param->length);
 		addrs_len -= len;
 		raw_addr_list += len;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index da5d82b..616a942 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
 		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
 			continue;
 
+		if (sctp_bind_addr_state(bp, &addr->a) != -1)
+			continue;
+
 		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
 					   SCTP_ADDR_SRC, GFP_ATOMIC);
 		if (error)
-- 
2.1.0

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Fri, Aug 19, 2016 at 07:30:22PM +0800, Xin Long wrote:
> From: Xin Long <lxin@redhat.com>
> 
> sctp.local_addr_list is a global address list that is supposed to include
> all the local addresses. sctp updates this list according to NETDEV_UP/
> NETDEV_DOWN notifications.
> 
> However, if multiple NICs have the same address, the global list will
> have duplicate addresses. Even if for one NIC, promote secondaries in
> __inet_del_ifa can also lead to accumulating duplicate addresses.
> 
> When sctp binds address 'ANY' and creates a connection, it copies all
> the addresses from global list into asoc's bind addr list, which makes
> sctp pack the duplicate addresses into INIT/INIT_ACK packets.
> 
> This patch is to filter the duplicate addresses when copying the addrs
> from global list in sctp_copy_local_addr_list and unpacking addr_param
> from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.
> 
> Signed-off-by: Xin Long <lxin@redhat.com>


Under what valid use case will multiple interfaces have the same network
address?

Neil

> ---
>  net/sctp/bind_addr.c | 3 +++
>  net/sctp/protocol.c  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index 401c607..1ebc184 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  		}
>  
>  		af->from_addr_param(&addr, rawaddr, htons(port), 0);
> +		if (sctp_bind_addr_state(bp, &addr) != -1)
> +			goto next;
>  		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
>  					    SCTP_ADDR_SRC, gfp);
>  		if (retval) {
> @@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  			break;
>  		}
>  
> +next:
>  		len = ntohs(param->length);
>  		addrs_len -= len;
>  		raw_addr_list += len;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index da5d82b..616a942 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
>  		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
>  			continue;
>  
> +		if (sctp_bind_addr_state(bp, &addr->a) != -1)
> +			continue;
> +
>  		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
>  					   SCTP_ADDR_SRC, GFP_ATOMIC);
>  		if (error)
> -- 
> 2.1.0
> 
> --
> 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
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Fri, Aug 19, 2016 at 07:30:22PM +0800, Xin Long wrote:
> From: Xin Long <lxin@redhat.com>
> 
> sctp.local_addr_list is a global address list that is supposed to include
> all the local addresses. sctp updates this list according to NETDEV_UP/
> NETDEV_DOWN notifications.
> 
> However, if multiple NICs have the same address, the global list will
> have duplicate addresses. Even if for one NIC, promote secondaries in
> __inet_del_ifa can also lead to accumulating duplicate addresses.
> 
> When sctp binds address 'ANY' and creates a connection, it copies all
> the addresses from global list into asoc's bind addr list, which makes
> sctp pack the duplicate addresses into INIT/INIT_ACK packets.
> 
> This patch is to filter the duplicate addresses when copying the addrs
> from global list in sctp_copy_local_addr_list and unpacking addr_param
> from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.
> 
> Signed-off-by: Xin Long <lxin@redhat.com>


Under what valid use case will multiple interfaces have the same network
address?

Neil

> ---
>  net/sctp/bind_addr.c | 3 +++
>  net/sctp/protocol.c  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index 401c607..1ebc184 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  		}
>  
>  		af->from_addr_param(&addr, rawaddr, htons(port), 0);
> +		if (sctp_bind_addr_state(bp, &addr) != -1)
> +			goto next;
>  		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
>  					    SCTP_ADDR_SRC, gfp);
>  		if (retval) {
> @@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  			break;
>  		}
>  
> +next:
>  		len = ntohs(param->length);
>  		addrs_len -= len;
>  		raw_addr_list += len;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index da5d82b..616a942 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
>  		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
>  			continue;
>  
> +		if (sctp_bind_addr_state(bp, &addr->a) != -1)
> +			continue;
> +
>  		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
>  					   SCTP_ADDR_SRC, GFP_ATOMIC);
>  		if (error)
> -- 
> 2.1.0
> 
> --
> 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
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Under what valid use case will multiple interfaces have the same network
> address?
>
Hi, Neil.
I'm not sure the specific valid use case.

The point is, do we trust the sctp global addr list has no duplicate
address ? In one of users' computer, he found hundreds of duplicate
addresses in INIT_ACK packets.
(https://bugzilla.redhat.com/show_bug.cgi?id=1308362)

If we think NETDEV_UP/DOWN event in addrs chain always come
in pairs, I can not explain how come this issue happened.

But from sctp end, we can avoid it with this patch. after all
binding any duplicate addresses is illegal, just like what
sctp_bind does.

what do you think ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Under what valid use case will multiple interfaces have the same network
> address?
>
Hi, Neil.
I'm not sure the specific valid use case.

The point is, do we trust the sctp global addr list has no duplicate
address ? In one of users' computer, he found hundreds of duplicate
addresses in INIT_ACK packets.
(https://bugzilla.redhat.com/show_bug.cgi?id\x1308362)

If we think NETDEV_UP/DOWN event in addrs chain always come
in pairs, I can not explain how come this issue happened.

But from sctp end, we can avoid it with this patch. after all
binding any duplicate addresses is illegal, just like what
sctp_bind does.

what do you think ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Fri, Aug 19, 2016 at 07:30:22PM +0800, Xin Long wrote:
> From: Xin Long <lxin@redhat.com>
> 
> sctp.local_addr_list is a global address list that is supposed to include
> all the local addresses. sctp updates this list according to NETDEV_UP/
> NETDEV_DOWN notifications.
> 
> However, if multiple NICs have the same address, the global list will
> have duplicate addresses. Even if for one NIC, promote secondaries in
> __inet_del_ifa can also lead to accumulating duplicate addresses.
> 
> When sctp binds address 'ANY' and creates a connection, it copies all
> the addresses from global list into asoc's bind addr list, which makes
> sctp pack the duplicate addresses into INIT/INIT_ACK packets.
> 
> This patch is to filter the duplicate addresses when copying the addrs
> from global list in sctp_copy_local_addr_list and unpacking addr_param
> from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.
> 
> Signed-off-by: Xin Long <lxin@redhat.com>
> ---
>  net/sctp/bind_addr.c | 3 +++
>  net/sctp/protocol.c  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index 401c607..1ebc184 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  		}
>  
>  		af->from_addr_param(&addr, rawaddr, htons(port), 0);
> +		if (sctp_bind_addr_state(bp, &addr) != -1)
> +			goto next;
>  		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
>  					    SCTP_ADDR_SRC, gfp);
>  		if (retval) {
> @@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  			break;
>  		}
>  
> +next:
>  		len = ntohs(param->length);
>  		addrs_len -= len;
>  		raw_addr_list += len;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index da5d82b..616a942 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
>  		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
>  			continue;
>  
> +		if (sctp_bind_addr_state(bp, &addr->a) != -1)
> +			continue;
> +
>  		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
>  					   SCTP_ADDR_SRC, GFP_ATOMIC);
>  		if (error)
> -- 
> 2.1.0
> 
> --
> 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
> 
Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
side, when you get a cookie unpacked and modify the remote peers address list,
it makes sense to check for duplicates.  On the local side however, I would,
instead of checking it when the list gets copied, I'd check it when the master
list gets updated (in the NETDEV_UP event notifier for the local address list,
and the sctp_add_bind_addr function for the endpoint address list).  That way
you can keep that nested for loop out of the send path on the local system.

Neil

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Fri, Aug 19, 2016 at 07:30:22PM +0800, Xin Long wrote:
> From: Xin Long <lxin@redhat.com>
> 
> sctp.local_addr_list is a global address list that is supposed to include
> all the local addresses. sctp updates this list according to NETDEV_UP/
> NETDEV_DOWN notifications.
> 
> However, if multiple NICs have the same address, the global list will
> have duplicate addresses. Even if for one NIC, promote secondaries in
> __inet_del_ifa can also lead to accumulating duplicate addresses.
> 
> When sctp binds address 'ANY' and creates a connection, it copies all
> the addresses from global list into asoc's bind addr list, which makes
> sctp pack the duplicate addresses into INIT/INIT_ACK packets.
> 
> This patch is to filter the duplicate addresses when copying the addrs
> from global list in sctp_copy_local_addr_list and unpacking addr_param
> from cookie in sctp_raw_to_bind_addrs to asoc's bind addr list.
> 
> Signed-off-by: Xin Long <lxin@redhat.com>
> ---
>  net/sctp/bind_addr.c | 3 +++
>  net/sctp/protocol.c  | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index 401c607..1ebc184 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -292,6 +292,8 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  		}
>  
>  		af->from_addr_param(&addr, rawaddr, htons(port), 0);
> +		if (sctp_bind_addr_state(bp, &addr) != -1)
> +			goto next;
>  		retval = sctp_add_bind_addr(bp, &addr, sizeof(addr),
>  					    SCTP_ADDR_SRC, gfp);
>  		if (retval) {
> @@ -300,6 +302,7 @@ int sctp_raw_to_bind_addrs(struct sctp_bind_addr *bp, __u8 *raw_addr_list,
>  			break;
>  		}
>  
> +next:
>  		len = ntohs(param->length);
>  		addrs_len -= len;
>  		raw_addr_list += len;
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index da5d82b..616a942 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -220,6 +220,9 @@ int sctp_copy_local_addr_list(struct net *net, struct sctp_bind_addr *bp,
>  		     !(copy_flags & SCTP_ADDR6_PEERSUPP)))
>  			continue;
>  
> +		if (sctp_bind_addr_state(bp, &addr->a) != -1)
> +			continue;
> +
>  		error = sctp_add_bind_addr(bp, &addr->a, sizeof(addr->a),
>  					   SCTP_ADDR_SRC, GFP_ATOMIC);
>  		if (error)
> -- 
> 2.1.0
> 
> --
> 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
> 
Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
side, when you get a cookie unpacked and modify the remote peers address list,
it makes sense to check for duplicates.  On the local side however, I would,
instead of checking it when the list gets copied, I'd check it when the master
list gets updated (in the NETDEV_UP event notifier for the local address list,
and the sctp_add_bind_addr function for the endpoint address list).  That way
you can keep that nested for loop out of the send path on the local system.

Neil

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> side, when you get a cookie unpacked and modify the remote peers address list,
> it makes sense to check for duplicates.  On the local side however, I would,
> instead of checking it when the list gets copied, I'd check it when the master
> list gets updated (in the NETDEV_UP event notifier for the local address list,

I was thinking about to check it in the NETDEV_UP, yes it can make the
master list has no duplicated addresses.  But what if two same addresses
events come, and they come from different NICs (though I can't point  out
the valid use case), then we filter there.

Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
addr in the master list, but it shouldn't have been removed, as another local
NIC still has that addr.

That's why I have to leave the master alone, just check when they are really
being bind to asoc addr list.

> and the sctp_add_bind_addr function for the endpoint address list).  That way

As to the endpoint address list, sctp has different process for binding
the address 'ANY' from assoc address list (note that this issue only
happened in binding the address 'ANY'). instead of  copying the master
address list to  the endpoint, it only adds address 'ANY' to the EP
address list. Only when starting a connection and create the assoc, it
copy the master address list to ASOC.

So no need to do it in sctp_add_bind_addr for endpoint address list.
Besides, sctp_add_bind_addr  is supposed to be called after checking
the duplicated address(I got it from sctp_do_bind()). :-)

> you can keep that nested for loop out of the send path on the local system.
>
>

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> side, when you get a cookie unpacked and modify the remote peers address list,
> it makes sense to check for duplicates.  On the local side however, I would,
> instead of checking it when the list gets copied, I'd check it when the master
> list gets updated (in the NETDEV_UP event notifier for the local address list,

I was thinking about to check it in the NETDEV_UP, yes it can make the
master list has no duplicated addresses.  But what if two same addresses
events come, and they come from different NICs (though I can't point  out
the valid use case), then we filter there.

Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
addr in the master list, but it shouldn't have been removed, as another local
NIC still has that addr.

That's why I have to leave the master alone, just check when they are really
being bind to asoc addr list.

> and the sctp_add_bind_addr function for the endpoint address list).  That way

As to the endpoint address list, sctp has different process for binding
the address 'ANY' from assoc address list (note that this issue only
happened in binding the address 'ANY'). instead of  copying the master
address list to  the endpoint, it only adds address 'ANY' to the EP
address list. Only when starting a connection and create the assoc, it
copy the master address list to ASOC.

So no need to do it in sctp_add_bind_addr for endpoint address list.
Besides, sctp_add_bind_addr  is supposed to be called after checking
the duplicated address(I got it from sctp_do_bind()). :-)

> you can keep that nested for loop out of the send path on the local system.
>
>

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Sat, Aug 20, 2016 at 02:41:01PM +0800, Xin Long wrote:
> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> > side, when you get a cookie unpacked and modify the remote peers address list,
> > it makes sense to check for duplicates.  On the local side however, I would,
> > instead of checking it when the list gets copied, I'd check it when the master
> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> 
> I was thinking about to check it in the NETDEV_UP, yes it can make the
> master list has no duplicated addresses.  But what if two same addresses
> events come, and they come from different NICs (though I can't point  out
> the valid use case), then we filter there.
> 
That I think would be a bug in the protocol code.  For the ipv4 case, all
addresses are owned by the system and the same addresses added to multiple
interfaces should not be allowed.  The same is true of ipv6 case.  The only
exception there is a link local address and that should still be unique within
the context of an address/dev tuple.

> Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
> addr in the master list, but it shouldn't have been removed, as another local
> NIC still has that addr.
> 
> That's why I have to leave the master alone, just check when they are really
> being bind to asoc addr list.
> 
> > and the sctp_add_bind_addr function for the endpoint address list).  That way
> 
> As to the endpoint address list, sctp has different process for binding
> the address 'ANY' from assoc address list (note that this issue only
> happened in binding the address 'ANY'). instead of  copying the master
> address list to  the endpoint, it only adds address 'ANY' to the EP
> address list. Only when starting a connection and create the assoc, it
> copy the master address list to ASOC.
> 
> So no need to do it in sctp_add_bind_addr for endpoint address list.
> Besides, sctp_add_bind_addr  is supposed to be called after checking
> the duplicated address(I got it from sctp_do_bind()). :-)
> 
> > you can keep that nested for loop out of the send path on the local system.
> >
> >
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Sat, Aug 20, 2016 at 02:41:01PM +0800, Xin Long wrote:
> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> > side, when you get a cookie unpacked and modify the remote peers address list,
> > it makes sense to check for duplicates.  On the local side however, I would,
> > instead of checking it when the list gets copied, I'd check it when the master
> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> 
> I was thinking about to check it in the NETDEV_UP, yes it can make the
> master list has no duplicated addresses.  But what if two same addresses
> events come, and they come from different NICs (though I can't point  out
> the valid use case), then we filter there.
> 
That I think would be a bug in the protocol code.  For the ipv4 case, all
addresses are owned by the system and the same addresses added to multiple
interfaces should not be allowed.  The same is true of ipv6 case.  The only
exception there is a link local address and that should still be unique within
the context of an address/dev tuple.

> Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
> addr in the master list, but it shouldn't have been removed, as another local
> NIC still has that addr.
> 
> That's why I have to leave the master alone, just check when they are really
> being bind to asoc addr list.
> 
> > and the sctp_add_bind_addr function for the endpoint address list).  That way
> 
> As to the endpoint address list, sctp has different process for binding
> the address 'ANY' from assoc address list (note that this issue only
> happened in binding the address 'ANY'). instead of  copying the master
> address list to  the endpoint, it only adds address 'ANY' to the EP
> address list. Only when starting a connection and create the assoc, it
> copy the master address list to ASOC.
> 
> So no need to do it in sctp_add_bind_addr for endpoint address list.
> Besides, sctp_add_bind_addr  is supposed to be called after checking
> the duplicated address(I got it from sctp_do_bind()). :-)
> 
> > you can keep that nested for loop out of the send path on the local system.
> >
> >
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

>> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
>> > side, when you get a cookie unpacked and modify the remote peers address list,
>> > it makes sense to check for duplicates.  On the local side however, I would,
>> > instead of checking it when the list gets copied, I'd check it when the master
>> > list gets updated (in the NETDEV_UP event notifier for the local address list,
>>
>> I was thinking about to check it in the NETDEV_UP, yes it can make the
>> master list has no duplicated addresses.  But what if two same addresses
>> events come, and they come from different NICs (though I can't point  out
>> the valid use case), then we filter there.
>>
> That I think would be a bug in the protocol code.  For the ipv4 case, all
> addresses are owned by the system and the same addresses added to multiple
> interfaces should not be allowed.  The same is true of ipv6 case.  The only
> exception there is a link local address and that should still be unique within
> the context of an address/dev tuple.
>
understand, just sounds a little harsh. :-)

For now, does it make sense to you to just leave as the master list
is, and check
the duplicate address when sctp is really binding them ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

>> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
>> > side, when you get a cookie unpacked and modify the remote peers address list,
>> > it makes sense to check for duplicates.  On the local side however, I would,
>> > instead of checking it when the list gets copied, I'd check it when the master
>> > list gets updated (in the NETDEV_UP event notifier for the local address list,
>>
>> I was thinking about to check it in the NETDEV_UP, yes it can make the
>> master list has no duplicated addresses.  But what if two same addresses
>> events come, and they come from different NICs (though I can't point  out
>> the valid use case), then we filter there.
>>
> That I think would be a bug in the protocol code.  For the ipv4 case, all
> addresses are owned by the system and the same addresses added to multiple
> interfaces should not be allowed.  The same is true of ipv6 case.  The only
> exception there is a link local address and that should still be unique within
> the context of an address/dev tuple.
>
understand, just sounds a little harsh. :-)

For now, does it make sense to you to just leave as the master list
is, and check
the duplicate address when sctp is really binding them ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> >> > side, when you get a cookie unpacked and modify the remote peers address list,
> >> > it makes sense to check for duplicates.  On the local side however, I would,
> >> > instead of checking it when the list gets copied, I'd check it when the master
> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> >>
> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
> >> master list has no duplicated addresses.  But what if two same addresses
> >> events come, and they come from different NICs (though I can't point  out
> >> the valid use case), then we filter there.
> >>
> > That I think would be a bug in the protocol code.  For the ipv4 case, all
> > addresses are owned by the system and the same addresses added to multiple
> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
> > exception there is a link local address and that should still be unique within
> > the context of an address/dev tuple.
> >
> understand, just sounds a little harsh. :-)
> 
> For now, does it make sense to you to just leave as the master list
> is, and check
> the duplicate address when sctp is really binding them ?
> 
I would think so, yes.
Neil

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> >> > side, when you get a cookie unpacked and modify the remote peers address list,
> >> > it makes sense to check for duplicates.  On the local side however, I would,
> >> > instead of checking it when the list gets copied, I'd check it when the master
> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> >>
> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
> >> master list has no duplicated addresses.  But what if two same addresses
> >> events come, and they come from different NICs (though I can't point  out
> >> the valid use case), then we filter there.
> >>
> > That I think would be a bug in the protocol code.  For the ipv4 case, all
> > addresses are owned by the system and the same addresses added to multiple
> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
> > exception there is a link local address and that should still be unique within
> > the context of an address/dev tuple.
> >
> understand, just sounds a little harsh. :-)
> 
> For now, does it make sense to you to just leave as the master list
> is, and check
> the duplicate address when sctp is really binding them ?
> 
I would think so, yes.
Neil

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Mon, Aug 22, 2016 at 10:25:38AM -0400, Neil Horman wrote:
> On Sat, Aug 20, 2016 at 02:41:01PM +0800, Xin Long wrote:
> > > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> > > side, when you get a cookie unpacked and modify the remote peers address list,
> > > it makes sense to check for duplicates.  On the local side however, I would,
> > > instead of checking it when the list gets copied, I'd check it when the master
> > > list gets updated (in the NETDEV_UP event notifier for the local address list,
> > 
> > I was thinking about to check it in the NETDEV_UP, yes it can make the
> > master list has no duplicated addresses.  But what if two same addresses
> > events come, and they come from different NICs (though I can't point  out
> > the valid use case), then we filter there.
> > 

I guess a valid use case would be the poor man's roaming between wifi
and ethernet with both mac addresses assigned to the same IP address, so
that you don't terminate your connections when moving from one to
another. This works quite well.

It could even be just a temporary config during setup. Like, a sysadmin
forgot to remove the address from a NIC before adding on the other one,
and then noticed it. For a while, the system would have the address
assigned to two interfaces.

> That I think would be a bug in the protocol code.  For the ipv4 case, all
> addresses are owned by the system and the same addresses added to multiple
> interfaces should not be allowed.  The same is true of ipv6 case.  The only
> exception there is a link local address and that should still be unique within
> the context of an address/dev tuple.
> 

Maybe it should not but there is nothing stopping you from doing so.

> > Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
> > addr in the master list, but it shouldn't have been removed, as another local
> > NIC still has that addr.
> > 
> > That's why I have to leave the master alone, just check when they are really
> > being bind to asoc addr list.
> > 

Or add a refcnt to its members. </idea>
NETDEV_UP, it gets a ++ if it's already there
NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
And the rest probably could stay the same.

> > > and the sctp_add_bind_addr function for the endpoint address list).  That way
> > 
> > As to the endpoint address list, sctp has different process for binding
> > the address 'ANY' from assoc address list (note that this issue only
> > happened in binding the address 'ANY'). instead of  copying the master
> > address list to  the endpoint, it only adds address 'ANY' to the EP
> > address list. Only when starting a connection and create the assoc, it
> > copy the master address list to ASOC.
> > 
> > So no need to do it in sctp_add_bind_addr for endpoint address list.
> > Besides, sctp_add_bind_addr  is supposed to be called after checking
> > the duplicated address(I got it from sctp_do_bind()). :-)
> > 
> > > you can keep that nested for loop out of the send path on the local system.
> > >
> > >
> > 
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Mon, Aug 22, 2016 at 10:25:38AM -0400, Neil Horman wrote:
> On Sat, Aug 20, 2016 at 02:41:01PM +0800, Xin Long wrote:
> > > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> > > side, when you get a cookie unpacked and modify the remote peers address list,
> > > it makes sense to check for duplicates.  On the local side however, I would,
> > > instead of checking it when the list gets copied, I'd check it when the master
> > > list gets updated (in the NETDEV_UP event notifier for the local address list,
> > 
> > I was thinking about to check it in the NETDEV_UP, yes it can make the
> > master list has no duplicated addresses.  But what if two same addresses
> > events come, and they come from different NICs (though I can't point  out
> > the valid use case), then we filter there.
> > 

I guess a valid use case would be the poor man's roaming between wifi
and ethernet with both mac addresses assigned to the same IP address, so
that you don't terminate your connections when moving from one to
another. This works quite well.

It could even be just a temporary config during setup. Like, a sysadmin
forgot to remove the address from a NIC before adding on the other one,
and then noticed it. For a while, the system would have the address
assigned to two interfaces.

> That I think would be a bug in the protocol code.  For the ipv4 case, all
> addresses are owned by the system and the same addresses added to multiple
> interfaces should not be allowed.  The same is true of ipv6 case.  The only
> exception there is a link local address and that should still be unique within
> the context of an address/dev tuple.
> 

Maybe it should not but there is nothing stopping you from doing so.

> > Later, sctp may receive one  NETDEV_DOWN event,sctp will remove that
> > addr in the master list, but it shouldn't have been removed, as another local
> > NIC still has that addr.
> > 
> > That's why I have to leave the master alone, just check when they are really
> > being bind to asoc addr list.
> > 

Or add a refcnt to its members. </idea>
NETDEV_UP, it gets a ++ if it's already there
NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
And the rest probably could stay the same.

> > > and the sctp_add_bind_addr function for the endpoint address list).  That way
> > 
> > As to the endpoint address list, sctp has different process for binding
> > the address 'ANY' from assoc address list (note that this issue only
> > happened in binding the address 'ANY'). instead of  copying the master
> > address list to  the endpoint, it only adds address 'ANY' to the EP
> > address list. Only when starting a connection and create the assoc, it
> > copy the master address list to ASOC.
> > 
> > So no need to do it in sctp_add_bind_addr for endpoint address list.
> > Besides, sctp_add_bind_addr  is supposed to be called after checking
> > the duplicated address(I got it from sctp_do_bind()). :-)
> > 
> > > you can keep that nested for loop out of the send path on the local system.
> > >
> > >
> > 
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Or add a refcnt to its members. </idea>
> NETDEV_UP, it gets a ++ if it's already there
> NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
> And the rest probably could stay the same.
>
Yes, it could also avoid the issue of amounts of duplicate addrs.
or add a nic index variable to  its members.

But I still prefer the current patch.
1. This issue only happens when server bind 'ANY' addresses.
    we don't need to add any new members to struct sctp_sockaddr_entry.
    especially if it's a really corner issue,  we fix this as an improvement.

2. It's yet two issues  here, the duplicate addrs may be from
   a) different local NICs.
   b) the same one NIC.
   It may be unexpectable to filter them in NETDEV_UP/DOWN events.

3. We check it only when sctp really binds it, just like sctp_do_bind.

What do you think ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

> Or add a refcnt to its members. </idea>
> NETDEV_UP, it gets a ++ if it's already there
> NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
> And the rest probably could stay the same.
>
Yes, it could also avoid the issue of amounts of duplicate addrs.
or add a nic index variable to  its members.

But I still prefer the current patch.
1. This issue only happens when server bind 'ANY' addresses.
    we don't need to add any new members to struct sctp_sockaddr_entry.
    especially if it's a really corner issue,  we fix this as an improvement.

2. It's yet two issues  here, the duplicate addrs may be from
   a) different local NICs.
   b) the same one NIC.
   It may be unexpectable to filter them in NETDEV_UP/DOWN events.

3. We check it only when sctp really binds it, just like sctp_do_bind.

What do you think ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Thu, Aug 25, 2016 at 12:03:30PM +0800, Xin Long wrote:
> > Or add a refcnt to its members. </idea>
> > NETDEV_UP, it gets a ++ if it's already there
> > NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
> > And the rest probably could stay the same.
> >
> Yes, it could also avoid the issue of amounts of duplicate addrs.
> or add a nic index variable to  its members.
> 
> But I still prefer the current patch.
> 1. This issue only happens when server bind 'ANY' addresses.
>     we don't need to add any new members to struct sctp_sockaddr_entry.
>     especially if it's a really corner issue,  we fix this as an improvement.
> 
> 2. It's yet two issues  here, the duplicate addrs may be from
>    a) different local NICs.
>    b) the same one NIC.
>    It may be unexpectable to filter them in NETDEV_UP/DOWN events.
> 
> 3. We check it only when sctp really binds it, just like sctp_do_bind.
> 
> What do you think ?

Yep, +1. LGTM the current patch, thanks.

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Thu, Aug 25, 2016 at 12:03:30PM +0800, Xin Long wrote:
> > Or add a refcnt to its members. </idea>
> > NETDEV_UP, it gets a ++ if it's already there
> > NETDEV_DOWN, it gets a -- and cleans it up if it reaches 0
> > And the rest probably could stay the same.
> >
> Yes, it could also avoid the issue of amounts of duplicate addrs.
> or add a nic index variable to  its members.
> 
> But I still prefer the current patch.
> 1. This issue only happens when server bind 'ANY' addresses.
>     we don't need to add any new members to struct sctp_sockaddr_entry.
>     especially if it's a really corner issue,  we fix this as an improvement.
> 
> 2. It's yet two issues  here, the duplicate addrs may be from
>    a) different local NICs.
>    b) the same one NIC.
>    It may be unexpectable to filter them in NETDEV_UP/DOWN events.
> 
> 3. We check it only when sctp really binds it, just like sctp_do_bind.
> 
> What do you think ?

Yep, +1. LGTM the current patch, thanks.

RE: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[David Laight]

From: Of Xin Long
> Sent: 25 August 2016 05:04
...
> But I still prefer the current patch.
> 1. This issue only happens when server bind 'ANY' addresses.
>     we don't need to add any new members to struct sctp_sockaddr_entry.
>     especially if it's a really corner issue,  we fix this as an improvement.
> 
> 2. It's yet two issues  here, the duplicate addrs may be from
>    a) different local NICs.
>    b) the same one NIC.
>    It may be unexpectable to filter them in NETDEV_UP/DOWN events.
> 
> 3. We check it only when sctp really binds it, just like sctp_do_bind.
> 
> What do you think ?

I want to know what kind of weed the SCTP authors were smoking when they
decided it was appropriate to put all of a systems IP addresses in the
cookie and cookie-ack messages.

It would be nice to have the local addresses selected by the kernel on the
basis of the remote address - removing the necessity of the application
to know the current network topology (and IP addresses) in order to bind
to the correct local addresses before making an outward call.

This sort of requires that local addresses for a connection are more of a
property of the routing table than anything else.

Consider the following network:

    ----+---------------+----------------------+---------
        |               |                      |
     x.x.1.1         x.x.1.2                y.y.1.2
    10.1.1.1        10.1.1.2               10.1.1.2
        |               |                      |
    ----+---------------+                      +---------

A connection from x.x.1.1 to x.x.1.2 needs to specify the 10.1.1.* addresses,
but a connection for x.x.1.1 to y.y.1.2 must not.

I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
that can accept connections from both x.x.1.2 and y.y.1.2.

	David

RE: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[David Laight]

RnJvbTogT2YgWGluIExvbmcNCj4gU2VudDogMjUgQXVndXN0IDIwMTYgMDU6MDQNCi4uLg0KPiBC
dXQgSSBzdGlsbCBwcmVmZXIgdGhlIGN1cnJlbnQgcGF0Y2guDQo+IDEuIFRoaXMgaXNzdWUgb25s
eSBoYXBwZW5zIHdoZW4gc2VydmVyIGJpbmQgJ0FOWScgYWRkcmVzc2VzLg0KPiAgICAgd2UgZG9u
J3QgbmVlZCB0byBhZGQgYW55IG5ldyBtZW1iZXJzIHRvIHN0cnVjdCBzY3RwX3NvY2thZGRyX2Vu
dHJ5Lg0KPiAgICAgZXNwZWNpYWxseSBpZiBpdCdzIGEgcmVhbGx5IGNvcm5lciBpc3N1ZSwgIHdl
IGZpeCB0aGlzIGFzIGFuIGltcHJvdmVtZW50Lg0KPiANCj4gMi4gSXQncyB5ZXQgdHdvIGlzc3Vl
cyAgaGVyZSwgdGhlIGR1cGxpY2F0ZSBhZGRycyBtYXkgYmUgZnJvbQ0KPiAgICBhKSBkaWZmZXJl
bnQgbG9jYWwgTklDcy4NCj4gICAgYikgdGhlIHNhbWUgb25lIE5JQy4NCj4gICAgSXQgbWF5IGJl
IHVuZXhwZWN0YWJsZSB0byBmaWx0ZXIgdGhlbSBpbiBORVRERVZfVVAvRE9XTiBldmVudHMuDQo+
IA0KPiAzLiBXZSBjaGVjayBpdCBvbmx5IHdoZW4gc2N0cCByZWFsbHkgYmluZHMgaXQsIGp1c3Qg
bGlrZSBzY3RwX2RvX2JpbmQuDQo+IA0KPiBXaGF0IGRvIHlvdSB0aGluayA/DQoNCkkgd2FudCB0
byBrbm93IHdoYXQga2luZCBvZiB3ZWVkIHRoZSBTQ1RQIGF1dGhvcnMgd2VyZSBzbW9raW5nIHdo
ZW4gdGhleQ0KZGVjaWRlZCBpdCB3YXMgYXBwcm9wcmlhdGUgdG8gcHV0IGFsbCBvZiBhIHN5c3Rl
bXMgSVAgYWRkcmVzc2VzIGluIHRoZQ0KY29va2llIGFuZCBjb29raWUtYWNrIG1lc3NhZ2VzLg0K
DQpJdCB3b3VsZCBiZSBuaWNlIHRvIGhhdmUgdGhlIGxvY2FsIGFkZHJlc3NlcyBzZWxlY3RlZCBi
eSB0aGUga2VybmVsIG9uIHRoZQ0KYmFzaXMgb2YgdGhlIHJlbW90ZSBhZGRyZXNzIC0gcmVtb3Zp
bmcgdGhlIG5lY2Vzc2l0eSBvZiB0aGUgYXBwbGljYXRpb24NCnRvIGtub3cgdGhlIGN1cnJlbnQg
bmV0d29yayB0b3BvbG9neSAoYW5kIElQIGFkZHJlc3NlcykgaW4gb3JkZXIgdG8gYmluZA0KdG8g
dGhlIGNvcnJlY3QgbG9jYWwgYWRkcmVzc2VzIGJlZm9yZSBtYWtpbmcgYW4gb3V0d2FyZCBjYWxs
Lg0KDQpUaGlzIHNvcnQgb2YgcmVxdWlyZXMgdGhhdCBsb2NhbCBhZGRyZXNzZXMgZm9yIGEgY29u
bmVjdGlvbiBhcmUgbW9yZSBvZiBhDQpwcm9wZXJ0eSBvZiB0aGUgcm91dGluZyB0YWJsZSB0aGFu
IGFueXRoaW5nIGVsc2UuDQoNCkNvbnNpZGVyIHRoZSBmb2xsb3dpbmcgbmV0d29yazoNCg0KICAg
IC0tLS0rLS0tLS0tLS0tLS0tLS0tKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0rLS0tLS0tLS0tDQog
ICAgICAgIHwgICAgICAgICAgICAgICB8ICAgICAgICAgICAgICAgICAgICAgIHwNCiAgICAgeC54
LjEuMSAgICAgICAgIHgueC4xLjIgICAgICAgICAgICAgICAgeS55LjEuMg0KICAgIDEwLjEuMS4x
ICAgICAgICAxMC4xLjEuMiAgICAgICAgICAgICAgIDEwLjEuMS4yDQogICAgICAgIHwgICAgICAg
ICAgICAgICB8ICAgICAgICAgICAgICAgICAgICAgIHwNCiAgICAtLS0tKy0tLS0tLS0tLS0tLS0t
LSsgICAgICAgICAgICAgICAgICAgICAgKy0tLS0tLS0tLQ0KDQpBIGNvbm5lY3Rpb24gZnJvbSB4
LnguMS4xIHRvIHgueC4xLjIgbmVlZHMgdG8gc3BlY2lmeSB0aGUgMTAuMS4xLiogYWRkcmVzc2Vz
LA0KYnV0IGEgY29ubmVjdGlvbiBmb3IgeC54LjEuMSB0byB5LnkuMS4yIG11c3Qgbm90Lg0KDQpJ
J20gbm90IGF0IHN1cmUgd2hldGhlciBpdCBpcyBwb3NzaWJsZSB0byBzZXR1cCBsaXN0ZW5lcihz
KSBvbiB4LnguMS4xDQp0aGF0IGNhbiBhY2NlcHQgY29ubmVjdGlvbnMgZnJvbSBib3RoIHgueC4x
LjIgYW5kIHkueS4xLjIuDQoNCglEYXZpZA0KDQo

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Fri, Sep 02, 2016 at 01:22:05PM +0000, David Laight wrote:
> From: Of Xin Long
> > Sent: 25 August 2016 05:04
> ...
> > But I still prefer the current patch.
> > 1. This issue only happens when server bind 'ANY' addresses.
> >     we don't need to add any new members to struct sctp_sockaddr_entry.
> >     especially if it's a really corner issue,  we fix this as an improvement.
> > 
> > 2. It's yet two issues  here, the duplicate addrs may be from
> >    a) different local NICs.
> >    b) the same one NIC.
> >    It may be unexpectable to filter them in NETDEV_UP/DOWN events.
> > 
> > 3. We check it only when sctp really binds it, just like sctp_do_bind.
> > 
> > What do you think ?
> 
> I want to know what kind of weed the SCTP authors were smoking when they
> decided it was appropriate to put all of a systems IP addresses in the
> cookie and cookie-ack messages.
> 

The 'best effort' one I guess :-)

> It would be nice to have the local addresses selected by the kernel on the
> basis of the remote address - removing the necessity of the application
> to know the current network topology (and IP addresses) in order to bind
> to the correct local addresses before making an outward call.
> 
> This sort of requires that local addresses for a connection are more of a
> property of the routing table than anything else.
> 
> Consider the following network:
> 
>     ----+---------------+----------------------+---------
>         |               |                      |
>      x.x.1.1         x.x.1.2                y.y.1.2
>     10.1.1.1        10.1.1.2               10.1.1.2
>         |               |                      |
>     ----+---------------+                      +---------
> 
> A connection from x.x.1.1 to x.x.1.2 needs to specify the 10.1.1.* addresses,
> but a connection for x.x.1.1 to y.y.1.2 must not.
> 

Exactly. Your example is an exact match with the issue I had last week.
That 10.1.1.2 of yours, may very well be 192.168.122.1 from libvirt.
Then the host may attempt to send the packet to itself, and abort the
association due to that.

I hated this because it took me a big while to find out about it. 

I guess they just didn't predict this situation of duplicated/internal
addresses. Otherwise, the address is there, reachable or not. It's in
the best effort situation.

> I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> that can accept connections from both x.x.1.2 and y.y.1.2.

You mean without an explicit bind()?

  Marcelo

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Marcelo Ricardo Leitner]

On Fri, Sep 02, 2016 at 01:22:05PM +0000, David Laight wrote:
> From: Of Xin Long
> > Sent: 25 August 2016 05:04
> ...
> > But I still prefer the current patch.
> > 1. This issue only happens when server bind 'ANY' addresses.
> >     we don't need to add any new members to struct sctp_sockaddr_entry.
> >     especially if it's a really corner issue,  we fix this as an improvement.
> > 
> > 2. It's yet two issues  here, the duplicate addrs may be from
> >    a) different local NICs.
> >    b) the same one NIC.
> >    It may be unexpectable to filter them in NETDEV_UP/DOWN events.
> > 
> > 3. We check it only when sctp really binds it, just like sctp_do_bind.
> > 
> > What do you think ?
> 
> I want to know what kind of weed the SCTP authors were smoking when they
> decided it was appropriate to put all of a systems IP addresses in the
> cookie and cookie-ack messages.
> 

The 'best effort' one I guess :-)

> It would be nice to have the local addresses selected by the kernel on the
> basis of the remote address - removing the necessity of the application
> to know the current network topology (and IP addresses) in order to bind
> to the correct local addresses before making an outward call.
> 
> This sort of requires that local addresses for a connection are more of a
> property of the routing table than anything else.
> 
> Consider the following network:
> 
>     ----+---------------+----------------------+---------
>         |               |                      |
>      x.x.1.1         x.x.1.2                y.y.1.2
>     10.1.1.1        10.1.1.2               10.1.1.2
>         |               |                      |
>     ----+---------------+                      +---------
> 
> A connection from x.x.1.1 to x.x.1.2 needs to specify the 10.1.1.* addresses,
> but a connection for x.x.1.1 to y.y.1.2 must not.
> 

Exactly. Your example is an exact match with the issue I had last week.
That 10.1.1.2 of yours, may very well be 192.168.122.1 from libvirt.
Then the host may attempt to send the packet to itself, and abort the
association due to that.

I hated this because it took me a big while to find out about it. 

I guess they just didn't predict this situation of duplicated/internal
addresses. Otherwise, the address is there, reachable or not. It's in
the best effort situation.

> I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> that can accept connections from both x.x.1.2 and y.y.1.2.

You mean without an explicit bind()?

  Marcelo

RE: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 02 September 2016 14:47
...
> > Consider the following network:
> >
> >     ----+---------------+----------------------+---------
> >         |               |                      |
> >      x.x.1.1         x.x.1.2                y.y.1.2
> >     10.1.1.1        10.1.1.2               10.1.1.2
> >         |               |                      |
> >     ----+---------------+                      +---------
...
> > I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> > that can accept connections from both x.x.1.2 and y.y.1.2.
> 
> You mean without an explicit bind()?

You might be able to bind one socket to x.x.1.1 and a second to 10.1.1.1
and x.x.1.1 so that connections to x.x.1.1 are only offered x.x.1.1 and
those to 10.1.1.1 are offered both addresses.

But you can't make the init-ack responses for connections to x.x.1.1
depend on whether the connect came from x.x.1.2 or y.y.1.2.

If the source and destination ports are fixed (as they usually are for M3UA)
you can sit trying to make an outward connection, relying on the init collision
code to work properly. Far from ideal!

	David

RE: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 02 September 2016 14:47
...
> > Consider the following network:
> >
> >     ----+---------------+----------------------+---------
> >         |               |                      |
> >      x.x.1.1         x.x.1.2                y.y.1.2
> >     10.1.1.1        10.1.1.2               10.1.1.2
> >         |               |                      |
> >     ----+---------------+                      +---------
...
> > I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> > that can accept connections from both x.x.1.2 and y.y.1.2.
> 
> You mean without an explicit bind()?

You might be able to bind one socket to x.x.1.1 and a second to 10.1.1.1
and x.x.1.1 so that connections to x.x.1.1 are only offered x.x.1.1 and
those to 10.1.1.1 are offered both addresses.

But you can't make the init-ack responses for connections to x.x.1.1
depend on whether the connect came from x.x.1.2 or y.y.1.2.

If the source and destination ports are fixed (as they usually are for M3UA)
you can sit trying to make an outward connection, relying on the init collision
code to work properly. Far from ideal!

	David

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

['Marcelo Ricardo Leitner']

On Fri, Sep 02, 2016 at 02:25:42PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 02 September 2016 14:47
> ...
> > > Consider the following network:
> > >
> > >     ----+---------------+----------------------+---------
> > >         |               |                      |
> > >      x.x.1.1         x.x.1.2                y.y.1.2
> > >     10.1.1.1        10.1.1.2               10.1.1.2
> > >         |               |                      |
> > >     ----+---------------+                      +---------
> ...
> > > I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> > > that can accept connections from both x.x.1.2 and y.y.1.2.
> > 
> > You mean without an explicit bind()?
> 
> You might be able to bind one socket to x.x.1.1 and a second to 10.1.1.1
> and x.x.1.1 so that connections to x.x.1.1 are only offered x.x.1.1 and
> those to 10.1.1.1 are offered both addresses.
> 
> But you can't make the init-ack responses for connections to x.x.1.1
> depend on whether the connect came from x.x.1.2 or y.y.1.2.
> 
> If the source and destination ports are fixed (as they usually are for M3UA)
> you can sit trying to make an outward connection, relying on the init collision
> code to work properly. Far from ideal!

Ahh yes, I see now. Yep..

  Marcelo

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

['Marcelo Ricardo Leitner']

On Fri, Sep 02, 2016 at 02:25:42PM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 02 September 2016 14:47
> ...
> > > Consider the following network:
> > >
> > >     ----+---------------+----------------------+---------
> > >         |               |                      |
> > >      x.x.1.1         x.x.1.2                y.y.1.2
> > >     10.1.1.1        10.1.1.2               10.1.1.2
> > >         |               |                      |
> > >     ----+---------------+                      +---------
> ...
> > > I'm not at sure whether it is possible to setup listener(s) on x.x.1.1
> > > that can accept connections from both x.x.1.2 and y.y.1.2.
> > 
> > You mean without an explicit bind()?
> 
> You might be able to bind one socket to x.x.1.1 and a second to 10.1.1.1
> and x.x.1.1 so that connections to x.x.1.1 are only offered x.x.1.1 and
> those to 10.1.1.1 are offered both addresses.
> 
> But you can't make the init-ack responses for connections to x.x.1.1
> depend on whether the connect came from x.x.1.2 or y.y.1.2.
> 
> If the source and destination ports are fixed (as they usually are for M3UA)
> you can sit trying to make an outward connection, relying on the init collision
> code to work properly. Far from ideal!

Ahh yes, I see now. Yep..

  Marcelo

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

On Wed, Aug 24, 2016 at 6:38 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
>> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
>> >> > side, when you get a cookie unpacked and modify the remote peers address list,
>> >> > it makes sense to check for duplicates.  On the local side however, I would,
>> >> > instead of checking it when the list gets copied, I'd check it when the master
>> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
>> >>
>> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
>> >> master list has no duplicated addresses.  But what if two same addresses
>> >> events come, and they come from different NICs (though I can't point  out
>> >> the valid use case), then we filter there.
>> >>
>> > That I think would be a bug in the protocol code.  For the ipv4 case, all
>> > addresses are owned by the system and the same addresses added to multiple
>> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
>> > exception there is a link local address and that should still be unique within
>> > the context of an address/dev tuple.
>> >
>> understand, just sounds a little harsh. :-)
>>
>> For now, does it make sense to you to just leave as the master list
>> is, and check
>> the duplicate address when sctp is really binding them ?
>>
> I would think so, yes.

Hi, Neil,

About this patch, I think we are on the page, right ?

If yes, I will repost v2, but other than improving some changelog,
no other change compare to v1. Do you agree ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Xin Long]

On Wed, Aug 24, 2016 at 6:38 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
>> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
>> >> > side, when you get a cookie unpacked and modify the remote peers address list,
>> >> > it makes sense to check for duplicates.  On the local side however, I would,
>> >> > instead of checking it when the list gets copied, I'd check it when the master
>> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
>> >>
>> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
>> >> master list has no duplicated addresses.  But what if two same addresses
>> >> events come, and they come from different NICs (though I can't point  out
>> >> the valid use case), then we filter there.
>> >>
>> > That I think would be a bug in the protocol code.  For the ipv4 case, all
>> > addresses are owned by the system and the same addresses added to multiple
>> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
>> > exception there is a link local address and that should still be unique within
>> > the context of an address/dev tuple.
>> >
>> understand, just sounds a little harsh. :-)
>>
>> For now, does it make sense to you to just leave as the master list
>> is, and check
>> the duplicate address when sctp is really binding them ?
>>
> I would think so, yes.

Hi, Neil,

About this patch, I think we are on the page, right ?

If yes, I will repost v2, but other than improving some changelog,
no other change compare to v1. Do you agree ?

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Sat, Dec 17, 2016 at 05:56:51PM +0800, Xin Long wrote:
> On Wed, Aug 24, 2016 at 6:38 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
> >> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> >> >> > side, when you get a cookie unpacked and modify the remote peers address list,
> >> >> > it makes sense to check for duplicates.  On the local side however, I would,
> >> >> > instead of checking it when the list gets copied, I'd check it when the master
> >> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> >> >>
> >> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
> >> >> master list has no duplicated addresses.  But what if two same addresses
> >> >> events come, and they come from different NICs (though I can't point  out
> >> >> the valid use case), then we filter there.
> >> >>
> >> > That I think would be a bug in the protocol code.  For the ipv4 case, all
> >> > addresses are owned by the system and the same addresses added to multiple
> >> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
> >> > exception there is a link local address and that should still be unique within
> >> > the context of an address/dev tuple.
> >> >
> >> understand, just sounds a little harsh. :-)
> >>
> >> For now, does it make sense to you to just leave as the master list
> >> is, and check
> >> the duplicate address when sctp is really binding them ?
> >>
> > I would think so, yes.
> 
> Hi, Neil,
> 
> About this patch, I think we are on the page, right ?
> 
Yes, I think we are.
Neil

> If yes, I will repost v2, but other than improving some changelog,
> no other change compare to v1. Do you agree ?
> 

Re: [PATCH net 2/2] sctp: not copying duplicate addrs to the assoc's bind address list

[Neil Horman]

On Sat, Dec 17, 2016 at 05:56:51PM +0800, Xin Long wrote:
> On Wed, Aug 24, 2016 at 6:38 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Wed, Aug 24, 2016 at 01:14:27PM +0800, Xin Long wrote:
> >> >> > Ah, I see what you're doing.  Ok, this makes some sense, at least on the receive
> >> >> > side, when you get a cookie unpacked and modify the remote peers address list,
> >> >> > it makes sense to check for duplicates.  On the local side however, I would,
> >> >> > instead of checking it when the list gets copied, I'd check it when the master
> >> >> > list gets updated (in the NETDEV_UP event notifier for the local address list,
> >> >>
> >> >> I was thinking about to check it in the NETDEV_UP, yes it can make the
> >> >> master list has no duplicated addresses.  But what if two same addresses
> >> >> events come, and they come from different NICs (though I can't point  out
> >> >> the valid use case), then we filter there.
> >> >>
> >> > That I think would be a bug in the protocol code.  For the ipv4 case, all
> >> > addresses are owned by the system and the same addresses added to multiple
> >> > interfaces should not be allowed.  The same is true of ipv6 case.  The only
> >> > exception there is a link local address and that should still be unique within
> >> > the context of an address/dev tuple.
> >> >
> >> understand, just sounds a little harsh. :-)
> >>
> >> For now, does it make sense to you to just leave as the master list
> >> is, and check
> >> the duplicate address when sctp is really binding them ?
> >>
> > I would think so, yes.
> 
> Hi, Neil,
> 
> About this patch, I think we are on the page, right ?
> 
Yes, I think we are.
Neil

> If yes, I will repost v2, but other than improving some changelog,
> no other change compare to v1. Do you agree ?
>