[PATCHv3 net-next 0/5] sctp: update from rfc7829

[PATCHv3 net-next 0/5] sctp: update from rfc7829

[Xin Long]

SCTP-PF was implemented based on a Internet-Draft in 2012:

  https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05

It's been updated quite a few by rfc7829 in 2016.

This patchset adds the following features:

  1. add SCTP_ADDR_POTENTIALLY_FAILED notification
  2. add pf_expose per netns/sock/asoc
  3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
  4. add ps_retrans per netns/sock/asoc/transport
     (Primary Path Switchover)
  5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt

v1->v2:
  - See Patch 2/5 and Patch 5/5.
v2->v3:
  - See Patch 1/5, 2/5 and 3/5.

Xin Long (5):
  sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification
  sctp: add pf_expose per netns and sock and asoc
  sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
  sctp: add support for Primary Path Switchover
  sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt

 include/net/netns/sctp.h     |  14 +++++
 include/net/sctp/constants.h |  10 +++
 include/net/sctp/structs.h   |  13 +++-
 include/uapi/linux/sctp.h    |  15 +++++
 net/sctp/associola.c         |  31 ++++-----
 net/sctp/protocol.c          |   6 ++
 net/sctp/sm_sideeffect.c     |   5 ++
 net/sctp/socket.c            | 147 ++++++++++++++++++++++++++++++++++++++-----
 net/sctp/sysctl.c            |  19 ++++++
 9 files changed, 226 insertions(+), 34 deletions(-)

-- 
2.1.0

[PATCHv3 net-next 0/5] sctp: update from rfc7829

[Xin Long]

SCTP-PF was implemented based on a Internet-Draft in 2012:

  https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05

It's been updated quite a few by rfc7829 in 2016.

This patchset adds the following features:

  1. add SCTP_ADDR_POTENTIALLY_FAILED notification
  2. add pf_expose per netns/sock/asoc
  3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
  4. add ps_retrans per netns/sock/asoc/transport
     (Primary Path Switchover)
  5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt

v1->v2:
  - See Patch 2/5 and Patch 5/5.
v2->v3:
  - See Patch 1/5, 2/5 and 3/5.

Xin Long (5):
  sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification
  sctp: add pf_expose per netns and sock and asoc
  sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
  sctp: add support for Primary Path Switchover
  sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt

 include/net/netns/sctp.h     |  14 +++++
 include/net/sctp/constants.h |  10 +++
 include/net/sctp/structs.h   |  13 +++-
 include/uapi/linux/sctp.h    |  15 +++++
 net/sctp/associola.c         |  31 ++++-----
 net/sctp/protocol.c          |   6 ++
 net/sctp/sm_sideeffect.c     |   5 ++
 net/sctp/socket.c            | 147 ++++++++++++++++++++++++++++++++++++++-----
 net/sctp/sysctl.c            |  19 ++++++
 9 files changed, 226 insertions(+), 34 deletions(-)

-- 
2.1.0

[PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

SCTP Quick failover draft section 5.1, point 5 has been removed
from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
Layer Protocol (ULP) about this state transition", as said in
section 3.2, point 8.

So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
in section 7.1, "which is reported if the affected address
becomes PF". Also remove transport cwnd's update when moving
from PF back to ACTIVE , which is no longer in rfc7829 either.

v1->v2:
  - no change
v2->v3:
  - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h |  2 ++
 net/sctp/associola.c      | 17 ++++-------------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index 6bce7f9..f4ab7bb 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -410,6 +410,8 @@ enum sctp_spc_state {
 	SCTP_ADDR_ADDED,
 	SCTP_ADDR_MADE_PRIM,
 	SCTP_ADDR_CONFIRMED,
+	SCTP_ADDR_POTENTIALLY_FAILED,
+#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
 };
 
 
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 1ba893b..4f9efba 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 			spc_state = SCTP_ADDR_CONFIRMED;
 		else
 			spc_state = SCTP_ADDR_AVAILABLE;
-		/* Don't inform ULP about transition from PF to
-		 * active state and set cwnd to 1 MTU, see SCTP
-		 * Quick failover draft section 5.1, point 5
-		 */
-		if (transport->state == SCTP_PF) {
-			ulp_notify = false;
-			transport->cwnd = asoc->pathmtu;
-		}
 		transport->state = SCTP_ACTIVE;
 		break;
 
@@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		 * to inactive state.  Also, release the cached route since
 		 * there may be a better route next time.
 		 */
-		if (transport->state != SCTP_UNCONFIRMED)
+		if (transport->state != SCTP_UNCONFIRMED) {
 			transport->state = SCTP_INACTIVE;
-		else {
+			spc_state = SCTP_ADDR_UNREACHABLE;
+		} else {
 			sctp_transport_dst_release(transport);
 			ulp_notify = false;
 		}
-
-		spc_state = SCTP_ADDR_UNREACHABLE;
 		break;
 
 	case SCTP_TRANSPORT_PF:
 		transport->state = SCTP_PF;
-		ulp_notify = false;
+		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
 		break;
 
 	default:
-- 
2.1.0

[PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

SCTP Quick failover draft section 5.1, point 5 has been removed
from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
Layer Protocol (ULP) about this state transition", as said in
section 3.2, point 8.

So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
in section 7.1, "which is reported if the affected address
becomes PF". Also remove transport cwnd's update when moving
from PF back to ACTIVE , which is no longer in rfc7829 either.

v1->v2:
  - no change
v2->v3:
  - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h |  2 ++
 net/sctp/associola.c      | 17 ++++-------------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index 6bce7f9..f4ab7bb 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -410,6 +410,8 @@ enum sctp_spc_state {
 	SCTP_ADDR_ADDED,
 	SCTP_ADDR_MADE_PRIM,
 	SCTP_ADDR_CONFIRMED,
+	SCTP_ADDR_POTENTIALLY_FAILED,
+#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
 };
 
 
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 1ba893b..4f9efba 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 			spc_state = SCTP_ADDR_CONFIRMED;
 		else
 			spc_state = SCTP_ADDR_AVAILABLE;
-		/* Don't inform ULP about transition from PF to
-		 * active state and set cwnd to 1 MTU, see SCTP
-		 * Quick failover draft section 5.1, point 5
-		 */
-		if (transport->state = SCTP_PF) {
-			ulp_notify = false;
-			transport->cwnd = asoc->pathmtu;
-		}
 		transport->state = SCTP_ACTIVE;
 		break;
 
@@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		 * to inactive state.  Also, release the cached route since
 		 * there may be a better route next time.
 		 */
-		if (transport->state != SCTP_UNCONFIRMED)
+		if (transport->state != SCTP_UNCONFIRMED) {
 			transport->state = SCTP_INACTIVE;
-		else {
+			spc_state = SCTP_ADDR_UNREACHABLE;
+		} else {
 			sctp_transport_dst_release(transport);
 			ulp_notify = false;
 		}
-
-		spc_state = SCTP_ADDR_UNREACHABLE;
 		break;
 
 	case SCTP_TRANSPORT_PF:
 		transport->state = SCTP_PF;
-		ulp_notify = false;
+		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
 		break;
 
 	default:
-- 
2.1.0

[PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Xin Long]

As said in rfc7829, section 3, point 12:

  The SCTP stack SHOULD expose the PF state of its destination
  addresses to the ULP as well as provide the means to notify the
  ULP of state transitions of its destination addresses from
  active to PF, and vice versa.  However, it is recommended that
  an SCTP stack implementing SCTP-PF also allows for the ULP to be
  kept ignorant of the PF state of its destinations and the
  associated state transitions, thus allowing for retention of the
  simpler state transition model of [RFC4960] in the ULP.

Not only does it allow to expose the PF state to ULP, but also
allow to ignore sctp-pf to ULP.

So this patch is to add pf_expose per netns, sock and asoc. And in
sctp_assoc_control_transport(), ulp_notify will be set to false if
asoc->expose is not set.

It also allows a user to change pf_expose per netns by sysctl, and
pf_expose per sock and asoc will be initialized with it.

Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
to not allow a user to query the state of a sctp-pf peer address
when pf_expose is not enabled, as said in section 7.3.

v1->v2:
  - Fix a build warning noticed by Nathan Chancellor.
v2->v3:
  - set pf_expose to UNUSED by default to keep compatible with old
    applications.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/netns/sctp.h     |  8 ++++++++
 include/net/sctp/constants.h | 10 ++++++++++
 include/net/sctp/structs.h   |  2 ++
 include/uapi/linux/sctp.h    |  1 +
 net/sctp/associola.c         | 13 ++++++++++---
 net/sctp/protocol.c          |  3 +++
 net/sctp/socket.c            | 13 +++++++++++--
 net/sctp/sysctl.c            | 10 ++++++++++
 8 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
index bdc0f27..f97d342 100644
--- a/include/net/netns/sctp.h
+++ b/include/net/netns/sctp.h
@@ -97,6 +97,14 @@ struct netns_sctp {
 	int pf_enable;
 
 	/*
+	 * Disable Potentially-Failed state exposure, ignored by default
+	 * pf_expose	-  0  : disable pf state exposure
+	 *		-  1  : enable  pf state exposure
+	 *		-  2  : compatible with old applications (by default)
+	 */
+	int pf_expose;
+
+	/*
 	 * Policy for preforming sctp/socket accounting
 	 * 0   - do socket level accounting, all assocs share sk_sndbuf
 	 * 1   - do sctp accounting, each asoc may use sk_sndbuf bytes
diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
index 823afc4..2818988 100644
--- a/include/net/sctp/constants.h
+++ b/include/net/sctp/constants.h
@@ -286,6 +286,16 @@ enum { SCTP_MAX_GABS = 16 };
 				 * functions simpler to write.
 				 */
 
+/* These are the values for pf exposure, UNUSED is to keep compatible with old
+ * applications by default.
+ */
+enum {
+	SCTP_PF_EXPOSE_DISABLE,
+	SCTP_PF_EXPOSE_ENABLE,
+	SCTP_PF_EXPOSE_UNUSED,
+};
+#define SCTP_PF_EXPOSE_MAX	SCTP_PF_EXPOSE_UNUSED
+
 /* These return values describe the success or failure of a number of
  * routines which form the lower interface to SCTP_outqueue.
  */
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 503fbc3..9a43738 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -215,6 +215,7 @@ struct sctp_sock {
 	__u32 adaptation_ind;
 	__u32 pd_point;
 	__u16	nodelay:1,
+		pf_expose:2,
 		reuse:1,
 		disable_fragments:1,
 		v4mapped:1,
@@ -2053,6 +2054,7 @@ struct sctp_association {
 
 	__u8 need_ecne:1,	/* Need to send an ECNE Chunk? */
 	     temp:1,		/* Is it a temporary association? */
+	     pf_expose:2,       /* Expose pf state? */
 	     force_delay:1;
 
 	__u8 strreset_enable;
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index f4ab7bb..d99b428 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -935,6 +935,7 @@ struct sctp_paddrinfo {
 enum sctp_spinfo_state {
 	SCTP_INACTIVE,
 	SCTP_PF,
+#define	SCTP_POTENTIALLY_FAILED		SCTP_PF
 	SCTP_ACTIVE,
 	SCTP_UNCONFIRMED,
 	SCTP_UNKNOWN = 0xffff  /* Value used for transport state unknown */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 4f9efba..46763c5 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
 	 */
 	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
 	asoc->pf_retrans  = sp->pf_retrans;
+	asoc->pf_expose   = sp->pf_expose;
 
 	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
 	asoc->rto_max = msecs_to_jiffies(sp->rtoinfo.srto_max);
@@ -796,8 +797,11 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		 * to heartbeat success, report the SCTP_ADDR_CONFIRMED
 		 * state to the user, otherwise report SCTP_ADDR_AVAILABLE.
 		 */
-		if (SCTP_UNCONFIRMED == transport->state &&
-		    SCTP_HEARTBEAT_SUCCESS == error)
+		if (transport->state == SCTP_PF &&
+		    asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+			ulp_notify = false;
+		else if (transport->state == SCTP_UNCONFIRMED &&
+			 error == SCTP_HEARTBEAT_SUCCESS)
 			spc_state = SCTP_ADDR_CONFIRMED;
 		else
 			spc_state = SCTP_ADDR_AVAILABLE;
@@ -820,7 +824,10 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 
 	case SCTP_TRANSPORT_PF:
 		transport->state = SCTP_PF;
-		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
+		if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+			ulp_notify = false;
+		else
+			spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
 		break;
 
 	default:
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 08d14d8..a18c7c4 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1220,6 +1220,9 @@ static int __net_init sctp_defaults_init(struct net *net)
 	/* Enable pf state by default */
 	net->sctp.pf_enable = 1;
 
+	/* Ignore pf exposure feature by default */
+	net->sctp.pf_expose = SCTP_PF_EXPOSE_UNUSED;
+
 	/* Association.Max.Retrans  - 10 attempts
 	 * Path.Max.Retrans         - 5  attempts (per destination address)
 	 * Max.Init.Retransmits     - 8  attempts
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 939b8d2..669e02e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5041,6 +5041,7 @@ static int sctp_init_sock(struct sock *sk)
 	sp->hbinterval  = net->sctp.hb_interval;
 	sp->pathmaxrxt  = net->sctp.max_retrans_path;
 	sp->pf_retrans  = net->sctp.pf_retrans;
+	sp->pf_expose   = net->sctp.pf_expose;
 	sp->pathmtu     = 0; /* allow default discovery */
 	sp->sackdelay   = net->sctp.sack_timeout;
 	sp->sackfreq	= 2;
@@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
 
 	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
 					   pinfo.spinfo_assoc_id);
-	if (!transport)
-		return -EINVAL;
+	if (!transport) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	if (transport->state == SCTP_PF &&
+	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
+		retval = -EACCES;
+		goto out;
+	}
 
 	pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
 	pinfo.spinfo_state = transport->state;
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 238cf17..5d1ad44 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
 static int rto_beta_min = 0;
 static int rto_alpha_max = 1000;
 static int rto_beta_max = 1000;
+static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
 
 static unsigned long max_autoclose_min = 0;
 static unsigned long max_autoclose_max =
@@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "pf_expose",
+		.data		= &init_net.sctp.pf_expose,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= &pf_expose_max,
+	},
 
 	{ /* sentinel */ }
 };
-- 
2.1.0

[PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Xin Long]

As said in rfc7829, section 3, point 12:

  The SCTP stack SHOULD expose the PF state of its destination
  addresses to the ULP as well as provide the means to notify the
  ULP of state transitions of its destination addresses from
  active to PF, and vice versa.  However, it is recommended that
  an SCTP stack implementing SCTP-PF also allows for the ULP to be
  kept ignorant of the PF state of its destinations and the
  associated state transitions, thus allowing for retention of the
  simpler state transition model of [RFC4960] in the ULP.

Not only does it allow to expose the PF state to ULP, but also
allow to ignore sctp-pf to ULP.

So this patch is to add pf_expose per netns, sock and asoc. And in
sctp_assoc_control_transport(), ulp_notify will be set to false if
asoc->expose is not set.

It also allows a user to change pf_expose per netns by sysctl, and
pf_expose per sock and asoc will be initialized with it.

Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
to not allow a user to query the state of a sctp-pf peer address
when pf_expose is not enabled, as said in section 7.3.

v1->v2:
  - Fix a build warning noticed by Nathan Chancellor.
v2->v3:
  - set pf_expose to UNUSED by default to keep compatible with old
    applications.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/netns/sctp.h     |  8 ++++++++
 include/net/sctp/constants.h | 10 ++++++++++
 include/net/sctp/structs.h   |  2 ++
 include/uapi/linux/sctp.h    |  1 +
 net/sctp/associola.c         | 13 ++++++++++---
 net/sctp/protocol.c          |  3 +++
 net/sctp/socket.c            | 13 +++++++++++--
 net/sctp/sysctl.c            | 10 ++++++++++
 8 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
index bdc0f27..f97d342 100644
--- a/include/net/netns/sctp.h
+++ b/include/net/netns/sctp.h
@@ -97,6 +97,14 @@ struct netns_sctp {
 	int pf_enable;
 
 	/*
+	 * Disable Potentially-Failed state exposure, ignored by default
+	 * pf_expose	-  0  : disable pf state exposure
+	 *		-  1  : enable  pf state exposure
+	 *		-  2  : compatible with old applications (by default)
+	 */
+	int pf_expose;
+
+	/*
 	 * Policy for preforming sctp/socket accounting
 	 * 0   - do socket level accounting, all assocs share sk_sndbuf
 	 * 1   - do sctp accounting, each asoc may use sk_sndbuf bytes
diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
index 823afc4..2818988 100644
--- a/include/net/sctp/constants.h
+++ b/include/net/sctp/constants.h
@@ -286,6 +286,16 @@ enum { SCTP_MAX_GABS = 16 };
 				 * functions simpler to write.
 				 */
 
+/* These are the values for pf exposure, UNUSED is to keep compatible with old
+ * applications by default.
+ */
+enum {
+	SCTP_PF_EXPOSE_DISABLE,
+	SCTP_PF_EXPOSE_ENABLE,
+	SCTP_PF_EXPOSE_UNUSED,
+};
+#define SCTP_PF_EXPOSE_MAX	SCTP_PF_EXPOSE_UNUSED
+
 /* These return values describe the success or failure of a number of
  * routines which form the lower interface to SCTP_outqueue.
  */
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 503fbc3..9a43738 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -215,6 +215,7 @@ struct sctp_sock {
 	__u32 adaptation_ind;
 	__u32 pd_point;
 	__u16	nodelay:1,
+		pf_expose:2,
 		reuse:1,
 		disable_fragments:1,
 		v4mapped:1,
@@ -2053,6 +2054,7 @@ struct sctp_association {
 
 	__u8 need_ecne:1,	/* Need to send an ECNE Chunk? */
 	     temp:1,		/* Is it a temporary association? */
+	     pf_expose:2,       /* Expose pf state? */
 	     force_delay:1;
 
 	__u8 strreset_enable;
diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index f4ab7bb..d99b428 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -935,6 +935,7 @@ struct sctp_paddrinfo {
 enum sctp_spinfo_state {
 	SCTP_INACTIVE,
 	SCTP_PF,
+#define	SCTP_POTENTIALLY_FAILED		SCTP_PF
 	SCTP_ACTIVE,
 	SCTP_UNCONFIRMED,
 	SCTP_UNKNOWN = 0xffff  /* Value used for transport state unknown */
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 4f9efba..46763c5 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
 	 */
 	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
 	asoc->pf_retrans  = sp->pf_retrans;
+	asoc->pf_expose   = sp->pf_expose;
 
 	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
 	asoc->rto_max = msecs_to_jiffies(sp->rtoinfo.srto_max);
@@ -796,8 +797,11 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		 * to heartbeat success, report the SCTP_ADDR_CONFIRMED
 		 * state to the user, otherwise report SCTP_ADDR_AVAILABLE.
 		 */
-		if (SCTP_UNCONFIRMED = transport->state &&
-		    SCTP_HEARTBEAT_SUCCESS = error)
+		if (transport->state = SCTP_PF &&
+		    asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+			ulp_notify = false;
+		else if (transport->state = SCTP_UNCONFIRMED &&
+			 error = SCTP_HEARTBEAT_SUCCESS)
 			spc_state = SCTP_ADDR_CONFIRMED;
 		else
 			spc_state = SCTP_ADDR_AVAILABLE;
@@ -820,7 +824,10 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 
 	case SCTP_TRANSPORT_PF:
 		transport->state = SCTP_PF;
-		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
+		if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+			ulp_notify = false;
+		else
+			spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
 		break;
 
 	default:
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 08d14d8..a18c7c4 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1220,6 +1220,9 @@ static int __net_init sctp_defaults_init(struct net *net)
 	/* Enable pf state by default */
 	net->sctp.pf_enable = 1;
 
+	/* Ignore pf exposure feature by default */
+	net->sctp.pf_expose = SCTP_PF_EXPOSE_UNUSED;
+
 	/* Association.Max.Retrans  - 10 attempts
 	 * Path.Max.Retrans         - 5  attempts (per destination address)
 	 * Max.Init.Retransmits     - 8  attempts
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 939b8d2..669e02e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5041,6 +5041,7 @@ static int sctp_init_sock(struct sock *sk)
 	sp->hbinterval  = net->sctp.hb_interval;
 	sp->pathmaxrxt  = net->sctp.max_retrans_path;
 	sp->pf_retrans  = net->sctp.pf_retrans;
+	sp->pf_expose   = net->sctp.pf_expose;
 	sp->pathmtu     = 0; /* allow default discovery */
 	sp->sackdelay   = net->sctp.sack_timeout;
 	sp->sackfreq	= 2;
@@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
 
 	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
 					   pinfo.spinfo_assoc_id);
-	if (!transport)
-		return -EINVAL;
+	if (!transport) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	if (transport->state = SCTP_PF &&
+	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
+		retval = -EACCES;
+		goto out;
+	}
 
 	pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
 	pinfo.spinfo_state = transport->state;
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 238cf17..5d1ad44 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
 static int rto_beta_min = 0;
 static int rto_alpha_max = 1000;
 static int rto_beta_max = 1000;
+static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
 
 static unsigned long max_autoclose_min = 0;
 static unsigned long max_autoclose_max @@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+	{
+		.procname	= "pf_expose",
+		.data		= &init_net.sctp.pf_expose,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= &pf_expose_max,
+	},
 
 	{ /* sentinel */ }
 };
-- 
2.1.0

[PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Xin Long]

This is a sockopt defined in section 7.3 of rfc7829: "Exposing
the Potentially Failed Path State", by which users can change
pf_expose per sock and asoc.

v1->v2:
  - no change.
v2->v3:
  - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
  - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h |  2 ++
 net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index d99b428..a190e4a 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_ASCONF_SUPPORTED	128
 #define SCTP_AUTH_SUPPORTED	129
 #define SCTP_ECN_SUPPORTED	130
+#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE	131
+#define SCTP_EXPOSE_PF_STATE	SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 669e02e..eccd689 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4589,6 +4589,40 @@ static int sctp_setsockopt_ecn_supported(struct sock *sk,
 	return retval;
 }
 
+static int sctp_setsockopt_pf_expose(struct sock *sk,
+				     char __user *optval,
+				     unsigned int optlen)
+{
+	struct sctp_assoc_value params;
+	struct sctp_association *asoc;
+	int retval = -EINVAL;
+
+	if (optlen != sizeof(params))
+		goto out;
+
+	if (copy_from_user(&params, optval, optlen)) {
+		retval = -EFAULT;
+		goto out;
+	}
+
+	if (params.assoc_value > SCTP_PF_EXPOSE_MAX)
+		goto out;
+
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id != SCTP_FUTURE_ASSOC &&
+	    sctp_style(sk, UDP))
+		goto out;
+
+	if (asoc)
+		asoc->pf_expose = params.assoc_value;
+	else
+		sctp_sk(sk)->pf_expose = params.assoc_value;
+	retval = 0;
+
+out:
+	return retval;
+}
+
 /* API 6.2 setsockopt(), getsockopt()
  *
  * Applications use setsockopt() and getsockopt() to set or retrieve
@@ -4798,6 +4832,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 	case SCTP_ECN_SUPPORTED:
 		retval = sctp_setsockopt_ecn_supported(sk, optval, optlen);
 		break;
+	case SCTP_EXPOSE_POTENTIALLY_FAILED_STATE:
+		retval = sctp_setsockopt_pf_expose(sk, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
@@ -7909,6 +7946,45 @@ static int sctp_getsockopt_ecn_supported(struct sock *sk, int len,
 	return retval;
 }
 
+static int sctp_getsockopt_pf_expose(struct sock *sk, int len,
+				     char __user *optval,
+				     int __user *optlen)
+{
+	struct sctp_assoc_value params;
+	struct sctp_association *asoc;
+	int retval = -EFAULT;
+
+	if (len < sizeof(params)) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	len = sizeof(params);
+	if (copy_from_user(&params, optval, len))
+		goto out;
+
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id != SCTP_FUTURE_ASSOC &&
+	    sctp_style(sk, UDP)) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	params.assoc_value = asoc ? asoc->pf_expose
+				  : sctp_sk(sk)->pf_expose;
+
+	if (put_user(len, optlen))
+		goto out;
+
+	if (copy_to_user(optval, &params, len))
+		goto out;
+
+	retval = 0;
+
+out:
+	return retval;
+}
+
 static int sctp_getsockopt(struct sock *sk, int level, int optname,
 			   char __user *optval, int __user *optlen)
 {
@@ -8121,6 +8197,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 	case SCTP_ECN_SUPPORTED:
 		retval = sctp_getsockopt_ecn_supported(sk, len, optval, optlen);
 		break;
+	case SCTP_EXPOSE_POTENTIALLY_FAILED_STATE:
+		retval = sctp_getsockopt_pf_expose(sk, len, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
-- 
2.1.0

[PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Xin Long]

This is a sockopt defined in section 7.3 of rfc7829: "Exposing
the Potentially Failed Path State", by which users can change
pf_expose per sock and asoc.

v1->v2:
  - no change.
v2->v3:
  - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
  - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h |  2 ++
 net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index d99b428..a190e4a 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_ASCONF_SUPPORTED	128
 #define SCTP_AUTH_SUPPORTED	129
 #define SCTP_ECN_SUPPORTED	130
+#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE	131
+#define SCTP_EXPOSE_PF_STATE	SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
 
 /* PR-SCTP policies */
 #define SCTP_PR_SCTP_NONE	0x0000
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 669e02e..eccd689 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4589,6 +4589,40 @@ static int sctp_setsockopt_ecn_supported(struct sock *sk,
 	return retval;
 }
 
+static int sctp_setsockopt_pf_expose(struct sock *sk,
+				     char __user *optval,
+				     unsigned int optlen)
+{
+	struct sctp_assoc_value params;
+	struct sctp_association *asoc;
+	int retval = -EINVAL;
+
+	if (optlen != sizeof(params))
+		goto out;
+
+	if (copy_from_user(&params, optval, optlen)) {
+		retval = -EFAULT;
+		goto out;
+	}
+
+	if (params.assoc_value > SCTP_PF_EXPOSE_MAX)
+		goto out;
+
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id != SCTP_FUTURE_ASSOC &&
+	    sctp_style(sk, UDP))
+		goto out;
+
+	if (asoc)
+		asoc->pf_expose = params.assoc_value;
+	else
+		sctp_sk(sk)->pf_expose = params.assoc_value;
+	retval = 0;
+
+out:
+	return retval;
+}
+
 /* API 6.2 setsockopt(), getsockopt()
  *
  * Applications use setsockopt() and getsockopt() to set or retrieve
@@ -4798,6 +4832,9 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 	case SCTP_ECN_SUPPORTED:
 		retval = sctp_setsockopt_ecn_supported(sk, optval, optlen);
 		break;
+	case SCTP_EXPOSE_POTENTIALLY_FAILED_STATE:
+		retval = sctp_setsockopt_pf_expose(sk, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
@@ -7909,6 +7946,45 @@ static int sctp_getsockopt_ecn_supported(struct sock *sk, int len,
 	return retval;
 }
 
+static int sctp_getsockopt_pf_expose(struct sock *sk, int len,
+				     char __user *optval,
+				     int __user *optlen)
+{
+	struct sctp_assoc_value params;
+	struct sctp_association *asoc;
+	int retval = -EFAULT;
+
+	if (len < sizeof(params)) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	len = sizeof(params);
+	if (copy_from_user(&params, optval, len))
+		goto out;
+
+	asoc = sctp_id2assoc(sk, params.assoc_id);
+	if (!asoc && params.assoc_id != SCTP_FUTURE_ASSOC &&
+	    sctp_style(sk, UDP)) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	params.assoc_value = asoc ? asoc->pf_expose
+				  : sctp_sk(sk)->pf_expose;
+
+	if (put_user(len, optlen))
+		goto out;
+
+	if (copy_to_user(optval, &params, len))
+		goto out;
+
+	retval = 0;
+
+out:
+	return retval;
+}
+
 static int sctp_getsockopt(struct sock *sk, int level, int optname,
 			   char __user *optval, int __user *optlen)
 {
@@ -8121,6 +8197,9 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 	case SCTP_ECN_SUPPORTED:
 		retval = sctp_getsockopt_ecn_supported(sk, len, optval, optlen);
 		break;
+	case SCTP_EXPOSE_POTENTIALLY_FAILED_STATE:
+		retval = sctp_getsockopt_pf_expose(sk, len, optval, optlen);
+		break;
 	default:
 		retval = -ENOPROTOOPT;
 		break;
-- 
2.1.0

[PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Xin Long]

This is a new feature defined in section 5 of rfc7829: "Primary Path
Switchover". By introducing a new tunable parameter:

  Primary.Switchover.Max.Retrans (PSMR)

The primary path will be changed to another active path when the path
error counter on the old primary path exceeds PSMR, so that "the SCTP
sender is allowed to continue data transmission on a new working path
even when the old primary destination address becomes active again".

This patch is to add this tunable parameter, 'ps_retrans' per netns,
sock, asoc and transport. It also allows a user to change ps_retrans
per netns by sysctl, and ps_retrans per sock/asoc/transport will be
initialized with it.

The check will be done in sctp_do_8_2_transport_strike() when this
feature is enabled.

Note this feature is disabled by initializing 'ps_retrans' per netns
as 0xffff by default, and its value can't be less than 'pf_retrans'
when changing by sysctl.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/netns/sctp.h   |  6 ++++++
 include/net/sctp/structs.h | 11 ++++++++---
 net/sctp/associola.c       |  3 +++
 net/sctp/protocol.c        |  3 +++
 net/sctp/sm_sideeffect.c   |  5 +++++
 net/sctp/socket.c          |  1 +
 net/sctp/sysctl.c          |  9 +++++++++
 7 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
index f97d342..c41ffdf 100644
--- a/include/net/netns/sctp.h
+++ b/include/net/netns/sctp.h
@@ -89,6 +89,12 @@ struct netns_sctp {
 	 */
 	int pf_retrans;
 
+	/* Primary.Switchover.Max.Retrans sysctl value
+	 * taken from:
+	 * https://tools.ietf.org/html/rfc7829
+	 */
+	int ps_retrans;
+
 	/*
 	 * Disable Potentially-Failed feature, the feature is enabled by default
 	 * pf_enable	-  0  : disable pf
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9a43738..3cc913f 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -184,7 +184,8 @@ struct sctp_sock {
 	__u32 flowlabel;
 	__u8  dscp;
 
-	int pf_retrans;
+	__u16 pf_retrans;
+	__u16 ps_retrans;
 
 	/* The initial Path MTU to use for new associations. */
 	__u32 pathmtu;
@@ -897,7 +898,9 @@ struct sctp_transport {
 	 * and will be initialized from the assocs value.  This can be changed
 	 * using the SCTP_PEER_ADDR_THLDS socket option
 	 */
-	int pf_retrans;
+	__u16 pf_retrans;
+	/* Used for primary path switchover. */
+	__u16 ps_retrans;
 	/* PMTU	      : The current known path MTU.  */
 	__u32 pathmtu;
 
@@ -1773,7 +1776,9 @@ struct sctp_association {
 	 * and will be initialized from the assocs value.  This can be
 	 * changed using the SCTP_PEER_ADDR_THLDS socket option
 	 */
-	int pf_retrans;
+	__u16 pf_retrans;
+	/* Used for primary path switchover. */
+	__u16 ps_retrans;
 
 	/* Maximum number of times the endpoint will retransmit INIT  */
 	__u16 max_init_attempts;
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 46763c5..a839244 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
 	 */
 	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
 	asoc->pf_retrans  = sp->pf_retrans;
+	asoc->ps_retrans  = sp->ps_retrans;
 	asoc->pf_expose   = sp->pf_expose;
 
 	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
@@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
 
 	/* And the partial failure retrans threshold */
 	peer->pf_retrans = asoc->pf_retrans;
+	/* And the primary path switchover retrans threshold */
+	peer->ps_retrans = asoc->ps_retrans;
 
 	/* Initialize the peer's SACK delay timeout based on the
 	 * association configured value.
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index a18c7c4..ea1de9b 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
 	/* Max.Burst		    - 4 */
 	net->sctp.max_burst			= SCTP_DEFAULT_MAX_BURST;
 
+	/* Disable of Primary Path Switchover by default */
+	net->sctp.ps_retrans = 0xffff;
+
 	/* Enable pf state by default */
 	net->sctp.pf_enable = 1;
 
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index e52b212..acd737d 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
 					     SCTP_FAILED_THRESHOLD);
 	}
 
+	if (transport->error_count > transport->ps_retrans &&
+	    asoc->peer.primary_path == transport &&
+	    asoc->peer.active_path != transport)
+		sctp_assoc_set_primary(asoc, asoc->peer.active_path);
+
 	/* E2) For the destination address for which the timer
 	 * expires, set RTO <- RTO * 2 ("back off the timer").  The
 	 * maximum value discussed in rule C7 above (RTO.max) may be
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index eccd689..38d102b 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
 	sp->hbinterval  = net->sctp.hb_interval;
 	sp->pathmaxrxt  = net->sctp.max_retrans_path;
 	sp->pf_retrans  = net->sctp.pf_retrans;
+	sp->ps_retrans  = net->sctp.ps_retrans;
 	sp->pf_expose   = net->sctp.pf_expose;
 	sp->pathmtu     = 0; /* allow default discovery */
 	sp->sackdelay   = net->sctp.sack_timeout;
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 5d1ad44..adf264a 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
+		.extra2		= &init_net.sctp.ps_retrans,
+	},
+	{
+		.procname	= "ps_retrans",
+		.data		= &init_net.sctp.ps_retrans,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &init_net.sctp.pf_retrans,
 		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
-- 
2.1.0

[PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Xin Long]

This is a new feature defined in section 5 of rfc7829: "Primary Path
Switchover". By introducing a new tunable parameter:

  Primary.Switchover.Max.Retrans (PSMR)

The primary path will be changed to another active path when the path
error counter on the old primary path exceeds PSMR, so that "the SCTP
sender is allowed to continue data transmission on a new working path
even when the old primary destination address becomes active again".

This patch is to add this tunable parameter, 'ps_retrans' per netns,
sock, asoc and transport. It also allows a user to change ps_retrans
per netns by sysctl, and ps_retrans per sock/asoc/transport will be
initialized with it.

The check will be done in sctp_do_8_2_transport_strike() when this
feature is enabled.

Note this feature is disabled by initializing 'ps_retrans' per netns
as 0xffff by default, and its value can't be less than 'pf_retrans'
when changing by sysctl.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/netns/sctp.h   |  6 ++++++
 include/net/sctp/structs.h | 11 ++++++++---
 net/sctp/associola.c       |  3 +++
 net/sctp/protocol.c        |  3 +++
 net/sctp/sm_sideeffect.c   |  5 +++++
 net/sctp/socket.c          |  1 +
 net/sctp/sysctl.c          |  9 +++++++++
 7 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
index f97d342..c41ffdf 100644
--- a/include/net/netns/sctp.h
+++ b/include/net/netns/sctp.h
@@ -89,6 +89,12 @@ struct netns_sctp {
 	 */
 	int pf_retrans;
 
+	/* Primary.Switchover.Max.Retrans sysctl value
+	 * taken from:
+	 * https://tools.ietf.org/html/rfc7829
+	 */
+	int ps_retrans;
+
 	/*
 	 * Disable Potentially-Failed feature, the feature is enabled by default
 	 * pf_enable	-  0  : disable pf
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 9a43738..3cc913f 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -184,7 +184,8 @@ struct sctp_sock {
 	__u32 flowlabel;
 	__u8  dscp;
 
-	int pf_retrans;
+	__u16 pf_retrans;
+	__u16 ps_retrans;
 
 	/* The initial Path MTU to use for new associations. */
 	__u32 pathmtu;
@@ -897,7 +898,9 @@ struct sctp_transport {
 	 * and will be initialized from the assocs value.  This can be changed
 	 * using the SCTP_PEER_ADDR_THLDS socket option
 	 */
-	int pf_retrans;
+	__u16 pf_retrans;
+	/* Used for primary path switchover. */
+	__u16 ps_retrans;
 	/* PMTU	      : The current known path MTU.  */
 	__u32 pathmtu;
 
@@ -1773,7 +1776,9 @@ struct sctp_association {
 	 * and will be initialized from the assocs value.  This can be
 	 * changed using the SCTP_PEER_ADDR_THLDS socket option
 	 */
-	int pf_retrans;
+	__u16 pf_retrans;
+	/* Used for primary path switchover. */
+	__u16 ps_retrans;
 
 	/* Maximum number of times the endpoint will retransmit INIT  */
 	__u16 max_init_attempts;
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 46763c5..a839244 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
 	 */
 	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
 	asoc->pf_retrans  = sp->pf_retrans;
+	asoc->ps_retrans  = sp->ps_retrans;
 	asoc->pf_expose   = sp->pf_expose;
 
 	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
@@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
 
 	/* And the partial failure retrans threshold */
 	peer->pf_retrans = asoc->pf_retrans;
+	/* And the primary path switchover retrans threshold */
+	peer->ps_retrans = asoc->ps_retrans;
 
 	/* Initialize the peer's SACK delay timeout based on the
 	 * association configured value.
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index a18c7c4..ea1de9b 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
 	/* Max.Burst		    - 4 */
 	net->sctp.max_burst			= SCTP_DEFAULT_MAX_BURST;
 
+	/* Disable of Primary Path Switchover by default */
+	net->sctp.ps_retrans = 0xffff;
+
 	/* Enable pf state by default */
 	net->sctp.pf_enable = 1;
 
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index e52b212..acd737d 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
 					     SCTP_FAILED_THRESHOLD);
 	}
 
+	if (transport->error_count > transport->ps_retrans &&
+	    asoc->peer.primary_path = transport &&
+	    asoc->peer.active_path != transport)
+		sctp_assoc_set_primary(asoc, asoc->peer.active_path);
+
 	/* E2) For the destination address for which the timer
 	 * expires, set RTO <- RTO * 2 ("back off the timer").  The
 	 * maximum value discussed in rule C7 above (RTO.max) may be
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index eccd689..38d102b 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
 	sp->hbinterval  = net->sctp.hb_interval;
 	sp->pathmaxrxt  = net->sctp.max_retrans_path;
 	sp->pf_retrans  = net->sctp.pf_retrans;
+	sp->ps_retrans  = net->sctp.ps_retrans;
 	sp->pf_expose   = net->sctp.pf_expose;
 	sp->pathmtu     = 0; /* allow default discovery */
 	sp->sackdelay   = net->sctp.sack_timeout;
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 5d1ad44..adf264a 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= SYSCTL_ZERO,
+		.extra2		= &init_net.sctp.ps_retrans,
+	},
+	{
+		.procname	= "ps_retrans",
+		.data		= &init_net.sctp.ps_retrans,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &init_net.sctp.pf_retrans,
 		.extra2		= SYSCTL_INT_MAX,
 	},
 	{
-- 
2.1.0

[PATCHv3 net-next 5/5] sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt

[Xin Long]

Section 7.2 of rfc7829: "Peer Address Thresholds (SCTP_PEER_ADDR_THLDS)
Socket Option" extends 'struct sctp_paddrthlds' with 'spt_pathcpthld'
added to allow a user to change ps_retrans per sock/asoc/transport, as
other 2 paddrthlds: pf_retrans, pathmaxrxt.

Note: to not break the user's program, here to support pf_retrans dump
and setting by adding a new sockopt SCTP_PEER_ADDR_THLDS_V2, and a new
structure sctp_paddrthlds_v2 instead of extending sctp_paddrthlds.

Also, when setting ps_retrans, the value is not allowed to be greater
than pf_retrans.

v1->v2:
  - use SCTP_PEER_ADDR_THLDS_V2 to set/get pf_retrans instead,
    as Marcelo and David Laight suggested.
v2->v3:
  - no change.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h | 10 +++++++++
 net/sctp/socket.c         | 54 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index a190e4a..28ad40d 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -105,6 +105,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_DEFAULT_SNDINFO	34
 #define SCTP_AUTH_DEACTIVATE_KEY	35
 #define SCTP_REUSE_PORT		36
+#define SCTP_PEER_ADDR_THLDS_V2	37
 
 /* Internal Socket Options. Some of the sctp library functions are
  * implemented using these socket options.
@@ -1087,6 +1088,15 @@ struct sctp_paddrthlds {
 	__u16 spt_pathpfthld;
 };
 
+/* Use a new structure with spt_pathcpthld for back compatibility */
+struct sctp_paddrthlds_v2 {
+	sctp_assoc_t spt_assoc_id;
+	struct sockaddr_storage spt_address;
+	__u16 spt_pathmaxrxt;
+	__u16 spt_pathpfthld;
+	__u16 spt_pathcpthld;
+};
+
 /*
  * Socket Option for Getting the Association/Stream-Specific PR-SCTP Status
  */
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 38d102b..3d2bad2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3943,18 +3943,22 @@ static int sctp_setsockopt_auto_asconf(struct sock *sk, char __user *optval,
  */
 static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 					    char __user *optval,
-					    unsigned int optlen)
+					    unsigned int optlen, bool v2)
 {
-	struct sctp_paddrthlds val;
+	struct sctp_paddrthlds_v2 val;
 	struct sctp_transport *trans;
 	struct sctp_association *asoc;
+	int len;
 
-	if (optlen < sizeof(struct sctp_paddrthlds))
+	len = v2 ? sizeof(val) : sizeof(struct sctp_paddrthlds);
+	if (optlen < len)
 		return -EINVAL;
-	if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval,
-			   sizeof(struct sctp_paddrthlds)))
+	if (copy_from_user(&val, optval, len))
 		return -EFAULT;
 
+	if (v2 && val.spt_pathpfthld > val.spt_pathcpthld)
+		return -EINVAL;
+
 	if (!sctp_is_any(sk, (const union sctp_addr *)&val.spt_address)) {
 		trans = sctp_addr_id2transport(sk, &val.spt_address,
 					       val.spt_assoc_id);
@@ -3963,6 +3967,8 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 
 		if (val.spt_pathmaxrxt)
 			trans->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			trans->ps_retrans = val.spt_pathcpthld;
 		trans->pf_retrans = val.spt_pathpfthld;
 
 		return 0;
@@ -3978,17 +3984,23 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 				    transports) {
 			if (val.spt_pathmaxrxt)
 				trans->pathmaxrxt = val.spt_pathmaxrxt;
+			if (v2)
+				trans->ps_retrans = val.spt_pathcpthld;
 			trans->pf_retrans = val.spt_pathpfthld;
 		}
 
 		if (val.spt_pathmaxrxt)
 			asoc->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			asoc->ps_retrans = val.spt_pathcpthld;
 		asoc->pf_retrans = val.spt_pathpfthld;
 	} else {
 		struct sctp_sock *sp = sctp_sk(sk);
 
 		if (val.spt_pathmaxrxt)
 			sp->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			sp->ps_retrans = val.spt_pathcpthld;
 		sp->pf_retrans = val.spt_pathpfthld;
 	}
 
@@ -4778,7 +4790,12 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 		retval = sctp_setsockopt_auto_asconf(sk, optval, optlen);
 		break;
 	case SCTP_PEER_ADDR_THLDS:
-		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen);
+		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen,
+							  false);
+		break;
+	case SCTP_PEER_ADDR_THLDS_V2:
+		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen,
+							  true);
 		break;
 	case SCTP_RECVRCVINFO:
 		retval = sctp_setsockopt_recvrcvinfo(sk, optval, optlen);
@@ -7217,18 +7234,19 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len,
  * http://www.ietf.org/id/draft-nishida-tsvwg-sctp-failover-05.txt
  */
 static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
-					    char __user *optval,
-					    int len,
-					    int __user *optlen)
+					    char __user *optval, int len,
+					    int __user *optlen, bool v2)
 {
-	struct sctp_paddrthlds val;
+	struct sctp_paddrthlds_v2 val;
 	struct sctp_transport *trans;
 	struct sctp_association *asoc;
+	int min;
 
-	if (len < sizeof(struct sctp_paddrthlds))
+	min = v2 ? sizeof(val) : sizeof(struct sctp_paddrthlds);
+	if (len < min)
 		return -EINVAL;
-	len = sizeof(struct sctp_paddrthlds);
-	if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len))
+	len = min;
+	if (copy_from_user(&val, optval, len))
 		return -EFAULT;
 
 	if (!sctp_is_any(sk, (const union sctp_addr *)&val.spt_address)) {
@@ -7239,6 +7257,7 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
 
 		val.spt_pathmaxrxt = trans->pathmaxrxt;
 		val.spt_pathpfthld = trans->pf_retrans;
+		val.spt_pathcpthld = trans->ps_retrans;
 
 		goto out;
 	}
