* [PATCHv3 net-next 0/5] sctp: update from rfc7829
@ 2019-10-14 6:14 ` Xin Long
0 siblings, 0 replies; 61+ messages in thread
From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight
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
^ permalink raw reply [flat|nested] 61+ messages in thread* [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-14 6:14 ` Xin Long @ 2019-10-14 6:14 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-14 6:14 ` Xin Long @ 2019-10-14 6:14 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt 2019-10-14 6:14 ` Xin Long @ 2019-10-14 6:14 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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(¶ms, 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(¶ms, 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, ¶ms, 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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(¶ms, 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(¶ms, 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, ¶ms, 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover 2019-10-14 6:14 ` Xin Long @ 2019-10-14 6:14 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 5/5] sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt 2019-10-14 6:14 ` Xin Long @ 2019-10-14 6:14 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* [PATCHv3 net-next 5/5] sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt @ 2019-10-14 6:14 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-14 6:14 UTC (permalink / raw) To: network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem, David Laight 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 ^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover 2019-10-14 6:14 ` Xin Long @ 2019-10-25 3:25 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:25 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover @ 2019-10-25 3:25 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:25 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover 2019-10-25 3:25 ` Marcelo Ricardo Leitner @ 2019-10-25 8:13 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:13 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover @ 2019-10-25 8:13 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:13 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt 2019-10-14 6:14 ` Xin Long @ 2019-10-25 3:24 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:24 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt @ 2019-10-25 3:24 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:24 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt 2019-10-25 3:24 ` Marcelo Ricardo Leitner @ 2019-10-25 8:05 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:05 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt @ 2019-10-25 8:05 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:05 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-14 6:14 ` Xin Long @ 2019-10-25 3:23 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:23 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 3:23 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:23 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-25 3:23 ` Marcelo Ricardo Leitner @ 2019-10-25 8:02 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:02 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 8:02 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 8:02 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-25 3:23 ` Marcelo Ricardo Leitner @ 2019-10-25 9:00 ` David Laight -1 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-25 9:00 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner', Xin Long Cc: network dev, linux-sctp, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 9:00 ` David Laight 0 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-25 9:00 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner', Xin Long Cc: network dev, linux-sctp, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-25 9:00 ` David Laight @ 2019-10-25 13:21 ` 'Marcelo Ricardo Leitner' -1 siblings, 0 replies; 61+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-10-25 13:21 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 13:21 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 61+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-10-25 13:21 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-25 13:21 ` 'Marcelo Ricardo Leitner' @ 2019-10-25 14:26 ` David Laight -1 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-25 14:26 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner' Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 14:26 ` David Laight 0 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-25 14:26 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner' Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-10-25 14:26 ` David Laight @ 2019-10-25 14:45 ` 'Marcelo Ricardo Leitner' -1 siblings, 0 replies; 61+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-10-25 14:45 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-10-25 14:45 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 61+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-10-25 14:45 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-14 6:14 ` Xin Long @ 2019-10-18 15:56 ` David Laight -1 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-18 15:56 UTC (permalink / raw) To: 'Xin Long', network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-18 15:56 ` David Laight 0 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-18 15:56 UTC (permalink / raw) To: 'Xin Long', network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-18 15:56 ` David Laight @ 2019-10-19 8:55 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-19 8:55 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem 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) > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-19 8:55 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-19 8:55 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem 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) > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-19 8:55 ` Xin Long @ 2019-10-22 11:13 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-22 11:13 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem 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) > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-22 11:13 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-22 11:13 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem 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) > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-19 8:55 ` Xin Long @ 2019-10-25 3:22 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:22 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem 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) > > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-25 3:22 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:22 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem 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) > > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-25 3:22 ` Marcelo Ricardo Leitner @ 2019-10-25 7:58 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 7:58 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: David Laight, network dev, linux-sctp, Neil Horman, davem 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) > > > > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-25 7:58 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 7:58 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: David Laight, network dev, linux-sctp, Neil Horman, davem 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) > > > > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-14 6:14 ` Xin Long @ 2019-10-25 3:21 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:21 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-25 3:21 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-25 3:21 UTC (permalink / raw) To: Xin Long; +Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-10-25 3:21 ` Marcelo Ricardo Leitner @ 2019-10-25 7:59 ` Xin Long -1 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 7:59 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-10-25 7:59 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-25 7:59 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: network dev, linux-sctp, Neil Horman, davem, David Laight 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 > > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-14 6:14 ` Xin Long @ 2019-10-14 12:42 ` Neil Horman -1 siblings, 0 replies; 61+ messages in thread From: Neil Horman @ 2019-10-14 12:42 UTC (permalink / raw) To: Xin Long Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, davem, David Laight 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> ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-14 12:42 ` Neil Horman 0 siblings, 0 replies; 61+ messages in thread From: Neil Horman @ 2019-10-14 12:42 UTC (permalink / raw) To: Xin Long Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, davem, David Laight 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> ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-14 6:14 ` Xin Long @ 2019-10-16 0:56 ` David Miller -1 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 0:56 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-16 0:56 ` David Miller 0 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 0:56 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-16 0:56 ` David Miller (?) @ 2019-10-16 10:42 ` David Laight 2019-10-17 4:56 ` Xin Long -1 siblings, 1 reply; 61+ messages in thread From: David Laight @ 2019-10-16 10:42 UTC (permalink / raw) To: 'David Miller', lucien.xin Cc: netdev, linux-sctp, marcelo.leitner, nhorman 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-16 10:42 ` David Laight @ 2019-10-17 4:56 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-17 4:56 UTC (permalink / raw) To: David Laight; +Cc: David Miller, netdev, linux-sctp, marcelo.leitner, nhorman 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) > ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-17 4:56 ` Xin Long 0 siblings, 0 replies; 61+ messages in thread From: Xin Long @ 2019-10-17 4:56 UTC (permalink / raw) To: David Laight; +Cc: David Miller, netdev, linux-sctp, marcelo.leitner, nhorman 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) > ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-17 4:56 ` Xin Long @ 2019-10-17 9:04 ` David Laight -1 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-17 9:04 UTC (permalink / raw) To: 'Xin Long' Cc: David Miller, netdev, linux-sctp, marcelo.leitner, nhorman 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) ^ permalink raw reply [flat|nested] 61+ messages in thread
* RE: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-17 9:04 ` David Laight 0 siblings, 0 replies; 61+ messages in thread From: David Laight @ 2019-10-17 9:04 UTC (permalink / raw) To: 'Xin Long' Cc: David Miller, netdev, linux-sctp, marcelo.leitner, nhorman RnJvbTogWGluIExvbmcNCj4gU2VudDogMTcgT2N0b2JlciAyMDE5IDA1OjU3DQouLi4NCj4gPiBJ J20gb25seSBhbiBTQ1RQIHVzZXIsIGJ1dCBJIHRoaW5rIHNvbWUgb2YgdGhlIEFQSSBjaGFuZ2Vz IGFyZW4ndCByaWdodC4NCj4gSGksIERhdmlkIEwuDQo+IA0KPiBJIHRoaW5rIHlvdSBtdXN0IGtu b3cgcXVpdGUgYSBmZXcgdXNlciBjYXNlcy4NCj4gDQo+IEJlZm9yZSBJIHJlcG9zdCwgY2FuIHlv dSBwbHMgZ2l2ZSB0aGUgZXhhY3QgcGxhY2VzIHdoZXJlIHRoZSBBUEkNCj4gY2hhbmdlcyBtYXkg bm90IGJlIHJpZ2h0IGFzIHlvdSd2ZSBhbHJlYWR5IGRvbmUgaW4gdjEgYW5kIHYyLCBzbw0KPiB0 aGF0IEkgY2FuIGNvcnJlY3QgdGhlbS4NCg0KSSdsbCB0cnkgdG8gZmluZCB0aW1lIHRvIGxvb2sg bGF0ZXIgdG9kYXkuDQoNCglEYXZpZA0KDQotDQpSZWdpc3RlcmVkIEFkZHJlc3MgTGFrZXNpZGUs IEJyYW1sZXkgUm9hZCwgTW91bnQgRmFybSwgTWlsdG9uIEtleW5lcywgTUsxIDFQVCwgVUsNClJl Z2lzdHJhdGlvbiBObzogMTM5NzM4NiAoV2FsZXMpDQo ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-14 6:14 ` Xin Long @ 2019-10-16 18:25 ` David Miller -1 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 18:25 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-16 18:25 ` David Miller 0 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 18:25 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-16 18:25 ` David Miller @ 2019-10-16 18:32 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-16 18:32 UTC (permalink / raw) To: David Miller; +Cc: lucien.xin, netdev, linux-sctp, nhorman, david.laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-16 18:32 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 61+ messages in thread From: Marcelo Ricardo Leitner @ 2019-10-16 18:32 UTC (permalink / raw) To: David Miller; +Cc: lucien.xin, netdev, linux-sctp, nhorman, david.laight 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 ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 2019-10-16 18:32 ` Marcelo Ricardo Leitner @ 2019-10-16 19:04 ` David Miller -1 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 19:04 UTC (permalink / raw) To: marcelo.leitner; +Cc: lucien.xin, netdev, linux-sctp, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCHv3 net-next 0/5] sctp: update from rfc7829 @ 2019-10-16 19:04 ` David Miller 0 siblings, 0 replies; 61+ messages in thread From: David Miller @ 2019-10-16 19:04 UTC (permalink / raw) To: marcelo.leitner; +Cc: lucien.xin, netdev, linux-sctp, nhorman, david.laight 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. ^ permalink raw reply [flat|nested] 61+ messages in thread
end of thread, other threads:[~2019-10-25 14:45 UTC | newest] Thread overview: 61+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-14 6:14 [PATCHv3 net-next 0/5] sctp: update from rfc7829 Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-14 6:14 ` [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-14 6:14 ` [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-14 6:14 ` [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-14 6:14 ` [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-14 6:14 ` [PATCHv3 net-next 5/5] sctp: add SCTP_PEER_ADDR_THLDS_V2 sockopt Xin Long 2019-10-14 6:14 ` Xin Long 2019-10-25 3:25 ` [PATCHv3 net-next 4/5] sctp: add support for Primary Path Switchover Marcelo Ricardo Leitner 2019-10-25 3:25 ` Marcelo Ricardo Leitner 2019-10-25 8:13 ` Xin Long 2019-10-25 8:13 ` Xin Long 2019-10-25 3:24 ` [PATCHv3 net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt Marcelo Ricardo Leitner 2019-10-25 3:24 ` Marcelo Ricardo Leitner 2019-10-25 8:05 ` Xin Long 2019-10-25 8:05 ` Xin Long 2019-10-25 3:23 ` [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc Marcelo Ricardo Leitner 2019-10-25 3:23 ` Marcelo Ricardo Leitner 2019-10-25 8:02 ` Xin Long 2019-10-25 8:02 ` Xin Long 2019-10-25 9:00 ` David Laight 2019-10-25 9:00 ` David Laight 2019-10-25 13:21 ` 'Marcelo Ricardo Leitner' 2019-10-25 13:21 ` 'Marcelo Ricardo Leitner' 2019-10-25 14:26 ` David Laight 2019-10-25 14:26 ` David Laight 2019-10-25 14:45 ` 'Marcelo Ricardo Leitner' 2019-10-25 14:45 ` 'Marcelo Ricardo Leitner' 2019-10-18 15:56 ` [PATCHv3 net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification David Laight 2019-10-18 15:56 ` David Laight 2019-10-19 8:55 ` Xin Long 2019-10-19 8:55 ` Xin Long 2019-10-22 11:13 ` Xin Long 2019-10-22 11:13 ` Xin Long 2019-10-25 3:22 ` Marcelo Ricardo Leitner 2019-10-25 3:22 ` Marcelo Ricardo Leitner 2019-10-25 7:58 ` Xin Long 2019-10-25 7:58 ` Xin Long 2019-10-25 3:21 ` Marcelo Ricardo Leitner 2019-10-25 3:21 ` Marcelo Ricardo Leitner 2019-10-25 7:59 ` Xin Long 2019-10-25 7:59 ` Xin Long 2019-10-14 12:42 ` [PATCHv3 net-next 0/5] sctp: update from rfc7829 Neil Horman 2019-10-14 12:42 ` Neil Horman 2019-10-16 0:56 ` David Miller 2019-10-16 0:56 ` David Miller 2019-10-16 10:42 ` David Laight 2019-10-17 4:56 ` Xin Long 2019-10-17 4:56 ` Xin Long 2019-10-17 9:04 ` David Laight 2019-10-17 9:04 ` David Laight 2019-10-16 18:25 ` David Miller 2019-10-16 18:25 ` David Miller 2019-10-16 18:32 ` Marcelo Ricardo Leitner 2019-10-16 18:32 ` Marcelo Ricardo Leitner 2019-10-16 19:04 ` David Miller 2019-10-16 19:04 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.