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

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

...

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.

...

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".

        David

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