@@ -7251,11 +7270,13 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
 	if (asoc) {
 		val.spt_pathpfthld = asoc->pf_retrans;
 		val.spt_pathmaxrxt = asoc->pathmaxrxt;
+		val.spt_pathcpthld = asoc->ps_retrans;
 	} else {
 		struct sctp_sock *sp = sctp_sk(sk);
 
 		val.spt_pathpfthld = sp->pf_retrans;
 		val.spt_pathmaxrxt = sp->pathmaxrxt;
+		val.spt_pathcpthld = sp->ps_retrans;
 	}
 
 out:
@@ -8135,7 +8156,12 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 		retval = sctp_getsockopt_auto_asconf(sk, len, optval, optlen);
 		break;
 	case SCTP_PEER_ADDR_THLDS:
-		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len, optlen);
+		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
+							  optlen, false);
+		break;
+	case SCTP_PEER_ADDR_THLDS_V2:
+		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
+							  optlen, true);
 		break;
 	case SCTP_GET_ASSOC_STATS:
 		retval = sctp_getsockopt_assoc_stats(sk, len, optval, optlen);
-- 
2.1.0

[PATCHv3 net-next 5/5] sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt

[Xin Long]

Section 7.2 of rfc7829: "Peer Address Thresholds (SCTP_PEER_ADDR_THLDS)
Socket Option" extends 'struct sctp_paddrthlds' with 'spt_pathcpthld'
added to allow a user to change ps_retrans per sock/asoc/transport, as
other 2 paddrthlds: pf_retrans, pathmaxrxt.

Note: to not break the user's program, here to support pf_retrans dump
and setting by adding a new sockopt SCTP_PEER_ADDR_THLDS_V2, and a new
structure sctp_paddrthlds_v2 instead of extending sctp_paddrthlds.

Also, when setting ps_retrans, the value is not allowed to be greater
than pf_retrans.

v1->v2:
  - use SCTP_PEER_ADDR_THLDS_V2 to set/get pf_retrans instead,
    as Marcelo and David Laight suggested.
v2->v3:
  - no change.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/uapi/linux/sctp.h | 10 +++++++++
 net/sctp/socket.c         | 54 +++++++++++++++++++++++++++++++++++------------
 2 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
index a190e4a..28ad40d 100644
--- a/include/uapi/linux/sctp.h
+++ b/include/uapi/linux/sctp.h
@@ -105,6 +105,7 @@ typedef __s32 sctp_assoc_t;
 #define SCTP_DEFAULT_SNDINFO	34
 #define SCTP_AUTH_DEACTIVATE_KEY	35
 #define SCTP_REUSE_PORT		36
+#define SCTP_PEER_ADDR_THLDS_V2	37
 
 /* Internal Socket Options. Some of the sctp library functions are
  * implemented using these socket options.
@@ -1087,6 +1088,15 @@ struct sctp_paddrthlds {
 	__u16 spt_pathpfthld;
 };
 
+/* Use a new structure with spt_pathcpthld for back compatibility */
+struct sctp_paddrthlds_v2 {
+	sctp_assoc_t spt_assoc_id;
+	struct sockaddr_storage spt_address;
+	__u16 spt_pathmaxrxt;
+	__u16 spt_pathpfthld;
+	__u16 spt_pathcpthld;
+};
+
 /*
  * Socket Option for Getting the Association/Stream-Specific PR-SCTP Status
  */
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 38d102b..3d2bad2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3943,18 +3943,22 @@ static int sctp_setsockopt_auto_asconf(struct sock *sk, char __user *optval,
  */
 static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 					    char __user *optval,
-					    unsigned int optlen)
+					    unsigned int optlen, bool v2)
 {
-	struct sctp_paddrthlds val;
+	struct sctp_paddrthlds_v2 val;
 	struct sctp_transport *trans;
 	struct sctp_association *asoc;
+	int len;
 
-	if (optlen < sizeof(struct sctp_paddrthlds))
+	len = v2 ? sizeof(val) : sizeof(struct sctp_paddrthlds);
+	if (optlen < len)
 		return -EINVAL;
-	if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval,
-			   sizeof(struct sctp_paddrthlds)))
+	if (copy_from_user(&val, optval, len))
 		return -EFAULT;
 
+	if (v2 && val.spt_pathpfthld > val.spt_pathcpthld)
+		return -EINVAL;
+
 	if (!sctp_is_any(sk, (const union sctp_addr *)&val.spt_address)) {
 		trans = sctp_addr_id2transport(sk, &val.spt_address,
 					       val.spt_assoc_id);
@@ -3963,6 +3967,8 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 
 		if (val.spt_pathmaxrxt)
 			trans->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			trans->ps_retrans = val.spt_pathcpthld;
 		trans->pf_retrans = val.spt_pathpfthld;
 
 		return 0;
@@ -3978,17 +3984,23 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk,
 				    transports) {
 			if (val.spt_pathmaxrxt)
 				trans->pathmaxrxt = val.spt_pathmaxrxt;
+			if (v2)
+				trans->ps_retrans = val.spt_pathcpthld;
 			trans->pf_retrans = val.spt_pathpfthld;
 		}
 
 		if (val.spt_pathmaxrxt)
 			asoc->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			asoc->ps_retrans = val.spt_pathcpthld;
 		asoc->pf_retrans = val.spt_pathpfthld;
 	} else {
 		struct sctp_sock *sp = sctp_sk(sk);
 
 		if (val.spt_pathmaxrxt)
 			sp->pathmaxrxt = val.spt_pathmaxrxt;
+		if (v2)
+			sp->ps_retrans = val.spt_pathcpthld;
 		sp->pf_retrans = val.spt_pathpfthld;
 	}
 
@@ -4778,7 +4790,12 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
 		retval = sctp_setsockopt_auto_asconf(sk, optval, optlen);
 		break;
 	case SCTP_PEER_ADDR_THLDS:
-		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen);
+		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen,
+							  false);
+		break;
+	case SCTP_PEER_ADDR_THLDS_V2:
+		retval = sctp_setsockopt_paddr_thresholds(sk, optval, optlen,
+							  true);
 		break;
 	case SCTP_RECVRCVINFO:
 		retval = sctp_setsockopt_recvrcvinfo(sk, optval, optlen);
@@ -7217,18 +7234,19 @@ static int sctp_getsockopt_assoc_ids(struct sock *sk, int len,
  * http://www.ietf.org/id/draft-nishida-tsvwg-sctp-failover-05.txt
  */
 static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
-					    char __user *optval,
-					    int len,
-					    int __user *optlen)
+					    char __user *optval, int len,
+					    int __user *optlen, bool v2)
 {
-	struct sctp_paddrthlds val;
+	struct sctp_paddrthlds_v2 val;
 	struct sctp_transport *trans;
 	struct sctp_association *asoc;
+	int min;
 
-	if (len < sizeof(struct sctp_paddrthlds))
+	min = v2 ? sizeof(val) : sizeof(struct sctp_paddrthlds);
+	if (len < min)
 		return -EINVAL;
-	len = sizeof(struct sctp_paddrthlds);
-	if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len))
+	len = min;
+	if (copy_from_user(&val, optval, len))
 		return -EFAULT;
 
 	if (!sctp_is_any(sk, (const union sctp_addr *)&val.spt_address)) {
@@ -7239,6 +7257,7 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
 
 		val.spt_pathmaxrxt = trans->pathmaxrxt;
 		val.spt_pathpfthld = trans->pf_retrans;
+		val.spt_pathcpthld = trans->ps_retrans;
 
 		goto out;
 	}
@@ -7251,11 +7270,13 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk,
 	if (asoc) {
 		val.spt_pathpfthld = asoc->pf_retrans;
 		val.spt_pathmaxrxt = asoc->pathmaxrxt;
+		val.spt_pathcpthld = asoc->ps_retrans;
 	} else {
 		struct sctp_sock *sp = sctp_sk(sk);
 
 		val.spt_pathpfthld = sp->pf_retrans;
 		val.spt_pathmaxrxt = sp->pathmaxrxt;
+		val.spt_pathcpthld = sp->ps_retrans;
 	}
 
 out:
@@ -8135,7 +8156,12 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
 		retval = sctp_getsockopt_auto_asconf(sk, len, optval, optlen);
 		break;
 	case SCTP_PEER_ADDR_THLDS:
-		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len, optlen);
+		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
+							  optlen, false);
+		break;
+	case SCTP_PEER_ADDR_THLDS_V2:
+		retval = sctp_getsockopt_paddr_thresholds(sk, optval, len,
+							  optlen, true);
 		break;
 	case SCTP_GET_ASSOC_STATS:
 		retval = sctp_getsockopt_assoc_stats(sk, len, optval, optlen);
-- 
2.1.0

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Neil Horman]

On Mon, Oct 14, 2019 at 02:14:43PM +0800, Xin Long wrote:
> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:
> 
>   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
>   2. add pf_expose per netns/sock/asoc
>   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   4. add ps_retrans per netns/sock/asoc/transport
>      (Primary Path Switchover)
>   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt
> 
> v1->v2:
>   - See Patch 2/5 and Patch 5/5.
> v2->v3:
>   - See Patch 1/5, 2/5 and 3/5.
> 
> Xin Long (5):
>   sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification
>   sctp: add pf_expose per netns and sock and asoc
>   sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   sctp: add support for Primary Path Switchover
>   sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt
> 
>  include/net/netns/sctp.h     |  14 +++++
>  include/net/sctp/constants.h |  10 +++
>  include/net/sctp/structs.h   |  13 +++-
>  include/uapi/linux/sctp.h    |  15 +++++
>  net/sctp/associola.c         |  31 ++++-----
>  net/sctp/protocol.c          |   6 ++
>  net/sctp/sm_sideeffect.c     |   5 ++
>  net/sctp/socket.c            | 147 ++++++++++++++++++++++++++++++++++++++-----
>  net/sctp/sysctl.c            |  19 ++++++
>  9 files changed, 226 insertions(+), 34 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Neil Horman]

On Mon, Oct 14, 2019 at 02:14:43PM +0800, Xin Long wrote:
> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:
> 
>   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
>   2. add pf_expose per netns/sock/asoc
>   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   4. add ps_retrans per netns/sock/asoc/transport
>      (Primary Path Switchover)
>   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt
> 
> v1->v2:
>   - See Patch 2/5 and Patch 5/5.
> v2->v3:
>   - See Patch 1/5, 2/5 and 3/5.
> 
> Xin Long (5):
>   sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification
>   sctp: add pf_expose per netns and sock and asoc
>   sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   sctp: add support for Primary Path Switchover
>   sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt
> 
>  include/net/netns/sctp.h     |  14 +++++
>  include/net/sctp/constants.h |  10 +++
>  include/net/sctp/structs.h   |  13 +++-
>  include/uapi/linux/sctp.h    |  15 +++++
>  net/sctp/associola.c         |  31 ++++-----
>  net/sctp/protocol.c          |   6 ++
>  net/sctp/sm_sideeffect.c     |   5 ++
>  net/sctp/socket.c            | 147 ++++++++++++++++++++++++++++++++++++++-----
>  net/sctp/sysctl.c            |  19 ++++++
>  9 files changed, 226 insertions(+), 34 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
Series
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 14 Oct 2019 14:14:43 +0800

> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:
> 
>   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
>   2. add pf_expose per netns/sock/asoc
>   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   4. add ps_retrans per netns/sock/asoc/transport
>      (Primary Path Switchover)
>   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt

I would like to see some SCTP expert ACKs here.

Thank you.

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 14 Oct 2019 14:14:43 +0800

> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:
> 
>   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
>   2. add pf_expose per netns/sock/asoc
>   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
>   4. add ps_retrans per netns/sock/asoc/transport
>      (Primary Path Switchover)
>   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt

I would like to see some SCTP expert ACKs here.

Thank you.

RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Laight]

From: David Miller
> Sent: 16 October 2019 01:57
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon, 14 Oct 2019 14:14:43 +0800
> 
> > SCTP-PF was implemented based on a Internet-Draft in 2012:
> >
> >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> >
> > It's been updated quite a few by rfc7829 in 2016.
> >
> > This patchset adds the following features:
> >
> >   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
> >   2. add pf_expose per netns/sock/asoc
> >   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
> >   4. add ps_retrans per netns/sock/asoc/transport
> >      (Primary Path Switchover)
> >   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt
> 
> I would like to see some SCTP expert ACKs here.

I'm only an SCTP user, but I think some of the API changes aren't right.
I'm not going to try to grok the sctp code - it makes my brain hurt.
(Even though I've written plenty of protocol stack code.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 14 Oct 2019 14:14:43 +0800

> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:

Sorry but I'm tossing these until an knowledgable SCTP person can
look at them.

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 14 Oct 2019 14:14:43 +0800

> SCTP-PF was implemented based on a Internet-Draft in 2012:
> 
>   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> 
> It's been updated quite a few by rfc7829 in 2016.
> 
> This patchset adds the following features:

Sorry but I'm tossing these until an knowledgable SCTP person can
look at them.

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Marcelo Ricardo Leitner]

On Wed, Oct 16, 2019 at 02:25:34PM -0400, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon, 14 Oct 2019 14:14:43 +0800
> 
> > SCTP-PF was implemented based on a Internet-Draft in 2012:
> > 
> >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> > 
> > It's been updated quite a few by rfc7829 in 2016.
> > 
> > This patchset adds the following features:
> 
> Sorry but I'm tossing these until an knowledgable SCTP person can
> look at them.

Hi Dave,

Maybe the email didn't get through but Neil actually already acked it,
2 days ago.
  Message-ID: <20191014124249.GB11844@hmswarspite.think-freely.org>

I won't be able to review it :-(

Thanks,
Marcelo

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Marcelo Ricardo Leitner]

On Wed, Oct 16, 2019 at 02:25:34PM -0400, David Miller wrote:
> From: Xin Long <lucien.xin@gmail.com>
> Date: Mon, 14 Oct 2019 14:14:43 +0800
> 
> > SCTP-PF was implemented based on a Internet-Draft in 2012:
> > 
> >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> > 
> > It's been updated quite a few by rfc7829 in 2016.
> > 
> > This patchset adds the following features:
> 
> Sorry but I'm tossing these until an knowledgable SCTP person can
> look at them.

Hi Dave,

Maybe the email didn't get through but Neil actually already acked it,
2 days ago.
  Message-ID: <20191014124249.GB11844@hmswarspite.think-freely.org>

I won't be able to review it :-(

Thanks,
Marcelo

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Wed, 16 Oct 2019 15:32:09 -0300

