[PATCH V2] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[PATCH V2] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Zhu Yanjun]

Hi, Vlad && Deepak && Michael && David && Daniel

V2: Following the advice from Daniel Borkmann, I modified the comments and short log.

>From Michael && Deepak
"
 lxr SCTP implementation, doesn't transit the path state to INACTIVE, if it was never confirmed. this leads to SCTP_PEER_ADDRESS_CHANGE notification after each failed probe from this time.
 Is there any specific reason to have same notification to SCTP User with each probe in RTO time period ?
 806 case SCTP_TRANSPORT_DOWN:
 807 /* If the transport was never confirmed, do not transition it
 808 * to inactive state. Also, release the cached route since
 809 * there may be a better route next time.
 810 */
 811 if (transport->state != SCTP_UNCONFIRMED)

 812 transport->state = SCTP_INACTIVE;

 http://lxr.free-electrons.com/source/net/sctp/associola.c#L806

 ......

 I would also only expect a  SCTP_PEER_ADDRESS_CHANGE notification when a path state changes, not on every  try.

"

I made a patch to disable sending SCTP_PEER_ADDRESS_CHANGE notification every try. Now the patch is in the attachment. Please check it.

Zhu Yanjun


Zhu Yanjun (1):
  sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

 net/sctp/associola.c | 1 +
 1 file changed, 1 insertion(+)

-- 
1.9.1

[PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Zhu Yanjun]

Since the transport has always been in state SCTP_UNCONFIRMED, it
therefore wasn't active before and hasn't been used before, and it
always has been, so it is unnecessary to bug the user with a 
notification.

Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>  
Suggested-by: Vlad Yasevich <vyasevich@gmail.com> 
Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
Suggested-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
---
 net/sctp/associola.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 9de23a2..2e23f6b 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -813,6 +813,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		else {
 			dst_release(transport->dst);
 			transport->dst = NULL;
+			ulp_notify = false;
 		}
 
 		spc_state = SCTP_ADDR_UNREACHABLE;
-- 
1.9.1

Re: [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Vlad Yasevich]

On 08/20/2014 05:31 AM, Zhu Yanjun wrote:
> Since the transport has always been in state SCTP_UNCONFIRMED, it
> therefore wasn't active before and hasn't been used before, and it
> always has been, so it is unnecessary to bug the user with a 
> notification.
> 
> Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>  
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com> 
> Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
> Suggested-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

Thanks
-vlad
> ---
>  net/sctp/associola.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 9de23a2..2e23f6b 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -813,6 +813,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  		else {
>  			dst_release(transport->dst);
>  			transport->dst = NULL;
> +			ulp_notify = false;
>  		}
>  
>  		spc_state = SCTP_ADDR_UNREACHABLE;
> 

Re: [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Daniel Borkmann]

On 08/20/2014 11:31 AM, Zhu Yanjun wrote:
> Since the transport has always been in state SCTP_UNCONFIRMED, it
> therefore wasn't active before and hasn't been used before, and it
> always has been, so it is unnecessary to bug the user with a
> notification.
>
> Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
> Suggested-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>

