[PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

The commit 8ffd3208 voids the previous patches f6778aab and 810c0719
for limiting the maximum autoclose value.  If userspace passes in -1
on 32-bit platform, the overflow check didn't work and autoclose
would be set to 0xffffffff.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrei Pelinescu-Onciul <andrei@iptel.org>
Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
Cc: David S. Miller <davem@davemloft.net>
---
 net/sctp/socket.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 13bf5fc..bb91281 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
 	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
-	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
+	sp->autoclose = min_t(unsigned long, sp->autoclose,
+			      MAX_SCHEDULE_TIMEOUT / HZ);
 
 	return 0;
 }
-- 
1.7.5.4

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Vladislav Yasevich]

On 12/08/2011 08:24 PM, Xi Wang wrote:
> The commit 8ffd3208 voids the previous patches f6778aab and 810c0719
> for limiting the maximum autoclose value.  If userspace passes in -1
> on 32-bit platform, the overflow check didn't work and autoclose
> would be set to 0xffffffff.
> 
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Andrei Pelinescu-Onciul <andrei@iptel.org>
> Cc: Vlad Yasevich <vladislav.yasevich@hp.com>
> Cc: David S. Miller <davem@davemloft.net>
> ---
>  net/sctp/socket.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 13bf5fc..bb91281 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2201,7 +2201,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
>  	if (copy_from_user(&sp->autoclose, optval, optlen))
>  		return -EFAULT;
>  	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
> -	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
> +	sp->autoclose = min_t(unsigned long, sp->autoclose,
> +			      MAX_SCHEDULE_TIMEOUT / HZ);

I think this should be u32 since that's what sp->autoclose is.

-vlad

>  
>  	return 0;
>  }

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

On Dec 9, 2011, at 12:38 PM, Vladislav Yasevich wrote:
> I think this should be u32 since that's what sp->autoclose is.

Do you mean something like this?

	min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ)

On 64-bit platform this would limit autoclose for no good
reason to

	(u32)(MAX_SCHEDULE_TIMEOUT / HZ),

which is basically 0x978d4fdf (assuming HZ is 250).  I guess the
intention was to allow autoclose to be any u32 on 64-bit platform.

- xi

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Vladislav Yasevich]

On 12/09/2011 01:04 PM, Xi Wang wrote:
> On Dec 9, 2011, at 12:38 PM, Vladislav Yasevich wrote:
>> I think this should be u32 since that's what sp->autoclose is.
> 
> Do you mean something like this?
> 
> 	min_t(u32, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ)
> 
> On 64-bit platform this would limit autoclose for no good
> reason to
> 
> 	(u32)(MAX_SCHEDULE_TIMEOUT / HZ),
> 
> which is basically 0x978d4fdf (assuming HZ is 250).  I guess the
> intention was to allow autoclose to be any u32 on 64-bit platform.
> 

Hm..  this is a bit strange.  This makes it so that on 32 bit platforms
we have one upper bound for autoclose and on 64 we have another even though
the type is platform dependent.  This could be considered a regression by
applications.

In addition this would result in confusion to user since the values
between setsockopt() and getsockopt() for autoclose would be different.

Looking at the latest spec, it actually looks like we are completely
mis-interpreting autoclose.  It's supposed to be in seconds.

For now, I'd suggest to make this consistent between 32 and 64 bits.
Having inconsistent values seems strange.

As a next set the api needs to be fixed to accept seconds as
argument.

-vlad

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

On Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote:
> Hm..  this is a bit strange.  This makes it so that on 32 bit platforms
> we have one upper bound for autoclose and on 64 we have another even though
> the type is platform dependent.  This could be considered a regression by
> applications.

Either looks good to me.  Timeout limit is essentially different on 32/64
platforms.

Another (probably uglier) option is to limit the value on 32-bit platform
only, like sock_setsockopt() in net/core/sock.c.

