On Thu, Nov 15, 2018 at 09:41:01PM -0200, Marcelo Ricardo Leitner wrote:

[ re-sending, without html this time ]

On Thu, Nov 15, 2018, 15:26 Neil Horman <nhorman@tuxdriver.com wrote:

quoted

On Thu, Nov 15, 2018 at 08:25:36PM -0200, Marcelo Ricardo Leitner wrote:

quoted

On Thu, Nov 15, 2018 at 04:43:10PM -0500, Neil Horman wrote:

quoted

On Thu, Nov 15, 2018 at 03:22:21PM -0200, Marcelo Ricardo Leitner

wrote:

quoted

quoted

quoted

On Thu, Nov 15, 2018 at 07:14:28PM +0800, Xin Long wrote:

quoted

As rfc7496#section4.5 says about SCTP_PR_SUPPORTED:

   This socket option allows the enabling or disabling of the
   negotiation of PR-SCTP support for future associations.  For

existing

quoted

quoted

quoted

quoted

   associations, it allows one to query whether or not PR-SCTP

support

quoted

quoted

quoted

quoted

   was negotiated on a particular association.

It means only sctp sock's prsctp_enable can be set.

Note that for the limitation of SCTP_{CURRENT|ALL}_ASSOC, we will
add it when introducing SCTP_{FUTURE|CURRENT|ALL}_ASSOC for linux
sctp in another patchset.

Fixes: 28aa4c26fce2 ("sctp: add SCTP_PR_SUPPORTED on sctp sockopt")
Reported-by: Ying Xu <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 739f3e5..e9b8232 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c

@@ -3940,7 +3940,6 @@ static int

sctp_setsockopt_pr_supported(struct sock *sk,

quoted

quoted

quoted

quoted

                                        unsigned int optlen)
 {
        struct sctp_assoc_value params;
-       struct sctp_association *asoc;
        int retval = -EINVAL;

        if (optlen != sizeof(params))

@@ -3951,16 +3950,10 @@ static int

sctp_setsockopt_pr_supported(struct sock *sk,

quoted

quoted

quoted

quoted

                goto out;
        }

-       asoc = sctp_id2assoc(sk, params.assoc_id);
-       if (asoc) {
-               asoc->prsctp_enable = !!params.assoc_value;
-       } else if (!params.assoc_id) {
-               struct sctp_sock *sp = sctp_sk(sk);
-
-               sp->ep->prsctp_enable = !!params.assoc_value;
-       } else {
+       if (sctp_style(sk, UDP) && sctp_id2assoc(sk,

params.assoc_id))

quoted

quoted

quoted

This would allow using a non-existent assoc id on UDP-style sockets

to

quoted

quoted

quoted

set it at the socket, which is not expected. It should be more like:

+ if (sctp_style(sk, UDP) && params.assoc_id)

How do you see that to be the case? sctp_id2assoc will return NULL if

an

quoted

quoted

association isn't found, so the use of sctp_id2assoc should work just

fine.

quoted

Right, it will return NULL, and because of that it won't bail out as
it should and will adjust the socket config instead.

Oh, duh, you're absolutely right, NULL will evalutate to false there, and
skip
the conditional goto out;

that said, It would make more sense to me to just change the sense of the
second
condition to !sctp_id2assoc(sk, params.assoc_id), so that we goto out if no
association is found.  it still seems a

That would break setting it on the socket without an assoc so far.

ok, yes, I see what xin is getting at now.  The RFC indicates that the
setsockopt method for this socket option is meant to set the prsctp enabled
value on _future_ associations, implying that we should not operate at all on
already existing associations (i.e. we should ignore the assoc_id in the passed
in structure and only operate on the socket).  That said, heres the entire text
of the RFC section:

4.5.  Socket Option for Getting and Setting the PR-SCTP Support
      (SCTP_PR_SUPPORTED)

   This socket option allows the enabling or disabling of the
   negotiation of PR-SCTP support for future associations.  For existing
   associations, it allows one to query whether or not PR-SCTP support
   was negotiated on a particular association.

   Whether or not PR-SCTP is enabled by default is implementation
   specific.

   This socket option uses IPPROTO_SCTP as its level and
   SCTP_PR_SUPPORTED as its name.  It can be used with getsockopt() and
   setsockopt().  The socket option value uses the following structure
   defined in [RFC6458]:

   struct sctp_assoc_value {
     sctp_assoc_t assoc_id;
     uint32_t assoc_value;
   };

   assoc_id:  This parameter is ignored for one-to-one style sockets.
      For one-to-many style sockets, this parameter indicates upon which
      association the user is performing an action.  The special
      sctp_assoc_t SCTP_FUTURE_ASSOC can also be used; it is an error to
      use SCTP_{CURRENT|ALL}_ASSOC in assoc_id.

   assoc_value:  A non-zero value encodes the enabling of PR-SCTP,
      whereas a value of 0 encodes the disabling of PR-SCTP.

   sctp_opt_info() needs to be extended to support SCTP_PR_SUPPORTED

My read of this suggests that for setting the prsctp_enabled flag, we only need
a valid socket (the presence or lack of associations is irrelevant), its only
for the getsockopt method that we need to specify an assoc_id, as the getsockopt
method operates on associations, while the setsockopt method operates at the
socket level (to be inherited as association init).

Given that, I'd argue that we can skip the check entirely, and just assign
sctp_sock(sk)->prsctp_enabled = !!param.assoc_value

directly.

Neil

bit dodgy to me to just check if

quoted

params.assoc_id is non-zero, as that will allow userspace to pass invalid
assoc
ids in and have those trigger pr support updates.

Quite the other way around, no? By only checking if associd is non zero and
exiting due to it we are making sure the user cannot use invalid IDs.

quoted

Neil