> On Wed, Oct 16, 2019 at 02:25:34PM -0400, David Miller wrote:
>> From: Xin Long <lucien.xin@gmail.com>
>> Date: Mon, 14 Oct 2019 14:14:43 +0800
>> 
>> > SCTP-PF was implemented based on a Internet-Draft in 2012:
>> > 
>> >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
>> > 
>> > It's been updated quite a few by rfc7829 in 2016.
>> > 
>> > This patchset adds the following features:
>> 
>> Sorry but I'm tossing these until an knowledgable SCTP person can
>> look at them.
> 
> Hi Dave,
> 
> Maybe the email didn't get through but Neil actually already acked it,
> 2 days ago.
>   Message-ID: <20191014124249.GB11844@hmswarspite.think-freely.org>
> 
> I won't be able to review it :-(

All I saw was David Laight replying saying he thought the APIs weren't
implemented correctly.

I have to admit that I'm really going to proceed carefully with SCTP
API changes because there has been a lot of discussions in the past
involving backwards-incompatible things happening.

I'm not saying that is happening here, but my confidence in SCTP API
changes is very low.

I want this to sit for a while and Xin can respin and resubmit in a
day or two.

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Wed, 16 Oct 2019 15:32:09 -0300

> On Wed, Oct 16, 2019 at 02:25:34PM -0400, David Miller wrote:
>> From: Xin Long <lucien.xin@gmail.com>
>> Date: Mon, 14 Oct 2019 14:14:43 +0800
>> 
>> > SCTP-PF was implemented based on a Internet-Draft in 2012:
>> > 
>> >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
>> > 
>> > It's been updated quite a few by rfc7829 in 2016.
>> > 
>> > This patchset adds the following features:
>> 
>> Sorry but I'm tossing these until an knowledgable SCTP person can
>> look at them.
> 
> Hi Dave,
> 
> Maybe the email didn't get through but Neil actually already acked it,
> 2 days ago.
>   Message-ID: <20191014124249.GB11844@hmswarspite.think-freely.org>
> 
> I won't be able to review it :-(

All I saw was David Laight replying saying he thought the APIs weren't
implemented correctly.

I have to admit that I'm really going to proceed carefully with SCTP
API changes because there has been a lot of discussions in the past
involving backwards-incompatible things happening.

I'm not saying that is happening here, but my confidence in SCTP API
changes is very low.

I want this to sit for a while and Xin can respin and resubmit in a
day or two.

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Xin Long]

On Wed, Oct 16, 2019 at 6:42 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: David Miller
> > Sent: 16 October 2019 01:57
> > From: Xin Long <lucien.xin@gmail.com>
> > Date: Mon, 14 Oct 2019 14:14:43 +0800
> >
> > > SCTP-PF was implemented based on a Internet-Draft in 2012:
> > >
> > >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> > >
> > > It's been updated quite a few by rfc7829 in 2016.
> > >
> > > This patchset adds the following features:
> > >
> > >   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
> > >   2. add pf_expose per netns/sock/asoc
> > >   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
> > >   4. add ps_retrans per netns/sock/asoc/transport
> > >      (Primary Path Switchover)
> > >   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt
> >
> > I would like to see some SCTP expert ACKs here.
>
> I'm only an SCTP user, but I think some of the API changes aren't right.
Hi, David L.

I think you must know quite a few user cases.

Before I repost, can you pls give the exact places where the API
changes may not be right as you've already done in v1 and v2, so
that I can correct them.

Thanks.

