* [PATCH net-next 0/5] sctp: update from rfc7829
@ 2019-09-09 7:56 ` Xin Long
0 siblings, 0 replies; 37+ messages in thread
From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw)
To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem
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
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 spt_pathcpthld in struct sctp_paddrthlds
include/net/netns/sctp.h | 13 ++++++
include/net/sctp/structs.h | 13 ++++--
include/uapi/linux/sctp.h | 4 ++
net/sctp/associola.c | 28 ++++++-------
net/sctp/protocol.c | 6 +++
net/sctp/sm_sideeffect.c | 5 +++
net/sctp/socket.c | 99 +++++++++++++++++++++++++++++++++++++++++++++-
net/sctp/sysctl.c | 16 ++++++++
8 files changed, 165 insertions(+), 19 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH net-next 0/5] sctp: update from rfc7829 @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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 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 spt_pathcpthld in struct sctp_paddrthlds include/net/netns/sctp.h | 13 ++++++ include/net/sctp/structs.h | 13 ++++-- include/uapi/linux/sctp.h | 4 ++ net/sctp/associola.c | 28 ++++++------- net/sctp/protocol.c | 6 +++ net/sctp/sm_sideeffect.c | 5 +++ net/sctp/socket.c | 99 +++++++++++++++++++++++++++++++++++++++++++++- net/sctp/sysctl.c | 16 ++++++++ 8 files changed, 165 insertions(+), 19 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification 2019-09-09 7:56 ` Xin Long @ 2019-09-09 7:56 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/associola.c | 17 ++++------------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index 6d5b164..45a85d7 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -410,6 +410,7 @@ enum sctp_spc_state { SCTP_ADDR_ADDED, SCTP_ADDR_MADE_PRIM, SCTP_ADDR_CONFIRMED, + SCTP_ADDR_POTENTIALLY_FAILED, }; diff --git a/net/sctp/associola.c b/net/sctp/associola.c index d2ffc9a..7278b7e 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -798,14 +798,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; @@ -814,19 +806,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] 37+ messages in thread
* [PATCH net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/associola.c | 17 ++++------------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index 6d5b164..45a85d7 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -410,6 +410,7 @@ enum sctp_spc_state { SCTP_ADDR_ADDED, SCTP_ADDR_MADE_PRIM, SCTP_ADDR_CONFIRMED, + SCTP_ADDR_POTENTIALLY_FAILED, }; diff --git a/net/sctp/associola.c b/net/sctp/associola.c index d2ffc9a..7278b7e 100644 --- a/net/sctp/associola.c +++ b/net/sctp/associola.c @@ -798,14 +798,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; @@ -814,19 +806,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] 37+ messages in thread
* [PATCH net-next 2/5] sctp: add pf_expose per netns and sock and asoc 2019-09-09 7:56 ` Xin Long @ 2019-09-09 7:56 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/netns/sctp.h | 7 +++++++ include/net/sctp/structs.h | 2 ++ include/uapi/linux/sctp.h | 1 + net/sctp/associola.c | 10 ++++++++-- net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 12 ++++++++++-- net/sctp/sysctl.c | 7 +++++++ 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h index bdc0f27..5234940c 100644 --- a/include/net/netns/sctp.h +++ b/include/net/netns/sctp.h @@ -97,6 +97,13 @@ struct netns_sctp { int pf_enable; /* + * Disable Potentially-Failed state exposure, enabled by default + * pf_expose - 0 : disable pf state exposure + * - >0 : enable pf state exposure + */ + 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/structs.h b/include/net/sctp/structs.h index 503fbc3..c2d3317 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:1, 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:1, /* Expose pf state? */ force_delay:1; __u8 strreset_enable; diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index 45a85d7..b6649d6 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -920,6 +920,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 7278b7e..461b1ffa 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); @@ -793,7 +794,9 @@ 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 && + if (transport->state == SCTP_PF && !asoc->pf_expose) + ulp_notify = false; + if (transport->state && SCTP_UNCONFIRMED && SCTP_HEARTBEAT_SUCCESS == error) spc_state = SCTP_ADDR_CONFIRMED; else @@ -817,7 +820,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) + 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 b48ffe8..de0f15f 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; + /* Enable pf state exposure by default */ + net->sctp.pf_expose = 1; + /* 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 3e50a97..33b93bb 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -5040,6 +5040,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; @@ -5520,8 +5521,15 @@ 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) { + 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..eacc9a1 100644 --- a/net/sctp/sysctl.c +++ b/net/sctp/sysctl.c @@ -318,6 +318,13 @@ 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, + }, { /* sentinel */ } }; -- 2.1.0 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH net-next 2/5] sctp: add pf_expose per netns and sock and asoc @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/netns/sctp.h | 7 +++++++ include/net/sctp/structs.h | 2 ++ include/uapi/linux/sctp.h | 1 + net/sctp/associola.c | 10 ++++++++-- net/sctp/protocol.c | 3 +++ net/sctp/socket.c | 12 ++++++++++-- net/sctp/sysctl.c | 7 +++++++ 7 files changed, 38 insertions(+), 4 deletions(-) diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h index bdc0f27..5234940c 100644 --- a/include/net/netns/sctp.h +++ b/include/net/netns/sctp.h @@ -97,6 +97,13 @@ struct netns_sctp { int pf_enable; /* + * Disable Potentially-Failed state exposure, enabled by default + * pf_expose - 0 : disable pf state exposure + * - >0 : enable pf state exposure + */ + 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/structs.h b/include/net/sctp/structs.h index 503fbc3..c2d3317 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:1, 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:1, /* Expose pf state? */ force_delay:1; __u8 strreset_enable; diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index 45a85d7..b6649d6 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -920,6 +920,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 7278b7e..461b1ffa 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); @@ -793,7 +794,9 @@ 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 && + if (transport->state = SCTP_PF && !asoc->pf_expose) + ulp_notify = false; + if (transport->state && SCTP_UNCONFIRMED && SCTP_HEARTBEAT_SUCCESS = error) spc_state = SCTP_ADDR_CONFIRMED; else @@ -817,7 +820,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) + 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 b48ffe8..de0f15f 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; + /* Enable pf state exposure by default */ + net->sctp.pf_expose = 1; + /* 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 3e50a97..33b93bb 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -5040,6 +5040,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; @@ -5520,8 +5521,15 @@ 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) { + 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..eacc9a1 100644 --- a/net/sctp/sysctl.c +++ b/net/sctp/sysctl.c @@ -318,6 +318,13 @@ 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, + }, { /* sentinel */ } }; -- 2.1.0 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt 2019-09-09 7:56 ` Xin Long @ 2019-09-09 7:56 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/socket.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index b6649d6..a15cc28 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -137,6 +137,7 @@ 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 /* PR-SCTP policies */ #define SCTP_PR_SCTP_NONE 0x0000 diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 33b93bb..e3a8e25 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4588,6 +4588,37 @@ 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; + } + + 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 @@ -4797,6 +4828,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; @@ -7906,6 +7940,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) { @@ -8118,6 +8191,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] 37+ messages in thread
* [PATCH net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/socket.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index b6649d6..a15cc28 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -137,6 +137,7 @@ 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 /* PR-SCTP policies */ #define SCTP_PR_SCTP_NONE 0x0000 diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 33b93bb..e3a8e25 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -4588,6 +4588,37 @@ 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; + } + + 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 @@ -4797,6 +4828,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; @@ -7906,6 +7940,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) { @@ -8118,6 +8191,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] 37+ messages in thread
* [PATCH net-next 4/5] sctp: add support for Primary Path Switchover 2019-09-09 7:56 ` Xin Long @ 2019-09-09 7:56 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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 5234940c..cab0903 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 c2d3317..3680a93 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 461b1ffa..f63c56d 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); @@ -625,6 +626,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 de0f15f..23f7631 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 1cf5bb5..3948697 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 e3a8e25..5e2098b 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -5074,6 +5074,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 eacc9a1..c9ebfc2 100644 --- a/net/sctp/sysctl.c +++ b/net/sctp/sysctl.c @@ -212,6 +212,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] 37+ messages in thread
* [PATCH net-next 4/5] sctp: add support for Primary Path Switchover @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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 5234940c..cab0903 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 c2d3317..3680a93 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 461b1ffa..f63c56d 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); @@ -625,6 +626,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 de0f15f..23f7631 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 1cf5bb5..3948697 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 e3a8e25..5e2098b 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -5074,6 +5074,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 eacc9a1..c9ebfc2 100644 --- a/net/sctp/sysctl.c +++ b/net/sctp/sysctl.c @@ -212,6 +212,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] 37+ messages in thread
* [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-09 7:56 ` Xin Long @ 2019-09-09 7:56 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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 that ps_retrans is not allowed to be greater than pf_retrans. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/socket.c | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index a15cc28..dfd81e1 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { struct sockaddr_storage spt_address; __u16 spt_pathmaxrxt; __u16 spt_pathpfthld; + __u16 spt_pathcpthld; }; /* diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 5e2098b..5b9774d 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, sizeof(struct sctp_paddrthlds))) return -EFAULT; + if (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 +3966,7 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, if (val.spt_pathmaxrxt) trans->pathmaxrxt = val.spt_pathmaxrxt; trans->pf_retrans = val.spt_pathpfthld; + trans->ps_retrans = val.spt_pathcpthld; return 0; } @@ -3978,17 +3982,20 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, if (val.spt_pathmaxrxt) trans->pathmaxrxt = val.spt_pathmaxrxt; trans->pf_retrans = val.spt_pathpfthld; + trans->ps_retrans = val.spt_pathcpthld; } if (val.spt_pathmaxrxt) asoc->pathmaxrxt = val.spt_pathmaxrxt; asoc->pf_retrans = val.spt_pathpfthld; + asoc->ps_retrans = val.spt_pathcpthld; } else { struct sctp_sock *sp = sctp_sk(sk); if (val.spt_pathmaxrxt) sp->pathmaxrxt = val.spt_pathmaxrxt; sp->pf_retrans = val.spt_pathpfthld; + sp->ps_retrans = val.spt_pathcpthld; } return 0; @@ -7232,6 +7239,7 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk, if (!trans) return -ENOENT; + val.spt_pathcpthld = trans->ps_retrans; val.spt_pathmaxrxt = trans->pathmaxrxt; val.spt_pathpfthld = trans->pf_retrans; @@ -7244,11 +7252,13 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk, return -EINVAL; if (asoc) { + val.spt_pathcpthld = asoc->ps_retrans; val.spt_pathpfthld = asoc->pf_retrans; val.spt_pathmaxrxt = asoc->pathmaxrxt; } else { struct sctp_sock *sp = sctp_sk(sk); + val.spt_pathcpthld = sp->ps_retrans; val.spt_pathpfthld = sp->pf_retrans; val.spt_pathmaxrxt = sp->pathmaxrxt; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-09 7:56 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-09 7:56 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem 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 that ps_retrans is not allowed to be greater than pf_retrans. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/uapi/linux/sctp.h | 1 + net/sctp/socket.c | 10 ++++++++++ 2 files changed, 11 insertions(+) diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index a15cc28..dfd81e1 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { struct sockaddr_storage spt_address; __u16 spt_pathmaxrxt; __u16 spt_pathpfthld; + __u16 spt_pathcpthld; }; /* diff --git a/net/sctp/socket.c b/net/sctp/socket.c index 5e2098b..5b9774d 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, sizeof(struct sctp_paddrthlds))) return -EFAULT; + if (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 +3966,7 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, if (val.spt_pathmaxrxt) trans->pathmaxrxt = val.spt_pathmaxrxt; trans->pf_retrans = val.spt_pathpfthld; + trans->ps_retrans = val.spt_pathcpthld; return 0; } @@ -3978,17 +3982,20 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, if (val.spt_pathmaxrxt) trans->pathmaxrxt = val.spt_pathmaxrxt; trans->pf_retrans = val.spt_pathpfthld; + trans->ps_retrans = val.spt_pathcpthld; } if (val.spt_pathmaxrxt) asoc->pathmaxrxt = val.spt_pathmaxrxt; asoc->pf_retrans = val.spt_pathpfthld; + asoc->ps_retrans = val.spt_pathcpthld; } else { struct sctp_sock *sp = sctp_sk(sk); if (val.spt_pathmaxrxt) sp->pathmaxrxt = val.spt_pathmaxrxt; sp->pf_retrans = val.spt_pathpfthld; + sp->ps_retrans = val.spt_pathcpthld; } return 0; @@ -7232,6 +7239,7 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk, if (!trans) return -ENOENT; + val.spt_pathcpthld = trans->ps_retrans; val.spt_pathmaxrxt = trans->pathmaxrxt; val.spt_pathpfthld = trans->pf_retrans; @@ -7244,11 +7252,13 @@ static int sctp_getsockopt_paddr_thresholds(struct sock *sk, return -EINVAL; if (asoc) { + val.spt_pathcpthld = asoc->ps_retrans; val.spt_pathpfthld = asoc->pf_retrans; val.spt_pathmaxrxt = asoc->pathmaxrxt; } else { struct sctp_sock *sp = sctp_sk(sk); + val.spt_pathcpthld = sp->ps_retrans; val.spt_pathpfthld = sp->pf_retrans; val.spt_pathmaxrxt = sp->pathmaxrxt; } -- 2.1.0 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-09 7:56 ` Xin Long (?) @ 2019-09-10 13:19 ` David Laight 2019-09-11 8:51 ` Xin Long -1 siblings, 1 reply; 37+ messages in thread From: David Laight @ 2019-09-10 13:19 UTC (permalink / raw) To: 'Xin Long', network dev, linux-sctp Cc: Marcelo Ricardo Leitner, Neil Horman, davem From: Xin Long > Sent: 09 September 2019 08:57 > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > --- > include/uapi/linux/sctp.h | 1 + > net/sctp/socket.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index a15cc28..dfd81e1 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > struct sockaddr_storage spt_address; > __u16 spt_pathmaxrxt; > __u16 spt_pathpfthld; > + __u16 spt_pathcpthld; > }; > > /* > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > index 5e2098b..5b9774d 100644 > --- a/net/sctp/socket.c > +++ b/net/sctp/socket.c > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, This code does: if (optlen < sizeof(struct sctp_paddrthlds)) return -EINVAL; So adding an extra field breaks existing application binaries that use this option. I've not checked the other patches or similar fubar. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-10 13:19 ` David Laight @ 2019-09-11 8:51 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 8:51 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > From: Xin Long > > Sent: 09 September 2019 08:57 > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > include/uapi/linux/sctp.h | 1 + > > net/sctp/socket.c | 10 ++++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index a15cc28..dfd81e1 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > struct sockaddr_storage spt_address; > > __u16 spt_pathmaxrxt; > > __u16 spt_pathpfthld; > > + __u16 spt_pathcpthld; > > }; > > > > /* > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index 5e2098b..5b9774d 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > This code does: > if (optlen < sizeof(struct sctp_paddrthlds)) > return -EINVAL; here will become: if (optlen >= sizeof(struct sctp_paddrthlds)) { optlen = sizeof(struct sctp_paddrthlds); } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4)) optlen = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); val.spt_pathcpthld = 0xffff; else { return -EINVAL; } if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; in sctp_getsockopt_paddr_thresholds(): if (len >= sizeof(struct sctp_paddrthlds)) len = sizeof(struct sctp_paddrthlds); else if (len >= ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4)) len = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); else return -EINVAL; if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) return -EFAULT; > > So adding an extra field breaks existing application binaries > that use this option. > > I've not checked the other patches or similar fubar. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 8:51 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 8:51 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > From: Xin Long > > Sent: 09 September 2019 08:57 > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > --- > > include/uapi/linux/sctp.h | 1 + > > net/sctp/socket.c | 10 ++++++++++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index a15cc28..dfd81e1 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > struct sockaddr_storage spt_address; > > __u16 spt_pathmaxrxt; > > __u16 spt_pathpfthld; > > + __u16 spt_pathcpthld; > > }; > > > > /* > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > index 5e2098b..5b9774d 100644 > > --- a/net/sctp/socket.c > > +++ b/net/sctp/socket.c > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > This code does: > if (optlen < sizeof(struct sctp_paddrthlds)) > return -EINVAL; here will become: if (optlen >= sizeof(struct sctp_paddrthlds)) { optlen = sizeof(struct sctp_paddrthlds); } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4)) optlen = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); val.spt_pathcpthld = 0xffff; else { return -EINVAL; } if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; in sctp_getsockopt_paddr_thresholds(): if (len >= sizeof(struct sctp_paddrthlds)) len = sizeof(struct sctp_paddrthlds); else if (len >= ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4)) len = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); else return -EINVAL; if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) return -EFAULT; > > So adding an extra field breaks existing application binaries > that use this option. > > I've not checked the other patches or similar fubar. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 8:51 ` Xin Long @ 2019-09-11 9:03 ` David Laight -1 siblings, 0 replies; 37+ messages in thread From: David Laight @ 2019-09-11 9:03 UTC (permalink / raw) To: 'Xin Long' Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem From: Xin Long [mailto:lucien.xin@gmail.com] > Sent: 11 September 2019 09:52 > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Xin Long > > > Sent: 09 September 2019 08:57 > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > --- > > > include/uapi/linux/sctp.h | 1 + > > > net/sctp/socket.c | 10 ++++++++++ > > > 2 files changed, 11 insertions(+) > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > index a15cc28..dfd81e1 100644 > > > --- a/include/uapi/linux/sctp.h > > > +++ b/include/uapi/linux/sctp.h > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > struct sockaddr_storage spt_address; > > > __u16 spt_pathmaxrxt; > > > __u16 spt_pathpfthld; > > > + __u16 spt_pathcpthld; > > > }; > > > > > > /* > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > index 5e2098b..5b9774d 100644 > > > --- a/net/sctp/socket.c > > > +++ b/net/sctp/socket.c > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > This code does: > > if (optlen < sizeof(struct sctp_paddrthlds)) > > return -EINVAL; > here will become: > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > optlen = sizeof(struct sctp_paddrthlds); > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > spt_pathcpthld), 4)) > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > spt_pathcpthld), 4); > val.spt_pathcpthld = 0xffff; > else { > return -EINVAL; > } Hmmm... If the kernel has to default 'val.spt_pathcpthld = 0xffff' then recompiling an existing application with the new uapi header is going to lead to very unexpected behaviour. The best you can hope for is that the application memset the structure to zero. But more likely it is 'random' on-stack data. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 9:03 ` David Laight 0 siblings, 0 replies; 37+ messages in thread From: David Laight @ 2019-09-11 9:03 UTC (permalink / raw) To: 'Xin Long' Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem RnJvbTogWGluIExvbmcgW21haWx0bzpsdWNpZW4ueGluQGdtYWlsLmNvbV0NCj4gU2VudDogMTEg U2VwdGVtYmVyIDIwMTkgMDk6NTINCj4gT24gVHVlLCBTZXAgMTAsIDIwMTkgYXQgOToxOSBQTSBE YXZpZCBMYWlnaHQgPERhdmlkLkxhaWdodEBhY3VsYWIuY29tPiB3cm90ZToNCj4gPg0KPiA+IEZy b206IFhpbiBMb25nDQo+ID4gPiBTZW50OiAwOSBTZXB0ZW1iZXIgMjAxOSAwODo1Nw0KPiA+ID4g U2VjdGlvbiA3LjIgb2YgcmZjNzgyOTogIlBlZXIgQWRkcmVzcyBUaHJlc2hvbGRzIChTQ1RQX1BF RVJfQUREUl9USExEUykNCj4gPiA+IFNvY2tldCBPcHRpb24iIGV4dGVuZHMgJ3N0cnVjdCBzY3Rw X3BhZGRydGhsZHMnIHdpdGggJ3NwdF9wYXRoY3B0aGxkJw0KPiA+ID4gYWRkZWQgdG8gYWxsb3cg YSB1c2VyIHRvIGNoYW5nZSBwc19yZXRyYW5zIHBlciBzb2NrL2Fzb2MvdHJhbnNwb3J0LCBhcw0K PiA+ID4gb3RoZXIgMiBwYWRkcnRobGRzOiBwZl9yZXRyYW5zLCBwYXRobWF4cnh0Lg0KPiA+ID4N Cj4gPiA+IE5vdGUgdGhhdCBwc19yZXRyYW5zIGlzIG5vdCBhbGxvd2VkIHRvIGJlIGdyZWF0ZXIg dGhhbiBwZl9yZXRyYW5zLg0KPiA+ID4NCj4gPiA+IFNpZ25lZC1vZmYtYnk6IFhpbiBMb25nIDxs dWNpZW4ueGluQGdtYWlsLmNvbT4NCj4gPiA+IC0tLQ0KPiA+ID4gIGluY2x1ZGUvdWFwaS9saW51 eC9zY3RwLmggfCAgMSArDQo+ID4gPiAgbmV0L3NjdHAvc29ja2V0LmMgICAgICAgICB8IDEwICsr KysrKysrKysNCj4gPiA+ICAyIGZpbGVzIGNoYW5nZWQsIDExIGluc2VydGlvbnMoKykNCj4gPiA+ DQo+ID4gPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS91YXBpL2xpbnV4L3NjdHAuaCBiL2luY2x1ZGUv dWFwaS9saW51eC9zY3RwLmgNCj4gPiA+IGluZGV4IGExNWNjMjguLmRmZDgxZTEgMTAwNjQ0DQo+ ID4gPiAtLS0gYS9pbmNsdWRlL3VhcGkvbGludXgvc2N0cC5oDQo+ID4gPiArKysgYi9pbmNsdWRl L3VhcGkvbGludXgvc2N0cC5oDQo+ID4gPiBAQCAtMTA2OSw2ICsxMDY5LDcgQEAgc3RydWN0IHNj dHBfcGFkZHJ0aGxkcyB7DQo+ID4gPiAgICAgICBzdHJ1Y3Qgc29ja2FkZHJfc3RvcmFnZSBzcHRf YWRkcmVzczsNCj4gPiA+ICAgICAgIF9fdTE2IHNwdF9wYXRobWF4cnh0Ow0KPiA+ID4gICAgICAg X191MTYgc3B0X3BhdGhwZnRobGQ7DQo+ID4gPiArICAgICBfX3UxNiBzcHRfcGF0aGNwdGhsZDsN Cj4gPiA+ICB9Ow0KPiA+ID4NCj4gPiA+ICAvKg0KPiA+ID4gZGlmZiAtLWdpdCBhL25ldC9zY3Rw L3NvY2tldC5jIGIvbmV0L3NjdHAvc29ja2V0LmMNCj4gPiA+IGluZGV4IDVlMjA5OGIuLjViOTc3 NGQgMTAwNjQ0DQo+ID4gPiAtLS0gYS9uZXQvc2N0cC9zb2NrZXQuYw0KPiA+ID4gKysrIGIvbmV0 L3NjdHAvc29ja2V0LmMNCj4gPiA+IEBAIC0zOTU0LDYgKzM5NTQsOSBAQCBzdGF0aWMgaW50IHNj dHBfc2V0c29ja29wdF9wYWRkcl90aHJlc2hvbGRzKHN0cnVjdCBzb2NrICpzaywNCj4gPg0KPiA+ IFRoaXMgY29kZSBkb2VzOg0KPiA+ICAgICAgICAgaWYgKG9wdGxlbiA8IHNpemVvZihzdHJ1Y3Qg c2N0cF9wYWRkcnRobGRzKSkNCj4gPiAgICAgICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7DQo+ IGhlcmUgd2lsbCBiZWNvbWU6DQo+IA0KPiAgICAgICAgIGlmIChvcHRsZW4gPj0gc2l6ZW9mKHN0 cnVjdCBzY3RwX3BhZGRydGhsZHMpKSB7DQo+ICAgICAgICAgICAgICAgICBvcHRsZW4gPSBzaXpl b2Yoc3RydWN0IHNjdHBfcGFkZHJ0aGxkcyk7DQo+ICAgICAgICAgfSBlbHNlIGlmIChvcHRsZW4g Pj0gQUxJR04ob2Zmc2V0b2Yoc3RydWN0IHNjdHBfcGFkZHJ0aGxkcywNCj4gICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzcHRfcGF0aGNwdGhsZCksIDQpKQ0KPiAg ICAgICAgICAgICAgICAgb3B0bGVuID0gQUxJR04ob2Zmc2V0b2Yoc3RydWN0IHNjdHBfcGFkZHJ0 aGxkcywNCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHNwdF9wYXRo Y3B0aGxkKSwgNCk7DQo+ICAgICAgICAgICAgICAgICB2YWwuc3B0X3BhdGhjcHRobGQgPSAweGZm ZmY7DQo+ICAgICAgICAgZWxzZSB7DQo+ICAgICAgICAgICAgICAgICByZXR1cm4gLUVJTlZBTDsN Cj4gICAgICAgICB9DQoNCkhtbW0uLi4NCklmIHRoZSBrZXJuZWwgaGFzIHRvIGRlZmF1bHQgJ3Zh bC5zcHRfcGF0aGNwdGhsZCA9IDB4ZmZmZicNCnRoZW4gcmVjb21waWxpbmcgYW4gZXhpc3Rpbmcg YXBwbGljYXRpb24gd2l0aCB0aGUgbmV3IHVhcGkNCmhlYWRlciBpcyBnb2luZyB0byBsZWFkIHRv IHZlcnkgdW5leHBlY3RlZCBiZWhhdmlvdXIuDQoNClRoZSBiZXN0IHlvdSBjYW4gaG9wZSBmb3Ig aXMgdGhhdCB0aGUgYXBwbGljYXRpb24gbWVtc2V0IHRoZQ0Kc3RydWN0dXJlIHRvIHplcm8uDQpC dXQgbW9yZSBsaWtlbHkgaXQgaXMgJ3JhbmRvbScgb24tc3RhY2sgZGF0YS4NCg0KCURhdmlkDQoN Ci0NClJlZ2lzdGVyZWQgQWRkcmVzcyBMYWtlc2lkZSwgQnJhbWxleSBSb2FkLCBNb3VudCBGYXJt LCBNaWx0b24gS2V5bmVzLCBNSzEgMVBULCBVSw0KUmVnaXN0cmF0aW9uIE5vOiAxMzk3Mzg2IChX YWxlcykNCg= ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 9:03 ` David Laight @ 2019-09-11 9:21 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 9:21 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > From: Xin Long [mailto:lucien.xin@gmail.com] > > Sent: 11 September 2019 09:52 > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Xin Long > > > > Sent: 09 September 2019 08:57 > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > --- > > > > include/uapi/linux/sctp.h | 1 + > > > > net/sctp/socket.c | 10 ++++++++++ > > > > 2 files changed, 11 insertions(+) > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > index a15cc28..dfd81e1 100644 > > > > --- a/include/uapi/linux/sctp.h > > > > +++ b/include/uapi/linux/sctp.h > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > struct sockaddr_storage spt_address; > > > > __u16 spt_pathmaxrxt; > > > > __u16 spt_pathpfthld; > > > > + __u16 spt_pathcpthld; > > > > }; > > > > > > > > /* > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > index 5e2098b..5b9774d 100644 > > > > --- a/net/sctp/socket.c > > > > +++ b/net/sctp/socket.c > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > This code does: > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > return -EINVAL; > > here will become: > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > optlen = sizeof(struct sctp_paddrthlds); > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > spt_pathcpthld), 4)) > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > spt_pathcpthld), 4); > > val.spt_pathcpthld = 0xffff; > > else { > > return -EINVAL; > > } > > Hmmm... > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > then recompiling an existing application with the new uapi > header is going to lead to very unexpected behaviour. > > The best you can hope for is that the application memset the > structure to zero. > But more likely it is 'random' on-stack data. 0xffff is a value to disable the feature 'Primary Path Switchover'. you're right that user might set it to zero unexpectly with their old application rebuilt. A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't matter if users set spt_pathcpthld properly when they're not aware of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 9:21 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 9:21 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > From: Xin Long [mailto:lucien.xin@gmail.com] > > Sent: 11 September 2019 09:52 > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Xin Long > > > > Sent: 09 September 2019 08:57 > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > --- > > > > include/uapi/linux/sctp.h | 1 + > > > > net/sctp/socket.c | 10 ++++++++++ > > > > 2 files changed, 11 insertions(+) > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > index a15cc28..dfd81e1 100644 > > > > --- a/include/uapi/linux/sctp.h > > > > +++ b/include/uapi/linux/sctp.h > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > struct sockaddr_storage spt_address; > > > > __u16 spt_pathmaxrxt; > > > > __u16 spt_pathpfthld; > > > > + __u16 spt_pathcpthld; > > > > }; > > > > > > > > /* > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > index 5e2098b..5b9774d 100644 > > > > --- a/net/sctp/socket.c > > > > +++ b/net/sctp/socket.c > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > This code does: > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > return -EINVAL; > > here will become: > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > optlen = sizeof(struct sctp_paddrthlds); > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > spt_pathcpthld), 4)) > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > spt_pathcpthld), 4); > > val.spt_pathcpthld = 0xffff; > > else { > > return -EINVAL; > > } > > Hmmm... > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > then recompiling an existing application with the new uapi > header is going to lead to very unexpected behaviour. > > The best you can hope for is that the application memset the > structure to zero. > But more likely it is 'random' on-stack data. 0xffff is a value to disable the feature 'Primary Path Switchover'. you're right that user might set it to zero unexpectly with their old application rebuilt. A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't matter if users set spt_pathcpthld properly when they're not aware of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 9:21 ` Xin Long @ 2019-09-11 9:38 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 9:38 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > Sent: 11 September 2019 09:52 > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > From: Xin Long > > > > > Sent: 09 September 2019 08:57 > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > --- > > > > > include/uapi/linux/sctp.h | 1 + > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > index a15cc28..dfd81e1 100644 > > > > > --- a/include/uapi/linux/sctp.h > > > > > +++ b/include/uapi/linux/sctp.h > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > struct sockaddr_storage spt_address; > > > > > __u16 spt_pathmaxrxt; > > > > > __u16 spt_pathpfthld; > > > > > + __u16 spt_pathcpthld; > > > > > }; > > > > > > > > > > /* > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > index 5e2098b..5b9774d 100644 > > > > > --- a/net/sctp/socket.c > > > > > +++ b/net/sctp/socket.c > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > This code does: > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > return -EINVAL; > > > here will become: > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > optlen = sizeof(struct sctp_paddrthlds); > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > spt_pathcpthld), 4)) > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > spt_pathcpthld), 4); > > > val.spt_pathcpthld = 0xffff; > > > else { > > > return -EINVAL; > > > } > > > > Hmmm... > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > then recompiling an existing application with the new uapi > > header is going to lead to very unexpected behaviour. > > > > The best you can hope for is that the application memset the > > structure to zero. > > But more likely it is 'random' on-stack data. > 0xffff is a value to disable the feature 'Primary Path Switchover'. > you're right that user might set it to zero unexpectly with their > old application rebuilt. > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > matter if users set spt_pathcpthld properly when they're not aware > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there (its value is 0xffff by default), we just need to do this in sctp_setsockopt_paddr_thresholds(): if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; if (sock_net(sk)->sctp.ps_retrans == 0xffff) val.spt_pathcpthld = 0xffff; if (val.spt_pathpfthld > val.spt_pathcpthld) return -EINVAL; > > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 9:38 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 9:38 UTC (permalink / raw) To: David Laight Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman, davem On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > Sent: 11 September 2019 09:52 > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > From: Xin Long > > > > > Sent: 09 September 2019 08:57 > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > --- > > > > > include/uapi/linux/sctp.h | 1 + > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > index a15cc28..dfd81e1 100644 > > > > > --- a/include/uapi/linux/sctp.h > > > > > +++ b/include/uapi/linux/sctp.h > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > struct sockaddr_storage spt_address; > > > > > __u16 spt_pathmaxrxt; > > > > > __u16 spt_pathpfthld; > > > > > + __u16 spt_pathcpthld; > > > > > }; > > > > > > > > > > /* > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > index 5e2098b..5b9774d 100644 > > > > > --- a/net/sctp/socket.c > > > > > +++ b/net/sctp/socket.c > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > This code does: > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > return -EINVAL; > > > here will become: > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > optlen = sizeof(struct sctp_paddrthlds); > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > spt_pathcpthld), 4)) > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > spt_pathcpthld), 4); > > > val.spt_pathcpthld = 0xffff; > > > else { > > > return -EINVAL; > > > } > > > > Hmmm... > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > then recompiling an existing application with the new uapi > > header is going to lead to very unexpected behaviour. > > > > The best you can hope for is that the application memset the > > structure to zero. > > But more likely it is 'random' on-stack data. > 0xffff is a value to disable the feature 'Primary Path Switchover'. > you're right that user might set it to zero unexpectly with their > old application rebuilt. > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > matter if users set spt_pathcpthld properly when they're not aware > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there (its value is 0xffff by default), we just need to do this in sctp_setsockopt_paddr_thresholds(): if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; if (sock_net(sk)->sctp.ps_retrans = 0xffff) val.spt_pathcpthld = 0xffff; if (val.spt_pathpfthld > val.spt_pathcpthld) return -EINVAL; > > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 9:38 ` Xin Long @ 2019-09-11 12:56 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 37+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-11 12:56 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > Sent: 11 September 2019 09:52 > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > From: Xin Long > > > > > > Sent: 09 September 2019 08:57 > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > --- > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > index a15cc28..dfd81e1 100644 > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > struct sockaddr_storage spt_address; > > > > > > __u16 spt_pathmaxrxt; > > > > > > __u16 spt_pathpfthld; > > > > > > + __u16 spt_pathcpthld; > > > > > > }; > > > > > > > > > > > > /* > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > index 5e2098b..5b9774d 100644 > > > > > > --- a/net/sctp/socket.c > > > > > > +++ b/net/sctp/socket.c > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > This code does: > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > return -EINVAL; > > > > here will become: > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > spt_pathcpthld), 4)) > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > spt_pathcpthld), 4); > > > > val.spt_pathcpthld = 0xffff; > > > > else { > > > > return -EINVAL; > > > > } > > > > > > Hmmm... > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > then recompiling an existing application with the new uapi > > > header is going to lead to very unexpected behaviour. > > > > > > The best you can hope for is that the application memset the > > > structure to zero. > > > But more likely it is 'random' on-stack data. > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > you're right that user might set it to zero unexpectly with their > > old application rebuilt. > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > matter if users set spt_pathcpthld properly when they're not aware > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > (its value is 0xffff by default), > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > optlen)) > return -EFAULT; > > if (sock_net(sk)->sctp.ps_retrans == 0xffff) > val.spt_pathcpthld = 0xffff; I'm confused with the snippets, but if I got them right, this is after dealing with proper len and could leave val.spt_pathcpthld uninitialized if the application used the old format and sysctl is != 0xffff. > > if (val.spt_pathpfthld > val.spt_pathcpthld) > return -EINVAL; > > > > > > > > > David > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 12:56 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 37+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-11 12:56 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > Sent: 11 September 2019 09:52 > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > From: Xin Long > > > > > > Sent: 09 September 2019 08:57 > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > --- > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > index a15cc28..dfd81e1 100644 > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > struct sockaddr_storage spt_address; > > > > > > __u16 spt_pathmaxrxt; > > > > > > __u16 spt_pathpfthld; > > > > > > + __u16 spt_pathcpthld; > > > > > > }; > > > > > > > > > > > > /* > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > index 5e2098b..5b9774d 100644 > > > > > > --- a/net/sctp/socket.c > > > > > > +++ b/net/sctp/socket.c > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > This code does: > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > return -EINVAL; > > > > here will become: > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > spt_pathcpthld), 4)) > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > spt_pathcpthld), 4); > > > > val.spt_pathcpthld = 0xffff; > > > > else { > > > > return -EINVAL; > > > > } > > > > > > Hmmm... > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > then recompiling an existing application with the new uapi > > > header is going to lead to very unexpected behaviour. > > > > > > The best you can hope for is that the application memset the > > > structure to zero. > > > But more likely it is 'random' on-stack data. > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > you're right that user might set it to zero unexpectly with their > > old application rebuilt. > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > matter if users set spt_pathcpthld properly when they're not aware > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > (its value is 0xffff by default), > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > optlen)) > return -EFAULT; > > if (sock_net(sk)->sctp.ps_retrans = 0xffff) > val.spt_pathcpthld = 0xffff; I'm confused with the snippets, but if I got them right, this is after dealing with proper len and could leave val.spt_pathcpthld uninitialized if the application used the old format and sysctl is !0xffff. > > if (val.spt_pathpfthld > val.spt_pathcpthld) > return -EINVAL; > > > > > > > > > David > > > > > > - > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > Registration No: 1397386 (Wales) > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 12:56 ` Marcelo Ricardo Leitner @ 2019-09-11 17:47 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 17:47 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Wed, Sep 11, 2019 at 8:56 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > > Sent: 11 September 2019 09:52 > > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > > > From: Xin Long > > > > > > > Sent: 09 September 2019 08:57 > > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > --- > > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > > index a15cc28..dfd81e1 100644 > > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > > struct sockaddr_storage spt_address; > > > > > > > __u16 spt_pathmaxrxt; > > > > > > > __u16 spt_pathpfthld; > > > > > > > + __u16 spt_pathcpthld; > > > > > > > }; > > > > > > > > > > > > > > /* > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > > index 5e2098b..5b9774d 100644 > > > > > > > --- a/net/sctp/socket.c > > > > > > > +++ b/net/sctp/socket.c > > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > > > This code does: > > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > > return -EINVAL; > > > > > here will become: > > > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > > spt_pathcpthld), 4)) > > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > > spt_pathcpthld), 4); > > > > > val.spt_pathcpthld = 0xffff; > > > > > else { > > > > > return -EINVAL; > > > > > } > > > > > > > > Hmmm... > > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > > then recompiling an existing application with the new uapi > > > > header is going to lead to very unexpected behaviour. > > > > > > > > The best you can hope for is that the application memset the > > > > structure to zero. > > > > But more likely it is 'random' on-stack data. > > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > > you're right that user might set it to zero unexpectly with their > > > old application rebuilt. > > > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > > matter if users set spt_pathcpthld properly when they're not aware > > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > > (its value is 0xffff by default), > > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > > optlen)) > > return -EFAULT; > > > > if (sock_net(sk)->sctp.ps_retrans == 0xffff) > > val.spt_pathcpthld = 0xffff; > > I'm confused with the snippets, but if I got them right, this is after > dealing with proper len and could leave val.spt_pathcpthld > uninitialized if the application used the old format and sysctl is != > 0xffff. right, how about this in sctp_setsockopt_paddr_thresholds(): offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); if (optlen < offset) return -EINVAL; if (optlen < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff) { optlen = offset; val.spt_pathcpthld = 0xffff; } else { optlen = sizeof(val); } if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; if (val.spt_pathpfthld > val.spt_pathcpthld) return -EINVAL; Which means we will 'skip' spt_pathcpthld if (it's using old format) or (ps_retrans is disabled and it's using new format). Note that ps_retrans < pf_retrans is not allowed in rfc7829. and in sctp_getsockopt_paddr_thresholds(): offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); if (len < offset) return -EINVAL; if (len < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff) len = offset; else len = sizeof(val); if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) return -EFAULT; > > > > > if (val.spt_pathpfthld > val.spt_pathcpthld) > > return -EINVAL; > > > > > > > > > > > > > David > > > > > > > > - > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > > Registration No: 1397386 (Wales) > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 17:47 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 17:47 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Wed, Sep 11, 2019 at 8:56 PM Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote: > > On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > > Sent: 11 September 2019 09:52 > > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > > > From: Xin Long > > > > > > > Sent: 09 September 2019 08:57 > > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > --- > > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > > index a15cc28..dfd81e1 100644 > > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > > struct sockaddr_storage spt_address; > > > > > > > __u16 spt_pathmaxrxt; > > > > > > > __u16 spt_pathpfthld; > > > > > > > + __u16 spt_pathcpthld; > > > > > > > }; > > > > > > > > > > > > > > /* > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > > index 5e2098b..5b9774d 100644 > > > > > > > --- a/net/sctp/socket.c > > > > > > > +++ b/net/sctp/socket.c > > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > > > This code does: > > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > > return -EINVAL; > > > > > here will become: > > > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > > spt_pathcpthld), 4)) > > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > > spt_pathcpthld), 4); > > > > > val.spt_pathcpthld = 0xffff; > > > > > else { > > > > > return -EINVAL; > > > > > } > > > > > > > > Hmmm... > > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > > then recompiling an existing application with the new uapi > > > > header is going to lead to very unexpected behaviour. > > > > > > > > The best you can hope for is that the application memset the > > > > structure to zero. > > > > But more likely it is 'random' on-stack data. > > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > > you're right that user might set it to zero unexpectly with their > > > old application rebuilt. > > > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > > matter if users set spt_pathcpthld properly when they're not aware > > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > > (its value is 0xffff by default), > > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > > optlen)) > > return -EFAULT; > > > > if (sock_net(sk)->sctp.ps_retrans = 0xffff) > > val.spt_pathcpthld = 0xffff; > > I'm confused with the snippets, but if I got them right, this is after > dealing with proper len and could leave val.spt_pathcpthld > uninitialized if the application used the old format and sysctl is !> 0xffff. right, how about this in sctp_setsockopt_paddr_thresholds(): offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); if (optlen < offset) return -EINVAL; if (optlen < sizeof(val) || sock_net(sk)->sctp.ps_retrans = 0xffff) { optlen = offset; val.spt_pathcpthld = 0xffff; } else { optlen = sizeof(val); } if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, optlen)) return -EFAULT; if (val.spt_pathpfthld > val.spt_pathcpthld) return -EINVAL; Which means we will 'skip' spt_pathcpthld if (it's using old format) or (ps_retrans is disabled and it's using new format). Note that ps_retrans < pf_retrans is not allowed in rfc7829. and in sctp_getsockopt_paddr_thresholds(): offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); if (len < offset) return -EINVAL; if (len < sizeof(val) || sock_net(sk)->sctp.ps_retrans = 0xffff) len = offset; else len = sizeof(val); if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) return -EFAULT; > > > > > if (val.spt_pathpfthld > val.spt_pathcpthld) > > return -EINVAL; > > > > > > > > > > > > > David > > > > > > > > - > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > > Registration No: 1397386 (Wales) > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-11 17:47 ` Xin Long @ 2019-09-12 22:51 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 37+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-12 22:51 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Thu, Sep 12, 2019 at 01:47:08AM +0800, Xin Long wrote: > On Wed, Sep 11, 2019 at 8:56 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > > > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > > > Sent: 11 September 2019 09:52 > > > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > > > > > From: Xin Long > > > > > > > > Sent: 09 September 2019 08:57 > > > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > > --- > > > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > > > index a15cc28..dfd81e1 100644 > > > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > > > struct sockaddr_storage spt_address; > > > > > > > > __u16 spt_pathmaxrxt; > > > > > > > > __u16 spt_pathpfthld; > > > > > > > > + __u16 spt_pathcpthld; > > > > > > > > }; > > > > > > > > > > > > > > > > /* > > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > > > index 5e2098b..5b9774d 100644 > > > > > > > > --- a/net/sctp/socket.c > > > > > > > > +++ b/net/sctp/socket.c > > > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > > > > > This code does: > > > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > > > return -EINVAL; > > > > > > here will become: > > > > > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > > > spt_pathcpthld), 4)) > > > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > > > spt_pathcpthld), 4); > > > > > > val.spt_pathcpthld = 0xffff; > > > > > > else { > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > Hmmm... > > > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > > > then recompiling an existing application with the new uapi > > > > > header is going to lead to very unexpected behaviour. > > > > > > > > > > The best you can hope for is that the application memset the > > > > > structure to zero. > > > > > But more likely it is 'random' on-stack data. > > > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > > > you're right that user might set it to zero unexpectly with their > > > > old application rebuilt. > > > > > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > > > matter if users set spt_pathcpthld properly when they're not aware > > > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > > > (its value is 0xffff by default), > > > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > > > > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > > > optlen)) > > > return -EFAULT; > > > > > > if (sock_net(sk)->sctp.ps_retrans == 0xffff) > > > val.spt_pathcpthld = 0xffff; > > > > I'm confused with the snippets, but if I got them right, this is after > > dealing with proper len and could leave val.spt_pathcpthld > > uninitialized if the application used the old format and sysctl is != > > 0xffff. > right, how about this in sctp_setsockopt_paddr_thresholds(): > > offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); > if (optlen < offset) > return -EINVAL; > if (optlen < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff) { > optlen = offset; > val.spt_pathcpthld = 0xffff; > } else { > optlen = sizeof(val); > } > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > optlen)) > return -EFAULT; > > if (val.spt_pathpfthld > val.spt_pathcpthld) > return -EINVAL; > > Which means we will 'skip' spt_pathcpthld if (it's using old format) or > (ps_retrans is disabled and it's using new format). This will be inconsistent if we end up having to add another field after spt_pathcpthld, because it will get ignored if ps_retrans == 0xffff. Lets not optimize out the fields please. This is already very tricky to get right. > Note that ps_retrans < pf_retrans is not allowed in rfc7829. > > and in sctp_getsockopt_paddr_thresholds(): > > offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); > if (len < offset) > return -EINVAL; > if (len < sizeof(val) || sock_net(sk)->sctp.ps_retrans == 0xffff) > len = offset; > else > len = sizeof(val); Here it is more visible. If net->...ps_retrans is disabled, remaining fields (currently just this one, but as we are extending it now, we have to think about the possibility of more as well) will be ignored, we and we may not want that. > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) > return -EFAULT; > > > > > > > > > > if (val.spt_pathpfthld > val.spt_pathcpthld) > > > return -EINVAL; > > > > > > > > > > > > > > > > > David > > > > > > > > > > - > > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > > > Registration No: 1397386 (Wales) > > > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-12 22:51 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 37+ messages in thread From: Marcelo Ricardo Leitner @ 2019-09-12 22:51 UTC (permalink / raw) To: Xin Long; +Cc: David Laight, network dev, linux-sctp, Neil Horman, davem On Thu, Sep 12, 2019 at 01:47:08AM +0800, Xin Long wrote: > On Wed, Sep 11, 2019 at 8:56 PM Marcelo Ricardo Leitner > <marcelo.leitner@gmail.com> wrote: > > > > On Wed, Sep 11, 2019 at 05:38:33PM +0800, Xin Long wrote: > > > On Wed, Sep 11, 2019 at 5:21 PM Xin Long <lucien.xin@gmail.com> wrote: > > > > > > > > On Wed, Sep 11, 2019 at 5:03 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > From: Xin Long [mailto:lucien.xin@gmail.com] > > > > > > Sent: 11 September 2019 09:52 > > > > > > On Tue, Sep 10, 2019 at 9:19 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > > > > > > > From: Xin Long > > > > > > > > Sent: 09 September 2019 08:57 > > > > > > > > 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 that ps_retrans is not allowed to be greater than pf_retrans. > > > > > > > > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > > > > > > > --- > > > > > > > > include/uapi/linux/sctp.h | 1 + > > > > > > > > net/sctp/socket.c | 10 ++++++++++ > > > > > > > > 2 files changed, 11 insertions(+) > > > > > > > > > > > > > > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > > > > > > > index a15cc28..dfd81e1 100644 > > > > > > > > --- a/include/uapi/linux/sctp.h > > > > > > > > +++ b/include/uapi/linux/sctp.h > > > > > > > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > > > > > > > struct sockaddr_storage spt_address; > > > > > > > > __u16 spt_pathmaxrxt; > > > > > > > > __u16 spt_pathpfthld; > > > > > > > > + __u16 spt_pathcpthld; > > > > > > > > }; > > > > > > > > > > > > > > > > /* > > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c > > > > > > > > index 5e2098b..5b9774d 100644 > > > > > > > > --- a/net/sctp/socket.c > > > > > > > > +++ b/net/sctp/socket.c > > > > > > > > @@ -3954,6 +3954,9 @@ static int sctp_setsockopt_paddr_thresholds(struct sock *sk, > > > > > > > > > > > > > > This code does: > > > > > > > if (optlen < sizeof(struct sctp_paddrthlds)) > > > > > > > return -EINVAL; > > > > > > here will become: > > > > > > > > > > > > if (optlen >= sizeof(struct sctp_paddrthlds)) { > > > > > > optlen = sizeof(struct sctp_paddrthlds); > > > > > > } else if (optlen >= ALIGN(offsetof(struct sctp_paddrthlds, > > > > > > spt_pathcpthld), 4)) > > > > > > optlen = ALIGN(offsetof(struct sctp_paddrthlds, > > > > > > spt_pathcpthld), 4); > > > > > > val.spt_pathcpthld = 0xffff; > > > > > > else { > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > Hmmm... > > > > > If the kernel has to default 'val.spt_pathcpthld = 0xffff' > > > > > then recompiling an existing application with the new uapi > > > > > header is going to lead to very unexpected behaviour. > > > > > > > > > > The best you can hope for is that the application memset the > > > > > structure to zero. > > > > > But more likely it is 'random' on-stack data. > > > > 0xffff is a value to disable the feature 'Primary Path Switchover'. > > > > you're right that user might set it to zero unexpectly with their > > > > old application rebuilt. > > > > > > > > A safer way is to introduce "sysctl net.sctp.ps_retrans", it won't > > > > matter if users set spt_pathcpthld properly when they're not aware > > > > of this feature, like "sysctl net.sctp.pF_retrans". Looks better? > > > Sorry for confusing, "sysctl net.sctp.ps_retrans" is already there > > > (its value is 0xffff by default), > > > we just need to do this in sctp_setsockopt_paddr_thresholds(): > > > > > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > > > optlen)) > > > return -EFAULT; > > > > > > if (sock_net(sk)->sctp.ps_retrans = 0xffff) > > > val.spt_pathcpthld = 0xffff; > > > > I'm confused with the snippets, but if I got them right, this is after > > dealing with proper len and could leave val.spt_pathcpthld > > uninitialized if the application used the old format and sysctl is !> > 0xffff. > right, how about this in sctp_setsockopt_paddr_thresholds(): > > offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); > if (optlen < offset) > return -EINVAL; > if (optlen < sizeof(val) || sock_net(sk)->sctp.ps_retrans = 0xffff) { > optlen = offset; > val.spt_pathcpthld = 0xffff; > } else { > optlen = sizeof(val); > } > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, > optlen)) > return -EFAULT; > > if (val.spt_pathpfthld > val.spt_pathcpthld) > return -EINVAL; > > Which means we will 'skip' spt_pathcpthld if (it's using old format) or > (ps_retrans is disabled and it's using new format). This will be inconsistent if we end up having to add another field after spt_pathcpthld, because it will get ignored if ps_retrans = 0xffff. Lets not optimize out the fields please. This is already very tricky to get right. > Note that ps_retrans < pf_retrans is not allowed in rfc7829. > > and in sctp_getsockopt_paddr_thresholds(): > > offset = ALIGN(offsetof(struct sctp_paddrthlds, spt_pathcpthld), 4); > if (len < offset) > return -EINVAL; > if (len < sizeof(val) || sock_net(sk)->sctp.ps_retrans = 0xffff) > len = offset; > else > len = sizeof(val); Here it is more visible. If net->...ps_retrans is disabled, remaining fields (currently just this one, but as we are extending it now, we have to think about the possibility of more as well) will be ignored, we and we may not want that. > > if (copy_from_user(&val, (struct sctp_paddrthlds __user *)optval, len)) > return -EFAULT; > > > > > > > > > > if (val.spt_pathpfthld > val.spt_pathcpthld) > > > return -EINVAL; > > > > > > > > > > > > > > > > > David > > > > > > > > > > - > > > > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > > > > Registration No: 1397386 (Wales) > > > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-12 22:51 ` Marcelo Ricardo Leitner (?) @ 2019-09-13 8:36 ` David Laight 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' -1 siblings, 1 reply; 37+ messages in thread From: David Laight @ 2019-09-13 8:36 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner', Xin Long Cc: network dev, linux-sctp, Neil Horman, davem From: Marcelo Ricardo Leitner > Sent: 12 September 2019 23:52 ... > Here it is more visible. If net->...ps_retrans is disabled, remaining > fields (currently just this one, but as we are extending it now, we > have to think about the possibility of more as well) will be ignored, > we and we may not want that. The only real way to add additional fields is to change the name of the structure - that way recompiled programs still work. You could require that programs zero the entire structure - but that is difficult to verify. And, in this case, it seems that the default has to be 0xffff rather than 0 - which is, in itself, horrid. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-13 8:36 ` David Laight @ 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 37+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-09-13 13:19 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem On Fri, Sep 13, 2019 at 08:36:22AM +0000, David Laight wrote: > From: Marcelo Ricardo Leitner > > Sent: 12 September 2019 23:52 > ... > > Here it is more visible. If net->...ps_retrans is disabled, remaining > > fields (currently just this one, but as we are extending it now, we > > have to think about the possibility of more as well) will be ignored, > > we and we may not want that. > > The only real way to add additional fields is to change the name > of the structure - that way recompiled programs still work. > > You could require that programs zero the entire structure - but > that is difficult to verify. > And, in this case, it seems that the default has to be 0xffff > rather than 0 - which is, in itself, horrid. Yep, and with that, a new sockopt as well. May not be the most beautiful way, but it's the safest. Applications can then probe if the sockopt is available or not and use what they want/can. Inner kernel code can then be rearranged like it was for the peeloff operation and peeloff + flags, and these issues just don't exist then. We actually had agreed on using new sockopts, on thread [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length Interestingly, we have/had the opposite problem with netlink. Like, it was allowing too much flexibility, such as silently ignoring unknown fields (which is what would happen with a new app running on an older kernel would trigger here) is bad because the app cannot know if it was actually used or not. Some gymnastics in the app could cut through the fat here, like probing getsockopt() return size, but then it may as well probe for the right sockopt to be used. Marcelo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 37+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-09-13 13:19 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem On Fri, Sep 13, 2019 at 08:36:22AM +0000, David Laight wrote: > From: Marcelo Ricardo Leitner > > Sent: 12 September 2019 23:52 > ... > > Here it is more visible. If net->...ps_retrans is disabled, remaining > > fields (currently just this one, but as we are extending it now, we > > have to think about the possibility of more as well) will be ignored, > > we and we may not want that. > > The only real way to add additional fields is to change the name > of the structure - that way recompiled programs still work. > > You could require that programs zero the entire structure - but > that is difficult to verify. > And, in this case, it seems that the default has to be 0xffff > rather than 0 - which is, in itself, horrid. Yep, and with that, a new sockopt as well. May not be the most beautiful way, but it's the safest. Applications can then probe if the sockopt is available or not and use what they want/can. Inner kernel code can then be rearranged like it was for the peeloff operation and peeloff + flags, and these issues just don't exist then. We actually had agreed on using new sockopts, on thread [PATCH net] sctp: make sctp_setsockopt_events() less strict about the option length Interestingly, we have/had the opposite problem with netlink. Like, it was allowing too much flexibility, such as silently ignoring unknown fields (which is what would happen with a new app running on an older kernel would trigger here) is bad because the app cannot know if it was actually used or not. Some gymnastics in the app could cut through the fat here, like probing getsockopt() return size, but then it may as well probe for the right sockopt to be used. Marcelo ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' (?) @ 2019-09-13 13:31 ` David Laight 2019-09-13 13:40 ` 'Marcelo Ricardo Leitner' -1 siblings, 1 reply; 37+ messages in thread From: David Laight @ 2019-09-13 13:31 UTC (permalink / raw) To: 'Marcelo Ricardo Leitner' Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem From: 'Marcelo Ricardo Leitner' > Sent: 13 September 2019 14:20 ... > Interestingly, we have/had the opposite problem with netlink. Like, it > was allowing too much flexibility, such as silently ignoring unknown > fields (which is what would happen with a new app running on an older > kernel would trigger here) is bad because the app cannot know if it > was actually used or not. Some gymnastics in the app could cut through > the fat here, like probing getsockopt() return size, but then it may > as well probe for the right sockopt to be used. Yes, it would also work if the kernel checked that all 'unexpected' fields were zero (up to some sanity limit of a few kB). Then an application complied with a 'new' header would work with an old kernel provided it didn't try so set any new fields. (And it zeroed the entire structure.) But you have to start off with that in mind. Alternatively stop the insanity of setting multiple options with one setsockopt call. If multiple system calls are an issue implement a system call that will set multiple options on the same socket. (Maybe through a CMSG()-like buffer). Then the application can set the ones it wants without having to do the read-modify-write sequence needed for some of the SCTP ones. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-13 13:31 ` David Laight @ 2019-09-13 13:40 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 37+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-09-13 13:40 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem On Fri, Sep 13, 2019 at 01:31:22PM +0000, David Laight wrote: > From: 'Marcelo Ricardo Leitner' > > Sent: 13 September 2019 14:20 > ... > > Interestingly, we have/had the opposite problem with netlink. Like, it > > was allowing too much flexibility, such as silently ignoring unknown > > fields (which is what would happen with a new app running on an older > > kernel would trigger here) is bad because the app cannot know if it > > was actually used or not. Some gymnastics in the app could cut through > > the fat here, like probing getsockopt() return size, but then it may > > as well probe for the right sockopt to be used. > > Yes, it would also work if the kernel checked that all 'unexpected' > fields were zero (up to some sanity limit of a few kB). Though this would have to be done by older kernels, which are not aware of this extra space by definition. > > Then an application complied with a 'new' header would work with > an old kernel provided it didn't try so set any new fields. > (And it zeroed the entire structure.) > > But you have to start off with that in mind. > > Alternatively stop the insanity of setting multiple options > with one setsockopt call. > If multiple system calls are an issue implement a system call > that will set multiple options on the same socket. > (Maybe through a CMSG()-like buffer). > Then the application can set the ones it wants without having > to do the read-modify-write sequence needed for some of the > SCTP ones. I'm not sure I get you here. You mean we could have, for example, one sockopt for each field on each struct we currently have? That would bring other problems to the table, like how to deal with fields that need to be updated together. Anyhow, I'm afraid our hands a bit tied here. That's how the RFCs are defining the interface and we shouldn't deviate too much from it. What would help is that the RFC definited these versioned structs itself. Because as it is, even if we start versioning it, Linux will have one versioning and other OSes will have another. Marcelo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-13 13:40 ` 'Marcelo Ricardo Leitner' 0 siblings, 0 replies; 37+ messages in thread From: 'Marcelo Ricardo Leitner' @ 2019-09-13 13:40 UTC (permalink / raw) To: David Laight; +Cc: Xin Long, network dev, linux-sctp, Neil Horman, davem On Fri, Sep 13, 2019 at 01:31:22PM +0000, David Laight wrote: > From: 'Marcelo Ricardo Leitner' > > Sent: 13 September 2019 14:20 > ... > > Interestingly, we have/had the opposite problem with netlink. Like, it > > was allowing too much flexibility, such as silently ignoring unknown > > fields (which is what would happen with a new app running on an older > > kernel would trigger here) is bad because the app cannot know if it > > was actually used or not. Some gymnastics in the app could cut through > > the fat here, like probing getsockopt() return size, but then it may > > as well probe for the right sockopt to be used. > > Yes, it would also work if the kernel checked that all 'unexpected' > fields were zero (up to some sanity limit of a few kB). Though this would have to be done by older kernels, which are not aware of this extra space by definition. > > Then an application complied with a 'new' header would work with > an old kernel provided it didn't try so set any new fields. > (And it zeroed the entire structure.) > > But you have to start off with that in mind. > > Alternatively stop the insanity of setting multiple options > with one setsockopt call. > If multiple system calls are an issue implement a system call > that will set multiple options on the same socket. > (Maybe through a CMSG()-like buffer). > Then the application can set the ones it wants without having > to do the read-modify-write sequence needed for some of the > SCTP ones. I'm not sure I get you here. You mean we could have, for example, one sockopt for each field on each struct we currently have? That would bring other problems to the table, like how to deal with fields that need to be updated together. Anyhow, I'm afraid our hands a bit tied here. That's how the RFCs are defining the interface and we shouldn't deviate too much from it. What would help is that the RFC definited these versioned structs itself. Because as it is, even if we start versioning it, Linux will have one versioning and other OSes will have another. Marcelo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-09 7:56 ` Xin Long @ 2019-09-10 17:27 ` David Miller -1 siblings, 0 replies; 37+ messages in thread From: David Miller @ 2019-09-10 17:27 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman From: Xin Long <lucien.xin@gmail.com> Date: Mon, 9 Sep 2019 15:56:51 +0800 > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index a15cc28..dfd81e1 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > struct sockaddr_storage spt_address; > __u16 spt_pathmaxrxt; > __u16 spt_pathpfthld; > + __u16 spt_pathcpthld; > }; > > /* As pointed out you can't add things to this structure without breaking existing binaries. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-10 17:27 ` David Miller 0 siblings, 0 replies; 37+ messages in thread From: David Miller @ 2019-09-10 17:27 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman From: Xin Long <lucien.xin@gmail.com> Date: Mon, 9 Sep 2019 15:56:51 +0800 > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index a15cc28..dfd81e1 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > struct sockaddr_storage spt_address; > __u16 spt_pathmaxrxt; > __u16 spt_pathpfthld; > + __u16 spt_pathcpthld; > }; > > /* As pointed out you can't add things to this structure without breaking existing binaries. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds 2019-09-10 17:27 ` David Miller @ 2019-09-11 8:14 ` Xin Long -1 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 8:14 UTC (permalink / raw) To: David Miller Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman On Wed, Sep 11, 2019 at 1:27 AM David Miller <davem@davemloft.net> wrote: > > From: Xin Long <lucien.xin@gmail.com> > Date: Mon, 9 Sep 2019 15:56:51 +0800 > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index a15cc28..dfd81e1 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > struct sockaddr_storage spt_address; > > __u16 spt_pathmaxrxt; > > __u16 spt_pathpfthld; > > + __u16 spt_pathcpthld; > > }; > > > > /* > > As pointed out you can't add things to this structure without breaking existing > binaries. we had the same problem when adding: spp_ipv6_flowlabel and spp_dscp into struct sctp_paddrparams. in: commit 0b0dce7a36fb9f1a9dd8245ea82d3a268c6943fe Author: Xin Long <lucien.xin@gmail.com> Date: Mon Jul 2 18:21:13 2018 +0800 sctp: add spp_ipv6_flowlabel and spp_dscp for sctp_paddrparams the solution was: if (optlen == sizeof(params)) { if (copy_from_user(¶ms, optval, optlen)) return -EFAULT; } else if (optlen == ALIGN(offsetof(struct sctp_paddrparams, spp_ipv6_flowlabel), 4)) { if (copy_from_user(¶ms, optval, optlen)) return -EFAULT; if (params.spp_flags & (SPP_DSCP | SPP_IPV6_FLOWLABEL)) return -EINVAL; } else { return -EINVAL; } I will do the same for this patch. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds @ 2019-09-11 8:14 ` Xin Long 0 siblings, 0 replies; 37+ messages in thread From: Xin Long @ 2019-09-11 8:14 UTC (permalink / raw) To: David Miller Cc: network dev, linux-sctp, Marcelo Ricardo Leitner, Neil Horman On Wed, Sep 11, 2019 at 1:27 AM David Miller <davem@davemloft.net> wrote: > > From: Xin Long <lucien.xin@gmail.com> > Date: Mon, 9 Sep 2019 15:56:51 +0800 > > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index a15cc28..dfd81e1 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -1069,6 +1069,7 @@ struct sctp_paddrthlds { > > struct sockaddr_storage spt_address; > > __u16 spt_pathmaxrxt; > > __u16 spt_pathpfthld; > > + __u16 spt_pathcpthld; > > }; > > > > /* > > As pointed out you can't add things to this structure without breaking existing > binaries. we had the same problem when adding: spp_ipv6_flowlabel and spp_dscp into struct sctp_paddrparams. in: commit 0b0dce7a36fb9f1a9dd8245ea82d3a268c6943fe Author: Xin Long <lucien.xin@gmail.com> Date: Mon Jul 2 18:21:13 2018 +0800 sctp: add spp_ipv6_flowlabel and spp_dscp for sctp_paddrparams the solution was: if (optlen = sizeof(params)) { if (copy_from_user(¶ms, optval, optlen)) return -EFAULT; } else if (optlen = ALIGN(offsetof(struct sctp_paddrparams, spp_ipv6_flowlabel), 4)) { if (copy_from_user(¶ms, optval, optlen)) return -EFAULT; if (params.spp_flags & (SPP_DSCP | SPP_IPV6_FLOWLABEL)) return -EINVAL; } else { return -EINVAL; } I will do the same for this patch. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2019-09-13 13:40 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-09 7:56 [PATCH net-next 0/5] sctp: update from rfc7829 Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-09 7:56 ` [PATCH net-next 1/5] sctp: add SCTP_ADDR_POTENTIALLY_FAILED notification Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-09 7:56 ` [PATCH net-next 2/5] sctp: add pf_expose per netns and sock and asoc Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-09 7:56 ` [PATCH net-next 3/5] sctp: add SCTP_EXPOSE_POTENTIALLY_FAILED_STATE sockopt Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-09 7:56 ` [PATCH net-next 4/5] sctp: add support for Primary Path Switchover Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-09 7:56 ` [PATCH net-next 5/5] sctp: add spt_pathcpthld in struct sctp_paddrthlds Xin Long 2019-09-09 7:56 ` Xin Long 2019-09-10 13:19 ` David Laight 2019-09-11 8:51 ` Xin Long 2019-09-11 8:51 ` Xin Long 2019-09-11 9:03 ` David Laight 2019-09-11 9:03 ` David Laight 2019-09-11 9:21 ` Xin Long 2019-09-11 9:21 ` Xin Long 2019-09-11 9:38 ` Xin Long 2019-09-11 9:38 ` Xin Long 2019-09-11 12:56 ` Marcelo Ricardo Leitner 2019-09-11 12:56 ` Marcelo Ricardo Leitner 2019-09-11 17:47 ` Xin Long 2019-09-11 17:47 ` Xin Long 2019-09-12 22:51 ` Marcelo Ricardo Leitner 2019-09-12 22:51 ` Marcelo Ricardo Leitner 2019-09-13 8:36 ` David Laight 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' 2019-09-13 13:19 ` 'Marcelo Ricardo Leitner' 2019-09-13 13:31 ` David Laight 2019-09-13 13:40 ` 'Marcelo Ricardo Leitner' 2019-09-13 13:40 ` 'Marcelo Ricardo Leitner' 2019-09-10 17:27 ` David Miller 2019-09-10 17:27 ` David Miller 2019-09-11 8:14 ` Xin Long 2019-09-11 8:14 ` Xin Long
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.