[PATCHv2 net 0/3] sctp: always send a chunk with the asoc that it belongs to

[PATCHv2 net 0/3] sctp: always send a chunk with the asoc that it belongs to

[Xin Long]

Currently when processing a duplicate COOKIE-ECHO chunk, a new temp
asoc would be created, then it creates the chunks with the new asoc.
However, later on it uses the old asoc to send these chunks, which
has caused quite a few issues.

This patchset is to fix this and make sure that the COOKIE-ACK and
SHUTDOWN chunks are created with the same asoc that will be used to
send them out.

v1->v2:
  - see Patch 3/3.

Xin Long (3):
  sctp: do asoc update earlier in sctp_sf_do_dupcook_a
  Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"
  sctp: do asoc update earlier in sctp_sf_do_dupcook_b

 include/net/sctp/command.h |  1 -
 net/sctp/sm_sideeffect.c   | 26 ------------------------
 net/sctp/sm_statefuns.c    | 50 ++++++++++++++++++++++++++++++++++++----------
 3 files changed, 39 insertions(+), 38 deletions(-)

-- 
2.1.0

[PATCHv2 net 1/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_a

[Xin Long]

There's a panic that occurs in a few of envs, the call trace is as below:

  [] general protection fault, ... 0x29acd70f1000a: 0000 [#1] SMP PTI
  [] RIP: 0010:sctp_ulpevent_notify_peer_addr_change+0x4b/0x1fa [sctp]
  []  sctp_assoc_control_transport+0x1b9/0x210 [sctp]
  []  sctp_do_8_2_transport_strike.isra.16+0x15c/0x220 [sctp]
  []  sctp_cmd_interpreter.isra.21+0x1231/0x1a10 [sctp]
  []  sctp_do_sm+0xc3/0x2a0 [sctp]
  []  sctp_generate_timeout_event+0x81/0xf0 [sctp]

This is caused by a transport use-after-free issue. When processing a
duplicate COOKIE-ECHO chunk in sctp_sf_do_dupcook_a(), both COOKIE-ACK
and SHUTDOWN chunks are allocated with the transort from the new asoc.
However, later in the sideeffect machine, the old asoc is used to send
them out and old asoc's shutdown_last_sent_to is set to the transport
that SHUTDOWN chunk attached to in sctp_cmd_setup_t2(), which actually
belongs to the new asoc. After the new_asoc is freed and the old asoc
T2 timeout, the old asoc's shutdown_last_sent_to that is already freed
would be accessed in sctp_sf_t2_timer_expire().

Thanks Alexander and Jere for helping dig into this issue.

To fix it, this patch is to do the asoc update first, then allocate
the COOKIE-ACK and SHUTDOWN chunks with the 'updated' old asoc. This
would make more sense, as a chunk from an asoc shouldn't be sent out
with another asoc. We had fixed quite a few issues caused by this.

Fixes: 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with COOKIE-ACK")
Reported-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
Reported-by: syzbot+bbe538efd1046586f587@syzkaller.appspotmail.com
Reported-by: Michal Tesar <mtesar@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/sm_statefuns.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 7632714..30cb946 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1852,20 +1852,35 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
 			SCTP_TO(SCTP_EVENT_TIMEOUT_T4_RTO));
 	sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL());
 
-	repl = sctp_make_cookie_ack(new_asoc, chunk);
+	/* Update the content of current association. */
+	if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) {
+		struct sctp_chunk *abort;
+
+		abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
+		if (abort) {
+			sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
+			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+		}
+		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
+		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
+				SCTP_PERR(SCTP_ERROR_RSRC_LOW));
+		SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
+		SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
+		goto nomem;
+	}
+
+	repl = sctp_make_cookie_ack(asoc, chunk);
 	if (!repl)
 		goto nomem;
 
 	/* Report association restart to upper layer. */
 	ev = sctp_ulpevent_make_assoc_change(asoc, 0, SCTP_RESTART, 0,
-					     new_asoc->c.sinit_num_ostreams,
-					     new_asoc->c.sinit_max_instreams,
+					     asoc->c.sinit_num_ostreams,
+					     asoc->c.sinit_max_instreams,
 					     NULL, GFP_ATOMIC);
 	if (!ev)
 		goto nomem_ev;
 