> I'm not going to try to grok the sctp code - it makes my brain hurt.
> (Even though I've written plenty of protocol stack code.)
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[Xin Long]

On Wed, Oct 16, 2019 at 6:42 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: David Miller
> > Sent: 16 October 2019 01:57
> > From: Xin Long <lucien.xin@gmail.com>
> > Date: Mon, 14 Oct 2019 14:14:43 +0800
> >
> > > SCTP-PF was implemented based on a Internet-Draft in 2012:
> > >
> > >   https://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05
> > >
> > > It's been updated quite a few by rfc7829 in 2016.
> > >
> > > This patchset adds the following features:
> > >
> > >   1. add SCTP_ADDR_POTENTIALLY_FAILED notification
> > >   2. add pf_expose per netns/sock/asoc
> > >   3. add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt
> > >   4. add ps_retrans per netns/sock/asoc/transport
> > >      (Primary Path Switchover)
> > >   5. add spt_pathcpthld for SCTP_PEER_ADDR_THLDS sockopt
> >
> > I would like to see some SCTP expert ACKs here.
>
> I'm only an SCTP user, but I think some of the API changes aren't right.
Hi, David L.

I think you must know quite a few user cases.

Before I repost, can you pls give the exact places where the API
changes may not be right as you've already done in v1 and v2, so
that I can correct them.

Thanks.

> I'm not going to try to grok the sctp code - it makes my brain hurt.
> (Even though I've written plenty of protocol stack code.)
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Laight]

From: Xin Long
> Sent: 17 October 2019 05:57
...
> > I'm only an SCTP user, but I think some of the API changes aren't right.
> Hi, David L.
> 
> I think you must know quite a few user cases.
> 
> Before I repost, can you pls give the exact places where the API
> changes may not be right as you've already done in v1 and v2, so
> that I can correct them.

I'll try to find time to look later today.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829

[David Laight]

RnJvbTogWGluIExvbmcNCj4gU2VudDogMTcgT2N0b2JlciAyMDE5IDA1OjU3DQouLi4NCj4gPiBJ
J20gb25seSBhbiBTQ1RQIHVzZXIsIGJ1dCBJIHRoaW5rIHNvbWUgb2YgdGhlIEFQSSBjaGFuZ2Vz
IGFyZW4ndCByaWdodC4NCj4gSGksIERhdmlkIEwuDQo+IA0KPiBJIHRoaW5rIHlvdSBtdXN0IGtu
b3cgcXVpdGUgYSBmZXcgdXNlciBjYXNlcy4NCj4gDQo+IEJlZm9yZSBJIHJlcG9zdCwgY2FuIHlv
dSBwbHMgZ2l2ZSB0aGUgZXhhY3QgcGxhY2VzIHdoZXJlIHRoZSBBUEkNCj4gY2hhbmdlcyBtYXkg
bm90IGJlIHJpZ2h0IGFzIHlvdSd2ZSBhbHJlYWR5IGRvbmUgaW4gdjEgYW5kIHYyLCBzbw0KPiB0
aGF0IEkgY2FuIGNvcnJlY3QgdGhlbS4NCg0KSSdsbCB0cnkgdG8gZmluZCB0aW1lIHRvIGxvb2sg
bGF0ZXIgdG9kYXkuDQoNCglEYXZpZA0KDQotDQpSZWdpc3RlcmVkIEFkZHJlc3MgTGFrZXNpZGUs
IEJyYW1sZXkgUm9hZCwgTW91bnQgRmFybSwgTWlsdG9uIEtleW5lcywgTUsxIDFQVCwgVUsNClJl
Z2lzdHJhdGlvbiBObzogMTM5NzM4NiAoV2FsZXMpDQo

RE: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[David Laight]

I've found v3 :-)
But it isn't that much better than v2.

From: Xin Long
> Sent: 14 October 2019 07:15
> SCTP Quick failover draft section 5.1, point 5 has been removed
> from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> Layer Protocol (ULP) about this state transition", as said in
> section 3.2, point 8.
> 
> So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> in section 7.1, "which is reported if the affected address
> becomes PF". Also remove transport cwnd's update when moving
> from PF back to ACTIVE , which is no longer in rfc7829 either.
> 
> v1->v2:
>   - no change
> v2->v3:
>   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/associola.c      | 17 ++++-------------
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index 6bce7f9..f4ab7bb 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -410,6 +410,8 @@ enum sctp_spc_state {
>  	SCTP_ADDR_ADDED,
>  	SCTP_ADDR_MADE_PRIM,
>  	SCTP_ADDR_CONFIRMED,
> +	SCTP_ADDR_POTENTIALLY_FAILED,
> +#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
>  };
> 
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 1ba893b..4f9efba 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  			spc_state = SCTP_ADDR_CONFIRMED;
>  		else
>  			spc_state = SCTP_ADDR_AVAILABLE;
> -		/* Don't inform ULP about transition from PF to
> -		 * active state and set cwnd to 1 MTU, see SCTP
> -		 * Quick failover draft section 5.1, point 5
> -		 */
> -		if (transport->state == SCTP_PF) {
> -			ulp_notify = false;
> -			transport->cwnd = asoc->pathmtu;
> -		}

This is wrong.
If the old state is PF and the application hasn't exposed PF the event should be
ignored.

>  		transport->state = SCTP_ACTIVE;
>  		break;
> 
> @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  		 * to inactive state.  Also, release the cached route since
>  		 * there may be a better route next time.
>  		 */
> -		if (transport->state != SCTP_UNCONFIRMED)
> +		if (transport->state != SCTP_UNCONFIRMED) {
>  			transport->state = SCTP_INACTIVE;
> -		else {
> +			spc_state = SCTP_ADDR_UNREACHABLE;
> +		} else {
>  			sctp_transport_dst_release(transport);
>  			ulp_notify = false;
>  		}
> -
> -		spc_state = SCTP_ADDR_UNREACHABLE;
>  		break;
> 
>  	case SCTP_TRANSPORT_PF:
>  		transport->state = SCTP_PF;
> -		ulp_notify = false;

Again the event should be supressed if PF isn't exposed.

> +		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>  		break;
> 
>  	default:
> --
> 2.1.0

I also haven't spotted where the test that the application has actually enabled
state transition events is in the code.
I'd have thought it would be anything is built and allocated.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[David Laight]

I've found v3 :-)
But it isn't that much better than v2.

From: Xin Long
> Sent: 14 October 2019 07:15
> SCTP Quick failover draft section 5.1, point 5 has been removed
> from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> Layer Protocol (ULP) about this state transition", as said in
> section 3.2, point 8.
> 
> So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> in section 7.1, "which is reported if the affected address
> becomes PF". Also remove transport cwnd's update when moving
> from PF back to ACTIVE , which is no longer in rfc7829 either.
> 
> v1->v2:
>   - no change
> v2->v3:
>   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/associola.c      | 17 ++++-------------
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index 6bce7f9..f4ab7bb 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -410,6 +410,8 @@ enum sctp_spc_state {
>  	SCTP_ADDR_ADDED,
>  	SCTP_ADDR_MADE_PRIM,
>  	SCTP_ADDR_CONFIRMED,
> +	SCTP_ADDR_POTENTIALLY_FAILED,
> +#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
>  };
> 
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 1ba893b..4f9efba 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  			spc_state = SCTP_ADDR_CONFIRMED;
>  		else
>  			spc_state = SCTP_ADDR_AVAILABLE;
> -		/* Don't inform ULP about transition from PF to
> -		 * active state and set cwnd to 1 MTU, see SCTP
> -		 * Quick failover draft section 5.1, point 5
> -		 */
> -		if (transport->state = SCTP_PF) {
> -			ulp_notify = false;
> -			transport->cwnd = asoc->pathmtu;
> -		}

This is wrong.
If the old state is PF and the application hasn't exposed PF the event should be
ignored.

>  		transport->state = SCTP_ACTIVE;
>  		break;
> 
> @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  		 * to inactive state.  Also, release the cached route since
>  		 * there may be a better route next time.
>  		 */
> -		if (transport->state != SCTP_UNCONFIRMED)
> +		if (transport->state != SCTP_UNCONFIRMED) {
>  			transport->state = SCTP_INACTIVE;
> -		else {
> +			spc_state = SCTP_ADDR_UNREACHABLE;
> +		} else {
>  			sctp_transport_dst_release(transport);
>  			ulp_notify = false;
>  		}
> -
> -		spc_state = SCTP_ADDR_UNREACHABLE;
>  		break;
> 
>  	case SCTP_TRANSPORT_PF:
>  		transport->state = SCTP_PF;
> -		ulp_notify = false;

Again the event should be supressed if PF isn't exposed.

> +		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>  		break;
> 
>  	default:
> --
> 2.1.0

I also haven't spotted where the test that the application has actually enabled
state transition events is in the code.
I'd have thought it would be anything is built and allocated.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote:
>
> I've found v3 :-)
ah okay. sorry.

> But it isn't that much better than v2.
>
> From: Xin Long
> > Sent: 14 October 2019 07:15
> > SCTP Quick failover draft section 5.1, point 5 has been removed
> > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > Layer Protocol (ULP) about this state transition", as said in
> > section 3.2, point 8.
> >
> > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > in section 7.1, "which is reported if the affected address
> > becomes PF". Also remove transport cwnd's update when moving
> > from PF back to ACTIVE , which is no longer in rfc7829 either.
> >
> > v1->v2:
> >   - no change
> > v2->v3:
> >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/associola.c      | 17 ++++-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index 6bce7f9..f4ab7bb 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> >       SCTP_ADDR_ADDED,
> >       SCTP_ADDR_MADE_PRIM,
> >       SCTP_ADDR_CONFIRMED,
> > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >  };
> >
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 1ba893b..4f9efba 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                       spc_state = SCTP_ADDR_CONFIRMED;
> >               else
> >                       spc_state = SCTP_ADDR_AVAILABLE;
> > -             /* Don't inform ULP about transition from PF to
> > -              * active state and set cwnd to 1 MTU, see SCTP
> > -              * Quick failover draft section 5.1, point 5
> > -              */
> > -             if (transport->state == SCTP_PF) {
> > -                     ulp_notify = false;
> > -                     transport->cwnd = asoc->pathmtu;
> > -             }
>
> This is wrong.
> If the old state is PF and the application hasn't exposed PF the event should be
> ignored.
yeps, in Patch 2/5:
+               if (transport->state == SCTP_PF &&
+                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+                       ulp_notify = false;
+               else if (transport->state == SCTP_UNCONFIRMED &&
+                        error == SCTP_HEARTBEAT_SUCCESS)
                        spc_state = SCTP_ADDR_CONFIRMED;
                else
                        spc_state = SCTP_ADDR_AVAILABLE;

>
> >               transport->state = SCTP_ACTIVE;
> >               break;
> >
> > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                * to inactive state.  Also, release the cached route since
> >                * there may be a better route next time.
> >                */
> > -             if (transport->state != SCTP_UNCONFIRMED)
> > +             if (transport->state != SCTP_UNCONFIRMED) {
> >                       transport->state = SCTP_INACTIVE;
> > -             else {
> > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > +             } else {
> >                       sctp_transport_dst_release(transport);
> >                       ulp_notify = false;
> >               }
> > -
> > -             spc_state = SCTP_ADDR_UNREACHABLE;
> >               break;
> >
> >       case SCTP_TRANSPORT_PF:
> >               transport->state = SCTP_PF;
> > -             ulp_notify = false;
>
> Again the event should be supressed if PF isn't exposed.
it will be suppressed after Patch 2/5:
+               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+                       ulp_notify = false;
+               else
+                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
                break;

>
> > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >               break;
> >
> >       default:
> > --
> > 2.1.0
>
> I also haven't spotted where the test that the application has actually enabled
> state transition events is in the code.
all events will be created, but dropped in sctp_ulpq_tail_event() when trying
to deliver up:

        /* Check if the user wishes to receive this event.  */
        if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
                goto out_free;

> I'd have thought it would be anything is built and allocated.
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote:
>
> I've found v3 :-)
ah okay. sorry.

> But it isn't that much better than v2.
>
> From: Xin Long
> > Sent: 14 October 2019 07:15
> > SCTP Quick failover draft section 5.1, point 5 has been removed
> > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > Layer Protocol (ULP) about this state transition", as said in
> > section 3.2, point 8.
> >
> > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > in section 7.1, "which is reported if the affected address
> > becomes PF". Also remove transport cwnd's update when moving
> > from PF back to ACTIVE , which is no longer in rfc7829 either.
> >
> > v1->v2:
> >   - no change
> > v2->v3:
> >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/associola.c      | 17 ++++-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index 6bce7f9..f4ab7bb 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> >       SCTP_ADDR_ADDED,
> >       SCTP_ADDR_MADE_PRIM,
> >       SCTP_ADDR_CONFIRMED,
> > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >  };
> >
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 1ba893b..4f9efba 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                       spc_state = SCTP_ADDR_CONFIRMED;
> >               else
> >                       spc_state = SCTP_ADDR_AVAILABLE;
> > -             /* Don't inform ULP about transition from PF to
> > -              * active state and set cwnd to 1 MTU, see SCTP
> > -              * Quick failover draft section 5.1, point 5
> > -              */
> > -             if (transport->state = SCTP_PF) {
> > -                     ulp_notify = false;
> > -                     transport->cwnd = asoc->pathmtu;
> > -             }
>
> This is wrong.
> If the old state is PF and the application hasn't exposed PF the event should be
> ignored.
yeps, in Patch 2/5:
+               if (transport->state = SCTP_PF &&
+                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+                       ulp_notify = false;
+               else if (transport->state = SCTP_UNCONFIRMED &&
+                        error = SCTP_HEARTBEAT_SUCCESS)
                        spc_state = SCTP_ADDR_CONFIRMED;
                else
                        spc_state = SCTP_ADDR_AVAILABLE;

>
> >               transport->state = SCTP_ACTIVE;
> >               break;
> >
> > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                * to inactive state.  Also, release the cached route since
> >                * there may be a better route next time.
> >                */
> > -             if (transport->state != SCTP_UNCONFIRMED)
> > +             if (transport->state != SCTP_UNCONFIRMED) {
> >                       transport->state = SCTP_INACTIVE;
> > -             else {
> > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > +             } else {
> >                       sctp_transport_dst_release(transport);
> >                       ulp_notify = false;
> >               }
> > -
> > -             spc_state = SCTP_ADDR_UNREACHABLE;
> >               break;
> >
> >       case SCTP_TRANSPORT_PF:
> >               transport->state = SCTP_PF;
> > -             ulp_notify = false;
>
> Again the event should be supressed if PF isn't exposed.
it will be suppressed after Patch 2/5:
+               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
+                       ulp_notify = false;
+               else
+                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
                break;

>
> > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >               break;
> >
> >       default:
> > --
> > 2.1.0
>
> I also haven't spotted where the test that the application has actually enabled
> state transition events is in the code.
all events will be created, but dropped in sctp_ulpq_tail_event() when trying
to deliver up:

        /* Check if the user wishes to receive this event.  */
        if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
                goto out_free;

> I'd have thought it would be anything is built and allocated.
>
>         David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

Hi, David L.

I will repost if you don't have any other dissent.

Thanks for your nice review.

On Sat, Oct 19, 2019 at 4:55 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > I've found v3 :-)
> ah okay. sorry.
>
> > But it isn't that much better than v2.
> >
> > From: Xin Long
> > > Sent: 14 October 2019 07:15
> > > SCTP Quick failover draft section 5.1, point 5 has been removed
> > > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > > Layer Protocol (ULP) about this state transition", as said in
> > > section 3.2, point 8.
> > >
> > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > > in section 7.1, "which is reported if the affected address
> > > becomes PF". Also remove transport cwnd's update when moving
> > > from PF back to ACTIVE , which is no longer in rfc7829 either.
> > >
> > > v1->v2:
> > >   - no change
> > > v2->v3:
> > >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  include/uapi/linux/sctp.h |  2 ++
> > >  net/sctp/associola.c      | 17 ++++-------------
> > >  2 files changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > > index 6bce7f9..f4ab7bb 100644
> > > --- a/include/uapi/linux/sctp.h
> > > +++ b/include/uapi/linux/sctp.h
> > > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> > >       SCTP_ADDR_ADDED,
> > >       SCTP_ADDR_MADE_PRIM,
> > >       SCTP_ADDR_CONFIRMED,
> > > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> > >  };
> > >
> > >
> > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > > index 1ba893b..4f9efba 100644
> > > --- a/net/sctp/associola.c
> > > +++ b/net/sctp/associola.c
> > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > >               else
> > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > -             /* Don't inform ULP about transition from PF to
> > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > -              * Quick failover draft section 5.1, point 5
> > > -              */
> > > -             if (transport->state == SCTP_PF) {
> > > -                     ulp_notify = false;
> > > -                     transport->cwnd = asoc->pathmtu;
> > > -             }
> >
> > This is wrong.
> > If the old state is PF and the application hasn't exposed PF the event should be
> > ignored.
> yeps, in Patch 2/5:
> +               if (transport->state == SCTP_PF &&
> +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else if (transport->state == SCTP_UNCONFIRMED &&
> +                        error == SCTP_HEARTBEAT_SUCCESS)
>                         spc_state = SCTP_ADDR_CONFIRMED;
>                 else
>                         spc_state = SCTP_ADDR_AVAILABLE;
>
> >
> > >               transport->state = SCTP_ACTIVE;
> > >               break;
> > >
> > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                * to inactive state.  Also, release the cached route since
> > >                * there may be a better route next time.
> > >                */
> > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > >                       transport->state = SCTP_INACTIVE;
> > > -             else {
> > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > +             } else {
> > >                       sctp_transport_dst_release(transport);
> > >                       ulp_notify = false;
> > >               }
> > > -
> > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > >               break;
> > >
> > >       case SCTP_TRANSPORT_PF:
> > >               transport->state = SCTP_PF;
> > > -             ulp_notify = false;
> >
> > Again the event should be supressed if PF isn't exposed.
> it will be suppressed after Patch 2/5:
> +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else
> +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>                 break;
>
> >
> > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > >               break;
> > >
> > >       default:
> > > --
> > > 2.1.0
> >
> > I also haven't spotted where the test that the application has actually enabled
> > state transition events is in the code.
> all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> to deliver up:
>
>         /* Check if the user wishes to receive this event.  */
>         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
>                 goto out_free;
>
> > I'd have thought it would be anything is built and allocated.
> >
> >         David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

Hi, David L.

I will repost if you don't have any other dissent.

Thanks for your nice review.

On Sat, Oct 19, 2019 at 4:55 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> On Fri, Oct 18, 2019 at 11:56 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > I've found v3 :-)
> ah okay. sorry.
>
> > But it isn't that much better than v2.
> >
> > From: Xin Long
> > > Sent: 14 October 2019 07:15
> > > SCTP Quick failover draft section 5.1, point 5 has been removed
> > > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > > Layer Protocol (ULP) about this state transition", as said in
> > > section 3.2, point 8.
> > >
> > > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > > in section 7.1, "which is reported if the affected address
> > > becomes PF". Also remove transport cwnd's update when moving
> > > from PF back to ACTIVE , which is no longer in rfc7829 either.
> > >
> > > v1->v2:
> > >   - no change
> > > v2->v3:
> > >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  include/uapi/linux/sctp.h |  2 ++
> > >  net/sctp/associola.c      | 17 ++++-------------
> > >  2 files changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > > index 6bce7f9..f4ab7bb 100644
> > > --- a/include/uapi/linux/sctp.h
> > > +++ b/include/uapi/linux/sctp.h
> > > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> > >       SCTP_ADDR_ADDED,
> > >       SCTP_ADDR_MADE_PRIM,
> > >       SCTP_ADDR_CONFIRMED,
> > > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> > >  };
> > >
> > >
> > > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > > index 1ba893b..4f9efba 100644
> > > --- a/net/sctp/associola.c
> > > +++ b/net/sctp/associola.c
> > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > >               else
> > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > -             /* Don't inform ULP about transition from PF to
> > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > -              * Quick failover draft section 5.1, point 5
> > > -              */
> > > -             if (transport->state = SCTP_PF) {
> > > -                     ulp_notify = false;
> > > -                     transport->cwnd = asoc->pathmtu;
> > > -             }
> >
> > This is wrong.
> > If the old state is PF and the application hasn't exposed PF the event should be
> > ignored.
> yeps, in Patch 2/5:
> +               if (transport->state = SCTP_PF &&
> +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else if (transport->state = SCTP_UNCONFIRMED &&
> +                        error = SCTP_HEARTBEAT_SUCCESS)
>                         spc_state = SCTP_ADDR_CONFIRMED;
>                 else
>                         spc_state = SCTP_ADDR_AVAILABLE;
>
> >
> > >               transport->state = SCTP_ACTIVE;
> > >               break;
> > >
> > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                * to inactive state.  Also, release the cached route since
> > >                * there may be a better route next time.
> > >                */
> > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > >                       transport->state = SCTP_INACTIVE;
> > > -             else {
> > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > +             } else {
> > >                       sctp_transport_dst_release(transport);
> > >                       ulp_notify = false;
> > >               }
> > > -
> > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > >               break;
> > >
> > >       case SCTP_TRANSPORT_PF:
> > >               transport->state = SCTP_PF;
> > > -             ulp_notify = false;
> >
> > Again the event should be supressed if PF isn't exposed.
> it will be suppressed after Patch 2/5:
> +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else
> +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>                 break;
>
> >
> > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > >               break;
> > >
> > >       default:
> > > --
> > > 2.1.0
> >
> > I also haven't spotted where the test that the application has actually enabled
> > state transition events is in the code.
> all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> to deliver up:
>
>         /* Check if the user wishes to receive this event.  */
>         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
>                 goto out_free;
>
> > I'd have thought it would be anything is built and allocated.
> >
> >         David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Marcelo Ricardo Leitner]

Hi,

Sorry for the long delay on this review.

On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote:
> SCTP Quick failover draft section 5.1, point 5 has been removed
> from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> Layer Protocol (ULP) about this state transition", as said in
> section 3.2, point 8.
> 
> So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> in section 7.1, "which is reported if the affected address
> becomes PF". Also remove transport cwnd's update when moving
> from PF back to ACTIVE , which is no longer in rfc7829 either.
> 
> v1->v2:
>   - no change
> v2->v3:
>   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/associola.c      | 17 ++++-------------
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index 6bce7f9..f4ab7bb 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -410,6 +410,8 @@ enum sctp_spc_state {
>  	SCTP_ADDR_ADDED,
>  	SCTP_ADDR_MADE_PRIM,
>  	SCTP_ADDR_CONFIRMED,
> +	SCTP_ADDR_POTENTIALLY_FAILED,
> +#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
>  };
>  
>  
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 1ba893b..4f9efba 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,

While at here, dealing with spc_state, please seize the moment and
initialize it to the enum instead:

@@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
                                  sctp_sn_error_t error)
 {
        bool ulp_notify = true;
-       int spc_state = 0;
+       int spc_state = SCTP_ADDR_AVAILABLE;


>  			spc_state = SCTP_ADDR_CONFIRMED;
>  		else
>  			spc_state = SCTP_ADDR_AVAILABLE;

This else could be removed (equals to initial value).

> -		/* Don't inform ULP about transition from PF to
> -		 * active state and set cwnd to 1 MTU, see SCTP
> -		 * Quick failover draft section 5.1, point 5
> -		 */
> -		if (transport->state == SCTP_PF) {
> -			ulp_notify = false;
> -			transport->cwnd = asoc->pathmtu;
> -		}
>  		transport->state = SCTP_ACTIVE;
>  		break;
>  
> @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  		 * to inactive state.  Also, release the cached route since
>  		 * there may be a better route next time.
>  		 */
> -		if (transport->state != SCTP_UNCONFIRMED)
> +		if (transport->state != SCTP_UNCONFIRMED) {
>  			transport->state = SCTP_INACTIVE;
> -		else {
> +			spc_state = SCTP_ADDR_UNREACHABLE;
> +		} else {
>  			sctp_transport_dst_release(transport);
>  			ulp_notify = false;
>  		}
> -
> -		spc_state = SCTP_ADDR_UNREACHABLE;
>  		break;
>  
>  	case SCTP_TRANSPORT_PF:
>  		transport->state = SCTP_PF;
> -		ulp_notify = false;
> +		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>  		break;
>  
>  	default:
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Marcelo Ricardo Leitner]

Hi,

Sorry for the long delay on this review.

On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote:
> SCTP Quick failover draft section 5.1, point 5 has been removed
> from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> Layer Protocol (ULP) about this state transition", as said in
> section 3.2, point 8.
> 
> So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> in section 7.1, "which is reported if the affected address
> becomes PF". Also remove transport cwnd's update when moving
> from PF back to ACTIVE , which is no longer in rfc7829 either.
> 
> v1->v2:
>   - no change
> v2->v3:
>   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/associola.c      | 17 ++++-------------
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index 6bce7f9..f4ab7bb 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -410,6 +410,8 @@ enum sctp_spc_state {
>  	SCTP_ADDR_ADDED,
>  	SCTP_ADDR_MADE_PRIM,
>  	SCTP_ADDR_CONFIRMED,
> +	SCTP_ADDR_POTENTIALLY_FAILED,
> +#define SCTP_ADDR_PF	SCTP_ADDR_POTENTIALLY_FAILED
>  };
>  
>  
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 1ba893b..4f9efba 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,

While at here, dealing with spc_state, please seize the moment and
initialize it to the enum instead:

@@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
                                  sctp_sn_error_t error)
 {
        bool ulp_notify = true;
-       int spc_state = 0;
+       int spc_state = SCTP_ADDR_AVAILABLE;


>  			spc_state = SCTP_ADDR_CONFIRMED;
>  		else
>  			spc_state = SCTP_ADDR_AVAILABLE;

This else could be removed (equals to initial value).

> -		/* Don't inform ULP about transition from PF to
> -		 * active state and set cwnd to 1 MTU, see SCTP
> -		 * Quick failover draft section 5.1, point 5
> -		 */
> -		if (transport->state = SCTP_PF) {
> -			ulp_notify = false;
> -			transport->cwnd = asoc->pathmtu;
> -		}
>  		transport->state = SCTP_ACTIVE;
>  		break;
>  
> @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>  		 * to inactive state.  Also, release the cached route since
>  		 * there may be a better route next time.
>  		 */
> -		if (transport->state != SCTP_UNCONFIRMED)
> +		if (transport->state != SCTP_UNCONFIRMED) {
>  			transport->state = SCTP_INACTIVE;
> -		else {
> +			spc_state = SCTP_ADDR_UNREACHABLE;
> +		} else {
>  			sctp_transport_dst_release(transport);
>  			ulp_notify = false;
>  		}
> -
> -		spc_state = SCTP_ADDR_UNREACHABLE;
>  		break;
>  
>  	case SCTP_TRANSPORT_PF:
>  		transport->state = SCTP_PF;
> -		ulp_notify = false;
> +		spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>  		break;
>  
>  	default:
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Marcelo Ricardo Leitner]

On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote:
> > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > >               else
> > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > -             /* Don't inform ULP about transition from PF to
> > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > -              * Quick failover draft section 5.1, point 5
> > > -              */
> > > -             if (transport->state == SCTP_PF) {
> > > -                     ulp_notify = false;
> > > -                     transport->cwnd = asoc->pathmtu;
> > > -             }
> >
> > This is wrong.
> > If the old state is PF and the application hasn't exposed PF the event should be
> > ignored.
> yeps, in Patch 2/5:
> +               if (transport->state == SCTP_PF &&
> +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else if (transport->state == SCTP_UNCONFIRMED &&
> +                        error == SCTP_HEARTBEAT_SUCCESS)
>                         spc_state = SCTP_ADDR_CONFIRMED;
>                 else
>                         spc_state = SCTP_ADDR_AVAILABLE;

Right, yet for one bisecting the kernel, a checkout on this patch will
see this change regardless of patch 2. Patches 1 and 2 could be
swapped to avoid this situation.

> 
> >
> > >               transport->state = SCTP_ACTIVE;
> > >               break;
> > >
> > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                * to inactive state.  Also, release the cached route since
> > >                * there may be a better route next time.
> > >                */
> > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > >                       transport->state = SCTP_INACTIVE;
> > > -             else {
> > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > +             } else {
> > >                       sctp_transport_dst_release(transport);
> > >                       ulp_notify = false;
> > >               }
> > > -
> > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > >               break;
> > >
> > >       case SCTP_TRANSPORT_PF:
> > >               transport->state = SCTP_PF;
> > > -             ulp_notify = false;
> >
> > Again the event should be supressed if PF isn't exposed.
> it will be suppressed after Patch 2/5:
> +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else
> +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>                 break;

Same here.

> 
> >
> > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > >               break;
> > >
> > >       default:
> > > --
> > > 2.1.0
> >
> > I also haven't spotted where the test that the application has actually enabled
> > state transition events is in the code.
> all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> to deliver up:
> 
>         /* Check if the user wishes to receive this event.  */
>         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
>                 goto out_free;
> 
> > I'd have thought it would be anything is built and allocated.
> >
> >         David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >
> 

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Marcelo Ricardo Leitner]

On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote:
> > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > >               else
> > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > -             /* Don't inform ULP about transition from PF to
> > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > -              * Quick failover draft section 5.1, point 5
> > > -              */
> > > -             if (transport->state = SCTP_PF) {
> > > -                     ulp_notify = false;
> > > -                     transport->cwnd = asoc->pathmtu;
> > > -             }
> >
> > This is wrong.
> > If the old state is PF and the application hasn't exposed PF the event should be
> > ignored.
> yeps, in Patch 2/5:
> +               if (transport->state = SCTP_PF &&
> +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else if (transport->state = SCTP_UNCONFIRMED &&
> +                        error = SCTP_HEARTBEAT_SUCCESS)
>                         spc_state = SCTP_ADDR_CONFIRMED;
>                 else
>                         spc_state = SCTP_ADDR_AVAILABLE;

Right, yet for one bisecting the kernel, a checkout on this patch will
see this change regardless of patch 2. Patches 1 and 2 could be
swapped to avoid this situation.

> 
> >
> > >               transport->state = SCTP_ACTIVE;
> > >               break;
> > >
> > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > >                * to inactive state.  Also, release the cached route since
> > >                * there may be a better route next time.
> > >                */
> > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > >                       transport->state = SCTP_INACTIVE;
> > > -             else {
> > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > +             } else {
> > >                       sctp_transport_dst_release(transport);
> > >                       ulp_notify = false;
> > >               }
> > > -
> > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > >               break;
> > >
> > >       case SCTP_TRANSPORT_PF:
> > >               transport->state = SCTP_PF;
> > > -             ulp_notify = false;
> >
> > Again the event should be supressed if PF isn't exposed.
> it will be suppressed after Patch 2/5:
> +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> +                       ulp_notify = false;
> +               else
> +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
>                 break;

Same here.

> 
> >
> > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > >               break;
> > >
> > >       default:
> > > --
> > > 2.1.0
> >
> > I also haven't spotted where the test that the application has actually enabled
> > state transition events is in the code.
> all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> to deliver up:
> 
>         /* Check if the user wishes to receive this event.  */
>         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
>                 goto out_free;
> 
> > I'd have thought it would be anything is built and allocated.
> >
> >         David
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)
> >
> 

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:45PM +0800, Xin Long wrote:
> As said in rfc7829, section 3, point 12:
> 
>   The SCTP stack SHOULD expose the PF state of its destination
>   addresses to the ULP as well as provide the means to notify the
>   ULP of state transitions of its destination addresses from
>   active to PF, and vice versa.  However, it is recommended that
>   an SCTP stack implementing SCTP-PF also allows for the ULP to be
>   kept ignorant of the PF state of its destinations and the
>   associated state transitions, thus allowing for retention of the
>   simpler state transition model of [RFC4960] in the ULP.
> 
> Not only does it allow to expose the PF state to ULP, but also
> allow to ignore sctp-pf to ULP.
> 
> So this patch is to add pf_expose per netns, sock and asoc. And in
> sctp_assoc_control_transport(), ulp_notify will be set to false if
> asoc->expose is not set.
> 
> It also allows a user to change pf_expose per netns by sysctl, and
> pf_expose per sock and asoc will be initialized with it.

I also do see value on this sysctl. We currently have an
implementation that sits in between the states that the RFC defines
and this allows the system to remain using the original Linux
behavior, while also forcing especially the disabled state. This can
help on porting applications to Linux.

> 
> Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
> to not allow a user to query the state of a sctp-pf peer address
> when pf_expose is not enabled, as said in section 7.3.
> 
> v1->v2:
>   - Fix a build warning noticed by Nathan Chancellor.
> v2->v3:
>   - set pf_expose to UNUSED by default to keep compatible with old
>     applications.

Hmmm UNUSED can be quite confusing.
What about "UNSET" instead? (though I'm not that happy with UNSET
either, but couldn't come up with a better name)
And make UNSET=0. (first on the enum)

So we have it like:
"If unset, the exposure is done as Linux used to do it, while setting
it to 1 blocks it and 2, enables it, according to the RFC."

Needs a new entry on Documentation/ip-sysctl.txt, btw. We have
pf_enable in there.

...

> @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
>  
>  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
>  					   pinfo.spinfo_assoc_id);
> -	if (!transport)
> -		return -EINVAL;
> +	if (!transport) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (transport->state == SCTP_PF &&
> +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> +		retval = -EACCES;
> +		goto out;
> +	}

As is on v3, this is NOT an UAPI violation. The user has to explicitly
set the system or the socket into the disabled state in order to
trigger this new check.

>  
>  	pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
>  	pinfo.spinfo_state = transport->state;
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 238cf17..5d1ad44 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
>  static int rto_beta_min = 0;
>  static int rto_alpha_max = 1000;
>  static int rto_beta_max = 1000;
> +static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
>  
>  static unsigned long max_autoclose_min = 0;
>  static unsigned long max_autoclose_max =
> @@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec,
>  	},
> +	{
> +		.procname	= "pf_expose",
> +		.data		= &init_net.sctp.pf_expose,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= &pf_expose_max,
> +	},
>  
>  	{ /* sentinel */ }
>  };
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:45PM +0800, Xin Long wrote:
> As said in rfc7829, section 3, point 12:
> 
>   The SCTP stack SHOULD expose the PF state of its destination
>   addresses to the ULP as well as provide the means to notify the
>   ULP of state transitions of its destination addresses from
>   active to PF, and vice versa.  However, it is recommended that
>   an SCTP stack implementing SCTP-PF also allows for the ULP to be
>   kept ignorant of the PF state of its destinations and the
>   associated state transitions, thus allowing for retention of the
>   simpler state transition model of [RFC4960] in the ULP.
> 
> Not only does it allow to expose the PF state to ULP, but also
> allow to ignore sctp-pf to ULP.
> 
> So this patch is to add pf_expose per netns, sock and asoc. And in
> sctp_assoc_control_transport(), ulp_notify will be set to false if
> asoc->expose is not set.
> 
> It also allows a user to change pf_expose per netns by sysctl, and
> pf_expose per sock and asoc will be initialized with it.

I also do see value on this sysctl. We currently have an
implementation that sits in between the states that the RFC defines
and this allows the system to remain using the original Linux
behavior, while also forcing especially the disabled state. This can
help on porting applications to Linux.

> 
> Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
> to not allow a user to query the state of a sctp-pf peer address
> when pf_expose is not enabled, as said in section 7.3.
> 
> v1->v2:
>   - Fix a build warning noticed by Nathan Chancellor.
> v2->v3:
>   - set pf_expose to UNUSED by default to keep compatible with old
>     applications.

Hmmm UNUSED can be quite confusing.
What about "UNSET" instead? (though I'm not that happy with UNSET
either, but couldn't come up with a better name)
And make UNSET=0. (first on the enum)

So we have it like:
"If unset, the exposure is done as Linux used to do it, while setting
it to 1 blocks it and 2, enables it, according to the RFC."

Needs a new entry on Documentation/ip-sysctl.txt, btw. We have
pf_enable in there.

...

> @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
>  
>  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
>  					   pinfo.spinfo_assoc_id);
> -	if (!transport)
> -		return -EINVAL;
> +	if (!transport) {
> +		retval = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (transport->state = SCTP_PF &&
> +	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> +		retval = -EACCES;
> +		goto out;
> +	}

As is on v3, this is NOT an UAPI violation. The user has to explicitly
set the system or the socket into the disabled state in order to
trigger this new check.

>  
>  	pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
>  	pinfo.spinfo_state = transport->state;
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 238cf17..5d1ad44 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
>  static int rto_beta_min = 0;
>  static int rto_alpha_max = 1000;
>  static int rto_beta_max = 1000;
> +static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
>  
>  static unsigned long max_autoclose_min = 0;
>  static unsigned long max_autoclose_max > @@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec,
>  	},
> +	{
> +		.procname	= "pf_expose",
> +		.data		= &init_net.sctp.pf_expose,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= &pf_expose_max,
> +	},
>  
>  	{ /* sentinel */ }
>  };
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:46PM +0800, Xin Long wrote:
> This is a sockopt defined in section 7.3 of rfc7829: "Exposing
> the Potentially Failed Path State", by which users can change
> pf_expose per sock and asoc.
> 
> v1->v2:
>   - no change.
> v2->v3:
>   - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
>   - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.

Hm, why again? Please add it to the changelog, as it's an exception on
the list below.

> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index d99b428..a190e4a 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_ASCONF_SUPPORTED	128
>  #define SCTP_AUTH_SUPPORTED	129
>  #define SCTP_ECN_SUPPORTED	130
> +#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE	131
> +#define SCTP_EXPOSE_PF_STATE	SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000

Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:46PM +0800, Xin Long wrote:
> This is a sockopt defined in section 7.3 of rfc7829: "Exposing
> the Potentially Failed Path State", by which users can change
> pf_expose per sock and asoc.
> 
> v1->v2:
>   - no change.
> v2->v3:
>   - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
>   - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.

Hm, why again? Please add it to the changelog, as it's an exception on
the list below.

> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/uapi/linux/sctp.h |  2 ++
>  net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> index d99b428..a190e4a 100644
> --- a/include/uapi/linux/sctp.h
> +++ b/include/uapi/linux/sctp.h
> @@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
>  #define SCTP_ASCONF_SUPPORTED	128
>  #define SCTP_AUTH_SUPPORTED	129
>  #define SCTP_ECN_SUPPORTED	130
> +#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE	131
> +#define SCTP_EXPOSE_PF_STATE	SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
>  
>  /* PR-SCTP policies */
>  #define SCTP_PR_SCTP_NONE	0x0000

Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:47PM +0800, Xin Long wrote:
> This is a new feature defined in section 5 of rfc7829: "Primary Path
> Switchover". By introducing a new tunable parameter:
> 
>   Primary.Switchover.Max.Retrans (PSMR)
> 
> The primary path will be changed to another active path when the path
> error counter on the old primary path exceeds PSMR, so that "the SCTP
> sender is allowed to continue data transmission on a new working path
> even when the old primary destination address becomes active again".
> 
> This patch is to add this tunable parameter, 'ps_retrans' per netns,
> sock, asoc and transport. It also allows a user to change ps_retrans
> per netns by sysctl, and ps_retrans per sock/asoc/transport will be
> initialized with it.
> 
> The check will be done in sctp_do_8_2_transport_strike() when this
> feature is enabled.
> 
> Note this feature is disabled by initializing 'ps_retrans' per netns
> as 0xffff by default, and its value can't be less than 'pf_retrans'
> when changing by sysctl.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/netns/sctp.h   |  6 ++++++
>  include/net/sctp/structs.h | 11 ++++++++---
>  net/sctp/associola.c       |  3 +++
>  net/sctp/protocol.c        |  3 +++
>  net/sctp/sm_sideeffect.c   |  5 +++++
>  net/sctp/socket.c          |  1 +
>  net/sctp/sysctl.c          |  9 +++++++++
>  7 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> index f97d342..c41ffdf 100644
> --- a/include/net/netns/sctp.h
> +++ b/include/net/netns/sctp.h
> @@ -89,6 +89,12 @@ struct netns_sctp {
>  	 */
>  	int pf_retrans;
>  
> +	/* Primary.Switchover.Max.Retrans sysctl value
> +	 * taken from:
> +	 * https://tools.ietf.org/html/rfc7829
> +	 */
> +	int ps_retrans;
> +
>  	/*
>  	 * Disable Potentially-Failed feature, the feature is enabled by default
>  	 * pf_enable	-  0  : disable pf
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 9a43738..3cc913f 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -184,7 +184,8 @@ struct sctp_sock {
>  	__u32 flowlabel;
>  	__u8  dscp;
>  
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	__u16 ps_retrans;
>  
>  	/* The initial Path MTU to use for new associations. */
>  	__u32 pathmtu;
> @@ -897,7 +898,9 @@ struct sctp_transport {
>  	 * and will be initialized from the assocs value.  This can be changed
>  	 * using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  	/* PMTU	      : The current known path MTU.  */
>  	__u32 pathmtu;
>  
> @@ -1773,7 +1776,9 @@ struct sctp_association {
>  	 * and will be initialized from the assocs value.  This can be
>  	 * changed using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  
>  	/* Maximum number of times the endpoint will retransmit INIT  */
>  	__u16 max_init_attempts;
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 46763c5..a839244 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
>  	 */
>  	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
>  	asoc->pf_retrans  = sp->pf_retrans;
> +	asoc->ps_retrans  = sp->ps_retrans;
>  	asoc->pf_expose   = sp->pf_expose;
>  
>  	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
> @@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
>  
>  	/* And the partial failure retrans threshold */
>  	peer->pf_retrans = asoc->pf_retrans;
> +	/* And the primary path switchover retrans threshold */
> +	peer->ps_retrans = asoc->ps_retrans;
>  
>  	/* Initialize the peer's SACK delay timeout based on the
>  	 * association configured value.
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index a18c7c4..ea1de9b 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
>  	/* Max.Burst		    - 4 */
>  	net->sctp.max_burst			= SCTP_DEFAULT_MAX_BURST;
>  
> +	/* Disable of Primary Path Switchover by default */
> +	net->sctp.ps_retrans = 0xffff;
> +
>  	/* Enable pf state by default */
>  	net->sctp.pf_enable = 1;
>  
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index e52b212..acd737d 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
>  					     SCTP_FAILED_THRESHOLD);
>  	}
>  
> +	if (transport->error_count > transport->ps_retrans &&
> +	    asoc->peer.primary_path == transport &&
> +	    asoc->peer.active_path != transport)
> +		sctp_assoc_set_primary(asoc, asoc->peer.active_path);
> +
>  	/* E2) For the destination address for which the timer
>  	 * expires, set RTO <- RTO * 2 ("back off the timer").  The
>  	 * maximum value discussed in rule C7 above (RTO.max) may be
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index eccd689..38d102b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
>  	sp->hbinterval  = net->sctp.hb_interval;
>  	sp->pathmaxrxt  = net->sctp.max_retrans_path;
>  	sp->pf_retrans  = net->sctp.pf_retrans;
> +	sp->ps_retrans  = net->sctp.ps_retrans;
>  	sp->pf_expose   = net->sctp.pf_expose;
>  	sp->pathmtu     = 0; /* allow default discovery */
>  	sp->sackdelay   = net->sctp.sack_timeout;
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 5d1ad44..adf264a 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= SYSCTL_ZERO,
> +		.extra2		= &init_net.sctp.ps_retrans,
> +	},
> +	{
> +		.procname	= "ps_retrans",
> +		.data		= &init_net.sctp.ps_retrans,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &init_net.sctp.pf_retrans,
>  		.extra2		= SYSCTL_INT_MAX,

pf_retrans got shrunk to 16 bits on other structs (why? I'm thinking
it was just to keep the structs size in check). INT_MAX doesn't make
sense anymore then and in fact it can lead to unexpected behavior.
Like, setting 0x10000 becoming effectively 0.
(yes, usage of >16bits values are probably unreasoable, but..)

>  	},
>  	{
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Marcelo Ricardo Leitner]

On Mon, Oct 14, 2019 at 02:14:47PM +0800, Xin Long wrote:
> This is a new feature defined in section 5 of rfc7829: "Primary Path
> Switchover". By introducing a new tunable parameter:
> 
>   Primary.Switchover.Max.Retrans (PSMR)
> 
> The primary path will be changed to another active path when the path
> error counter on the old primary path exceeds PSMR, so that "the SCTP
> sender is allowed to continue data transmission on a new working path
> even when the old primary destination address becomes active again".
> 
> This patch is to add this tunable parameter, 'ps_retrans' per netns,
> sock, asoc and transport. It also allows a user to change ps_retrans
> per netns by sysctl, and ps_retrans per sock/asoc/transport will be
> initialized with it.
> 
> The check will be done in sctp_do_8_2_transport_strike() when this
> feature is enabled.
> 
> Note this feature is disabled by initializing 'ps_retrans' per netns
> as 0xffff by default, and its value can't be less than 'pf_retrans'
> when changing by sysctl.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  include/net/netns/sctp.h   |  6 ++++++
>  include/net/sctp/structs.h | 11 ++++++++---
>  net/sctp/associola.c       |  3 +++
>  net/sctp/protocol.c        |  3 +++
>  net/sctp/sm_sideeffect.c   |  5 +++++
>  net/sctp/socket.c          |  1 +
>  net/sctp/sysctl.c          |  9 +++++++++
>  7 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> index f97d342..c41ffdf 100644
> --- a/include/net/netns/sctp.h
> +++ b/include/net/netns/sctp.h
> @@ -89,6 +89,12 @@ struct netns_sctp {
>  	 */
>  	int pf_retrans;
>  
> +	/* Primary.Switchover.Max.Retrans sysctl value
> +	 * taken from:
> +	 * https://tools.ietf.org/html/rfc7829
> +	 */
> +	int ps_retrans;
> +
>  	/*
>  	 * Disable Potentially-Failed feature, the feature is enabled by default
>  	 * pf_enable	-  0  : disable pf
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 9a43738..3cc913f 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -184,7 +184,8 @@ struct sctp_sock {
>  	__u32 flowlabel;
>  	__u8  dscp;
>  
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	__u16 ps_retrans;
>  
>  	/* The initial Path MTU to use for new associations. */
>  	__u32 pathmtu;
> @@ -897,7 +898,9 @@ struct sctp_transport {
>  	 * and will be initialized from the assocs value.  This can be changed
>  	 * using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  	/* PMTU	      : The current known path MTU.  */
>  	__u32 pathmtu;
>  
> @@ -1773,7 +1776,9 @@ struct sctp_association {
>  	 * and will be initialized from the assocs value.  This can be
>  	 * changed using the SCTP_PEER_ADDR_THLDS socket option
>  	 */
> -	int pf_retrans;
> +	__u16 pf_retrans;
> +	/* Used for primary path switchover. */
> +	__u16 ps_retrans;
>  
>  	/* Maximum number of times the endpoint will retransmit INIT  */
>  	__u16 max_init_attempts;
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 46763c5..a839244 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
>  	 */
>  	asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
>  	asoc->pf_retrans  = sp->pf_retrans;
> +	asoc->ps_retrans  = sp->ps_retrans;
>  	asoc->pf_expose   = sp->pf_expose;
>  
>  	asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
> @@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
>  
>  	/* And the partial failure retrans threshold */
>  	peer->pf_retrans = asoc->pf_retrans;
> +	/* And the primary path switchover retrans threshold */
> +	peer->ps_retrans = asoc->ps_retrans;
>  
>  	/* Initialize the peer's SACK delay timeout based on the
>  	 * association configured value.
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index a18c7c4..ea1de9b 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
>  	/* Max.Burst		    - 4 */
>  	net->sctp.max_burst			= SCTP_DEFAULT_MAX_BURST;
>  
> +	/* Disable of Primary Path Switchover by default */
> +	net->sctp.ps_retrans = 0xffff;
> +
>  	/* Enable pf state by default */
>  	net->sctp.pf_enable = 1;
>  
> diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> index e52b212..acd737d 100644
> --- a/net/sctp/sm_sideeffect.c
> +++ b/net/sctp/sm_sideeffect.c
> @@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
>  					     SCTP_FAILED_THRESHOLD);
>  	}
>  
> +	if (transport->error_count > transport->ps_retrans &&
> +	    asoc->peer.primary_path = transport &&
> +	    asoc->peer.active_path != transport)
> +		sctp_assoc_set_primary(asoc, asoc->peer.active_path);
> +
>  	/* E2) For the destination address for which the timer
>  	 * expires, set RTO <- RTO * 2 ("back off the timer").  The
>  	 * maximum value discussed in rule C7 above (RTO.max) may be
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index eccd689..38d102b 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
>  	sp->hbinterval  = net->sctp.hb_interval;
>  	sp->pathmaxrxt  = net->sctp.max_retrans_path;
>  	sp->pf_retrans  = net->sctp.pf_retrans;
> +	sp->ps_retrans  = net->sctp.ps_retrans;
>  	sp->pf_expose   = net->sctp.pf_expose;
>  	sp->pathmtu     = 0; /* allow default discovery */
>  	sp->sackdelay   = net->sctp.sack_timeout;
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 5d1ad44..adf264a 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= SYSCTL_ZERO,
> +		.extra2		= &init_net.sctp.ps_retrans,
> +	},
> +	{
> +		.procname	= "ps_retrans",
> +		.data		= &init_net.sctp.ps_retrans,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &init_net.sctp.pf_retrans,
>  		.extra2		= SYSCTL_INT_MAX,

pf_retrans got shrunk to 16 bits on other structs (why? I'm thinking
it was just to keep the structs size in check). INT_MAX doesn't make
sense anymore then and in fact it can lead to unexpected behavior.
Like, setting 0x10000 becoming effectively 0.
(yes, usage of >16bits values are probably unreasoable, but..)

>  	},
>  	{
> -- 
> 2.1.0
> 

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote:
> > > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > > >               else
> > > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > > -             /* Don't inform ULP about transition from PF to
> > > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > > -              * Quick failover draft section 5.1, point 5
> > > > -              */
> > > > -             if (transport->state == SCTP_PF) {
> > > > -                     ulp_notify = false;
> > > > -                     transport->cwnd = asoc->pathmtu;
> > > > -             }
> > >
> > > This is wrong.
> > > If the old state is PF and the application hasn't exposed PF the event should be
> > > ignored.
> > yeps, in Patch 2/5:
> > +               if (transport->state == SCTP_PF &&
> > +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> > +                       ulp_notify = false;
> > +               else if (transport->state == SCTP_UNCONFIRMED &&
> > +                        error == SCTP_HEARTBEAT_SUCCESS)
> >                         spc_state = SCTP_ADDR_CONFIRMED;
> >                 else
> >                         spc_state = SCTP_ADDR_AVAILABLE;
>
> Right, yet for one bisecting the kernel, a checkout on this patch will
> see this change regardless of patch 2. Patches 1 and 2 could be
> swapped to avoid this situation.
>
> >
> > >
> > > >               transport->state = SCTP_ACTIVE;
> > > >               break;
> > > >
> > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > > >                * to inactive state.  Also, release the cached route since
> > > >                * there may be a better route next time.
> > > >                */
> > > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > > >                       transport->state = SCTP_INACTIVE;
> > > > -             else {
> > > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > > +             } else {
> > > >                       sctp_transport_dst_release(transport);
> > > >                       ulp_notify = false;
> > > >               }
> > > > -
> > > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > > >               break;
> > > >
> > > >       case SCTP_TRANSPORT_PF:
> > > >               transport->state = SCTP_PF;
> > > > -             ulp_notify = false;
> > >
> > > Again the event should be supressed if PF isn't exposed.
> > it will be suppressed after Patch 2/5:
> > +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> > +                       ulp_notify = false;
> > +               else
> > +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >                 break;
>
> Same here.
okay, will swap them. thanks.

>
> >
> > >
> > > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > > >               break;
> > > >
> > > >       default:
> > > > --
> > > > 2.1.0
> > >
> > > I also haven't spotted where the test that the application has actually enabled
> > > state transition events is in the code.
> > all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> > to deliver up:
> >
> >         /* Check if the user wishes to receive this event.  */
> >         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
> >                 goto out_free;
> >
> > > I'd have thought it would be anything is built and allocated.
> > >
> > >         David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > >
> >

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sat, Oct 19, 2019 at 04:55:01PM +0800, Xin Long wrote:
> > > > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > > >                       spc_state = SCTP_ADDR_CONFIRMED;
> > > >               else
> > > >                       spc_state = SCTP_ADDR_AVAILABLE;
> > > > -             /* Don't inform ULP about transition from PF to
> > > > -              * active state and set cwnd to 1 MTU, see SCTP
> > > > -              * Quick failover draft section 5.1, point 5
> > > > -              */
> > > > -             if (transport->state = SCTP_PF) {
> > > > -                     ulp_notify = false;
> > > > -                     transport->cwnd = asoc->pathmtu;
> > > > -             }
> > >
> > > This is wrong.
> > > If the old state is PF and the application hasn't exposed PF the event should be
> > > ignored.
> > yeps, in Patch 2/5:
> > +               if (transport->state = SCTP_PF &&
> > +                   asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> > +                       ulp_notify = false;
> > +               else if (transport->state = SCTP_UNCONFIRMED &&
> > +                        error = SCTP_HEARTBEAT_SUCCESS)
> >                         spc_state = SCTP_ADDR_CONFIRMED;
> >                 else
> >                         spc_state = SCTP_ADDR_AVAILABLE;
>
> Right, yet for one bisecting the kernel, a checkout on this patch will
> see this change regardless of patch 2. Patches 1 and 2 could be
> swapped to avoid this situation.
>
> >
> > >
> > > >               transport->state = SCTP_ACTIVE;
> > > >               break;
> > > >
> > > > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> > > >                * to inactive state.  Also, release the cached route since
> > > >                * there may be a better route next time.
> > > >                */
> > > > -             if (transport->state != SCTP_UNCONFIRMED)
> > > > +             if (transport->state != SCTP_UNCONFIRMED) {
> > > >                       transport->state = SCTP_INACTIVE;
> > > > -             else {
> > > > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > > > +             } else {
> > > >                       sctp_transport_dst_release(transport);
> > > >                       ulp_notify = false;
> > > >               }
> > > > -
> > > > -             spc_state = SCTP_ADDR_UNREACHABLE;
> > > >               break;
> > > >
> > > >       case SCTP_TRANSPORT_PF:
> > > >               transport->state = SCTP_PF;
> > > > -             ulp_notify = false;
> > >
> > > Again the event should be supressed if PF isn't exposed.
> > it will be suppressed after Patch 2/5:
> > +               if (asoc->pf_expose != SCTP_PF_EXPOSE_ENABLE)
> > +                       ulp_notify = false;
> > +               else
> > +                       spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >                 break;
>
> Same here.
okay, will swap them. thanks.

>
> >
> > >
> > > > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> > > >               break;
> > > >
> > > >       default:
> > > > --
> > > > 2.1.0
> > >
> > > I also haven't spotted where the test that the application has actually enabled
> > > state transition events is in the code.
> > all events will be created, but dropped in sctp_ulpq_tail_event() when trying
> > to deliver up:
> >
> >         /* Check if the user wishes to receive this event.  */
> >         if (!sctp_ulpevent_is_enabled(event, ulpq->asoc->subscribe))
> >                 goto out_free;
> >
> > > I'd have thought it would be anything is built and allocated.
> > >
> > >         David
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > >
> >

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> Hi,
>
> Sorry for the long delay on this review.
>
> On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote:
> > SCTP Quick failover draft section 5.1, point 5 has been removed
> > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > Layer Protocol (ULP) about this state transition", as said in
> > section 3.2, point 8.
> >
> > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > in section 7.1, "which is reported if the affected address
> > becomes PF". Also remove transport cwnd's update when moving
> > from PF back to ACTIVE , which is no longer in rfc7829 either.
> >
> > v1->v2:
> >   - no change
> > v2->v3:
> >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/associola.c      | 17 ++++-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index 6bce7f9..f4ab7bb 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> >       SCTP_ADDR_ADDED,
> >       SCTP_ADDR_MADE_PRIM,
> >       SCTP_ADDR_CONFIRMED,
> > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >  };
> >
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 1ba893b..4f9efba 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>
> While at here, dealing with spc_state, please seize the moment and
> initialize it to the enum instead:
>
> @@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>                                   sctp_sn_error_t error)
>  {
>         bool ulp_notify = true;
> -       int spc_state = 0;
> +       int spc_state = SCTP_ADDR_AVAILABLE;
>
>
> >                       spc_state = SCTP_ADDR_CONFIRMED;
> >               else
> >                       spc_state = SCTP_ADDR_AVAILABLE;
>
> This else could be removed (equals to initial value).
yes, will improve it.

>
> > -             /* Don't inform ULP about transition from PF to
> > -              * active state and set cwnd to 1 MTU, see SCTP
> > -              * Quick failover draft section 5.1, point 5
> > -              */
> > -             if (transport->state == SCTP_PF) {
> > -                     ulp_notify = false;
> > -                     transport->cwnd = asoc->pathmtu;
> > -             }
> >               transport->state = SCTP_ACTIVE;
> >               break;
> >
> > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                * to inactive state.  Also, release the cached route since
> >                * there may be a better route next time.
> >                */
> > -             if (transport->state != SCTP_UNCONFIRMED)
> > +             if (transport->state != SCTP_UNCONFIRMED) {
> >                       transport->state = SCTP_INACTIVE;
> > -             else {
> > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > +             } else {
> >                       sctp_transport_dst_release(transport);
> >                       ulp_notify = false;
> >               }
> > -
> > -             spc_state = SCTP_ADDR_UNREACHABLE;
> >               break;
> >
> >       case SCTP_TRANSPORT_PF:
> >               transport->state = SCTP_PF;
> > -             ulp_notify = false;
> > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >               break;
> >
> >       default:
> > --
> > 2.1.0
> >

Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification

[Xin Long]

On Fri, Oct 25, 2019 at 11:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> Hi,
>
> Sorry for the long delay on this review.
>
> On Mon, Oct 14, 2019 at 02:14:44PM +0800, Xin Long wrote:
> > SCTP Quick failover draft section 5.1, point 5 has been removed
> > from rfc7829. Instead, "the sender SHOULD (i) notify the Upper
> > Layer Protocol (ULP) about this state transition", as said in
> > section 3.2, point 8.
> >
> > So this patch is to add SCTP_ADDR_POTENTIALLY_FAILED, defined
> > in section 7.1, "which is reported if the affected address
> > becomes PF". Also remove transport cwnd's update when moving
> > from PF back to ACTIVE , which is no longer in rfc7829 either.
> >
> > v1->v2:
> >   - no change
> > v2->v3:
> >   - define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/associola.c      | 17 ++++-------------
> >  2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index 6bce7f9..f4ab7bb 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -410,6 +410,8 @@ enum sctp_spc_state {
> >       SCTP_ADDR_ADDED,
> >       SCTP_ADDR_MADE_PRIM,
> >       SCTP_ADDR_CONFIRMED,
> > +     SCTP_ADDR_POTENTIALLY_FAILED,
> > +#define SCTP_ADDR_PF SCTP_ADDR_POTENTIALLY_FAILED
> >  };
> >
> >
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 1ba893b..4f9efba 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -801,14 +801,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>
> While at here, dealing with spc_state, please seize the moment and
> initialize it to the enum instead:
>
> @@ -787,7 +787,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
>                                   sctp_sn_error_t error)
>  {
>         bool ulp_notify = true;
> -       int spc_state = 0;
> +       int spc_state = SCTP_ADDR_AVAILABLE;
>
>
> >                       spc_state = SCTP_ADDR_CONFIRMED;
> >               else
> >                       spc_state = SCTP_ADDR_AVAILABLE;
>
> This else could be removed (equals to initial value).
yes, will improve it.

>
> > -             /* Don't inform ULP about transition from PF to
> > -              * active state and set cwnd to 1 MTU, see SCTP
> > -              * Quick failover draft section 5.1, point 5
> > -              */
> > -             if (transport->state = SCTP_PF) {
> > -                     ulp_notify = false;
> > -                     transport->cwnd = asoc->pathmtu;
> > -             }
> >               transport->state = SCTP_ACTIVE;
> >               break;
> >
> > @@ -817,19 +809,18 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
> >                * to inactive state.  Also, release the cached route since
> >                * there may be a better route next time.
> >                */
> > -             if (transport->state != SCTP_UNCONFIRMED)
> > +             if (transport->state != SCTP_UNCONFIRMED) {
> >                       transport->state = SCTP_INACTIVE;
> > -             else {
> > +                     spc_state = SCTP_ADDR_UNREACHABLE;
> > +             } else {
> >                       sctp_transport_dst_release(transport);
> >                       ulp_notify = false;
> >               }
> > -
> > -             spc_state = SCTP_ADDR_UNREACHABLE;
> >               break;
> >
> >       case SCTP_TRANSPORT_PF:
> >               transport->state = SCTP_PF;
> > -             ulp_notify = false;
> > +             spc_state = SCTP_ADDR_POTENTIALLY_FAILED;
> >               break;
> >
> >       default:
> > --
> > 2.1.0
> >

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Xin Long]

On Fri, Oct 25, 2019 at 11:23 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:45PM +0800, Xin Long wrote:
> > As said in rfc7829, section 3, point 12:
> >
> >   The SCTP stack SHOULD expose the PF state of its destination
> >   addresses to the ULP as well as provide the means to notify the
> >   ULP of state transitions of its destination addresses from
> >   active to PF, and vice versa.  However, it is recommended that
> >   an SCTP stack implementing SCTP-PF also allows for the ULP to be
> >   kept ignorant of the PF state of its destinations and the
> >   associated state transitions, thus allowing for retention of the
> >   simpler state transition model of [RFC4960] in the ULP.
> >
> > Not only does it allow to expose the PF state to ULP, but also
> > allow to ignore sctp-pf to ULP.
> >
> > So this patch is to add pf_expose per netns, sock and asoc. And in
> > sctp_assoc_control_transport(), ulp_notify will be set to false if
> > asoc->expose is not set.
> >
> > It also allows a user to change pf_expose per netns by sysctl, and
> > pf_expose per sock and asoc will be initialized with it.
>
> I also do see value on this sysctl. We currently have an
> implementation that sits in between the states that the RFC defines
> and this allows the system to remain using the original Linux
> behavior, while also forcing especially the disabled state. This can
> help on porting applications to Linux.
Agreed.

>
> >
> > Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
> > to not allow a user to query the state of a sctp-pf peer address
> > when pf_expose is not enabled, as said in section 7.3.
> >
> > v1->v2:
> >   - Fix a build warning noticed by Nathan Chancellor.
> > v2->v3:
> >   - set pf_expose to UNUSED by default to keep compatible with old
> >     applications.
>
> Hmmm UNUSED can be quite confusing.
> What about "UNSET" instead? (though I'm not that happy with UNSET
> either, but couldn't come up with a better name)
> And make UNSET=0. (first on the enum)
>
> So we have it like:
> "If unset, the exposure is done as Linux used to do it, while setting
> it to 1 blocks it and 2, enables it, according to the RFC."
>
> Needs a new entry on Documentation/ip-sysctl.txt, btw. We have
> pf_enable in there.
will add it meanwhile. Thanks.

>
> ...
>
> > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> >
> >       transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> >                                          pinfo.spinfo_assoc_id);
> > -     if (!transport)
> > -             return -EINVAL;
> > +     if (!transport) {
> > +             retval = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     if (transport->state == SCTP_PF &&
> > +         transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > +             retval = -EACCES;
> > +             goto out;
> > +     }
>
> As is on v3, this is NOT an UAPI violation. The user has to explicitly
> set the system or the socket into the disabled state in order to
> trigger this new check.
Agreed.

>
> >
> >       pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
> >       pinfo.spinfo_state = transport->state;
> > diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> > index 238cf17..5d1ad44 100644
> > --- a/net/sctp/sysctl.c
> > +++ b/net/sctp/sysctl.c
> > @@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
> >  static int rto_beta_min = 0;
> >  static int rto_alpha_max = 1000;
> >  static int rto_beta_max = 1000;
> > +static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
> >
> >  static unsigned long max_autoclose_min = 0;
> >  static unsigned long max_autoclose_max =
> > @@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
> >               .mode           = 0644,
> >               .proc_handler   = proc_dointvec,
> >       },
> > +     {
> > +             .procname       = "pf_expose",
> > +             .data           = &init_net.sctp.pf_expose,
> > +             .maxlen         = sizeof(int),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = SYSCTL_ZERO,
> > +             .extra2         = &pf_expose_max,
> > +     },
> >
> >       { /* sentinel */ }
> >  };
> > --
> > 2.1.0
> >

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[Xin Long]

On Fri, Oct 25, 2019 at 11:23 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:45PM +0800, Xin Long wrote:
> > As said in rfc7829, section 3, point 12:
> >
> >   The SCTP stack SHOULD expose the PF state of its destination
> >   addresses to the ULP as well as provide the means to notify the
> >   ULP of state transitions of its destination addresses from
> >   active to PF, and vice versa.  However, it is recommended that
> >   an SCTP stack implementing SCTP-PF also allows for the ULP to be
> >   kept ignorant of the PF state of its destinations and the
> >   associated state transitions, thus allowing for retention of the
> >   simpler state transition model of [RFC4960] in the ULP.
> >
> > Not only does it allow to expose the PF state to ULP, but also
> > allow to ignore sctp-pf to ULP.
> >
> > So this patch is to add pf_expose per netns, sock and asoc. And in
> > sctp_assoc_control_transport(), ulp_notify will be set to false if
> > asoc->expose is not set.
> >
> > It also allows a user to change pf_expose per netns by sysctl, and
> > pf_expose per sock and asoc will be initialized with it.
>
> I also do see value on this sysctl. We currently have an
> implementation that sits in between the states that the RFC defines
> and this allows the system to remain using the original Linux
> behavior, while also forcing especially the disabled state. This can
> help on porting applications to Linux.
Agreed.

>
> >
> > Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
> > to not allow a user to query the state of a sctp-pf peer address
> > when pf_expose is not enabled, as said in section 7.3.
> >
> > v1->v2:
> >   - Fix a build warning noticed by Nathan Chancellor.
> > v2->v3:
> >   - set pf_expose to UNUSED by default to keep compatible with old
> >     applications.
>
> Hmmm UNUSED can be quite confusing.
> What about "UNSET" instead? (though I'm not that happy with UNSET
> either, but couldn't come up with a better name)
> And make UNSET=0. (first on the enum)
>
> So we have it like:
> "If unset, the exposure is done as Linux used to do it, while setting
> it to 1 blocks it and 2, enables it, according to the RFC."
>
> Needs a new entry on Documentation/ip-sysctl.txt, btw. We have
> pf_enable in there.
will add it meanwhile. Thanks.

>
> ...
>
> > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> >
> >       transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> >                                          pinfo.spinfo_assoc_id);
> > -     if (!transport)
> > -             return -EINVAL;
> > +     if (!transport) {
> > +             retval = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     if (transport->state = SCTP_PF &&
> > +         transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> > +             retval = -EACCES;
> > +             goto out;
> > +     }
>
> As is on v3, this is NOT an UAPI violation. The user has to explicitly
> set the system or the socket into the disabled state in order to
> trigger this new check.
Agreed.

>
> >
> >       pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc);
> >       pinfo.spinfo_state = transport->state;
> > diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> > index 238cf17..5d1ad44 100644
> > --- a/net/sctp/sysctl.c
> > +++ b/net/sctp/sysctl.c
> > @@ -34,6 +34,7 @@ static int rto_alpha_min = 0;
> >  static int rto_beta_min = 0;
> >  static int rto_alpha_max = 1000;
> >  static int rto_beta_max = 1000;
> > +static int pf_expose_max = SCTP_PF_EXPOSE_MAX;
> >
> >  static unsigned long max_autoclose_min = 0;
> >  static unsigned long max_autoclose_max > > @@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = {
> >               .mode           = 0644,
> >               .proc_handler   = proc_dointvec,
> >       },
> > +     {
> > +             .procname       = "pf_expose",
> > +             .data           = &init_net.sctp.pf_expose,
> > +             .maxlen         = sizeof(int),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = SYSCTL_ZERO,
> > +             .extra2         = &pf_expose_max,
> > +     },
> >
> >       { /* sentinel */ }
> >  };
> > --
> > 2.1.0
> >

Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Xin Long]

On Fri, Oct 25, 2019 at 11:24 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:46PM +0800, Xin Long wrote:
> > This is a sockopt defined in section 7.3 of rfc7829: "Exposing
> > the Potentially Failed Path State", by which users can change
> > pf_expose per sock and asoc.
> >
> > v1->v2:
> >   - no change.
> > v2->v3:
> >   - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
> >   - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.
>
> Hm, why again? Please add it to the changelog, as it's an exception on
> the list below.
will do. thanks.

>
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 81 insertions(+)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index d99b428..a190e4a 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
> >  #define SCTP_ASCONF_SUPPORTED        128
> >  #define SCTP_AUTH_SUPPORTED  129
> >  #define SCTP_ECN_SUPPORTED   130
> > +#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE 131
> > +#define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
> >
> >  /* PR-SCTP policies */
> >  #define SCTP_PR_SCTP_NONE    0x0000

Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt

[Xin Long]

On Fri, Oct 25, 2019 at 11:24 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:46PM +0800, Xin Long wrote:
> > This is a sockopt defined in section 7.3 of rfc7829: "Exposing
> > the Potentially Failed Path State", by which users can change
> > pf_expose per sock and asoc.
> >
> > v1->v2:
> >   - no change.
> > v2->v3:
> >   - return -EINVAL if params.assoc_value > SCTP_PF_EXPOSE_MAX.
> >   - define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE.
>
> Hm, why again? Please add it to the changelog, as it's an exception on
> the list below.
will do. thanks.

>
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/uapi/linux/sctp.h |  2 ++
> >  net/sctp/socket.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 81 insertions(+)
> >
> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h
> > index d99b428..a190e4a 100644
> > --- a/include/uapi/linux/sctp.h
> > +++ b/include/uapi/linux/sctp.h
> > @@ -137,6 +137,8 @@ typedef __s32 sctp_assoc_t;
> >  #define SCTP_ASCONF_SUPPORTED        128
> >  #define SCTP_AUTH_SUPPORTED  129
> >  #define SCTP_ECN_SUPPORTED   130
> > +#define SCTP_EXPOSE_POTENTIALLY_FAILED_STATE 131
> > +#define SCTP_EXPOSE_PF_STATE SCTP_EXPOSE_POTENTIALLY_FAILED_STATE
> >
> >  /* PR-SCTP policies */
> >  #define SCTP_PR_SCTP_NONE    0x0000

Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Xin Long]

On Fri, Oct 25, 2019 at 11:25 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:47PM +0800, Xin Long wrote:
> > This is a new feature defined in section 5 of rfc7829: "Primary Path
> > Switchover". By introducing a new tunable parameter:
> >
> >   Primary.Switchover.Max.Retrans (PSMR)
> >
> > The primary path will be changed to another active path when the path
> > error counter on the old primary path exceeds PSMR, so that "the SCTP
> > sender is allowed to continue data transmission on a new working path
> > even when the old primary destination address becomes active again".
> >
> > This patch is to add this tunable parameter, 'ps_retrans' per netns,
> > sock, asoc and transport. It also allows a user to change ps_retrans
> > per netns by sysctl, and ps_retrans per sock/asoc/transport will be
> > initialized with it.
> >
> > The check will be done in sctp_do_8_2_transport_strike() when this
> > feature is enabled.
> >
> > Note this feature is disabled by initializing 'ps_retrans' per netns
> > as 0xffff by default, and its value can't be less than 'pf_retrans'
> > when changing by sysctl.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/netns/sctp.h   |  6 ++++++
> >  include/net/sctp/structs.h | 11 ++++++++---
> >  net/sctp/associola.c       |  3 +++
> >  net/sctp/protocol.c        |  3 +++
> >  net/sctp/sm_sideeffect.c   |  5 +++++
> >  net/sctp/socket.c          |  1 +
> >  net/sctp/sysctl.c          |  9 +++++++++
> >  7 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> > index f97d342..c41ffdf 100644
> > --- a/include/net/netns/sctp.h
> > +++ b/include/net/netns/sctp.h
> > @@ -89,6 +89,12 @@ struct netns_sctp {
> >        */
> >       int pf_retrans;
> >
> > +     /* Primary.Switchover.Max.Retrans sysctl value
> > +      * taken from:
> > +      * https://tools.ietf.org/html/rfc7829
> > +      */
> > +     int ps_retrans;
> > +
> >       /*
> >        * Disable Potentially-Failed feature, the feature is enabled by default
> >        * pf_enable    -  0  : disable pf
> > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> > index 9a43738..3cc913f 100644
> > --- a/include/net/sctp/structs.h
> > +++ b/include/net/sctp/structs.h
> > @@ -184,7 +184,8 @@ struct sctp_sock {
> >       __u32 flowlabel;
> >       __u8  dscp;
> >
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     __u16 ps_retrans;
> >
> >       /* The initial Path MTU to use for new associations. */
> >       __u32 pathmtu;
> > @@ -897,7 +898,9 @@ struct sctp_transport {
> >        * and will be initialized from the assocs value.  This can be changed
> >        * using the SCTP_PEER_ADDR_THLDS socket option
> >        */
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     /* Used for primary path switchover. */
> > +     __u16 ps_retrans;
> >       /* PMTU       : The current known path MTU.  */
> >       __u32 pathmtu;
> >
> > @@ -1773,7 +1776,9 @@ struct sctp_association {
> >        * and will be initialized from the assocs value.  This can be
> >        * changed using the SCTP_PEER_ADDR_THLDS socket option
> >        */
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     /* Used for primary path switchover. */
> > +     __u16 ps_retrans;
> >
> >       /* Maximum number of times the endpoint will retransmit INIT  */
> >       __u16 max_init_attempts;
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 46763c5..a839244 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
> >        */
> >       asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
> >       asoc->pf_retrans  = sp->pf_retrans;
> > +     asoc->ps_retrans  = sp->ps_retrans;
> >       asoc->pf_expose   = sp->pf_expose;
> >
> >       asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
> > @@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
> >
> >       /* And the partial failure retrans threshold */
> >       peer->pf_retrans = asoc->pf_retrans;
> > +     /* And the primary path switchover retrans threshold */
> > +     peer->ps_retrans = asoc->ps_retrans;
> >
> >       /* Initialize the peer's SACK delay timeout based on the
> >        * association configured value.
> > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> > index a18c7c4..ea1de9b 100644
> > --- a/net/sctp/protocol.c
> > +++ b/net/sctp/protocol.c
> > @@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
> >       /* Max.Burst                - 4 */
> >       net->sctp.max_burst                     = SCTP_DEFAULT_MAX_BURST;
> >
> > +     /* Disable of Primary Path Switchover by default */
> > +     net->sctp.ps_retrans = 0xffff;
> > +
> >       /* Enable pf state by default */
> >       net->sctp.pf_enable = 1;
> >
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index e52b212..acd737d 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
> >                                            SCTP_FAILED_THRESHOLD);
> >       }
> >
> > +     if (transport->error_count > transport->ps_retrans &&
> > +         asoc->peer.primary_path == transport &&
> > +         asoc->peer.active_path != transport)
> > +             sctp_assoc_set_primary(asoc, asoc->peer.active_path);
> > +
> >       /* E2) For the destination address for which the timer
> >        * expires, set RTO <- RTO * 2 ("back off the timer").  The
> >        * maximum value discussed in rule C7 above (RTO.max) may be
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index eccd689..38d102b 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
> >       sp->hbinterval  = net->sctp.hb_interval;
> >       sp->pathmaxrxt  = net->sctp.max_retrans_path;
> >       sp->pf_retrans  = net->sctp.pf_retrans;
> > +     sp->ps_retrans  = net->sctp.ps_retrans;
> >       sp->pf_expose   = net->sctp.pf_expose;
> >       sp->pathmtu     = 0; /* allow default discovery */
> >       sp->sackdelay   = net->sctp.sack_timeout;
> > diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> > index 5d1ad44..adf264a 100644
> > --- a/net/sctp/sysctl.c
> > +++ b/net/sctp/sysctl.c
> > @@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
> >               .mode           = 0644,
> >               .proc_handler   = proc_dointvec_minmax,
> >               .extra1         = SYSCTL_ZERO,
> > +             .extra2         = &init_net.sctp.ps_retrans,
> > +     },
> > +     {
> > +             .procname       = "ps_retrans",
> > +             .data           = &init_net.sctp.ps_retrans,
> > +             .maxlen         = sizeof(int),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = &init_net.sctp.pf_retrans,
> >               .extra2         = SYSCTL_INT_MAX,
>
> pf_retrans got shrunk to 16 bits on other structs (why? I'm thinking
> it was just to keep the structs size in check). INT_MAX doesn't make
> sense anymore then and in fact it can lead to unexpected behavior.
> Like, setting 0x10000 becoming effectively 0.
> (yes, usage of >16bits values are probably unreasoable, but..)
I'll add "#define SCTP_PS_RETRANS_MAX 0xffff", which will be used
by both initializaion and sysctl extra2.

>
> >       },
> >       {
> > --
> > 2.1.0
> >

Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover

[Xin Long]

On Fri, Oct 25, 2019 at 11:25 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Mon, Oct 14, 2019 at 02:14:47PM +0800, Xin Long wrote:
> > This is a new feature defined in section 5 of rfc7829: "Primary Path
> > Switchover". By introducing a new tunable parameter:
> >
> >   Primary.Switchover.Max.Retrans (PSMR)
> >
> > The primary path will be changed to another active path when the path
> > error counter on the old primary path exceeds PSMR, so that "the SCTP
> > sender is allowed to continue data transmission on a new working path
> > even when the old primary destination address becomes active again".
> >
> > This patch is to add this tunable parameter, 'ps_retrans' per netns,
> > sock, asoc and transport. It also allows a user to change ps_retrans
> > per netns by sysctl, and ps_retrans per sock/asoc/transport will be
> > initialized with it.
> >
> > The check will be done in sctp_do_8_2_transport_strike() when this
> > feature is enabled.
> >
> > Note this feature is disabled by initializing 'ps_retrans' per netns
> > as 0xffff by default, and its value can't be less than 'pf_retrans'
> > when changing by sysctl.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  include/net/netns/sctp.h   |  6 ++++++
> >  include/net/sctp/structs.h | 11 ++++++++---
> >  net/sctp/associola.c       |  3 +++
> >  net/sctp/protocol.c        |  3 +++
> >  net/sctp/sm_sideeffect.c   |  5 +++++
> >  net/sctp/socket.c          |  1 +
> >  net/sctp/sysctl.c          |  9 +++++++++
> >  7 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> > index f97d342..c41ffdf 100644
> > --- a/include/net/netns/sctp.h
> > +++ b/include/net/netns/sctp.h
> > @@ -89,6 +89,12 @@ struct netns_sctp {
> >        */
> >       int pf_retrans;
> >
> > +     /* Primary.Switchover.Max.Retrans sysctl value
> > +      * taken from:
> > +      * https://tools.ietf.org/html/rfc7829
> > +      */
> > +     int ps_retrans;
> > +
> >       /*
> >        * Disable Potentially-Failed feature, the feature is enabled by default
> >        * pf_enable    -  0  : disable pf
> > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> > index 9a43738..3cc913f 100644
> > --- a/include/net/sctp/structs.h
> > +++ b/include/net/sctp/structs.h
> > @@ -184,7 +184,8 @@ struct sctp_sock {
> >       __u32 flowlabel;
> >       __u8  dscp;
> >
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     __u16 ps_retrans;
> >
> >       /* The initial Path MTU to use for new associations. */
> >       __u32 pathmtu;
> > @@ -897,7 +898,9 @@ struct sctp_transport {
> >        * and will be initialized from the assocs value.  This can be changed
> >        * using the SCTP_PEER_ADDR_THLDS socket option
> >        */
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     /* Used for primary path switchover. */
> > +     __u16 ps_retrans;
> >       /* PMTU       : The current known path MTU.  */
> >       __u32 pathmtu;
> >
> > @@ -1773,7 +1776,9 @@ struct sctp_association {
> >        * and will be initialized from the assocs value.  This can be
> >        * changed using the SCTP_PEER_ADDR_THLDS socket option
> >        */
> > -     int pf_retrans;
> > +     __u16 pf_retrans;
> > +     /* Used for primary path switchover. */
> > +     __u16 ps_retrans;
> >
> >       /* Maximum number of times the endpoint will retransmit INIT  */
> >       __u16 max_init_attempts;
> > diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> > index 46763c5..a839244 100644
> > --- a/net/sctp/associola.c
> > +++ b/net/sctp/associola.c
> > @@ -86,6 +86,7 @@ static struct sctp_association *sctp_association_init(
> >        */
> >       asoc->max_retrans = sp->assocparams.sasoc_asocmaxrxt;
> >       asoc->pf_retrans  = sp->pf_retrans;
> > +     asoc->ps_retrans  = sp->ps_retrans;
> >       asoc->pf_expose   = sp->pf_expose;
> >
> >       asoc->rto_initial = msecs_to_jiffies(sp->rtoinfo.srto_initial);
> > @@ -628,6 +629,8 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
> >
> >       /* And the partial failure retrans threshold */
> >       peer->pf_retrans = asoc->pf_retrans;
> > +     /* And the primary path switchover retrans threshold */
> > +     peer->ps_retrans = asoc->ps_retrans;
> >
> >       /* Initialize the peer's SACK delay timeout based on the
> >        * association configured value.
> > diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> > index a18c7c4..ea1de9b 100644
> > --- a/net/sctp/protocol.c
> > +++ b/net/sctp/protocol.c
> > @@ -1217,6 +1217,9 @@ static int __net_init sctp_defaults_init(struct net *net)
> >       /* Max.Burst                - 4 */
> >       net->sctp.max_burst                     = SCTP_DEFAULT_MAX_BURST;
> >
> > +     /* Disable of Primary Path Switchover by default */
> > +     net->sctp.ps_retrans = 0xffff;
> > +
> >       /* Enable pf state by default */
> >       net->sctp.pf_enable = 1;
> >
> > diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
> > index e52b212..acd737d 100644
> > --- a/net/sctp/sm_sideeffect.c
> > +++ b/net/sctp/sm_sideeffect.c
> > @@ -567,6 +567,11 @@ static void sctp_do_8_2_transport_strike(struct sctp_cmd_seq *commands,
> >                                            SCTP_FAILED_THRESHOLD);
> >       }
> >
> > +     if (transport->error_count > transport->ps_retrans &&
> > +         asoc->peer.primary_path = transport &&
> > +         asoc->peer.active_path != transport)
> > +             sctp_assoc_set_primary(asoc, asoc->peer.active_path);
> > +
> >       /* E2) For the destination address for which the timer
> >        * expires, set RTO <- RTO * 2 ("back off the timer").  The
> >        * maximum value discussed in rule C7 above (RTO.max) may be
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index eccd689..38d102b 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -5078,6 +5078,7 @@ static int sctp_init_sock(struct sock *sk)
> >       sp->hbinterval  = net->sctp.hb_interval;
> >       sp->pathmaxrxt  = net->sctp.max_retrans_path;
> >       sp->pf_retrans  = net->sctp.pf_retrans;
> > +     sp->ps_retrans  = net->sctp.ps_retrans;
> >       sp->pf_expose   = net->sctp.pf_expose;
> >       sp->pathmtu     = 0; /* allow default discovery */
> >       sp->sackdelay   = net->sctp.sack_timeout;
> > diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> > index 5d1ad44..adf264a 100644
> > --- a/net/sctp/sysctl.c
> > +++ b/net/sctp/sysctl.c
> > @@ -213,6 +213,15 @@ static struct ctl_table sctp_net_table[] = {
> >               .mode           = 0644,
> >               .proc_handler   = proc_dointvec_minmax,
> >               .extra1         = SYSCTL_ZERO,
> > +             .extra2         = &init_net.sctp.ps_retrans,
> > +     },
> > +     {
> > +             .procname       = "ps_retrans",
> > +             .data           = &init_net.sctp.ps_retrans,
> > +             .maxlen         = sizeof(int),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_dointvec_minmax,
> > +             .extra1         = &init_net.sctp.pf_retrans,
> >               .extra2         = SYSCTL_INT_MAX,
>
> pf_retrans got shrunk to 16 bits on other structs (why? I'm thinking
> it was just to keep the structs size in check). INT_MAX doesn't make
> sense anymore then and in fact it can lead to unexpected behavior.
> Like, setting 0x10000 becoming effectively 0.
> (yes, usage of >16bits values are probably unreasoable, but..)
I'll add "#define SCTP_PS_RETRANS_MAX 0xffff", which will be used
by both initializaion and sysctl extra2.

>
> >       },
> >       {
> > --
> > 2.1.0
> >

RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 25 October 2019 04:24
...
> > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> >
> >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> >  					   pinfo.spinfo_assoc_id);
> > -	if (!transport)
> > -		return -EINVAL;
> > +	if (!transport) {
> > +		retval = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	if (transport->state == SCTP_PF &&
> > +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > +		retval = -EACCES;
> > +		goto out;
> > +	}
> 
> As is on v3, this is NOT an UAPI violation. The user has to explicitly
> set the system or the socket into the disabled state in order to
> trigger this new check.

Only because the default isn't to be backwards compatible with the
old kernel and old applications.

An old application running on a system that has the protocol parts of
PF enabled mustn't see any PF events, states or obscure error returns.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[David Laight]

From: Marcelo Ricardo Leitner
> Sent: 25 October 2019 04:24
...
> > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> >
> >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> >  					   pinfo.spinfo_assoc_id);
> > -	if (!transport)
> > -		return -EINVAL;
> > +	if (!transport) {
> > +		retval = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	if (transport->state = SCTP_PF &&
> > +	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> > +		retval = -EACCES;
> > +		goto out;
> > +	}
> 
> As is on v3, this is NOT an UAPI violation. The user has to explicitly
> set the system or the socket into the disabled state in order to
> trigger this new check.

Only because the default isn't to be backwards compatible with the
old kernel and old applications.

An old application running on a system that has the protocol parts of
PF enabled mustn't see any PF events, states or obscure error returns.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

['Marcelo Ricardo Leitner']

On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 25 October 2019 04:24
> ...
> > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > >
> > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > >  					   pinfo.spinfo_assoc_id);
> > > -	if (!transport)
> > > -		return -EINVAL;
> > > +	if (!transport) {
> > > +		retval = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (transport->state == SCTP_PF &&
> > > +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > > +		retval = -EACCES;
> > > +		goto out;
> > > +	}
> > 
> > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > set the system or the socket into the disabled state in order to
> > trigger this new check.
> 
> Only because the default isn't to be backwards compatible with the
                           ^^^^^

You meant "is", right? Then we're agreeing.

> old kernel and old applications.
> 
> An old application running on a system that has the protocol parts of
> PF enabled mustn't see any PF events, states or obscure error returns.

Yes. With the patchset, the application will only see the new error
return if it (the app or the sysadmin) explicitly choose to be more
compliant to the RFC. There's no harm in that.

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

['Marcelo Ricardo Leitner']

On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> From: Marcelo Ricardo Leitner
> > Sent: 25 October 2019 04:24
> ...
> > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > >
> > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > >  					   pinfo.spinfo_assoc_id);
> > > -	if (!transport)
> > > -		return -EINVAL;
> > > +	if (!transport) {
> > > +		retval = -EINVAL;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (transport->state = SCTP_PF &&
> > > +	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> > > +		retval = -EACCES;
> > > +		goto out;
> > > +	}
> > 
> > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > set the system or the socket into the disabled state in order to
> > trigger this new check.
> 
> Only because the default isn't to be backwards compatible with the
                           ^^^^^

You meant "is", right? Then we're agreeing.

> old kernel and old applications.
> 
> An old application running on a system that has the protocol parts of
> PF enabled mustn't see any PF events, states or obscure error returns.

Yes. With the patchset, the application will only see the new error
return if it (the app or the sysadmin) explicitly choose to be more
compliant to the RFC. There's no harm in that.

RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[David Laight]

From: 'Marcelo Ricardo Leitner'
> Sent: 25 October 2019 14:22
> On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> > From: Marcelo Ricardo Leitner
> > > Sent: 25 October 2019 04:24
> > ...
> > > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > > >
> > > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > > >  					   pinfo.spinfo_assoc_id);
> > > > -	if (!transport)
> > > > -		return -EINVAL;
> > > > +	if (!transport) {
> > > > +		retval = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (transport->state == SCTP_PF &&
> > > > +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > > > +		retval = -EACCES;
> > > > +		goto out;
> > > > +	}
> > >
> > > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > > set the system or the socket into the disabled state in order to
> > > trigger this new check.
> >
> > Only because the default isn't to be backwards compatible with the
>                            ^^^^^
> 
> You meant "is", right? Then we're agreeing.

No, I meant isn't.
The application must see a backwards compatible interface unless
the application itself requests something different.
The sysadmin can't be allowed to change the API seen by old applications.

AFAICT if the protocol part of PF is enabled (which handles primary path
failure better than the older version) and ' transport->state == SCTP_PF'
is true then an old application binary will  get a completely unexpected -EACCESS
rather than a valid state (out of the old valid states) if it requests 'peer addr_info'.

You cannot assume that just because some sysctl is set (because someone
building a distribution suddenly decided it was a 'good idea') that an
application binary will not fall in a big heap due to an error condition
that couldn't ever happen before.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

[David Laight]

From: 'Marcelo Ricardo Leitner'
> Sent: 25 October 2019 14:22
> On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> > From: Marcelo Ricardo Leitner
> > > Sent: 25 October 2019 04:24
> > ...
> > > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > > >
> > > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > > >  					   pinfo.spinfo_assoc_id);
> > > > -	if (!transport)
> > > > -		return -EINVAL;
> > > > +	if (!transport) {
> > > > +		retval = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (transport->state = SCTP_PF &&
> > > > +	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> > > > +		retval = -EACCES;
> > > > +		goto out;
> > > > +	}
> > >
> > > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > > set the system or the socket into the disabled state in order to
> > > trigger this new check.
> >
> > Only because the default isn't to be backwards compatible with the
>                            ^^^^^
> 
> You meant "is", right? Then we're agreeing.

No, I meant isn't.
The application must see a backwards compatible interface unless
the application itself requests something different.
The sysadmin can't be allowed to change the API seen by old applications.

AFAICT if the protocol part of PF is enabled (which handles primary path
failure better than the older version) and ' transport->state = SCTP_PF'
is true then an old application binary will  get a completely unexpected -EACCESS
rather than a valid state (out of the old valid states) if it requests 'peer addr_info'.

You cannot assume that just because some sysctl is set (because someone
building a distribution suddenly decided it was a 'good idea') that an
application binary will not fall in a big heap due to an error condition
that couldn't ever happen before.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

['Marcelo Ricardo Leitner']

On Fri, Oct 25, 2019 at 02:26:57PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 25 October 2019 14:22
> > On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 25 October 2019 04:24
> > > ...
> > > > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > > > >
> > > > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > > > >  					   pinfo.spinfo_assoc_id);
> > > > > -	if (!transport)
> > > > > -		return -EINVAL;
> > > > > +	if (!transport) {
> > > > > +		retval = -EINVAL;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	if (transport->state == SCTP_PF &&
> > > > > +	    transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
> > > > > +		retval = -EACCES;
> > > > > +		goto out;
> > > > > +	}
> > > >
> > > > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > > > set the system or the socket into the disabled state in order to
> > > > trigger this new check.
> > >
> > > Only because the default isn't to be backwards compatible with the
> >                            ^^^^^
> > 
> > You meant "is", right? Then we're agreeing.
> 
> No, I meant isn't.

Then you missed this detail in the patch. The default here IS to be
backwards compatible.

> The application must see a backwards compatible interface unless
> the application itself requests something different.
> The sysadmin can't be allowed to change the API seen by old applications.

Disagree. Sysadmins should be able to harden their systems as much as
they want/need. Yet, if that causes issues with old applications,
that's on them.

> 
> AFAICT if the protocol part of PF is enabled (which handles primary path
> failure better than the older version) and ' transport->state == SCTP_PF'
> is true then an old application binary will  get a completely unexpected -EACCESS
> rather than a valid state (out of the old valid states) if it requests 'peer addr_info'.
> 
> You cannot assume that just because some sysctl is set (because someone
> building a distribution suddenly decided it was a 'good idea') that an
> application binary will not fall in a big heap due to an error condition
> that couldn't ever happen before.

Agree, but that assumption doesn't have a room here. If the
distribution decided to harden the system, that's on them. Ditto for
many many other decisions, like having SELinux policies to block sshd
to bind only on port 22 and so, or for building the kernel without
SCTP_COOKIE_HMAC_MD5 because they think it's weak, etc.

  Marcelo

Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc

['Marcelo Ricardo Leitner']

On Fri, Oct 25, 2019 at 02:26:57PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 25 October 2019 14:22
> > On Fri, Oct 25, 2019 at 09:00:45AM +0000, David Laight wrote:
> > > From: Marcelo Ricardo Leitner
> > > > Sent: 25 October 2019 04:24
> > > ...
> > > > > @@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len,
> > > > >
> > > > >  	transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address,
> > > > >  					   pinfo.spinfo_assoc_id);
> > > > > -	if (!transport)
> > > > > -		return -EINVAL;
> > > > > +	if (!transport) {
> > > > > +		retval = -EINVAL;
> > > > > +		goto out;
> > > > > +	}
> > > > > +
> > > > > +	if (transport->state = SCTP_PF &&
> > > > > +	    transport->asoc->pf_expose = SCTP_PF_EXPOSE_DISABLE) {
> > > > > +		retval = -EACCES;
> > > > > +		goto out;
> > > > > +	}
> > > >
> > > > As is on v3, this is NOT an UAPI violation. The user has to explicitly
> > > > set the system or the socket into the disabled state in order to
> > > > trigger this new check.
> > >
> > > Only because the default isn't to be backwards compatible with the
> >                            ^^^^^
> > 
> > You meant "is", right? Then we're agreeing.
> 
> No, I meant isn't.

Then you missed this detail in the patch. The default here IS to be
backwards compatible.

> The application must see a backwards compatible interface unless
> the application itself requests something different.
> The sysadmin can't be allowed to change the API seen by old applications.

Disagree. Sysadmins should be able to harden their systems as much as
they want/need. Yet, if that causes issues with old applications,
that's on them.

> 
> AFAICT if the protocol part of PF is enabled (which handles primary path
> failure better than the older version) and ' transport->state = SCTP_PF'
> is true then an old application binary will  get a completely unexpected -EACCESS
> rather than a valid state (out of the old valid states) if it requests 'peer addr_info'.
> 
> You cannot assume that just because some sysctl is set (because someone
> building a distribution suddenly decided it was a 'good idea') that an
> application binary will not fall in a big heap due to an error condition
> that couldn't ever happen before.

Agree, but that assumption doesn't have a room here. If the
distribution decided to harden the system, that's on them. Ditto for
many many other decisions, like having SELinux policies to block sshd
to bind only on port 22 and so, or for building the kernel without
SCTP_COOKIE_HMAC_MD5 because they think it's weak, etc.

  Marcelo