[PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state

[PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state

[Xin Long]

In SCTP protocol, it is using the same timer (T2 timer) for SHUTDOWN and
SHUTDOWN_ACK retransmission. However in sctp conntrack the default timeout
value for SCTP_CONNTRACK_SHUTDOWN_ACK_SENT state is 3 secs while it's 300
msecs for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV state.

As Paolo Valerio noticed, this might cause unwanted expiration of the ct
entry. In my test, with 1s tc netem delay set on the NAT path, after the
SHUTDOWN is sent, the sctp ct entry enters SCTP_CONNTRACK_SHUTDOWN_SEND
state. However, due to 300ms (too short) delay, when the SHUTDOWN_ACK is
sent back from the peer, the sctp ct entry has expired and been deleted,
and then the SHUTDOWN_ACK has to be dropped.

Also, it is confusing these two sysctl options always show 0 due to all
timeout values using sec as unit:

  net.netfilter.nf_conntrack_sctp_timeout_shutdown_recd = 0
  net.netfilter.nf_conntrack_sctp_timeout_shutdown_sent = 0

This patch fixes it by also using 3 secs for sctp shutdown send and recv
state in sctp conntrack, which is also RTO.initial value in SCTP protocol.

Note that the very short time value for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV
was probably used for a rare scenario where SHUTDOWN is sent on 1st path
but SHUTDOWN_ACK is replied on 2nd path, then a new connection started
immediately on 1st path. So this patch also moves from SHUTDOWN_SEND/RECV
to CLOSE when receiving INIT in the ORIGINAL direction.

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
Reported-by: Paolo Valerio <pvalerio@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/netfilter/nf_conntrack_proto_sctp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 91eacc9b0b98..b6bcc8f2f46b 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -49,8 +49,8 @@ static const unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] = {
 	[SCTP_CONNTRACK_COOKIE_WAIT]		= 3 SECS,
 	[SCTP_CONNTRACK_COOKIE_ECHOED]		= 3 SECS,
 	[SCTP_CONNTRACK_ESTABLISHED]		= 210 SECS,
-	[SCTP_CONNTRACK_SHUTDOWN_SENT]		= 300 SECS / 1000,
-	[SCTP_CONNTRACK_SHUTDOWN_RECD]		= 300 SECS / 1000,
+	[SCTP_CONNTRACK_SHUTDOWN_SENT]		= 3 SECS,
+	[SCTP_CONNTRACK_SHUTDOWN_RECD]		= 3 SECS,
 	[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]	= 3 SECS,
 	[SCTP_CONNTRACK_HEARTBEAT_SENT]		= 30 SECS,
 };
@@ -105,7 +105,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
 	{
 /*	ORIGINAL	*/
 /*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS */
-/* init         */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCW},
+/* init         */ {sCL, sCL, sCW, sCE, sES, sCL, sCL, sSA, sCW},
 /* init_ack     */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},
 /* abort        */ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL},
 /* shutdown     */ {sCL, sCL, sCW, sCE, sSS, sSS, sSR, sSA, sCL},
-- 
2.39.1