#if (BITS_PER_LONG == 32)
	if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
		sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;
#endif

> In addition this would result in confusion to user since the values
> between setsockopt() and getsockopt() for autoclose would be different.

Are you suggesting to reject the value and return -EINVAL, rather than
silently limiting the autoclose value?

- xi

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Vladislav Yasevich]

On 12/13/2011 05:00 PM, Xi Wang wrote:
> On Dec 12, 2011, at 5:18 PM, Vladislav Yasevich wrote:
>> Hm..  this is a bit strange.  This makes it so that on 32 bit platforms
>> we have one upper bound for autoclose and on 64 we have another even though
>> the type is platform dependent.  This could be considered a regression by
>> applications.
> 
> Either looks good to me.  Timeout limit is essentially different on 32/64
> platforms.

I don't think it really should be different.  Notice that our rto values
remain consistent.  I really thing that this should be consistent from
the user's point of view.

> 
> Another (probably uglier) option is to limit the value on 32-bit platform
> only, like sock_setsockopt() in net/core/sock.c.
> 
> #if (BITS_PER_LONG == 32)
> 	if (sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
> 		sp->autoclose = MAX_SCHEDULE_TIMEOUT / HZ;
> #endif

I agree, this is ugly.  It might make more sense to define a max autoclose
value and expose it through /sys.  That way the values remains consistent.

-vlad

> 
>> In addition this would result in confusion to user since the values
>> between setsockopt() and getsockopt() for autoclose would be different.
> 
> Are you suggesting to reject the value and return -EINVAL, rather than
> silently limiting the autoclose value?
> 
> - xi
> 

Re: [PATCH RESEND] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

On Dec 13, 2011, at 5:15 PM, Vladislav Yasevich wrote:
> I agree, this is ugly.  It might make more sense to define a max autoclose
> value and expose it through /sys.  That way the values remains consistent.

Hmm, this sounds nice.  I will give it a try in v2. 

- xi

[PATCH v2] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

Commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for
limiting the maximum autoclose value.  If userspace passes in -1 on
32-bit platform, the overflow check didn't work and autoclose would be
set to 0xffffffff.

This patch defines a max_autoclose for limiting the value and exposes
it through sysctl.

Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 include/net/sctp/structs.h |    4 ++++
 net/sctp/protocol.c        |    3 +++
 net/sctp/socket.c          |    4 ++--
 net/sctp/sysctl.c          |    7 +++++++
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e90e7a9..781f16d 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -241,6 +241,9 @@ extern struct sctp_globals {
 	 * bits is an indicator of when to send and window update SACK.
 	 */
 	int rwnd_update_shift;
+
+	/* Threshold for autoclose timeout. */
+	unsigned int max_autoclose;
 } sctp_globals;
 
 #define sctp_rto_initial		(sctp_globals.rto_initial)
@@ -281,6 +284,7 @@ extern struct sctp_globals {
 #define sctp_auth_enable		(sctp_globals.auth_enable)
 #define sctp_checksum_disable		(sctp_globals.checksum_disable)
 #define sctp_rwnd_upd_shift		(sctp_globals.rwnd_update_shift)
+#define sctp_max_autoclose		(sctp_globals.max_autoclose)
 
 /* SCTP Socket type: UDP or TCP style. */
 typedef enum {
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 61b9fca..4e07b18 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void)
 	sctp_max_instreams    		= SCTP_DEFAULT_INSTREAMS;
 	sctp_max_outstreams   		= SCTP_DEFAULT_OUTSTREAMS;
 
+	/* Initialize maximum autoclose timeout. */
+	sctp_max_autoclose		= INT_MAX;
+
 	/* Initialize handle used for association ids. */
 	idr_init(&sctp_assocs_id);
 
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 13bf5fc..f8c8e66 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2200,8 +2200,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 		return -EINVAL;
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
-	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
-	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
+	/* make sure it won't exceed sctp_max_autoclose / HZ */
+	sp->autoclose = min(sp->autoclose, sctp_max_autoclose / HZ);
 
 	return 0;
 }
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 6b39529..b74686e 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -258,6 +258,13 @@ static ctl_table sctp_table[] = {
 		.extra1		= &one,
 		.extra2		= &rwnd_scale_max,
 	},
+	{
+		.procname	= "max_autoclose",
+		.data		= &sctp_max_autoclose,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec_jiffies,
+	},
 
 	{ /* sentinel */ }
 };