-	/* Update the content of current association. */
-	sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
 	sctp_add_cmd_sf(commands, SCTP_CMD_EVENT_ULP, SCTP_ULPEVENT(ev));
 	if ((sctp_state(asoc, SHUTDOWN_PENDING) ||
 	     sctp_state(asoc, SHUTDOWN_SENT)) &&
-- 
2.1.0

[PATCHv2 net 2/3] Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"

[Xin Long]

This can be reverted as shutdown and cookie_ack chunk are using the
same asoc since the last patch.

This reverts commit 145cb2f7177d94bc54563ed26027e952ee0ae03c.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 net/sctp/sm_statefuns.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 30cb946..e8ccc4e 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1892,7 +1892,7 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
 		 */
 		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
 		return sctp_sf_do_9_2_start_shutdown(net, ep, asoc,
-						     SCTP_ST_CHUNK(0), repl,
+						     SCTP_ST_CHUNK(0), NULL,
 						     commands);
 	} else {
 		sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
@@ -5536,7 +5536,7 @@ enum sctp_disposition sctp_sf_do_9_2_start_shutdown(
 	 * in the Cumulative TSN Ack field the last sequential TSN it
 	 * has received from the peer.
 	 */
-	reply = sctp_make_shutdown(asoc, arg);
+	reply = sctp_make_shutdown(asoc, NULL);
 	if (!reply)
 		goto nomem;
 
@@ -6134,7 +6134,7 @@ enum sctp_disposition sctp_sf_autoclose_timer_expire(
 	disposition = SCTP_DISPOSITION_CONSUME;
 	if (sctp_outq_is_empty(&asoc->outqueue)) {
 		disposition = sctp_sf_do_9_2_start_shutdown(net, ep, asoc, type,
-							    NULL, commands);
+							    arg, commands);
 	}
 
 	return disposition;
-- 
2.1.0

[PATCHv2 net 3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b

[Xin Long]

The same thing should be done for sctp_sf_do_dupcook_b().
Meanwhile, SCTP_CMD_UPDATE_ASSOC cmd can be removed.

v1->v2:
  - Fix the return value in sctp_sf_do_assoc_update().

Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/sctp/command.h |  1 -
 net/sctp/sm_sideeffect.c   | 26 -------------------------
 net/sctp/sm_statefuns.c    | 47 +++++++++++++++++++++++++++++-----------------
 3 files changed, 30 insertions(+), 44 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index e8df72e..5e84888 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h
@@ -68,7 +68,6 @@ enum sctp_verb {
 	SCTP_CMD_ASSOC_FAILED,	 /* Handle association failure. */
 	SCTP_CMD_DISCARD_PACKET, /* Discard the whole packet. */
 	SCTP_CMD_GEN_SHUTDOWN,   /* Generate a SHUTDOWN chunk. */
-	SCTP_CMD_UPDATE_ASSOC,   /* Update association information. */
 	SCTP_CMD_PURGE_OUTQUEUE, /* Purge all data waiting to be sent. */
 	SCTP_CMD_SETUP_T2,       /* Hi-level, setup T2-shutdown parms.  */
 	SCTP_CMD_RTO_PENDING,	 /* Set transport's rto_pending. */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 0948f14..ce15d59 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -826,28 +826,6 @@ static void sctp_cmd_setup_t2(struct sctp_cmd_seq *cmds,
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = t->rto;
 }
 
-static void sctp_cmd_assoc_update(struct sctp_cmd_seq *cmds,
-				  struct sctp_association *asoc,
-				  struct sctp_association *new)
-{
-	struct net *net = asoc->base.net;
-	struct sctp_chunk *abort;
-
-	if (!sctp_assoc_update(asoc, new))
-		return;
-
-	abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
-	if (abort) {
-		sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
-		sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
-	}
-	sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
-	sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
-			SCTP_PERR(SCTP_ERROR_RSRC_LOW));
-	SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
-	SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
-}
-
 /* Helper function to change the state of an association. */
 static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
 			       struct sctp_association *asoc,
@@ -1301,10 +1279,6 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
 			sctp_endpoint_add_asoc(ep, asoc);
 			break;
 
-		case SCTP_CMD_UPDATE_ASSOC:
-		       sctp_cmd_assoc_update(commands, asoc, cmd->obj.asoc);
-		       break;
-
 		case SCTP_CMD_PURGE_OUTQUEUE:
 		       sctp_outq_teardown(&asoc->outqueue);
 		       break;
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index e8ccc4e..a428449 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1773,6 +1773,30 @@ enum sctp_disposition sctp_sf_do_5_2_3_initack(
 		return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
 }
 
+static int sctp_sf_do_assoc_update(struct sctp_association *asoc,
+				   struct sctp_association *new,
+				   struct sctp_cmd_seq *cmds)
+{
+	struct net *net = asoc->base.net;
+	struct sctp_chunk *abort;
+
+	if (!sctp_assoc_update(asoc, new))
+		return 0;
+
+	abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
+	if (abort) {
+		sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
+		sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
+	}
+	sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
+	sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
+			SCTP_PERR(SCTP_ERROR_RSRC_LOW));
+	SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
+	SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
+
+	return -ENOMEM;
+}
+
 /* Unexpected COOKIE-ECHO handler for peer restart (Table 2, action 'A')
  *
  * Section 5.2.4
@@ -1853,21 +1877,8 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
 	sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL());
 
 	/* Update the content of current association. */
-	if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) {
-		struct sctp_chunk *abort;
-
-		abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
-		if (abort) {
-			sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
-			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
-		}
-		sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
-		sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
-				SCTP_PERR(SCTP_ERROR_RSRC_LOW));
-		SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
-		SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
+	if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
 		goto nomem;
-	}
 
 	repl = sctp_make_cookie_ack(asoc, chunk);
 	if (!repl)