RE: [PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state

[Sriram Yagnaraman]

> -----Original Message-----
> From: Xin Long <lucien.xin@gmail.com>
> Sent: Sunday, 13 August 2023 18:56
> To: network dev <netdev@vger.kernel.org>; netfilter-devel@vger.kernel.org;
> linux-sctp@vger.kernel.org
> Cc: davem@davemloft.net; kuba@kernel.org; Eric Dumazet
> <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Pablo Neira
> Ayuso <pablo@netfilter.org>; Jozsef Kadlecsik <kadlec@netfilter.org>; Florian
> Westphal <fw@strlen.de>; Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com>
> Subject: [PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown
> send and recv state
> 
> In SCTP protocol, it is using the same timer (T2 timer) for SHUTDOWN and
> SHUTDOWN_ACK retransmission. However in sctp conntrack the default
> timeout value for SCTP_CONNTRACK_SHUTDOWN_ACK_SENT state is 3 secs
> while it's 300 msecs for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV state.
> 
> As Paolo Valerio noticed, this might cause unwanted expiration of the ct entry.
> In my test, with 1s tc netem delay set on the NAT path, after the SHUTDOWN is
> sent, the sctp ct entry enters SCTP_CONNTRACK_SHUTDOWN_SEND state.
> However, due to 300ms (too short) delay, when the SHUTDOWN_ACK is sent
> back from the peer, the sctp ct entry has expired and been deleted, and then
> the SHUTDOWN_ACK has to be dropped.
> 
> Also, it is confusing these two sysctl options always show 0 due to all timeout
> values using sec as unit:
> 
>   net.netfilter.nf_conntrack_sctp_timeout_shutdown_recd = 0
>   net.netfilter.nf_conntrack_sctp_timeout_shutdown_sent = 0
> 
> This patch fixes it by also using 3 secs for sctp shutdown send and recv state in
> sctp conntrack, which is also RTO.initial value in SCTP protocol.
> 
> Note that the very short time value for
> SCTP_CONNTRACK_SHUTDOWN_SEND/RECV was probably used for a rare
> scenario where SHUTDOWN is sent on 1st path but SHUTDOWN_ACK is replied
> on 2nd path, then a new connection started immediately on 1st path. So this
> patch also moves from SHUTDOWN_SEND/RECV to CLOSE when receiving INIT
> in the ORIGINAL direction.
> 
> Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
> Reported-by: Paolo Valerio <pvalerio@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/netfilter/nf_conntrack_proto_sctp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c
> b/net/netfilter/nf_conntrack_proto_sctp.c
> index 91eacc9b0b98..b6bcc8f2f46b 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -49,8 +49,8 @@ static const unsigned int
> sctp_timeouts[SCTP_CONNTRACK_MAX] = {
>  	[SCTP_CONNTRACK_COOKIE_WAIT]		= 3 SECS,
>  	[SCTP_CONNTRACK_COOKIE_ECHOED]		= 3 SECS,
>  	[SCTP_CONNTRACK_ESTABLISHED]		= 210 SECS,
> -	[SCTP_CONNTRACK_SHUTDOWN_SENT]		= 300 SECS /
> 1000,
> -	[SCTP_CONNTRACK_SHUTDOWN_RECD]		= 300 SECS /
> 1000,
> +	[SCTP_CONNTRACK_SHUTDOWN_SENT]		= 3 SECS,
> +	[SCTP_CONNTRACK_SHUTDOWN_RECD]		= 3 SECS,
>  	[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]	= 3 SECS,
>  	[SCTP_CONNTRACK_HEARTBEAT_SENT]		= 30 SECS,
>  };
> @@ -105,7 +105,7 @@ static const u8
> sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
>  	{
>  /*	ORIGINAL	*/
>  /*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS */
> -/* init         */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCW},
> +/* init         */ {sCL, sCL, sCW, sCE, sES, sCL, sCL, sSA, sCW},
>  /* init_ack     */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},
>  /* abort        */ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL},
>  /* shutdown     */ {sCL, sCL, sCW, sCE, sSS, sSS, sSR, sSA, sCL},
> --
> 2.39.1

FWIW, I like this patch. Should Documentation/networking/nf_conntrack-sysctl.rst be updated to reflect the new timeout values?

