[PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Xin Long]

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@redhat.com>
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,
 					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))
 		goto out;
-	}
+
+	sctp_sk(sk)->ep->prsctp_enable = !!params.assoc_value;
 
 	retval = 0;
 
-- 
2.1.0

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Marcelo Ricardo Leitner]

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@redhat.com>
> 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,
>  					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)

>  		goto out;
> -	}
> +
> +	sctp_sk(sk)->ep->prsctp_enable = !!params.assoc_value;
>  
>  	retval = 0;
>  
> -- 
> 2.1.0
> 

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Neil Horman]

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@redhat.com>
> > 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,
> >  					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.
Just checking params.assoc_id would instead fail the setting of any association
id that isn't 0, which I don't think is what we want at all.

Neil

> 
> >  		goto out;
> > -	}
> > +
> > +	sctp_sk(sk)->ep->prsctp_enable = !!params.assoc_value;
> >  
> >  	retval = 0;
> >  
> > -- 
> > 2.1.0
> > 
> 

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Marcelo Ricardo Leitner]

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@redhat.com>
> > > 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,
> > >  					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.

> Just checking params.assoc_id would instead fail the setting of any association
> id that isn't 0, which I don't think is what we want at all.

I think it is.

For exisitng associations, we can't set this anymore because it was
already negotiated on the handshake
(sctp_process_ext_param()/SCTP_CID_FWD_TSN) and there is no way back
after it. 
For non-existing assocs, they will always inherit it from the socket
value.

Question then is which semantics we want on validating the parameter
here. We have cases such as in sctp_setsockopt_delayed_ack() on which
it will reject using invalid asoc_ids as a way to mean the socket
itself for UDP-style sockets:

        asoc = sctp_id2assoc(sk, params.sack_assoc_id);
        if (!asoc && params.sack_assoc_id && sctp_style(sk, UDP))
                return -EINVAL;

As we are returning the same error for both situations(invalid assoc id
and setting it on existing asoc), we don't need the asoc pointer
itself and can avoid sctp_id2assoc() call, leading to the if() I
suggested.

  Marcelo

> 
> Neil
> 
> > 
> > >  		goto out;
> > > -	}
> > > +
> > > +	sctp_sk(sk)->ep->prsctp_enable = !!params.assoc_value;
> > >  
> > >  	retval = 0;
> > >  
> > > -- 
> > > 2.1.0
> > > 
> > 

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Neil Horman]

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@redhat.com>
> > > > 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,
> > > >  					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 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.

Neil

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Neil Horman]

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:
> 
> > 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@redhat.com>
> > > > > > 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,
> > > > > >                                         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
> >
> >
> >

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Xin Long]

On Sat, Nov 17, 2018 at 12:12 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> 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:
> >
> > > 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@redhat.com>
> > > > > > > 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,
> > > > > > >                                         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.
RFC seems to have no clear demands for this, I will just drop the check
in this patch, thanks.

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Marcelo Ricardo Leitner]

On Sun, Nov 18, 2018 at 04:02:25PM +0900, Xin Long wrote:
> On Sat, Nov 17, 2018 at 12:12 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > 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:
> > >
> > > > 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@redhat.com>
> > > > > > > > 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,
> > > > > > > >                                         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.
> RFC seems to have no clear demands for this, I will just drop the check
> in this patch, thanks.

RFC may not have clear demands, but I still don't see a reason for not
rejecting bogus arguments that can potentially lead to confusion.
We usually do argument parsing in the other way around: restrict as
much as possible, and relax when needed. That avoids applications to
build bad behaviors that we would end up having to cope with it.
Anyhow, I won't oppose to this any further.

@Dave: please give me till Tue to review the other patches. I'm
traveling and will be offline till Mon night. Thanks.

  Marcelo

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Neil Horman]

On Mon, Nov 19, 2018 at 02:14:50AM -0200, Marcelo Ricardo Leitner wrote:
> On Sun, Nov 18, 2018 at 04:02:25PM +0900, Xin Long wrote:
> > On Sat, Nov 17, 2018 at 12:12 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > 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:
> > > >
> > > > > 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@redhat.com>
> > > > > > > > > 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,
> > > > > > > > >                                         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.
> > RFC seems to have no clear demands for this, I will just drop the check
> > in this patch, thanks.
> 
> RFC may not have clear demands, but I still don't see a reason for not
> rejecting bogus arguments that can potentially lead to confusion.
> We usually do argument parsing in the other way around: restrict as
> much as possible, and relax when needed. That avoids applications to
> build bad behaviors that we would end up having to cope with it.
> Anyhow, I won't oppose to this any further.
> 

I think we could make the same argument about the assoc_value field on the
getsockopt method.  Its an output in that path, but we make no checks regarding
its input value.

It seems the long and short of it here is that this interface is overloaded for
multiple functions and some values are simply don't care states in certain
paths.  What we should do is document it as such in a subsequent version of the
RFC and any related man pages.  What we shouldn't do is impose artificial
constraints on those don't care values where none need to exist.

Neil

> @Dave: please give me till Tue to review the other patches. I'm
> traveling and will be offline till Mon night. Thanks.
> 
>   Marcelo
> 

Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt

[Xin Long]

On Fri, Nov 16, 2018 at 2:22 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> 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@redhat.com>
> > 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,
> >                                       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))
I got this semantic from BSD's SCTP_PR_SUPPORTED sockopt:
                        SCTP_FIND_STCB(inp, stcb, av->assoc_id);

                        if (stcb) {
                                SCTP_LTRACE_ERR_RET(...);
                                error = EINVAL;
                                SCTP_TCB_UNLOCK(stcb);
                        } else {
                                ...
                        }

>
> 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)
This way is more strict, but it seems reasonable.

When a user sets params.assoc_id for UDP type socket, it should be
thought as he WANTs to apply this on assoc, which is not allowed here.