diff mbox series

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

Message ID 4e2e8aad9c4646ec3a51833cbbf95a006a98b756.1691945735.git.lucien.xin@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [nf] netfilter: set default timeout to 3 secs for sctp shutdown send and recv state | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1330 this patch: 1330
netdev/cc_maintainers fail 3 blamed authors not CCed: yasuyuki.kozakai@toshiba.co.jp laforge@netfilter.org acme@mandriva.com; 4 maintainers not CCed: yasuyuki.kozakai@toshiba.co.jp coreteam@netfilter.org laforge@netfilter.org acme@mandriva.com
netdev/build_clang success Errors and warnings before: 1353 this patch: 1353
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1353 this patch: 1353
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 18 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Xin Long Aug. 13, 2023, 4:55 p.m. UTC 
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(-)

Comments

Sriram Yagnaraman Aug. 15, 2023, 7:59 a.m. UTC | #1 
> -----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?
Simon Horman Aug. 15, 2023, 7:59 a.m. UTC | #2 
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>
Xin Long Aug. 15, 2023, 2:01 p.m. UTC | #3 
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.
diff mbox series

Patch

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},