[PATCH net 0/2] sctp: Fix problems with peer restart when in SHUTDOWN-PENDING state and socket is closed

[PATCH net 0/2] sctp: Fix problems with peer restart when in SHUTDOWN-PENDING state and socket is closed

[Jere Leppänen]

These patches are related to the scenario described in commit
bdf6fa52f01b ("sctp: handle association restarts when the socket is
closed."). To recap, when our association is in SHUTDOWN-PENDING state
and we've closed our one-to-one socket, while the peer crashes without
being detected, restarts and reconnects using the same addresses and
ports, we start association shutdown.

In this case, Cumulative TSN Ack in the SHUTDOWN that we send has
always been incorrect. Additionally, bundling of the SHUTDOWN with the
COOKIE-ACK was broken by a later commit. This series fixes both of
these issues.

Jere Leppänen (2):
  sctp: Fix bundling of SHUTDOWN with COOKIE-ACK
  sctp: Fix SHUTDOWN CTSN Ack in the peer restart case

 net/sctp/sm_make_chunk.c | 6 +++++-
 net/sctp/sm_statefuns.c  | 6 +++---
 2 files changed, 8 insertions(+), 4 deletions(-)


base-commit: a460fc5d4c170806a31e590df37ead3ab951315c
-- 
2.25.2

[PATCH net 1/2] sctp: Fix bundling of SHUTDOWN with COOKIE-ACK

[Jere Leppänen]

When we start shutdown in sctp_sf_do_dupcook_a(), we want to bundle
the SHUTDOWN with the COOKIE-ACK to ensure that the peer receives them
at the same time and in the correct order. This bundling was broken by
commit 4ff40b86262b ("sctp: set chunk transport correctly when it's a
new asoc"), which assigns a transport for the COOKIE-ACK, but not for
the SHUTDOWN.

Fix this by passing a reference to the COOKIE-ACK chunk as an argument
to sctp_sf_do_9_2_start_shutdown() and onward to
sctp_make_shutdown(). This way the SHUTDOWN chunk is assigned the same
transport as the COOKIE-ACK chunk, which allows them to be bundled.

In sctp_sf_do_9_2_start_shutdown(), the void *arg parameter was
previously unused. Now that we're taking it into use, it must be a
valid pointer to a chunk, or NULL. There is only one call site where
it's not, in sctp_sf_autoclose_timer_expire(). Fix that too.

Fixes: 4ff40b86262b ("sctp: set chunk transport correctly when it's a new asoc")
Signed-off-by: Jere Leppänen <jere.leppanen@nokia.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 6a16af4b1ef6..26788f4a3b9e 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1865,7 +1865,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), NULL,
+						     SCTP_ST_CHUNK(0), repl,
 						     commands);
 	} else {
 		sctp_add_cmd_sf(commands, SCTP_CMD_NEW_STATE,
@@ -5470,7 +5470,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, NULL);
+	reply = sctp_make_shutdown(asoc, arg);
 	if (!reply)
 		goto nomem;
 
@@ -6068,7 +6068,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,
-							    arg, commands);
+							    NULL, commands);
 	}
 
 	return disposition;
-- 
2.25.2

[PATCH net 2/2] sctp: Fix SHUTDOWN CTSN Ack in the peer restart case

[Jere Leppänen]

When starting shutdown in sctp_sf_do_dupcook_a(), get the value for
SHUTDOWN Cumulative TSN Ack from the new association, which is
reconstructed from the cookie, instead of the old association, which
the peer doesn't have anymore.

Otherwise the SHUTDOWN is either ignored or replied to with an ABORT
by the peer because CTSN Ack doesn't match the peer's Initial TSN.

Fixes: bdf6fa52f01b ("sctp: handle association restarts when the socket is closed.")
Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
---
 net/sctp/sm_make_chunk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 09050c1d5517..f7cb0b7faec2 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -858,7 +858,11 @@ struct sctp_chunk *sctp_make_shutdown(const struct sctp_association *asoc,
 	struct sctp_chunk *retval;
 	__u32 ctsn;
 
-	ctsn = sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map);
+	if (chunk && chunk->asoc)
+		ctsn = sctp_tsnmap_get_ctsn(&chunk->asoc->peer.tsn_map);
+	else
+		ctsn = sctp_tsnmap_get_ctsn(&asoc->peer.tsn_map);
+
 	shut.cum_tsn_ack = htonl(ctsn);
 
 	retval = sctp_make_control(asoc, SCTP_CID_SHUTDOWN, 0,
-- 
2.25.2

Re: [PATCH net 1/2] sctp: Fix bundling of SHUTDOWN with COOKIE-ACK

[Marcelo Ricardo Leitner]

On Tue, Apr 21, 2020 at 10:03:41PM +0300, Jere Leppänen wrote:
> When we start shutdown in sctp_sf_do_dupcook_a(), we want to bundle
> the SHUTDOWN with the COOKIE-ACK to ensure that the peer receives them
> at the same time and in the correct order. This bundling was broken by
> commit 4ff40b86262b ("sctp: set chunk transport correctly when it's a
> new asoc"), which assigns a transport for the COOKIE-ACK, but not for
> the SHUTDOWN.
> 
> Fix this by passing a reference to the COOKIE-ACK chunk as an argument
> to sctp_sf_do_9_2_start_shutdown() and onward to
> sctp_make_shutdown(). This way the SHUTDOWN chunk is assigned the same
> transport as the COOKIE-ACK chunk, which allows them to be bundled.
> 
> In sctp_sf_do_9_2_start_shutdown(), the void *arg parameter was
> previously unused. Now that we're taking it into use, it must be a
> valid pointer to a chunk, or NULL. There is only one call site where
> it's not, in sctp_sf_autoclose_timer_expire(). Fix that too.
> 
> Fixes: 4ff40b86262b ("sctp: set chunk transport correctly when it's a new asoc")
> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net 2/2] sctp: Fix SHUTDOWN CTSN Ack in the peer restart case

[Marcelo Ricardo Leitner]

On Tue, Apr 21, 2020 at 10:03:42PM +0300, Jere Leppänen wrote:
> When starting shutdown in sctp_sf_do_dupcook_a(), get the value for
> SHUTDOWN Cumulative TSN Ack from the new association, which is
> reconstructed from the cookie, instead of the old association, which
> the peer doesn't have anymore.
> 
> Otherwise the SHUTDOWN is either ignored or replied to with an ABORT
> by the peer because CTSN Ack doesn't match the peer's Initial TSN.
> 
> Fixes: bdf6fa52f01b ("sctp: handle association restarts when the socket is closed.")
> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>

Nice patches. Thanks.

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net 0/2] sctp: Fix problems with peer restart when in SHUTDOWN-PENDING state and socket is closed

[David Miller]

From: Jere Leppänen <jere.leppanen@nokia.com>
Date: Tue, 21 Apr 2020 22:03:40 +0300

> These patches are related to the scenario described in commit
> bdf6fa52f01b ("sctp: handle association restarts when the socket is
> closed."). To recap, when our association is in SHUTDOWN-PENDING state
> and we've closed our one-to-one socket, while the peer crashes without
> being detected, restarts and reconnects using the same addresses and
> ports, we start association shutdown.
> 
> In this case, Cumulative TSN Ack in the SHUTDOWN that we send has
> always been incorrect. Additionally, bundling of the SHUTDOWN with the
> COOKIE-ACK was broken by a later commit. This series fixes both of
> these issues.

Series applied, thanks.