Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed
@ 2020-05-20 15:15 Jere Leppänen
  2020-05-20 19:46 ` Marcelo Ricardo Leitner
  2020-05-22 22:48 ` David Miller
  0 siblings, 2 replies; 4+ messages in thread
From: Jere Leppänen @ 2020-05-20 15:15 UTC (permalink / raw)
  To: linux-sctp, netdev, Vlad Yasevich, Neil Horman,
	Marcelo Ricardo Leitner, David S . Miller
  Cc: David Laight

Commit bdf6fa52f01b ("sctp: handle association restarts when the
socket is closed.") starts shutdown when an association is restarted,
if in SHUTDOWN-PENDING state and the socket is closed. However, the
rationale stated in that commit applies also when in SHUTDOWN-SENT
state - we don't want to move an association to ESTABLISHED state when
the socket has been closed, because that results in an association
that is unreachable from user space.

The problem scenario:

1.  Client crashes and/or restarts.

2.  Server (using one-to-one socket) calls close(). SHUTDOWN is lost.

3.  Client reconnects using the same addresses and ports.

4.  Server's association is restarted. The association and the socket
    move to ESTABLISHED state, even though the server process has
    closed its descriptor.

Also, after step 4 when the server process exits, some resources are
leaked in an attempt to release the underlying inet sock structure in
ESTABLISHED state:

    IPv4: Attempt to release TCP socket in state 1 00000000377288c7

Fix by acting the same way as in SHUTDOWN-PENDING state. That is, if
an association is restarted in SHUTDOWN-SENT state and the socket is
closed, then start shutdown and don't move the association or the
socket to ESTABLISHED state.

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

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 26788f4a3b9e..e86620fbd90f 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1856,12 +1856,13 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
 	/* 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) &&
+	if ((sctp_state(asoc, SHUTDOWN_PENDING) ||
+	     sctp_state(asoc, SHUTDOWN_SENT)) &&
 	    (sctp_sstate(asoc->base.sk, CLOSING) ||
 	     sock_flag(asoc->base.sk, SOCK_DEAD))) {
-		/* if were currently in SHUTDOWN_PENDING, but the socket
-		 * has been closed by user, don't transition to ESTABLISHED.
-		 * Instead trigger SHUTDOWN bundled with COOKIE_ACK.
+		/* If the socket has been closed by user, don't
+		 * transition to ESTABLISHED. Instead trigger SHUTDOWN
+		 * bundled with COOKIE_ACK.
 		 */
 		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
 		return sctp_sf_do_9_2_start_shutdown(net, ep, asoc,

base-commit: 20a785aa52c82246055a089e55df9dac47d67da1
-- 
2.25.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed
  2020-05-20 15:15 [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed Jere Leppänen
@ 2020-05-20 19:46 ` Marcelo Ricardo Leitner
  2020-05-20 21:50   ` Jere Leppänen
  2020-05-22 22:48 ` David Miller
  1 sibling, 1 reply; 4+ messages in thread
From: Marcelo Ricardo Leitner @ 2020-05-20 19:46 UTC (permalink / raw)
  To: Jere Leppänen
  Cc: linux-sctp, netdev, Vlad Yasevich, Neil Horman, David S . Miller,
	David Laight

On Wed, May 20, 2020 at 06:15:31PM +0300, Jere Leppänen wrote:
> Commit bdf6fa52f01b ("sctp: handle association restarts when the
> socket is closed.") starts shutdown when an association is restarted,
> if in SHUTDOWN-PENDING state and the socket is closed. However, the
> rationale stated in that commit applies also when in SHUTDOWN-SENT
> state - we don't want to move an association to ESTABLISHED state when
> the socket has been closed, because that results in an association
> that is unreachable from user space.
> 
> The problem scenario:
> 
> 1.  Client crashes and/or restarts.
> 
> 2.  Server (using one-to-one socket) calls close(). SHUTDOWN is lost.
> 
> 3.  Client reconnects using the same addresses and ports.
> 
> 4.  Server's association is restarted. The association and the socket
>     move to ESTABLISHED state, even though the server process has
>     closed its descriptor.
> 
> Also, after step 4 when the server process exits, some resources are
> leaked in an attempt to release the underlying inet sock structure in
> ESTABLISHED state:
> 
>     IPv4: Attempt to release TCP socket in state 1 00000000377288c7
> 
> Fix by acting the same way as in SHUTDOWN-PENDING state. That is, if
> an association is restarted in SHUTDOWN-SENT state and the socket is
> closed, then start shutdown and don't move the association or the
> socket to ESTABLISHED state.
> 
> Fixes: bdf6fa52f01b ("sctp: handle association restarts when the socket is closed.")
> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
> ---
>  net/sctp/sm_statefuns.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 26788f4a3b9e..e86620fbd90f 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1856,12 +1856,13 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
>  	/* 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) &&
> +	if ((sctp_state(asoc, SHUTDOWN_PENDING) ||
> +	     sctp_state(asoc, SHUTDOWN_SENT)) &&
>  	    (sctp_sstate(asoc->base.sk, CLOSING) ||
>  	     sock_flag(asoc->base.sk, SOCK_DEAD))) {
> -		/* if were currently in SHUTDOWN_PENDING, but the socket
> -		 * has been closed by user, don't transition to ESTABLISHED.
> -		 * Instead trigger SHUTDOWN bundled with COOKIE_ACK.
> +		/* If the socket has been closed by user, don't
> +		 * transition to ESTABLISHED. Instead trigger SHUTDOWN
> +		 * bundled with COOKIE_ACK.
>  		 */
>  		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
>  		return sctp_sf_do_9_2_start_shutdown(net, ep, asoc,
> 
> base-commit: 20a785aa52c82246055a089e55df9dac47d67da1

This last line is not standard, but git didn't complain about it here.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed
  2020-05-20 19:46 ` Marcelo Ricardo Leitner
@ 2020-05-20 21:50   ` Jere Leppänen
  0 siblings, 0 replies; 4+ messages in thread
From: Jere Leppänen @ 2020-05-20 21:50 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: Jere Leppänen, linux-sctp, netdev, Vlad Yasevich,
	Neil Horman, David S . Miller, David Laight


[-- Attachment #1: Type: text/plain, Size: 3360 bytes --]

On Wed, 20 May 2020, Marcelo Ricardo Leitner wrote:

> On Wed, May 20, 2020 at 06:15:31PM +0300, Jere Leppänen wrote:
> > Commit bdf6fa52f01b ("sctp: handle association restarts when the
> > socket is closed.") starts shutdown when an association is restarted,
> > if in SHUTDOWN-PENDING state and the socket is closed. However, the
> > rationale stated in that commit applies also when in SHUTDOWN-SENT
> > state - we don't want to move an association to ESTABLISHED state when
> > the socket has been closed, because that results in an association
> > that is unreachable from user space.
> > 
> > The problem scenario:
> > 
> > 1.  Client crashes and/or restarts.
> > 
> > 2.  Server (using one-to-one socket) calls close(). SHUTDOWN is lost.
> > 
> > 3.  Client reconnects using the same addresses and ports.
> > 
> > 4.  Server's association is restarted. The association and the socket
> >     move to ESTABLISHED state, even though the server process has
> >     closed its descriptor.
> > 
> > Also, after step 4 when the server process exits, some resources are
> > leaked in an attempt to release the underlying inet sock structure in
> > ESTABLISHED state:
> > 
> >     IPv4: Attempt to release TCP socket in state 1 00000000377288c7
> > 
> > Fix by acting the same way as in SHUTDOWN-PENDING state. That is, if
> > an association is restarted in SHUTDOWN-SENT state and the socket is
> > closed, then start shutdown and don't move the association or the
> > socket to ESTABLISHED state.
> > 
> > Fixes: bdf6fa52f01b ("sctp: handle association restarts when the socket is closed.")
> > Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>
> > ---
> >  net/sctp/sm_statefuns.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> > index 26788f4a3b9e..e86620fbd90f 100644
> > --- a/net/sctp/sm_statefuns.c
> > +++ b/net/sctp/sm_statefuns.c
> > @@ -1856,12 +1856,13 @@ static enum sctp_disposition sctp_sf_do_dupcook_a(
> >  	/* 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) &&
> > +	if ((sctp_state(asoc, SHUTDOWN_PENDING) ||
> > +	     sctp_state(asoc, SHUTDOWN_SENT)) &&
> >  	    (sctp_sstate(asoc->base.sk, CLOSING) ||
> >  	     sock_flag(asoc->base.sk, SOCK_DEAD))) {
> > -		/* if were currently in SHUTDOWN_PENDING, but the socket
> > -		 * has been closed by user, don't transition to ESTABLISHED.
> > -		 * Instead trigger SHUTDOWN bundled with COOKIE_ACK.
> > +		/* If the socket has been closed by user, don't
> > +		 * transition to ESTABLISHED. Instead trigger SHUTDOWN
> > +		 * bundled with COOKIE_ACK.
> >  		 */
> >  		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(repl));
> >  		return sctp_sf_do_9_2_start_shutdown(net, ep, asoc,
> > 
> > base-commit: 20a785aa52c82246055a089e55df9dac47d67da1
> 
> This last line is not standard, but git didn't complain about it here.

The git format-patch --base option. It's mentioned in the docs:

https://www.kernel.org/doc/html/v5.6/process/submitting-patches.html#providing-base-tree-information

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

Thanks for the speedy ack, Marcelo.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed
  2020-05-20 15:15 [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed Jere Leppänen
  2020-05-20 19:46 ` Marcelo Ricardo Leitner
@ 2020-05-22 22:48 ` David Miller
  1 sibling, 0 replies; 4+ messages in thread
From: David Miller @ 2020-05-22 22:48 UTC (permalink / raw)
  To: jere.leppanen
  Cc: linux-sctp, netdev, vyasevich, nhorman, marcelo.leitner, David.Laight

From: Jere Leppänen <jere.leppanen@nokia.com>
Date: Wed, 20 May 2020 18:15:31 +0300

> Commit bdf6fa52f01b ("sctp: handle association restarts when the
> socket is closed.") starts shutdown when an association is restarted,
> if in SHUTDOWN-PENDING state and the socket is closed. However, the
> rationale stated in that commit applies also when in SHUTDOWN-SENT
> state - we don't want to move an association to ESTABLISHED state when
> the socket has been closed, because that results in an association
> that is unreachable from user space.
> 
> The problem scenario:
> 
> 1.  Client crashes and/or restarts.
> 
> 2.  Server (using one-to-one socket) calls close(). SHUTDOWN is lost.
> 
> 3.  Client reconnects using the same addresses and ports.
> 
> 4.  Server's association is restarted. The association and the socket
>     move to ESTABLISHED state, even though the server process has
>     closed its descriptor.
> 
> Also, after step 4 when the server process exits, some resources are
> leaked in an attempt to release the underlying inet sock structure in
> ESTABLISHED state:
> 
>     IPv4: Attempt to release TCP socket in state 1 00000000377288c7
> 
> Fix by acting the same way as in SHUTDOWN-PENDING state. That is, if
> an association is restarted in SHUTDOWN-SENT state and the socket is
> closed, then start shutdown and don't move the association or the
> socket to ESTABLISHED state.
> 
> Fixes: bdf6fa52f01b ("sctp: handle association restarts when the socket is closed.")
> Signed-off-by: Jere Leppänen <jere.leppanen@nokia.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 15:15 [PATCH net 1/1] sctp: Start shutdown on association restart if in SHUTDOWN-SENT state and socket is closed Jere Leppänen
2020-05-20 19:46 ` Marcelo Ricardo Leitner
2020-05-20 21:50   ` Jere Leppänen
2020-05-22 22:48 ` David Miller

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git