[PATCH 0/3] sctp conntrack fixes

[PATCH 0/3] sctp conntrack fixes

[Sriram Yagnaraman]

A less diruptive change as opposed to below RFC patch:
https://lore.kernel.org/netfilter-devel/20230104113143.21769-1-sriram.yagnaraman@est.tech/

This contains a couple of bug fixes to existing bugs that were found
during the review of the above patch series, and also a patch that
unifies the ESTABLISHED states for primary and secondary paths.

Sriram Yagnaraman (3):
  netfilter: conntrack: fix vtag checks for ABORT/SHUTDOWN_COMPLETE
  netfilter: conntrack: fix bug in for_each_sctp_chunk
  netfilter: conntrack: unify established states for SCTP paths

 .../uapi/linux/netfilter/nf_conntrack_sctp.h  |   4 +-
 .../linux/netfilter/nfnetlink_cttimeout.h     |   4 +-
 net/netfilter/nf_conntrack_proto_sctp.c       | 119 +++++++++---------
 net/netfilter/nf_conntrack_standalone.c       |  16 ---
 4 files changed, 60 insertions(+), 83 deletions(-)

-- 
2.34.1

[PATCH 1/3] netfilter: conntrack: fix vtag checks for ABORT/SHUTDOWN_COMPLETE

[Sriram Yagnaraman]

RFC 9260, Sec 8.5.1 states that for ABORT/SHUTDOWN_COMPLETE, the chunk
MUST be accepted if the vtag of the packet matches its own tag and the
T bit is not set OR if it is set to its peer's vtag and the T bit is set
in chunk flags. Otherwise the packet MUST be silently dropped.

