Message-ID: <20181116151216.GA12928@neilslaptop.think-freely.org>
Date:   Fri, 16 Nov 2018 10:12:16 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     lucien xin <lucien.xin@...il.com>, netdev <netdev@...r.kernel.org>,
        linux-sctp@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

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@...driver.com wrote:
> 
> > On Thu, Nov 15, 2018 at 08:25:36PM -0200, Marcelo Ricardo Leitner wrote:
> > > On Thu, Nov 15, 2018 at 04:43:10PM -0500, Neil Horman wrote:
> > > > On Thu, Nov 15, 2018 at 03:22:21PM -0200, Marcelo Ricardo Leitner
> > wrote:
> > > > > On Thu, Nov 15, 2018 at 07:14:28PM +0800, Xin Long wrote:
> > > > > > 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
> > > > > >    associations, it allows one to query whether or not PR-SCTP
> > support
> > > > > >    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 <yinxu@...hat.com>
> > > > > > Signed-off-by: Xin Long <lucien.xin@...il.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,
> > > > > >                                         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,
> > > > > >                 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))
> > > > >
> > > > > This would allow using a non-existent assoc id on UDP-style sockets
> > to
> > > > > 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
> > > > association isn't found, so the use of sctp_id2assoc should work just
> > fine.
> > >
> > > 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
> > 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.
> 
> 
> > Neil
> >
> >
> >

Powered by blists - more mailing lists