Looks a bit better, thanks!

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Re: [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[David Miller]

From: Zhu Yanjun <zyjzyj2000@gmail.com>
Date: Wed, 20 Aug 2014 17:31:43 +0800

> Since the transport has always been in state SCTP_UNCONFIRMED, it
> therefore wasn't active before and hasn't been used before, and it
> always has been, so it is unnecessary to bug the user with a 
> notification.
> 
> Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>  
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com> 
> Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
> Suggested-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>

Applied, thanks.

Re: [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Willy Tarreau]

Hi,

On Thu, Nov 20, 2014 at 02:04:40PM +0800, Zhu Yanjun wrote:
> 2.6.x kernels require a similar logic change as commit 2c0d6ac894a
> [sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe] 
> introduces for newer kernels.

(...)
Queued for 2.6.32.64, thanks!

Willy

[PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Zhu Yanjun]

2.6.x kernels require a similar logic change as commit 2c0d6ac894a
[sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe] 
introduces for newer kernels.

Since the transport has always been in state SCTP_UNCONFIRMED, it
therefore wasn't active before and hasn't been used before, and it
always has been, so it is unnecessary to bug the user with a
notification.

Reported-by: Deepak Khandelwal <khandelwal.deepak.1987@gmail.com>
Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
Suggested-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
Acked-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

---
 net/sctp/associola.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 7eed77a..0f63396 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -824,6 +824,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 	struct sctp_ulpevent *event;
 	struct sockaddr_storage addr;
 	int spc_state = 0;
+	bool ulp_notify = true;
 
 	/* Record the transition on the transport.  */
 	switch (command) {
@@ -850,6 +851,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		else {
 			dst_release(transport->dst);
 			transport->dst = NULL;
+			ulp_notify = false;
 		}
 
 		spc_state = SCTP_ADDR_UNREACHABLE;
@@ -862,12 +864,14 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 	/* Generate and send a SCTP_PEER_ADDR_CHANGE notification to the
 	 * user.
 	 */
-	memset(&addr, 0, sizeof(struct sockaddr_storage));
-	memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
-	event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
+	if (ulp_notify) {
+		memset(&addr, 0, sizeof(struct sockaddr_storage));
+		memcpy(&addr, &transport->ipaddr, transport->af_specific->sockaddr_len);
+		event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
 				0, spc_state, error, GFP_ATOMIC);
-	if (event)
-		sctp_ulpq_tail_event(&asoc->ulpq, event);
+		if (event)
+			sctp_ulpq_tail_event(&asoc->ulpq, event);
+	}
 
 	/* Select new active and retran paths. */
 
-- 
1.9.1

Re: [PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Daniel Borkmann]

On 08/15/2014 11:27 AM, Zhu Yanjun wrote:
> When a failed probe comes along UNCONFIRMED path, it is not necessary
> to send SCTP_PEER_ADDR_CHANGE notification.

I do not find this in the RFC, but it seems reasonable - at least, I would
have liked to see a more elaborate commit message from you explaining why
it's okay to do; at least RFC6458 I read:

   SCTP_ADDR_UNREACHABLE:

     The address specified can no longer be reached. Any data sent
     to this address is rerouted to an alternate until this address
     becomes reachable. This notification is provided whenever an
     address *becomes* unreachable.

Given that the transport has always been in state SCTP_UNCONFIRMED, it
therefore wasn't active before and hasn't been used before, and one could
argue that it doesn't "become" "unreachable" but always has been, so we
wouldn't need to bug the user with a notification about it.

> Reported-by: DEEPAK KHANDELWAL <khandelwal.deepak.1987@gmail.com>

Nit: please write names normally: Deepak Khandelwal

> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
> Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
> ---
>   net/sctp/associola.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 9de23a2..2e23f6b 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -813,6 +813,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>   		else {
>   			dst_release(transport->dst);
>   			transport->dst = NULL;
> +			ulp_notify = false;
>   		}
>
>   		spc_state = SCTP_ADDR_UNREACHABLE;
>

[PATCH 1/1] sctp: not send SCTP_PEER_ADDR_CHANGE notifications with failed probe

[Zhu Yanjun]

When a failed probe comes along UNCONFIRMED path, it is not necessary
to send SCTP_PEER_ADDR_CHANGE notification.

Reported-by: DEEPAK KHANDELWAL <khandelwal.deepak.1987@gmail.com>  
Suggested-by: Vlad Yasevich <vyasevich@gmail.com> 
Suggested-by: Michael Tuexen <tuexen@fh-muenster.de>
Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
---
 net/sctp/associola.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 9de23a2..2e23f6b 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -813,6 +813,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		else {
 			dst_release(transport->dst);
 			transport->dst = NULL;
+			ulp_notify = false;
 		}
 
 		spc_state = SCTP_ADDR_UNREACHABLE;
-- 
1.9.1