Update vtag verification for ABORT/SHUTDOWN_COMPLETE based on the above
description.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 net/netfilter/nf_conntrack_proto_sctp.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index d88b92a8ffca..2b2c549ba678 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -424,22 +424,29 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct,
 	for_each_sctp_chunk (skb, sch, _sch, offset, dataoff, count) {
 		/* Special cases of Verification tag check (Sec 8.5.1) */
 		if (sch->type == SCTP_CID_INIT) {
-			/* Sec 8.5.1 (A) */
+			/* (A) vtag MUST be zero */
 			if (sh->vtag != 0)
 				goto out_unlock;
 		} else if (sch->type == SCTP_CID_ABORT) {
-			/* Sec 8.5.1 (B) */
-			if (sh->vtag != ct->proto.sctp.vtag[dir] &&
-			    sh->vtag != ct->proto.sctp.vtag[!dir])
+			/* (B) vtag MUST match own vtag if T flag is unset OR
+			 * MUST match peer's vtag if T flag is set
+			 */
+			if ((!(sch->flags & SCTP_CHUNK_FLAG_T) &&
+			     sh->vtag != ct->proto.sctp.vtag[dir]) ||
+			    ((sch->flags & SCTP_CHUNK_FLAG_T) &&
+			     sh->vtag != ct->proto.sctp.vtag[!dir]))
 				goto out_unlock;
 		} else if (sch->type == SCTP_CID_SHUTDOWN_COMPLETE) {
-			/* Sec 8.5.1 (C) */
-			if (sh->vtag != ct->proto.sctp.vtag[dir] &&
-			    sh->vtag != ct->proto.sctp.vtag[!dir] &&
-			    sch->flags & SCTP_CHUNK_FLAG_T)
+			/* (C) vtag MUST match own vtag if T flag is unset OR
+			 * MUST match peer's vtag if T flag is set
+			 */
+			if ((!(sch->flags & SCTP_CHUNK_FLAG_T) &&
+			     sh->vtag != ct->proto.sctp.vtag[dir]) ||
+			    ((sch->flags & SCTP_CHUNK_FLAG_T) &&
+			     sh->vtag != ct->proto.sctp.vtag[!dir]))
 				goto out_unlock;
 		} else if (sch->type == SCTP_CID_COOKIE_ECHO) {
-			/* Sec 8.5.1 (D) */
+			/* (D) vtag must be same as init_vtag as found in INIT_ACK */
 			if (sh->vtag != ct->proto.sctp.vtag[dir])
 				goto out_unlock;
 		} else if (sch->type == SCTP_CID_HEARTBEAT) {
-- 
2.34.1

[PATCH 2/3] netfilter: conntrack: fix bug in for_each_sctp_chunk

[Sriram Yagnaraman]

skb_header_pointer() will return NULL if offset + sizeof(_sch) exceeds
skb->len, so this offset < skb->len test is redundant.

if sch->length == 0, this will end up in an infinite loop, add a check
for sch->length > 0

Suggested-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 net/netfilter/nf_conntrack_proto_sctp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 2b2c549ba678..c561c1213704 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -160,8 +160,8 @@ static void sctp_print_conntrack(struct seq_file *s, struct nf_conn *ct)
 
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
 for ((offset) = (dataoff) + sizeof(struct sctphdr), (count) = 0;	\
-	(offset) < (skb)->len &&					\
-	((sch) = skb_header_pointer((skb), (offset), sizeof(_sch), &(_sch)));	\
+	((sch) = skb_header_pointer((skb), (offset), sizeof(_sch), &(_sch))) &&	\
+	(sch)->length;	\
 	(offset) += (ntohs((sch)->length) + 3) & ~3, (count)++)
 
 /* Some validity checks to make sure the chunks are fine */
-- 
2.34.1

[PATCH 3/3] netfilter: conntrack: unify established states for SCTP paths

[Sriram Yagnaraman]

An SCTP endpoint can start an association through a path and tear it
down over another one. That means the initial path will not see the
shutdown sequence, and the conntrack entry will remain in ESTABLISHED
state for 5 days.

By merging the HEARTBEAT_ACKED and ESTABLISHED states into one
ESTABLISHED state, there remains no difference between a primary or
secondary path. The timeout for the merged ESTABLISHED state is set to
210 seconds (hb_interval * max_path_retrans + rto_max). So, even if a
path doesn't see the shutdown sequence, it will expire in a reasonable
amount of time.

Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
---
 .../uapi/linux/netfilter/nf_conntrack_sctp.h  |  4 +-
 .../linux/netfilter/nfnetlink_cttimeout.h     |  4 +-
 net/netfilter/nf_conntrack_proto_sctp.c       | 90 ++++++++-----------
 net/netfilter/nf_conntrack_standalone.c       | 16 ----
 4 files changed, 42 insertions(+), 72 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
index c742469afe21..150fc3c056ea 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
@@ -15,8 +15,8 @@ enum sctp_conntrack {
 	SCTP_CONNTRACK_SHUTDOWN_RECD,
 	SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
 	SCTP_CONNTRACK_HEARTBEAT_SENT,
-	SCTP_CONNTRACK_HEARTBEAT_ACKED,
-	SCTP_CONNTRACK_DATA_SENT,
+	SCTP_CONNTRACK_HEARTBEAT_ACKED,	/* no longer used */
+	SCTP_CONNTRACK_DATA_SENT,	/* no longer used */
 	SCTP_CONNTRACK_MAX
 };
 
diff --git a/include/uapi/linux/netfilter/nfnetlink_cttimeout.h b/include/uapi/linux/netfilter/nfnetlink_cttimeout.h
index 94e74034706d..282343d433a6 100644
--- a/include/uapi/linux/netfilter/nfnetlink_cttimeout.h
+++ b/include/uapi/linux/netfilter/nfnetlink_cttimeout.h
@@ -94,8 +94,8 @@ enum ctattr_timeout_sctp {
 	CTA_TIMEOUT_SCTP_SHUTDOWN_RECD,
 	CTA_TIMEOUT_SCTP_SHUTDOWN_ACK_SENT,
 	CTA_TIMEOUT_SCTP_HEARTBEAT_SENT,
-	CTA_TIMEOUT_SCTP_HEARTBEAT_ACKED,
-	CTA_TIMEOUT_SCTP_DATA_SENT,
+	CTA_TIMEOUT_SCTP_HEARTBEAT_ACKED,	/* no longer used */
+	CTA_TIMEOUT_SCTP_DATA_SENT,		/* no longer used */
 	__CTA_TIMEOUT_SCTP_MAX
 };
 #define CTA_TIMEOUT_SCTP_MAX (__CTA_TIMEOUT_SCTP_MAX - 1)
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index c561c1213704..22417757ce94 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -27,22 +27,16 @@
 #include <net/netfilter/nf_conntrack_ecache.h>
 #include <net/netfilter/nf_conntrack_timeout.h>
 
-/* FIXME: Examine ipfilter's timeouts and conntrack transitions more
-   closely.  They're more complex. --RR
-
-   And so for me for SCTP :D -Kiran */
-
 static const char *const sctp_conntrack_names[] = {
-	"NONE",
-	"CLOSED",
-	"COOKIE_WAIT",
-	"COOKIE_ECHOED",
-	"ESTABLISHED",
-	"SHUTDOWN_SENT",
-	"SHUTDOWN_RECD",
-	"SHUTDOWN_ACK_SENT",
-	"HEARTBEAT_SENT",
-	"HEARTBEAT_ACKED",
+	[SCTP_CONNTRACK_NONE] = "NONE",
+	[SCTP_CONNTRACK_CLOSED] = "CLOSED",
+	[SCTP_CONNTRACK_COOKIE_WAIT] = "COOKIE_WAIT",
+	[SCTP_CONNTRACK_COOKIE_ECHOED] = "COOKIE_ECHOED",
+	[SCTP_CONNTRACK_ESTABLISHED] = "ESTABLISHED",
+	[SCTP_CONNTRACK_SHUTDOWN_SENT] = "SHUTDOWN_SENT",
+	[SCTP_CONNTRACK_SHUTDOWN_RECD] = "SHUTDOWN_RECD",
+	[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT] = "SHUTDOWN_ACK_SENT",
+	[SCTP_CONNTRACK_HEARTBEAT_SENT] = "HEARTBEAT_SENT",
 };
 
 #define SECS  * HZ
@@ -54,13 +48,11 @@ static const unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] = {
 	[SCTP_CONNTRACK_CLOSED]			= 10 SECS,
 	[SCTP_CONNTRACK_COOKIE_WAIT]		= 3 SECS,
 	[SCTP_CONNTRACK_COOKIE_ECHOED]		= 3 SECS,
-	[SCTP_CONNTRACK_ESTABLISHED]		= 5 DAYS,
+	[SCTP_CONNTRACK_ESTABLISHED]		= 210 SECS,
 	[SCTP_CONNTRACK_SHUTDOWN_SENT]		= 300 SECS / 1000,
 	[SCTP_CONNTRACK_SHUTDOWN_RECD]		= 300 SECS / 1000,
 	[SCTP_CONNTRACK_SHUTDOWN_ACK_SENT]	= 3 SECS,
 	[SCTP_CONNTRACK_HEARTBEAT_SENT]		= 30 SECS,
-	[SCTP_CONNTRACK_HEARTBEAT_ACKED]	= 210 SECS,
-	[SCTP_CONNTRACK_DATA_SENT]		= 30 SECS,
 };
 
 #define	SCTP_FLAG_HEARTBEAT_VTAG_FAILED	1
@@ -74,8 +66,6 @@ static const unsigned int sctp_timeouts[SCTP_CONNTRACK_MAX] = {
 #define	sSR SCTP_CONNTRACK_SHUTDOWN_RECD
 #define	sSA SCTP_CONNTRACK_SHUTDOWN_ACK_SENT
 #define	sHS SCTP_CONNTRACK_HEARTBEAT_SENT
-#define	sHA SCTP_CONNTRACK_HEARTBEAT_ACKED
-#define	sDS SCTP_CONNTRACK_DATA_SENT
 #define	sIV SCTP_CONNTRACK_MAX
 
 /*
@@ -97,11 +87,7 @@ SHUTDOWN_ACK_SENT - We have seen a SHUTDOWN_ACK chunk in the direction opposite
 		    to that of the SHUTDOWN chunk.
 CLOSED            - We have seen a SHUTDOWN_COMPLETE chunk in the direction of
 		    the SHUTDOWN chunk. Connection is closed.
-HEARTBEAT_SENT    - We have seen a HEARTBEAT in a new flow.
-HEARTBEAT_ACKED   - We have seen a HEARTBEAT-ACK/DATA/SACK in the direction
-		    opposite to that of the HEARTBEAT/DATA chunk. Secondary connection
-		    is established.
-DATA_SENT         - We have seen a DATA/SACK in a new flow.
+HEARTBEAT_SENT    - We have seen a HEARTBEAT/DATA/SACK in a new flow.
 */
 
 /* TODO
@@ -118,35 +104,35 @@ cookie echoed to closed.
 static const u8 sctp_conntracks[2][12][SCTP_CONNTRACK_MAX] = {
 	{
 /*	ORIGINAL	*/
-/*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sDS */
-/* init         */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCW, sHA, sCW},
-/* init_ack     */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL, sHA, sCL},
-/* abort        */ {sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sCL},
-/* shutdown     */ {sCL, sCL, sCW, sCE, sSS, sSS, sSR, sSA, sCL, sSS, sCL},
-/* shutdown_ack */ {sSA, sCL, sCW, sCE, sES, sSA, sSA, sSA, sSA, sHA, sSA},
-/* error        */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL, sHA, sCL},/* Can't have Stale cookie*/
-/* cookie_echo  */ {sCL, sCL, sCE, sCE, sES, sSS, sSR, sSA, sCL, sHA, sCL},/* 5.2.4 - Big TODO */
-/* cookie_ack   */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL, sHA, sCL},/* Can't come in orig dir */
-/* shutdown_comp*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sCL, sCL, sHA, sCL},
-/* heartbeat    */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sDS},
-/* heartbeat_ack*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sDS},
-/* data/sack    */ {sDS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sDS}
+/*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS */
+/* init         */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, 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},
+/* shutdown_ack */ {sSA, sCL, sCW, sCE, sES, sSA, sSA, sSA, sSA},
+/* error        */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't have Stale cookie*/
+/* cookie_echo  */ {sCL, sCL, sCE, sCE, sES, sSS, sSR, sSA, sCL},/* 5.2.4 - Big TODO */
+/* cookie_ack   */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */
+/* shutdown_comp*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sCL, sCL},
+/* heartbeat    */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
+/* heartbeat_ack*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
+/* data/sack    */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS}
 	},
 	{
 /*	REPLY	*/
-/*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sDS */
-/* init         */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV, sHA, sIV},/* INIT in sCL Big TODO */
-/* init_ack     */ {sIV, sCW, sCW, sCE, sES, sSS, sSR, sSA, sIV, sHA, sIV},
-/* abort        */ {sIV, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sIV, sCL, sIV},
-/* shutdown     */ {sIV, sCL, sCW, sCE, sSR, sSS, sSR, sSA, sIV, sSR, sIV},
-/* shutdown_ack */ {sIV, sCL, sCW, sCE, sES, sSA, sSA, sSA, sIV, sHA, sIV},
-/* error        */ {sIV, sCL, sCW, sCL, sES, sSS, sSR, sSA, sIV, sHA, sIV},
-/* cookie_echo  */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV, sHA, sIV},/* Can't come in reply dir */
-/* cookie_ack   */ {sIV, sCL, sCW, sES, sES, sSS, sSR, sSA, sIV, sHA, sIV},
-/* shutdown_comp*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sCL, sIV, sHA, sIV},
-/* heartbeat    */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS, sHA, sHA},
-/* heartbeat_ack*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHA, sHA, sHA},
-/* data/sack    */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHA, sHA, sHA},
+/*                  sNO, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS */
+/* init         */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV},/* INIT in sCL Big TODO */
+/* init_ack     */ {sIV, sCW, sCW, sCE, sES, sSS, sSR, sSA, sIV},
+/* abort        */ {sIV, sCL, sCL, sCL, sCL, sCL, sCL, sCL, sIV},
+/* shutdown     */ {sIV, sCL, sCW, sCE, sSR, sSS, sSR, sSA, sIV},
+/* shutdown_ack */ {sIV, sCL, sCW, sCE, sES, sSA, sSA, sSA, sIV},
+/* error        */ {sIV, sCL, sCW, sCL, sES, sSS, sSR, sSA, sIV},
+/* cookie_echo  */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */
+/* cookie_ack   */ {sIV, sCL, sCW, sES, sES, sSS, sSR, sSA, sIV},
+/* shutdown_comp*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sCL, sIV},
+/* heartbeat    */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
+/* heartbeat_ack*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sES},
+/* data/sack    */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sES},
 	}
 };
 
