From: Xin Long <lucien.xin@gmail.com>

Sent: 19 October 2019 09:45
On Fri, Oct 18, 2019 at 11:34 PM David Laight [off-list ref] wrote:

quoted

From: Xin Long

quoted

Sent: 08 October 2019 12:25
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.

...

quoted

index 08d14d8..a303011 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;
+

For compatibility with existing applications pf_expose MUST default to 0.
I'm not even sure it makes sense to have a sysctl for it.

You're reivewing v2, pls go and check v3 where it's:

net->sctp.pf_expose = SCTP_PF_EXPOSE_UNUSED

I'll dig out that tri-state logic again later.

quoted

...

quoted

@@ -5521,8 +5522,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;
+     }

Ugg...
To avoid reporting the unexpected 'SCTP_PF' state you probable need
to lie about the state (probably reporting 'working' - or whatever state
it would be in if PF detection wasn't enabled.

return EACCES is from RFC. see v3 where it's become:

+       if (transport->state == SCTP_PF &&
+           transport->asoc->pf_expose == SCTP_PF_EXPOSE_DISABLE) {
+               retval = -EACCES;
+               goto out;
+       }

no more compatibility issue.

Hmmm....
Never mind what the RFC says about returning EACCESS, that
is still an API change.

quoted

quoted

--- 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,
+     },

Setting this will break existing applications.
So I don't think the default should be settable.

If the user sets this new sysctl, he must have realized what's going to happen.
I don't think this will cause "compatibility issue".

The problem is that support is application dependant, not system dependant.
All it takes is a distro to decide to default to enabling it and all old apps break.

Given the application has to enable other things there is no reason not to
require this to be enabled by every application that wants to see the events (etc).

Note that this is different from doing the protocol part of PF - which is likely
to help applications when the 'primary' path is dodgy.

	David

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