@@ -1940,14 +1951,16 @@ static enum sctp_disposition sctp_sf_do_dupcook_b(
 	if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
 		return SCTP_DISPOSITION_DISCARD;
 
-	/* Update the content of current association.  */
-	sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
 	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
 			SCTP_STATE(SCTP_STATE_ESTABLISHED));
 	SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
 	sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
 
-	repl = sctp_make_cookie_ack(new_asoc, chunk);
+	/* Update the content of current association.  */
+	if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
+		goto nomem;
+
+	repl = sctp_make_cookie_ack(asoc, chunk);
 	if (!repl)
 		goto nomem;
 
-- 
2.1.0

Re: [PATCHv2 net 3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b

[Leppanen, Jere (Nokia - FI/Espoo)]

On Fri, 30 Apr 2021, Xin Long wrote:

> The same thing should be done for sctp_sf_do_dupcook_b().
> Meanwhile, SCTP_CMD_UPDATE_ASSOC cmd can be removed.
>
> v1->v2:
>  - Fix the return value in sctp_sf_do_assoc_update().
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> include/net/sctp/command.h |  1 -
> net/sctp/sm_sideeffect.c   | 26 -------------------------
> net/sctp/sm_statefuns.c    | 47 +++++++++++++++++++++++++++++-----------------
> 3 files changed, 30 insertions(+), 44 deletions(-)
>
> diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> index e8df72e..5e84888 100644
> --- a/include/net/sctp/command.h
> +++ b/include/net/sctp/command.h
> @@ -68,7 +68,6 @@ enum sctp_verb {
>       SCTP_CMD_ASSOC_FAILED,   /* Handle association failure. */
>       SCTP_CMD_DISCARD_PACKET, /* Discard the whole packet. */
>       SCTP_CMD_GEN_SHUTDOWN,   /* Generate a SHUTDOWN chunk. */
> -     SCTP_CMD_UPDATE_ASSOC,   /* Update association information. */
>       SCTP_CMD_PURGE_OUTQUEUE, /* Purge all data waiting to be sent. */
>       SCTP_CMD_SETUP_T2,       /* Hi-level, setup T2-shutdown parms.  */
>       SCTP_CMD_RTO_PENDING,    /* Set transport's rto_pending. */
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index 0948f14..ce15d59 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -826,28 +826,6 @@ static void sctp_cmd_setup_t2(struct sctp_cmd_seq *cmds,
>       asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = t->rto;
> }
>
> -static void sctp_cmd_assoc_update(struct sctp_cmd_seq *cmds,
> -                               struct sctp_association *asoc,
> -                               struct sctp_association *new)
> -{
> -     struct net *net = asoc->base.net;
> -     struct sctp_chunk *abort;
> -
> -     if (!sctp_assoc_update(asoc, new))
> -             return;
> -
> -     abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> -     if (abort) {
> -             sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> -             sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> -     }
> -     sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> -     sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
> -                     SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> -     SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> -     SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> -}
> -
> /* Helper function to change the state of an association. */
> static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
>                              struct sctp_association *asoc,
> @@ -1301,10 +1279,6 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
>                       sctp_endpoint_add_asoc(ep, asoc);
>                       break;
>
> -             case SCTP_CMD_UPDATE_ASSOC:
> -                    sctp_cmd_assoc_update(commands, asoc, cmd->obj.asoc);
> -                    break;
> -
>               case SCTP_CMD_PURGE_OUTQUEUE:
>                      sctp_outq_teardown(&asoc->outqueue);
>                      break;
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index e8ccc4e..a428449 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1773,6 +1773,30 @@ enum sctp_disposition sctp_sf_do_5_2_3_initack(
>               return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
> }
>
> +static int sctp_sf_do_assoc_update(struct sctp_association *asoc,
> +                                struct sctp_association *new,
> +                                struct sctp_cmd_seq *cmds)
> +{
> +     struct net *net = asoc->base.net;
> +     struct sctp_chunk *abort;
> +
> +     if (!sctp_assoc_update(asoc, new))
> +             return 0;
> +
> +     abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> +     if (abort) {
> +             sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> +             sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> +     }
> +     sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> +     sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
> +                     SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> +     SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> +     SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> +
> +     return -ENOMEM;
> +}
> +
> /* Unexpected COOKIE-ECHO handler for peer restart (Table 2, action 'A')
>  *
>  * Section 5.2.4
> @@ -1853,21 +1877,8 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
>       sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL());
>
>       /* Update the content of current association. */
> -     if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) {
> -             struct sctp_chunk *abort;
> -
> -             abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> -             if (abort) {
> -                     sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> -                     sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> -             }
> -             sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> -             sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> -                             SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> -             SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> -             SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> +     if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
>               goto nomem;
> -     }
>
>       repl = sctp_make_cookie_ack(asoc, chunk);
>       if (!repl)
> @@ -1940,14 +1951,16 @@ static enum sctp_disposition sctp_sf_do_dupcook_b(
>       if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
>               return SCTP_DISPOSITION_DISCARD;
>
> -     /* Update the content of current association.  */
> -     sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
>       sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
>                       SCTP_STATE(SCTP_STATE_ESTABLISHED));
>       SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
>       sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
>
> -     repl = sctp_make_cookie_ack(new_asoc, chunk);
> +     /* Update the content of current association.  */
> +     if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
> +             goto nomem;

Wouldn't it be better to do the update before SCTP_CMD_NEW_STATE?
Or do we have some reason to move to SCTP_STATE_ESTABLISHED even
if the update fails?

> +
> +     repl = sctp_make_cookie_ack(asoc, chunk);
>       if (!repl)
>               goto nomem;
>
> -- 
> 2.1.0

Re: [PATCHv2 net 3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b

[Xin Long]

On Sat, May 1, 2021 at 12:57 PM Leppanen, Jere (Nokia - FI/Espoo)
<jere.leppanen@nokia.com> wrote:
>
> On Fri, 30 Apr 2021, Xin Long wrote:
>
> > The same thing should be done for sctp_sf_do_dupcook_b().
> > Meanwhile, SCTP_CMD_UPDATE_ASSOC cmd can be removed.
> >
> > v1->v2:
> >  - Fix the return value in sctp_sf_do_assoc_update().
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> > include/net/sctp/command.h |  1 -
> > net/sctp/sm_sideeffect.c   | 26 -------------------------
> > net/sctp/sm_statefuns.c    | 47 +++++++++++++++++++++++++++++-----------------
> > 3 files changed, 30 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> > index e8df72e..5e84888 100644
> > --- a/include/net/sctp/command.h
> > +++ b/include/net/sctp/command.h
> > @@ -68,7 +68,6 @@ enum sctp_verb {
> >       SCTP_CMD_ASSOC_FAILED,   /* Handle association failure. */
> >       SCTP_CMD_DISCARD_PACKET, /* Discard the whole packet. */
> >       SCTP_CMD_GEN_SHUTDOWN,   /* Generate a SHUTDOWN chunk. */
> > -     SCTP_CMD_UPDATE_ASSOC,   /* Update association information. */
> >       SCTP_CMD_PURGE_OUTQUEUE, /* Purge all data waiting to be sent. */
> >       SCTP_CMD_SETUP_T2,       /* Hi-level, setup T2-shutdown parms.  */
> >       SCTP_CMD_RTO_PENDING,    /* Set transport's rto_pending. */
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index 0948f14..ce15d59 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -826,28 +826,6 @@ static void sctp_cmd_setup_t2(struct sctp_cmd_seq *cmds,
> >       asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = t->rto;
> > }
> >
> > -static void sctp_cmd_assoc_update(struct sctp_cmd_seq *cmds,
> > -                               struct sctp_association *asoc,
> > -                               struct sctp_association *new)
> > -{
> > -     struct net *net = asoc->base.net;
> > -     struct sctp_chunk *abort;
> > -
> > -     if (!sctp_assoc_update(asoc, new))
> > -             return;
> > -
> > -     abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> > -     if (abort) {
> > -             sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> > -             sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> > -     }
> > -     sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> > -     sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
> > -                     SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> > -     SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> > -     SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> > -}
> > -
> > /* Helper function to change the state of an association. */
> > static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
> >                              struct sctp_association *asoc,
> > @@ -1301,10 +1279,6 @@ static int sctp_cmd_interpreter(enum sctp_event_type event_type,
> >                       sctp_endpoint_add_asoc(ep, asoc);
> >                       break;
> >
> > -             case SCTP_CMD_UPDATE_ASSOC:
> > -                    sctp_cmd_assoc_update(commands, asoc, cmd->obj.asoc);
> > -                    break;
> > -
> >               case SCTP_CMD_PURGE_OUTQUEUE:
> >                      sctp_outq_teardown(&asoc->outqueue);
> >                      break;
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index e8ccc4e..a428449 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -1773,6 +1773,30 @@ enum sctp_disposition sctp_sf_do_5_2_3_initack(
> >               return sctp_sf_discard_chunk(net, ep, asoc, type, arg, commands);
> > }
> >
> > +static int sctp_sf_do_assoc_update(struct sctp_association *asoc,
> > +                                struct sctp_association *new,
> > +                                struct sctp_cmd_seq *cmds)
> > +{
> > +     struct net *net = asoc->base.net;
> > +     struct sctp_chunk *abort;
> > +
> > +     if (!sctp_assoc_update(asoc, new))
> > +             return 0;
> > +
> > +     abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> > +     if (abort) {
> > +             sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> > +             sctp_add_cmd_sf(cmds, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> > +     }
> > +     sctp_add_cmd_sf(cmds, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> > +     sctp_add_cmd_sf(cmds, SCTP_CMD_ASSOC_FAILED,
> > +                     SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> > +     SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> > +     SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> > +
> > +     return -ENOMEM;
> > +}
> > +
> > /* Unexpected COOKIE-ECHO handler for peer restart (Table 2, action 'A')
> >  *
> >  * Section 5.2.4
> > @@ -1853,21 +1877,8 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
> >       sctp_add_cmd_sf(commands, SCTP_CMD_PURGE_ASCONF_QUEUE, SCTP_NULL());
> >
> >       /* Update the content of current association. */
> > -     if (sctp_assoc_update((struct sctp_association *)asoc, new_asoc)) {
> > -             struct sctp_chunk *abort;
> > -
> > -             abort = sctp_make_abort(asoc, NULL, sizeof(struct sctp_errhdr));
> > -             if (abort) {
> > -                     sctp_init_cause(abort, SCTP_ERROR_RSRC_LOW, 0);
> > -                     sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));
> > -             }
> > -             sctp_add_cmd_sf(commands, SCTP_CMD_SET_SK_ERR, SCTP_ERROR(ECONNABORTED));
> > -             sctp_add_cmd_sf(commands, SCTP_CMD_ASSOC_FAILED,
> > -                             SCTP_PERR(SCTP_ERROR_RSRC_LOW));
> > -             SCTP_INC_STATS(net, SCTP_MIB_ABORTEDS);
> > -             SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB);
> > +     if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
> >               goto nomem;
> > -     }
> >
> >       repl = sctp_make_cookie_ack(asoc, chunk);
> >       if (!repl)
> > @@ -1940,14 +1951,16 @@ static enum sctp_disposition sctp_sf_do_dupcook_b(
> >       if (!sctp_auth_chunk_verify(net, chunk, new_asoc))
> >               return SCTP_DISPOSITION_DISCARD;
> >
> > -     /* Update the content of current association.  */
> > -     sctp_add_cmd_sf(commands, SCTP_CMD_UPDATE_ASSOC, SCTP_ASOC(new_asoc));
> >       sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
> >                       SCTP_STATE(SCTP_STATE_ESTABLISHED));
> >       SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB);
> >       sctp_add_cmd_sf(commands, SCTP_CMD_HB_TIMERS_START, SCTP_NULL());
> >
> > -     repl = sctp_make_cookie_ack(new_asoc, chunk);
> > +     /* Update the content of current association.  */
> > +     if (sctp_sf_do_assoc_update((struct sctp_association *)asoc, new_asoc, commands))
> > +             goto nomem;
>
> Wouldn't it be better to do the update before SCTP_CMD_NEW_STATE?
> Or do we have some reason to move to SCTP_STATE_ESTABLISHED even
> if the update fails?
Yes, as sctp_sf_do_assoc_update() adds SCTP_DEC_STATS(net, SCTP_MIB_CURRESTAB),
and before that we have to keep SCTP_INC_STATS(net, SCTP_MIB_CURRESTAB) added
in there, which is normally right after NEW_STATE cmd with ESTABLISHED.

>
> > +
> > +     repl = sctp_make_cookie_ack(asoc, chunk);
> >       if (!repl)
> >               goto nomem;
> >
> > --
> > 2.1.0

Re: [PATCHv2 net 2/3] Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"

[Leppanen, Jere (Nokia - FI/Espoo)]

On Fri, 30 Apr 2021, Xin Long wrote:

> This can be reverted as shutdown and cookie_ack chunk are using the
> same asoc since the last patch.
>
> This reverts commit 145cb2f7177d94bc54563ed26027e952ee0ae03c.

I think this should not be reverted. Without it, SHUTDOWN would
get its transport from sctp_assoc_choose_alter_transport(), which
could be different from the COOKIE-ACK transport, which would
prevent bundling.

>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/sm_statefuns.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 30cb946..e8ccc4e 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1892,7 +1892,7 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
>                */
>               sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
>               return sctp_sf_do_9_2_start_shutdown(net, ep, asoc,
> -                                                  SCTP_ST_CHUNK(0), repl,
> +                                                  SCTP_ST_CHUNK(0), NULL,
>                                                    commands);
>       } else {
>               sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
> @@ -5536,7 +5536,7 @@ enum sctp_disposition sctp_sf_do_9_2_start_shutdown(
>        * in the Cumulative TSN Ack field the last sequential TSN it
>        * has received from the peer.
>        */
> -     reply = sctp_make_shutdown(asoc, arg);
> +     reply = sctp_make_shutdown(asoc, NULL);
>       if (!reply)
>               goto nomem;
>
> @@ -6134,7 +6134,7 @@ enum sctp_disposition sctp_sf_autoclose_timer_expire(
>       disposition = SCTP_DISPOSITION_CONSUME;
>       if (sctp_outq_is_empty(&asoc->outqueue)) {
>               disposition = sctp_sf_do_9_2_start_shutdown(net, ep, asoc, type,
> -                                                         NULL, commands);
> +                                                         arg, commands);
>       }
>
>       return disposition;
> -- 
> 2.1.0

Re: [PATCHv2 net 0/3] sctp: always send a chunk with the asoc that it belongs to

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Sat,  1 May 2021 04:02:57 +0800 you wrote:
> Currently when processing a duplicate COOKIE-ECHO chunk, a new temp
> asoc would be created, then it creates the chunks with the new asoc.
> However, later on it uses the old asoc to send these chunks, which
> has caused quite a few issues.
> 
> This patchset is to fix this and make sure that the COOKIE-ACK and
> SHUTDOWN chunks are created with the same asoc that will be used to
> send them out.
> 
> [...]

Here is the summary with links:
  - [PATCHv2,net,1/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_a
    https://git.kernel.org/netdev/net/c/35b4f24415c8
  - [PATCHv2,net,2/3] Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"
    https://git.kernel.org/netdev/net/c/7e9269a5acec
  - [PATCHv2,net,3/3] sctp: do asoc update earlier in sctp_sf_do_dupcook_b
    https://git.kernel.org/netdev/net/c/51eac7f2f06b

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCHv2 net 0/3] sctp: always send a chunk with the asoc that it belongs to

[Leppanen, Jere (Nokia - FI/Espoo)]

On Fri, 30 Apr 2021, Xin Long wrote:

> Currently when processing a duplicate COOKIE-ECHO chunk, a new temp
> asoc would be created, then it creates the chunks with the new asoc.
> However, later on it uses the old asoc to send these chunks, which
> has caused quite a few issues.
>
> This patchset is to fix this and make sure that the COOKIE-ACK and
> SHUTDOWN chunks are created with the same asoc that will be used to
> send them out.

Again, much thanks for looking into this. Patches 1 and 3 are
almost the same as my patch, which as I mentioned I've been
testing on and off for the past couple of weeks, and haven't
found any problems. (Then again, I didn't find any problems last
time either.)

I think 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with
COOKIE-ACK") should not be reverted (I'll reply to the patch).

12dfd78e3a74 ("sctp: Fix SHUTDOWN CTSN Ack in the peer restart
case") should be reverted. With association update no longer a
side effect, we can get CTSN normally from current assoc, since
it has been updated before sctp_make_shutdown().

>
> v1->v2:
>  - see Patch 3/3.
>
> Xin Long (3):
>  sctp: do asoc update earlier in sctp_sf_do_dupcook_a
>  Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"
>  sctp: do asoc update earlier in sctp_sf_do_dupcook_b
>
> include/net/sctp/command.h |  1 -
> net/sctp/sm_sideeffect.c   | 26 ------------------------
> net/sctp/sm_statefuns.c    | 50 ++++++++++++++++++++++++++++++++++++----------
> 3 files changed, 39 insertions(+), 38 deletions(-)
>
> --
> 2.1.0

Re: [PATCHv2 net 0/3] sctp: always send a chunk with the asoc that it belongs to

[Xin Long]

On Sat, May 1, 2021 at 12:54 PM Leppanen, Jere (Nokia - FI/Espoo)
<jere.leppanen@nokia.com> wrote:
>
> On Fri, 30 Apr 2021, Xin Long wrote:
>
> > Currently when processing a duplicate COOKIE-ECHO chunk, a new temp
> > asoc would be created, then it creates the chunks with the new asoc.
> > However, later on it uses the old asoc to send these chunks, which
> > has caused quite a few issues.
> >
> > This patchset is to fix this and make sure that the COOKIE-ACK and
> > SHUTDOWN chunks are created with the same asoc that will be used to
> > send them out.
>
> Again, much thanks for looking into this. Patches 1 and 3 are
> almost the same as my patch, which as I mentioned I've been
> testing on and off for the past couple of weeks, and haven't
> found any problems. (Then again, I didn't find any problems last
> time either.)
>
> I think 145cb2f7177d ("sctp: Fix bundling of SHUTDOWN with
> COOKIE-ACK") should not be reverted (I'll reply to the patch).
>
> 12dfd78e3a74 ("sctp: Fix SHUTDOWN CTSN Ack in the peer restart
> case") should be reverted. With association update no longer a
> side effect, we can get CTSN normally from current assoc, since
> it has been updated before sctp_make_shutdown().
Good catch, didn't notice this one.
Will do it. Thanks.

>
> >
> > v1->v2:
> >  - see Patch 3/3.
> >
> > Xin Long (3):
> >  sctp: do asoc update earlier in sctp_sf_do_dupcook_a
> >  Revert "sctp: Fix bundling of SHUTDOWN with COOKIE-ACK"
> >  sctp: do asoc update earlier in sctp_sf_do_dupcook_b
> >
> > include/net/sctp/command.h |  1 -
> > net/sctp/sm_sideeffect.c   | 26 ------------------------
> > net/sctp/sm_statefuns.c    | 50 ++++++++++++++++++++++++++++++++++++----------
> > 3 files changed, 39 insertions(+), 38 deletions(-)
> >
> > --
> > 2.1.0