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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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

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

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

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

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

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

...

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

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

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