-- 
1.7.5.4

Re: [PATCH v2] sctp: fix incorrect overflow check on autoclose

[Vlad Yasevich]

On 12/14/2011 04:48 PM, Xi Wang wrote:
> Commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for
> limiting the maximum autoclose value.  If userspace passes in -1 on
> 32-bit platform, the overflow check didn't work and autoclose would be
> set to 0xffffffff.
> 
> This patch defines a max_autoclose for limiting the value and exposes
> it through sysctl.
> 
> Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> ---
>  include/net/sctp/structs.h |    4 ++++
>  net/sctp/protocol.c        |    3 +++
>  net/sctp/socket.c          |    4 ++--
>  net/sctp/sysctl.c          |    7 +++++++
>  4 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index e90e7a9..781f16d 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -241,6 +241,9 @@ extern struct sctp_globals {
>  	 * bits is an indicator of when to send and window update SACK.
>  	 */
>  	int rwnd_update_shift;
> +
> +	/* Threshold for autoclose timeout. */
> +	unsigned int max_autoclose;
>  } sctp_globals;
>  
>  #define sctp_rto_initial		(sctp_globals.rto_initial)
> @@ -281,6 +284,7 @@ extern struct sctp_globals {
>  #define sctp_auth_enable		(sctp_globals.auth_enable)
>  #define sctp_checksum_disable		(sctp_globals.checksum_disable)
>  #define sctp_rwnd_upd_shift		(sctp_globals.rwnd_update_shift)
> +#define sctp_max_autoclose		(sctp_globals.max_autoclose)
>  
>  /* SCTP Socket type: UDP or TCP style. */
>  typedef enum {
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 61b9fca..4e07b18 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void)
>  	sctp_max_instreams    		= SCTP_DEFAULT_INSTREAMS;
>  	sctp_max_outstreams   		= SCTP_DEFAULT_OUTSTREAMS;
>  
> +	/* Initialize maximum autoclose timeout. */
> +	sctp_max_autoclose		= INT_MAX;
> +
>  	/* Initialize handle used for association ids. */
>  	idr_init(&sctp_assocs_id);
>  
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 13bf5fc..f8c8e66 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2200,8 +2200,8 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
>  		return -EINVAL;
>  	if (copy_from_user(&sp->autoclose, optval, optlen))
>  		return -EFAULT;
> -	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
> -	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
> +	/* make sure it won't exceed sctp_max_autoclose / HZ */
> +	sp->autoclose = min(sp->autoclose, sctp_max_autoclose / HZ);
>  
>  	return 0;
>  }
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 6b39529..b74686e 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -258,6 +258,13 @@ static ctl_table sctp_table[] = {
>  		.extra1		= &one,
>  		.extra2		= &rwnd_scale_max,
>  	},
> +	{
> +		.procname	= "max_autoclose",
> +		.data		= &sctp_max_autoclose,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode		= 0644,
> +		.proc_handler	= &proc_dointvec_jiffies,
> +	},
>

I think it would be better to keep this value in seconds and get rid
of division in the setsockopt code.  We could then have a min and max
values where max value could be something like 2 days.  I really don't
see an autoclose value that is bigger then that being very useful.  In
fact, most of the time these values are very small as one wants to close
out idle associations.

-vlad

>  	{ /* sentinel */ }
>  };

Re: [PATCH v2] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

