On Tue, Aug 15, 2023 at 3:59 AM Sriram Yagnaraman
[off-list ref] wrote:

quoted

-----Original Message-----
From: Xin Long <lucien.xin@gmail.com>
Sent: Sunday, 13 August 2023 18:56
To: network dev <redacted>; netfilter-devel@vger.kernel.org;
linux-sctp@vger.kernel.org
Cc: davem@davemloft.net; kuba@kernel.org; Eric Dumazet
[off-list ref]; Paolo Abeni [off-list ref]; Pablo Neira
Ayuso [off-list ref]; Jozsef Kadlecsik [off-list ref]; Florian
Westphal [off-list ref]; Marcelo Ricardo Leitner
[off-list ref]
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 <redacted>
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.