Re: [PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state

[Simon Horman]

On Sun, Aug 13, 2023 at 12:55:35PM -0400, Xin Long wrote:
> In SCTP protocol, it is using the same timer (T2 timer) for SHUTDOWN and
> SHUTDOWN_ACK retransmission. However in sctp conntrack the default timeout
> value for SCTP_CONNTRACK_SHUTDOWN_ACK_SENT state is 3 secs while it's 300
> msecs for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV state.
> 
> As Paolo Valerio noticed, this might cause unwanted expiration of the ct
> entry. In my test, with 1s tc netem delay set on the NAT path, after the
> SHUTDOWN is sent, the sctp ct entry enters SCTP_CONNTRACK_SHUTDOWN_SEND
> state. However, due to 300ms (too short) delay, when the SHUTDOWN_ACK is
> sent back from the peer, the sctp ct entry has expired and been deleted,
> and then the SHUTDOWN_ACK has to be dropped.
> 
> Also, it is confusing these two sysctl options always show 0 due to all
> timeout values using sec as unit:
> 
>   net.netfilter.nf_conntrack_sctp_timeout_shutdown_recd = 0
>   net.netfilter.nf_conntrack_sctp_timeout_shutdown_sent = 0
> 
> This patch fixes it by also using 3 secs for sctp shutdown send and recv
> state in sctp conntrack, which is also RTO.initial value in SCTP protocol.
> 
> Note that the very short time value for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV
> was probably used for a rare scenario where SHUTDOWN is sent on 1st path
> but SHUTDOWN_ACK is replied on 2nd path, then a new connection started
> immediately on 1st path. So this patch also moves from SHUTDOWN_SEND/RECV
> to CLOSE when receiving INIT in the ORIGINAL direction.
> 
> Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
> Reported-by: Paolo Valerio <pvalerio@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state

[Xin Long]

On Tue, Aug 15, 2023 at 3:59 AM Sriram Yagnaraman
<sriram.yagnaraman@est.tech> wrote:
>
>
>
> > -----Original Message-----
> > From: Xin Long <lucien.xin@gmail.com>
> > Sent: Sunday, 13 August 2023 18:56
> > To: network dev <netdev@vger.kernel.org>; netfilter-devel@vger.kernel.org;
> > linux-sctp@vger.kernel.org
> > Cc: davem@davemloft.net; kuba@kernel.org; Eric Dumazet
> > <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Pablo Neira
> > Ayuso <pablo@netfilter.org>; Jozsef Kadlecsik <kadlec@netfilter.org>; Florian
> > Westphal <fw@strlen.de>; Marcelo Ricardo Leitner
> > <marcelo.leitner@gmail.com>
> > Subject: [PATCH nf] netfilter: set default timeout to 3 secs for sctp shutdown
> > send and recv state
> >
> > In SCTP protocol, it is using the same timer (T2 timer) for SHUTDOWN and
> > SHUTDOWN_ACK retransmission. However in sctp conntrack the default
> > timeout value for SCTP_CONNTRACK_SHUTDOWN_ACK_SENT state is 3 secs
> > while it's 300 msecs for SCTP_CONNTRACK_SHUTDOWN_SEND/RECV state.
> >
> > As Paolo Valerio noticed, this might cause unwanted expiration of the ct entry.
> > In my test, with 1s tc netem delay set on the NAT path, after the SHUTDOWN is
> > sent, the sctp ct entry enters SCTP_CONNTRACK_SHUTDOWN_SEND state.
> > However, due to 300ms (too short) delay, when the SHUTDOWN_ACK is sent
> > back from the peer, the sctp ct entry has expired and been deleted, and then
> > the SHUTDOWN_ACK has to be dropped.
> >
> > Also, it is confusing these two sysctl options always show 0 due to all timeout
> > values using sec as unit:
> >
> >   net.netfilter.nf_conntrack_sctp_timeout_shutdown_recd = 0
> >   net.netfilter.nf_conntrack_sctp_timeout_shutdown_sent = 0
> >
> > This patch fixes it by also using 3 secs for sctp shutdown send and recv state in
> > sctp conntrack, which is also RTO.initial value in SCTP protocol.
> >
> > Note that the very short time value for
> > SCTP_CONNTRACK_SHUTDOWN_SEND/RECV was probably used for a rare
> > scenario where SHUTDOWN is sent on 1st path but SHUTDOWN_ACK is replied
> > on 2nd path, then a new connection started immediately on 1st path. So this
> > patch also moves from SHUTDOWN_SEND/RECV to CLOSE when receiving INIT
> > in the ORIGINAL direction.
> >
> > Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")
> > Reported-by: Paolo Valerio <pvalerio@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/netfilter/nf_conntrack_proto_sctp.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/netfilter/nf_conntrack_proto_sctp.c
> > b/net/netfilter/nf_conntrack_proto_sctp.c
> > index 91eacc9b0b98..b6bcc8f2f46b 100644
> > --- a/net/netfilter/nf_conntrack_proto_sctp.c
> > +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> > @@ -49,8 +49,8 @@ static const unsigned int
> > sctp_timeouts[SCTP_CONNTRACK_MAX] = {
> >       [SCTP_CONNTRACK_COOKIE_WAIT]            = 3 SECS,
> >       [SCTP_CONNTRACK_COOKIE_ECHOED]          = 3 SECS,
> >       [SCTP_CONNTRACK_ESTABLISHED]            = 210 SECS,
> > -     [SCTP_CONNTRACK_SHUTDOWN_SENT]          = 300 SECS /
> > 1000,
> > -     [SCTP_CONNTRACK_SHUTDOWN_RECD]          = 300 SECS /
> > 1000,
> > +     [SCTP_CONNTRACK_SHUTDOWN_SENT]          = 3 SECS,
> > +     [SCTP_CONNTRACK_SHUTDOWN_RECD]          = 3 SECS,
> >       [SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]      = 3 SECS,
> >       [SCTP_CONNTRACK_HEARTBEAT_SENT]         = 30 SECS,
> >  };
> > @@ -105,7 +105,7 @@ static const u8
> > sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
> >       {
> >  /*   ORIGINAL        */
> >  /*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS */
> > -/* init         */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCW},
> > +/* init         */ {sCL, sCL, sCW, sCE, sES, sCL, sCL, sSA, sCW},
> >  /* init_ack     */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},
> >  /* abort        */ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL},
> >  /* shutdown     */ {sCL, sCL, sCW, sCE, sSS, sSS, sSR, sSA, sCL},
> > --
> > 2.39.1
>
> FWIW, I like this patch. Should Documentation/networking/nf_conntrack-sysctl.rst be updated to reflect the new timeout values?
Good catch, will post v2 with the 0.3 -> 3 change in nf_conntrack-sysctl.rst.

Thanks for the review.