On Dec 15, 2011, at 4:07 PM, Vlad Yasevich wrote:
> I think it would be better to keep this value in seconds and get rid
> of division in the setsockopt code.  We could then have a min and max
> values where max value could be something like 2 days.  I really don't
> see an autoclose value that is bigger then that being very useful.  In
> fact, most of the time these values are very small as one wants to close
> out idle associations.

Now I start to think exposing a new sysctl option might be a little
overkill since autoclose is often small.

How about this?

1) Simply store autoclose in seconds in setsockopt.

2) Avoid overflow in associola.c.

	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
		(sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
		? MAX_SCHEDULE_TIMEOUT
		: (unsigned long)sp->autoclose * HZ;

Or we could use INT_MAX instead of MAX_SCHEDULE_TIMEOUT if you want to
keep that value consistent across 32/64 bits.

- xi

Re: [PATCH v2] sctp: fix incorrect overflow check on autoclose

[Vlad Yasevich]

On 12/15/2011 05:13 PM, Xi Wang wrote:
> On Dec 15, 2011, at 4:07 PM, Vlad Yasevich wrote:
>> I think it would be better to keep this value in seconds and get rid
>> of division in the setsockopt code.  We could then have a min and max
>> values where max value could be something like 2 days.  I really don't
>> see an autoclose value that is bigger then that being very useful.  In
>> fact, most of the time these values are very small as one wants to close
>> out idle associations.
> 
> Now I start to think exposing a new sysctl option might be a little
> overkill since autoclose is often small.
> 
> How about this?
> 
> 1) Simply store autoclose in seconds in setsockopt.
> 
> 2) Avoid overflow in associola.c.
> 
> 	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
> 		(sp->autoclose > MAX_SCHEDULE_TIMEOUT / HZ)
> 		? MAX_SCHEDULE_TIMEOUT
> 		: (unsigned long)sp->autoclose * HZ;
> 
> Or we could use INT_MAX instead of MAX_SCHEDULE_TIMEOUT if you want to
> keep that value consistent across 32/64 bits.

This would work as well.  I do like the max configurable though as it
might be a nice feature, but the above code is exactly what I was
thinking about too.

-vlad

> 
> - xi
> 

Re: [PATCH v2] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

On Dec 16, 2011, at 8:00 AM, Vlad Yasevich wrote:
> This would work as well.  I do like the max configurable though as it
> might be a nice feature, but the above code is exactly what I was
> thinking about too.

Okay I will do a v3 based on that with the max autoclose. ;-)

- xi

[PATCH v3] sctp: fix incorrect overflow check on autoclose

[Xi Wang]

Commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for
limiting the autoclose value.  If userspace passes in -1 on 32-bit
platform, the overflow check didn't work and autoclose would be set
to 0xffffffff.

This patch defines a max_autoclose (in seconds) for limiting the value
and exposes it through sysctl, with the following intentions.

1) Avoid overflowing autoclose * HZ.

2) Keep the default autoclose bound consistent across 32- and 64-bit
   platforms (INT_MAX / HZ in this patch).

3) Keep the autoclose value consistent between setsockopt() and
   getsockopt() calls.

Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 include/net/sctp/structs.h |    4 ++++
 net/sctp/associola.c       |    2 +-
 net/sctp/protocol.c        |    3 +++
 net/sctp/socket.c          |    2 --
 net/sctp/sysctl.c          |   13 +++++++++++++
 5 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e90e7a9..a15432da 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -241,6 +241,9 @@ extern struct sctp_globals {
 	 * bits is an indicator of when to send and window update SACK.
 	 */
 	int rwnd_update_shift;
+
+	/* Threshold for autoclose timeout, in seconds. */
+	unsigned long max_autoclose;
 } sctp_globals;
 
 #define sctp_rto_initial		(sctp_globals.rto_initial)
