[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 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

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

[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

[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

[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

[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

[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

[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

[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

['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