@@ -264,7 +250,7 @@ static int sctp_new_state(enum ip_conntrack_dir dir,
 		i = 11;
 		break;
 	default:
-		/* Other chunks like DATA or SACK do not change the state */
+		/* Other chunks do not change the state */
 		pr_debug("Unknown chunk type, Will stay in %s\n",
 			 sctp_conntrack_names[cur_state]);
 		return cur_state;
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 0250725e38a4..460294bd4b60 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -601,8 +601,6 @@ enum nf_ct_sysctl_index {
 	NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_SHUTDOWN_RECD,
 	NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_SHUTDOWN_ACK_SENT,
 	NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_HEARTBEAT_SENT,
-	NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_HEARTBEAT_ACKED,
-	NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_DATA_SENT,
 #endif
 #ifdef CONFIG_NF_CT_PROTO_DCCP
 	NF_SYSCTL_CT_PROTO_TIMEOUT_DCCP_REQUEST,
@@ -887,18 +885,6 @@ static struct ctl_table nf_ct_sysctl_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_jiffies,
 	},
-	[NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_HEARTBEAT_ACKED] = {
-		.procname       = "nf_conntrack_sctp_timeout_heartbeat_acked",
-		.maxlen         = sizeof(unsigned int),
-		.mode           = 0644,
-		.proc_handler   = proc_dointvec_jiffies,
-	},
-	[NF_SYSCTL_CT_PROTO_TIMEOUT_SCTP_DATA_SENT] = {
-		.procname       = "nf_conntrack_sctp_timeout_data_sent",
-		.maxlen         = sizeof(unsigned int),
-		.mode           = 0644,
-		.proc_handler   = proc_dointvec_jiffies,
-	},
 #endif
 #ifdef CONFIG_NF_CT_PROTO_DCCP
 	[NF_SYSCTL_CT_PROTO_TIMEOUT_DCCP_REQUEST] = {
@@ -1042,8 +1028,6 @@ static void nf_conntrack_standalone_init_sctp_sysctl(struct net *net,
 	XASSIGN(SHUTDOWN_RECD, sn);
 	XASSIGN(SHUTDOWN_ACK_SENT, sn);
 	XASSIGN(HEARTBEAT_SENT, sn);
-	XASSIGN(HEARTBEAT_ACKED, sn);
-	XASSIGN(DATA_SENT, sn);
 #undef XASSIGN
 #endif
 }
-- 
2.34.1

Re: [PATCH 1/3] netfilter: conntrack: fix vtag checks for ABORT/SHUTDOWN_COMPLETE

[Pablo Neira Ayuso]

On Mon, Jan 16, 2023 at 10:35:54AM +0100, Sriram Yagnaraman wrote:
> RFC 9260, Sec 8.5.1 states that for ABORT/SHUTDOWN_COMPLETE, the chunk
> MUST be accepted if the vtag of the packet matches its own tag and the
> T bit is not set OR if it is set to its peer's vtag and the T bit is set
> in chunk flags. Otherwise the packet MUST be silently dropped.
> 
> Update vtag verification for ABORT/SHUTDOWN_COMPLETE based on the above
> description.

I suspect this is broken since the beginning? Then a good Fixes: tag
candidate it...

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")

?

Re: [PATCH 2/3] netfilter: conntrack: fix bug in for_each_sctp_chunk

[Pablo Neira Ayuso]

On Mon, Jan 16, 2023 at 10:35:55AM +0100, Sriram Yagnaraman wrote:
> skb_header_pointer() will return NULL if offset + sizeof(_sch) exceeds
> skb->len, so this offset < skb->len test is redundant.
> 
> if sch->length == 0, this will end up in an infinite loop, add a check
> for sch->length > 0

If this is broken since the beginning, then:

Fixes: 9fb9cbb1082d ("[NETFILTER]: Add nf_conntrack subsystem.")

is sufficiently old for -stable kernels to pick up this.

Let me know if this looks good to you, thanks

Re: [PATCH 3/3] netfilter: conntrack: unify established states for SCTP paths

[Pablo Neira Ayuso]

On Mon, Jan 16, 2023 at 10:35:56AM +0100, Sriram Yagnaraman wrote:
> An SCTP endpoint can start an association through a path and tear it
> down over another one. That means the initial path will not see the
> shutdown sequence, and the conntrack entry will remain in ESTABLISHED
> state for 5 days.
> 
> By merging the HEARTBEAT_ACKED and ESTABLISHED states into one
> ESTABLISHED state, there remains no difference between a primary or
> secondary path. The timeout for the merged ESTABLISHED state is set to
> 210 seconds (hb_interval * max_path_retrans + rto_max). So, even if a
> path doesn't see the shutdown sequence, it will expire in a reasonable
> amount of time.
> 
> Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> ---
>  .../uapi/linux/netfilter/nf_conntrack_sctp.h  |  4 +-
>  .../linux/netfilter/nfnetlink_cttimeout.h     |  4 +-
>  net/netfilter/nf_conntrack_proto_sctp.c       | 90 ++++++++-----------
>  net/netfilter/nf_conntrack_standalone.c       | 16 ----
>  4 files changed, 42 insertions(+), 72 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> index c742469afe21..150fc3c056ea 100644
> --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> @@ -15,8 +15,8 @@ enum sctp_conntrack {
>  	SCTP_CONNTRACK_SHUTDOWN_RECD,
>  	SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
>  	SCTP_CONNTRACK_HEARTBEAT_SENT,
> -	SCTP_CONNTRACK_HEARTBEAT_ACKED,
> -	SCTP_CONNTRACK_DATA_SENT,
> +	SCTP_CONNTRACK_HEARTBEAT_ACKED,	/* no longer used */
> +	SCTP_CONNTRACK_DATA_SENT,	/* no longer used */

_DATA_SENT was added in the previous development cycle, to my
knowledged it has been present in 6.1-rc only. Then I think you can
post a patch to revert this explaining why there is no need for
_DATA_SENT anymore. You can revert it before this patch (with my
suggestion, your series will contain with 4 patches).

One question of mine: Did you extract the new established timeout from
RFC, where this formula came from?

210 seconds = hb_interval * max_path_retrans + rto_max

And thanks, if this works for you, I prefer this incremental approach
by improving the existing SCTP tracker.

Re: [PATCH 3/3] netfilter: conntrack: unify established states for SCTP paths

[Pablo Neira Ayuso]

On Tue, Jan 17, 2023 at 12:54:40PM +0100, Pablo Neira Ayuso wrote:
> On Mon, Jan 16, 2023 at 10:35:56AM +0100, Sriram Yagnaraman wrote:
> > An SCTP endpoint can start an association through a path and tear it
> > down over another one. That means the initial path will not see the
> > shutdown sequence, and the conntrack entry will remain in ESTABLISHED
> > state for 5 days.
> > 
> > By merging the HEARTBEAT_ACKED and ESTABLISHED states into one
> > ESTABLISHED state, there remains no difference between a primary or
> > secondary path. The timeout for the merged ESTABLISHED state is set to
> > 210 seconds (hb_interval * max_path_retrans + rto_max). So, even if a
> > path doesn't see the shutdown sequence, it will expire in a reasonable
> > amount of time.
> > 
> > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > ---
> >  .../uapi/linux/netfilter/nf_conntrack_sctp.h  |  4 +-
> >  .../linux/netfilter/nfnetlink_cttimeout.h     |  4 +-
> >  net/netfilter/nf_conntrack_proto_sctp.c       | 90 ++++++++-----------
> >  net/netfilter/nf_conntrack_standalone.c       | 16 ----
> >  4 files changed, 42 insertions(+), 72 deletions(-)
> > 
> > diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > index c742469afe21..150fc3c056ea 100644
> > --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > @@ -15,8 +15,8 @@ enum sctp_conntrack {
> >  	SCTP_CONNTRACK_SHUTDOWN_RECD,
> >  	SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
> >  	SCTP_CONNTRACK_HEARTBEAT_SENT,
> > -	SCTP_CONNTRACK_HEARTBEAT_ACKED,
> > -	SCTP_CONNTRACK_DATA_SENT,
> > +	SCTP_CONNTRACK_HEARTBEAT_ACKED,	/* no longer used */
> > +	SCTP_CONNTRACK_DATA_SENT,	/* no longer used */
> 
> _DATA_SENT was added in the previous development cycle, to my
> knowledged it has been present in 6.1-rc only. Then I think you can

Actually, I mean 6.2-rc releases.

> post a patch to revert this explaining why there is no need for
> _DATA_SENT anymore. You can revert it before this patch (with my
> suggestion, your series will contain with 4 patches).
> 
> One question of mine: Did you extract the new established timeout from
> RFC, where this formula came from?
> 
> 210 seconds = hb_interval * max_path_retrans + rto_max
> 
> And thanks, if this works for you, I prefer this incremental approach
> by improving the existing SCTP tracker.

RE: [PATCH 3/3] netfilter: conntrack: unify established states for SCTP paths

[Sriram Yagnaraman]

> -----Original Message-----
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Sent: Tuesday, 17 January 2023 13:02
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> Cc: netfilter-devel@vger.kernel.org; Florian Westphal <fw@strlen.de>;
> Marcelo Ricardo Leitner <mleitner@redhat.com>; Long Xin
> <lxin@redhat.com>; Claudio Porfiri <claudio.porfiri@ericsson.com>
> Subject: Re: [PATCH 3/3] netfilter: conntrack: unify established states for SCTP
> paths
> 
> On Tue, Jan 17, 2023 at 12:54:40PM +0100, Pablo Neira Ayuso wrote:
> > On Mon, Jan 16, 2023 at 10:35:56AM +0100, Sriram Yagnaraman wrote:
> > > An SCTP endpoint can start an association through a path and tear it
> > > down over another one. That means the initial path will not see the
> > > shutdown sequence, and the conntrack entry will remain in
> > > ESTABLISHED state for 5 days.
> > >
> > > By merging the HEARTBEAT_ACKED and ESTABLISHED states into one
> > > ESTABLISHED state, there remains no difference between a primary or
> > > secondary path. The timeout for the merged ESTABLISHED state is set
> > > to
> > > 210 seconds (hb_interval * max_path_retrans + rto_max). So, even if
> > > a path doesn't see the shutdown sequence, it will expire in a
> > > reasonable amount of time.
> > >
> > > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> > > ---
> > >  .../uapi/linux/netfilter/nf_conntrack_sctp.h  |  4 +-
> > >  .../linux/netfilter/nfnetlink_cttimeout.h     |  4 +-
> > >  net/netfilter/nf_conntrack_proto_sctp.c       | 90 ++++++++-----------
> > >  net/netfilter/nf_conntrack_standalone.c       | 16 ----
> > >  4 files changed, 42 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > index c742469afe21..150fc3c056ea 100644
> > > --- a/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > +++ b/include/uapi/linux/netfilter/nf_conntrack_sctp.h
> > > @@ -15,8 +15,8 @@ enum sctp_conntrack {
> > >  	SCTP_CONNTRACK_SHUTDOWN_RECD,
> > >  	SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
> > >  	SCTP_CONNTRACK_HEARTBEAT_SENT,
> > > -	SCTP_CONNTRACK_HEARTBEAT_ACKED,
> > > -	SCTP_CONNTRACK_DATA_SENT,
> > > +	SCTP_CONNTRACK_HEARTBEAT_ACKED,	/* no longer used */
> > > +	SCTP_CONNTRACK_DATA_SENT,	/* no longer used */
> >
> > _DATA_SENT was added in the previous development cycle, to my
> > knowledged it has been present in 6.1-rc only. Then I think you can
> 
> Actually, I mean 6.2-rc releases.
> 
> > post a patch to revert this explaining why there is no need for
> > _DATA_SENT anymore. You can revert it before this patch (with my
> > suggestion, your series will contain with 4 patches).

I only removed the DATA_SENT state, SCTP tracker still reacts to DATA/SACK chunks to move to HEARTBEAT_SENT state. But I realize that reacting to DATA/SACK works only for new connections, while on connection re-use we still depend on HEARTBEAT for the secondary paths. Perhaps, I should revert the whole patch as you suggest.

> >
> > One question of mine: Did you extract the new established timeout from
> > RFC, where this formula came from?
> >
> > 210 seconds = hb_interval * max_path_retrans + rto_max

Took it from the HEARTBEAT_ACKED state timeout, explained in the patch that introduced the state: https://lore.kernel.org/all/20150714122311.8DA8EA0C9A@unicorn.suse.cz/
An SCTP endpoint will retry a path every "hb_interval" seconds for "max_path_retrans" times with a timeout of "rto_max", before treating it as unreachable. So, I am guessing that is the science behind the formula.

> >
> > And thanks, if this works for you, I prefer this incremental approach
> > by improving the existing SCTP tracker.

Thanks for taking the time to review. I will come back with more improvements once this series is approved :)