@@ -281,6 +284,7 @@ extern struct sctp_globals {
 #define sctp_auth_enable		(sctp_globals.auth_enable)
 #define sctp_checksum_disable		(sctp_globals.checksum_disable)
 #define sctp_rwnd_upd_shift		(sctp_globals.rwnd_update_shift)
+#define sctp_max_autoclose		(sctp_globals.max_autoclose)
 
 /* SCTP Socket type: UDP or TCP style. */
 typedef enum {
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 152b5b3..acd2edb 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -173,7 +173,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
-		(unsigned long)sp->autoclose * HZ;
+		min_t(unsigned long, sp->autoclose, sctp_max_autoclose) * HZ;
 
 	/* Initializes the timers */
 	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 61b9fca..6f6ad86 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void)
 	sctp_max_instreams    		= SCTP_DEFAULT_INSTREAMS;
 	sctp_max_outstreams   		= SCTP_DEFAULT_OUTSTREAMS;
 
+	/* Initialize maximum autoclose timeout. */
+	sctp_max_autoclose		= INT_MAX / HZ;
+
 	/* Initialize handle used for association ids. */
 	idr_init(&sctp_assocs_id);
 
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 13bf5fc..54a7cd2 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2200,8 +2200,6 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
 		return -EINVAL;
 	if (copy_from_user(&sp->autoclose, optval, optlen))
 		return -EFAULT;
-	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
-	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
 
 	return 0;
 }
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 6b39529..60ffbd0 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -53,6 +53,10 @@ static int sack_timer_min = 1;
 static int sack_timer_max = 500;
 static int addr_scope_max = 3; /* check sctp_scope_policy_t in include/net/sctp/constants.h for max entries */
 static int rwnd_scale_max = 16;
+static unsigned long max_autoclose_min = 0;
+static unsigned long max_autoclose_max =
+	(MAX_SCHEDULE_TIMEOUT / HZ > UINT_MAX)
+	? UINT_MAX : MAX_SCHEDULE_TIMEOUT / HZ;
 
 extern long sysctl_sctp_mem[3];
 extern int sysctl_sctp_rmem[3];
@@ -258,6 +262,15 @@ static ctl_table sctp_table[] = {
 		.extra1		= &one,
 		.extra2		= &rwnd_scale_max,
 	},
+	{
+		.procname	= "max_autoclose",
+		.data		= &sctp_max_autoclose,
+		.maxlen		= sizeof(unsigned long),
+		.mode		= 0644,
+		.proc_handler	= &proc_doulongvec_minmax,
+		.extra1		= &max_autoclose_min,
+		.extra2		= &max_autoclose_max,
+	},
 
 	{ /* sentinel */ }
 };
-- 
1.7.5.4

Re: [PATCH v3] sctp: fix incorrect overflow check on autoclose

[Vladislav Yasevich]

On 12/16/2011 05:44 PM, Xi Wang wrote:
> Commit 8ffd3208 voids the previous patches f6778aab and 810c0719 for
> limiting the autoclose value.  If userspace passes in -1 on 32-bit
> platform, the overflow check didn't work and autoclose would be set
> to 0xffffffff.
> 
> This patch defines a max_autoclose (in seconds) for limiting the value
> and exposes it through sysctl, with the following intentions.
> 
> 1) Avoid overflowing autoclose * HZ.
> 
> 2) Keep the default autoclose bound consistent across 32- and 64-bit
>    platforms (INT_MAX / HZ in this patch).
> 
> 3) Keep the autoclose value consistent between setsockopt() and
>    getsockopt() calls.
> 
> Suggested-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>

Looks good to me.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

