Re: [PATCHv3 net-next 2/5] sctp: add pf_expose per netns and sock and asoc
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: 2019-10-25 03:23:45
Also in:
linux-sctp
On Mon, Oct 14, 2019 at 02:14:45PM +0800, Xin Long wrote:
As said in rfc7829, section 3, point 12: The SCTP stack SHOULD expose the PF state of its destination addresses to the ULP as well as provide the means to notify the ULP of state transitions of its destination addresses from active to PF, and vice versa. However, it is recommended that an SCTP stack implementing SCTP-PF also allows for the ULP to be kept ignorant of the PF state of its destinations and the associated state transitions, thus allowing for retention of the simpler state transition model of [RFC4960] in the ULP. Not only does it allow to expose the PF state to ULP, but also allow to ignore sctp-pf to ULP. So this patch is to add pf_expose per netns, sock and asoc. And in sctp_assoc_control_transport(), ulp_notify will be set to false if asoc->expose is not set. It also allows a user to change pf_expose per netns by sysctl, and pf_expose per sock and asoc will be initialized with it.
I also do see value on this sysctl. We currently have an implementation that sits in between the states that the RFC defines and this allows the system to remain using the original Linux behavior, while also forcing especially the disabled state. This can help on porting applications to Linux.
Note that pf_expose also works for SCTP_GET_PEER_ADDR_INFO sockopt,
to not allow a user to query the state of a sctp-pf peer address
when pf_expose is not enabled, as said in section 7.3.
v1->v2:
- Fix a build warning noticed by Nathan Chancellor.
v2->v3:
- set pf_expose to UNUSED by default to keep compatible with old
applications.Hmmm UNUSED can be quite confusing. What about "UNSET" instead? (though I'm not that happy with UNSET either, but couldn't come up with a better name) And make UNSET=0. (first on the enum) So we have it like: "If unset, the exposure is done as Linux used to do it, while setting it to 1 blocks it and 2, enables it, according to the RFC." Needs a new entry on Documentation/ip-sysctl.txt, btw. We have pf_enable in there. ...
quoted hunk ↗ jump to hunk
@@ -5521,8 +5522,16 @@ static int sctp_getsockopt_peer_addr_info(struct sock *sk, int len, transport = sctp_addr_id2transport(sk, &pinfo.spinfo_address, pinfo.spinfo_assoc_id); - if (!transport) - return -EINVAL; + if (!transport) { + retval = -EINVAL; + goto out; + } + + if (transport->state == SCTP_PF && + transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) { + retval = -EACCES; + goto out; + }
As is on v3, this is NOT an UAPI violation. The user has to explicitly set the system or the socket into the disabled state in order to trigger this new check.
quoted hunk ↗ jump to hunk
pinfo.spinfo_assoc_id = sctp_assoc2id(transport->asoc); pinfo.spinfo_state = transport->state;diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c index 238cf17..5d1ad44 100644 --- a/net/sctp/sysctl.c +++ b/net/sctp/sysctl.c@@ -34,6 +34,7 @@ static int rto_alpha_min = 0; static int rto_beta_min = 0; static int rto_alpha_max = 1000; static int rto_beta_max = 1000; +static int pf_expose_max = SCTP_PF_EXPOSE_MAX; static unsigned long max_autoclose_min = 0; static unsigned long max_autoclose_max =@@ -318,6 +319,15 @@ static struct ctl_table sctp_net_table[] = { .mode = 0644, .proc_handler = proc_dointvec, }, + { + .procname = "pf_expose", + .data = &init_net.sctp.pf_expose, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax, + .extra1 = SYSCTL_ZERO, + .extra2 = &pf_expose_max, + }, { /* sentinel */ } };-- 2.1.0