Patchwork [1/2] sctp: do not enable peer features if we can't do them.

login
register
mail settings
Submitter Vlad Yasevich
Date Sept. 18, 2008, 9:31 p.m.
Message ID <1221773464-28845-1-git-send-email-vladislav.yasevich@hp.com>
Download mbox | patch
Permalink /patch/552/
State Accepted
Headers show

Comments

Vlad Yasevich - Sept. 18, 2008, 9:31 p.m.
Do not enable peer features like addip and auth, if they
are administratively disabled localy.  If the peer resports
that he supports something that we don't, neither end can
use it so enabling it is pointless.  This solves a problem
when talking to a peer that has auth and addip enabled while
we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.

Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Vlad Yasevich - Sept. 18, 2008, 11:01 p.m.
David

Can you also queue this one and Patch 2/2 to stable.  The problems
are there as well.

Thanks
-vlad

Vlad Yasevich wrote:
> Do not enable peer features like addip and auth, if they
> are administratively disabled localy.  If the peer resports
> that he supports something that we don't, neither end can
> use it so enabling it is pointless.  This solves a problem
> when talking to a peer that has auth and addip enabled while
> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
>  net/sctp/sm_make_chunk.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index e8ca4e5..fe94f42 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1886,11 +1886,13 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
>  			    /* if the peer reports AUTH, assume that he
>  			     * supports AUTH.
>  			     */
> -			    asoc->peer.auth_capable = 1;
> +			    if (sctp_auth_enable)
> +				    asoc->peer.auth_capable = 1;
>  			    break;
>  		    case SCTP_CID_ASCONF:
>  		    case SCTP_CID_ASCONF_ACK:
> -			    asoc->peer.asconf_capable = 1;
> +			    if (sctp_addip_enable)
> +				    asoc->peer.asconf_capable = 1;
>  			    break;
>  		    default:
>  			    break;
> @@ -2460,6 +2462,9 @@ do_addr_param:
>  		break;
>  
>  	case SCTP_PARAM_SET_PRIMARY:
> +		if (!sctp_addip_enable)
> +			goto fall_through;
> +
>  		addr_param = param.v + sizeof(sctp_addip_param_t);
>  
>  		af = sctp_get_af_specific(param_type2af(param.p->type));
David Miller - Sept. 18, 2008, 11:18 p.m.
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 18 Sep 2008 19:01:23 -0400

> Can you also queue this one and Patch 2/2 to stable.  The problems
> are there as well.

Sure thing.
David Miller - Sept. 18, 2008, 11:29 p.m.
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 18 Sep 2008 17:31:03 -0400

> Do not enable peer features like addip and auth, if they
> are administratively disabled localy.  If the peer resports
> that he supports something that we don't, neither end can
> use it so enabling it is pointless.  This solves a problem
> when talking to a peer that has auth and addip enabled while
> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>

I applied this, but it is at best borderline for outside the
merge window.  It doesn't fix an OOPS nor a security issue nor
an entry in the 2.6.x regression list, therefore strictly speaking
this fix is not appropriate at this time.

Please apply this criteria when deciding whether to submit future
fixes for net-2.6 inclusion.
Vlad Yasevich - Sept. 19, 2008, 3:01 a.m.
David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Thu, 18 Sep 2008 17:31:03 -0400
> 
>> Do not enable peer features like addip and auth, if they
>> are administratively disabled localy.  If the peer resports
>> that he supports something that we don't, neither end can
>> use it so enabling it is pointless.  This solves a problem
>> when talking to a peer that has auth and addip enabled while
>> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
>>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> I applied this, but it is at best borderline for outside the
> merge window.  It doesn't fix an OOPS nor a security issue nor
> an entry in the 2.6.x regression list, therefore strictly speaking
> this fix is not appropriate at this time.
> 
> Please apply this criteria when deciding whether to submit future
> fixes for net-2.6 inclusion.
> 

It is a major interoperability issue.  With the default sysctl settings,
we can not establish connection to BSD systems.  Yes, there is a workaround
of turning on the 2 required sysctl settings, but that is totally suboptimal.

I've thought about this fix for a while, and in my opinion, the interoperability
problem is large enough to warrant the fix at this time and the backport to
table.

Of course you are free to not include this in net-2.6, but I hope you will.

Thanks
-vlad

Patch

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index e8ca4e5..fe94f42 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1886,11 +1886,13 @@  static void sctp_process_ext_param(struct sctp_association *asoc,
 			    /* if the peer reports AUTH, assume that he
 			     * supports AUTH.
 			     */
-			    asoc->peer.auth_capable = 1;
+			    if (sctp_auth_enable)
+				    asoc->peer.auth_capable = 1;
 			    break;
 		    case SCTP_CID_ASCONF:
 		    case SCTP_CID_ASCONF_ACK:
-			    asoc->peer.asconf_capable = 1;
+			    if (sctp_addip_enable)
+				    asoc->peer.asconf_capable = 1;
 			    break;
 		    default:
 			    break;
@@ -2460,6 +2462,9 @@  do_addr_param:
 		break;
 
 	case SCTP_PARAM_SET_PRIMARY:
+		if (!sctp_addip_enable)
+			goto fall_through;
+
 		addr_param = param.v + sizeof(sctp_addip_param_t);
 
 		af = sctp_get_af_specific(param_type2af(param.p->type));