P.S. Sorry, took so long.  Just got back from a long vacation.
> ---
>  include/net/sctp/structs.h |    4 ++++
>  net/sctp/associola.c       |    2 +-
>  net/sctp/protocol.c        |    3 +++
>  net/sctp/socket.c          |    2 --
>  net/sctp/sysctl.c          |   13 +++++++++++++
>  5 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index e90e7a9..a15432da 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -241,6 +241,9 @@ extern struct sctp_globals {
>  	 * bits is an indicator of when to send and window update SACK.
>  	 */
>  	int rwnd_update_shift;
> +
> +	/* Threshold for autoclose timeout, in seconds. */
> +	unsigned long max_autoclose;
>  } sctp_globals;
>  
>  #define sctp_rto_initial		(sctp_globals.rto_initial)
> @@ -281,6 +284,7 @@ extern struct sctp_globals {
>  #define sctp_auth_enable		(sctp_globals.auth_enable)
>  #define sctp_checksum_disable		(sctp_globals.checksum_disable)
>  #define sctp_rwnd_upd_shift		(sctp_globals.rwnd_update_shift)
> +#define sctp_max_autoclose		(sctp_globals.max_autoclose)
>  
>  /* SCTP Socket type: UDP or TCP style. */
>  typedef enum {
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 152b5b3..acd2edb 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -173,7 +173,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
>  	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
> -		(unsigned long)sp->autoclose * HZ;
> +		min_t(unsigned long, sp->autoclose, sctp_max_autoclose) * HZ;
>  
>  	/* Initializes the timers */
>  	for (i = SCTP_EVENT_TIMEOUT_NONE; i < SCTP_NUM_TIMEOUT_TYPES; ++i)
> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
> index 61b9fca..6f6ad86 100644
> --- a/net/sctp/protocol.c
> +++ b/net/sctp/protocol.c
> @@ -1285,6 +1285,9 @@ SCTP_STATIC __init int sctp_init(void)
>  	sctp_max_instreams    		= SCTP_DEFAULT_INSTREAMS;
>  	sctp_max_outstreams   		= SCTP_DEFAULT_OUTSTREAMS;
>  
> +	/* Initialize maximum autoclose timeout. */
> +	sctp_max_autoclose		= INT_MAX / HZ;
> +
>  	/* Initialize handle used for association ids. */
>  	idr_init(&sctp_assocs_id);
>  
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 13bf5fc..54a7cd2 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2200,8 +2200,6 @@ static int sctp_setsockopt_autoclose(struct sock *sk, char __user *optval,
>  		return -EINVAL;
>  	if (copy_from_user(&sp->autoclose, optval, optlen))
>  		return -EFAULT;
> -	/* make sure it won't exceed MAX_SCHEDULE_TIMEOUT */
> -	sp->autoclose = min_t(long, sp->autoclose, MAX_SCHEDULE_TIMEOUT / HZ);
>  
>  	return 0;
>  }
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 6b39529..60ffbd0 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -53,6 +53,10 @@ static int sack_timer_min = 1;
>  static int sack_timer_max = 500;
>  static int addr_scope_max = 3; /* check sctp_scope_policy_t in include/net/sctp/constants.h for max entries */
>  static int rwnd_scale_max = 16;
> +static unsigned long max_autoclose_min = 0;
> +static unsigned long max_autoclose_max =
> +	(MAX_SCHEDULE_TIMEOUT / HZ > UINT_MAX)
> +	? UINT_MAX : MAX_SCHEDULE_TIMEOUT / HZ;
>  
>  extern long sysctl_sctp_mem[3];
>  extern int sysctl_sctp_rmem[3];
> @@ -258,6 +262,15 @@ static ctl_table sctp_table[] = {
>  		.extra1		= &one,
>  		.extra2		= &rwnd_scale_max,
>  	},
> +	{
> +		.procname	= "max_autoclose",
> +		.data		= &sctp_max_autoclose,
> +		.maxlen		= sizeof(unsigned long),
> +		.mode		= 0644,
> +		.proc_handler	= &proc_doulongvec_minmax,
> +		.extra1		= &max_autoclose_min,
> +		.extra2		= &max_autoclose_max,
> +	},
>  
>  	{ /* sentinel */ }
>  };