[PATCH v5 0/2] sctp: check the rto_min and rto_max

[PATCH v5 0/2] sctp: check the rto_min and rto_max

[Wang Weidong]

v4 -> v5
  - patch1: add marco in constants.h and fix up spacing as
    suggested by Daniel
  - patch2: add a patch for fix up do_hmac_alg for according
    to do_rto_min[max]

Wang Weidong (2):
  sctp: check the rto_min and rto_max
  sctp: fix up a spacing

 include/net/sctp/constants.h |  3 ++
 net/sctp/socket.c            |  5 +++
 net/sctp/sysctl.c            | 80 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 77 insertions(+), 11 deletions(-)

-- 
1.7.12

[PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

rto_min should be smaller than rto_max while rto_max should be larger
than rto_min. Add two proc_handler for the checking. Add the check in
sctp_setsockopt_rtoinfo.

Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 include/net/sctp/constants.h |  3 ++
 net/sctp/socket.c            |  5 +++
 net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
index 2f0a565..d276978 100644
--- a/include/net/sctp/constants.h
+++ b/include/net/sctp/constants.h
@@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
 #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
 #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
 
+#define SCTP_ONE                1        /* 1 ms */
+#define SCTP_TIMER_MAX          86400000 /* ms in one day */
+
 /* Maximum number of new data packets that can be sent in a burst.  */
 #define SCTP_DEFAULT_MAX_BURST		4
 
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 72046b9..13411ad 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
 	if (copy_from_user(&rtoinfo, optval, optlen))
 		return -EFAULT;
 
+	if (rtoinfo.srto_min < SCTP_ONE ||
+	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
+	    rtoinfo.srto_max < rtoinfo.srto_min)
+		return -EINVAL;
+
 	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
 
 	/* Set the values to the specific association */
diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 6b36561..33c56c6 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -40,8 +40,8 @@
 #include <linux/sysctl.h>
 
 static int zero = 0;
-static int one = 1;
-static int timer_max = 86400000; /* ms in one day */
+static int one = SCTP_ONE;
+static int timer_max = SCTP_TIMER_MAX;
 static int int_max = INT_MAX;
 static int sack_timer_min = 1;
 static int sack_timer_max = 500;
@@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 				void __user *buffer, size_t *lenp,
 
 				loff_t *ppos);
+static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
+				void __user *buffer, size_t *lenp,
+				loff_t *ppos);
+
 static struct ctl_table sctp_table[] = {
 	{
 		.procname	= "sctp_mem",
@@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
 		.data		= &init_net.sctp.rto_min,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= proc_sctp_do_rto_min,
 		.extra1         = &one,
-		.extra2         = &timer_max
+		.extra2         = &init_net.sctp.rto_max
 	},
 	{
 		.procname	= "rto_max",
 		.data		= &init_net.sctp.rto_max,
 		.maxlen		= sizeof(unsigned int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1         = &one,
+		.proc_handler	= proc_sctp_do_rto_max,
+		.extra1         = &init_net.sctp.rto_min,
 		.extra2         = &timer_max
 	},
 	{
@@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
 	return ret;
 }
 
+static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	struct net *net = current->nsproxy->net_ns;
+	int new_value;
+	struct ctl_table tbl;
+	unsigned int min = *(unsigned int *) ctl->extra1;
+	unsigned int max = *(unsigned int *) ctl->extra2;
+	int ret;
+
+	memset(&tbl, 0, sizeof(struct ctl_table));
+	tbl.maxlen = sizeof(unsigned int);
+
+	if (write)
+		tbl.data = &new_value;
+	else
+		tbl.data = &net->sctp.rto_min;
+	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
+	if (write) {
+		if (ret || new_value > max || new_value < min)
+			return -EINVAL;
+		net->sctp.rto_min = new_value;
+	}
+	return ret;
+}
+
+static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
+				void __user*buffer, size_t *lenp,
+				loff_t *ppos)
+{
+	struct net *net = current->nsproxy->net_ns;
+	int new_value;
+	struct ctl_table tbl;
+	unsigned int min = *(unsigned int *) ctl->extra1;
+	unsigned int max = *(unsigned int *) ctl->extra2;
+	int ret;
+
+	memset(&tbl, 0, sizeof(struct ctl_table));
+	tbl.maxlen = sizeof(unsigned int);
+
+	if (write)
+		tbl.data = &new_value;
+	else
+		tbl.data = &net->sctp.rto_max;
+	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
+	if (write) {
+                if (ret || new_value > max || new_value < min)
+			return -EINVAL;
+		net->sctp.rto_max = new_value;
+	}
+	return ret;
+}
+
 int sctp_sysctl_net_register(struct net *net)
 {
 	struct ctl_table *table;
-- 
1.7.12

[PATCH v5 2/2] sctp: fix up a spacing

[Wang Weidong]

fix up spacing of proc_sctp_do_hmac_alg for according to the
proc_sctp_do_rto_min[max] in sysctl.c

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/sysctl.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
index 33c56c6..0ea22b7 100644
--- a/net/sctp/sysctl.c
+++ b/net/sctp/sysctl.c
@@ -56,10 +56,8 @@ extern long sysctl_sctp_mem[3];
 extern int sysctl_sctp_rmem[3];
 extern int sysctl_sctp_wmem[3];
 
-static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
-				int write,
+static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, int write,
 				void __user *buffer, size_t *lenp,
-
 				loff_t *ppos);
 static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
 				void __user *buffer, size_t *lenp,
@@ -301,8 +299,7 @@ static struct ctl_table sctp_net_table[] = {
 	{ /* sentinel */ }
 };
 
-static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
-				int write,
+static int proc_sctp_do_hmac_alg(struct ctl_table *ctl, int write,
 				void __user *buffer, size_t *lenp,
 				loff_t *ppos)
 {
-- 
1.7.12

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Daniel Borkmann]

On 12/07/2013 08:17 AM, Wang Weidong wrote:
> rto_min should be smaller than rto_max while rto_max should be larger
> than rto_min. Add two proc_handler for the checking. Add the check in
> sctp_setsockopt_rtoinfo.
>
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---

Thanks Wang, also for your second patch.

Second one looks good to me, thanks for the cleanup!

I was wondering where 86400000 comes from? Looking through the git
history didn't give much clues and the RFC4960 neither. Clearly,
section 15 of RFC4960 *recommends* as initial values ...

   RTO.Initial - 3 seconds
   RTO.Min - 1 second
   RTO.Max - 60 seconds

... which we have as constants in [1] and are assigned to globals
initially in [2,3] with those recommended values. That's all good.

But still [not *directly* related to your patch though], where does
86400000 come from? I expect that's for the max SCTP heartbeat
interval or max cookie lifetime?

Hence, timer_max is used in multiple contexts for timers here,
which seems a bit confusing. If it really makes sense to have such
a huge RTO max though, then it at least needs a comment explaining
so. ;)

Wang, just a minor nitpick, I would much rather name them those
defines like ...

   SCTP_RTO_MIN_LIMIT
   SCTP_RTO_MAX_LIMIT

... just so that we have recommended and upper limit constants with
a more clear naming as obviously SCTP_ONE doesn't make much sense.

  [1] include/net/sctp/constants.h +269
  [2] include/net/netns/sctp.h +39
  [3] net/sctp/protocol.c +1169

More below ...

>   include/net/sctp/constants.h |  3 ++
>   net/sctp/socket.c            |  5 +++
>   net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>   3 files changed, 75 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> index 2f0a565..d276978 100644
> --- a/include/net/sctp/constants.h
> +++ b/include/net/sctp/constants.h
> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>   #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>   #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>
> +#define SCTP_ONE                1        /* 1 ms */
> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
> +
>   /* Maximum number of new data packets that can be sent in a burst.  */
>   #define SCTP_DEFAULT_MAX_BURST		4
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 72046b9..13411ad 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>   	if (copy_from_user(&rtoinfo, optval, optlen))
>   		return -EFAULT;
>
> +	if (rtoinfo.srto_min < SCTP_ONE ||
> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
> +	    rtoinfo.srto_max < rtoinfo.srto_min)
> +		return -EINVAL;
> +
>   	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>
>   	/* Set the values to the specific association */
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 6b36561..33c56c6 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -40,8 +40,8 @@
>   #include <linux/sysctl.h>
>
>   static int zero = 0;
> -static int one = 1;
> -static int timer_max = 86400000; /* ms in one day */
> +static int one = SCTP_ONE;
> +static int timer_max = SCTP_TIMER_MAX;

So here, I'd do something like this ...

static int one = 1;              (leaving this as is)
static int timer_max = 86400000; /* Max 1 day for HB interval, cookie life-time*/

static int rto_timer_min = SCTP_RTO_MIN_LIMIT;
static int rto_timer_max = SCTP_RTO_MAX_LIMIT;

... so that we have sack_timer_{min,max} and rto_timer_{min,max}.

Opinions, thoughts?

>   static int int_max = INT_MAX;
>   static int sack_timer_min = 1;
>   static int sack_timer_max = 500;
> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>   				void __user *buffer, size_t *lenp,
>
>   				loff_t *ppos);
> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
> +				void __user *buffer, size_t *lenp,
> +				loff_t *ppos);
> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
> +				void __user *buffer, size_t *lenp,
> +				loff_t *ppos);
> +
>   static struct ctl_table sctp_table[] = {
>   	{
>   		.procname	= "sctp_mem",
> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>   		.data		= &init_net.sctp.rto_min,
>   		.maxlen		= sizeof(unsigned int),
>   		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> +		.proc_handler	= proc_sctp_do_rto_min,
>   		.extra1         = &one,
> -		.extra2         = &timer_max
> +		.extra2         = &init_net.sctp.rto_max
>   	},
>   	{
>   		.procname	= "rto_max",
>   		.data		= &init_net.sctp.rto_max,
>   		.maxlen		= sizeof(unsigned int),
>   		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> -		.extra1         = &one,
> +		.proc_handler	= proc_sctp_do_rto_max,
> +		.extra1         = &init_net.sctp.rto_min,
>   		.extra2         = &timer_max
>   	},
>   	{
> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>   	return ret;
>   }
>
> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
> +				void __user*buffer, size_t *lenp,
> +				loff_t *ppos)
> +{
> +	struct net *net = current->nsproxy->net_ns;
> +	int new_value;
> +	struct ctl_table tbl;
> +	unsigned int min = *(unsigned int *) ctl->extra1;
> +	unsigned int max = *(unsigned int *) ctl->extra2;
> +	int ret;
> +
> +	memset(&tbl, 0, sizeof(struct ctl_table));
> +	tbl.maxlen = sizeof(unsigned int);
> +
> +	if (write)
> +		tbl.data = &new_value;
> +	else
> +		tbl.data = &net->sctp.rto_min;
> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +	if (write) {
> +		if (ret || new_value > max || new_value < min)
> +			return -EINVAL;
> +		net->sctp.rto_min = new_value;
> +	}
> +	return ret;
> +}
> +
> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
> +				void __user*buffer, size_t *lenp,
> +				loff_t *ppos)
> +{
> +	struct net *net = current->nsproxy->net_ns;
> +	int new_value;
> +	struct ctl_table tbl;
> +	unsigned int min = *(unsigned int *) ctl->extra1;
> +	unsigned int max = *(unsigned int *) ctl->extra2;
> +	int ret;
> +
> +	memset(&tbl, 0, sizeof(struct ctl_table));
> +	tbl.maxlen = sizeof(unsigned int);
> +
> +	if (write)
> +		tbl.data = &new_value;
> +	else
> +		tbl.data = &net->sctp.rto_max;
> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +	if (write) {
> +                if (ret || new_value > max || new_value < min)
> +			return -EINVAL;
> +		net->sctp.rto_max = new_value;
> +	}
> +	return ret;
> +}
> +
>   int sctp_sysctl_net_register(struct net *net)
>   {
>   	struct ctl_table *table;
>

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

 From Wang Weidong <wangweidong1@huawei.com>

On 2013/12/7 20:43, Daniel Borkmann wrote:
> On 12/07/2013 08:17 AM, Wang Weidong wrote:
>> rto_min should be smaller than rto_max while rto_max should be larger
>> than rto_min. Add two proc_handler for the checking. Add the check in
>> sctp_setsockopt_rtoinfo.
>>
>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>
> Thanks Wang, also for your second patch.
>
> Second one looks good to me, thanks for the cleanup!
>
> I was wondering where 86400000 comes from? Looking through the git
> history didn't give much clues and the RFC4960 neither. Clearly,
> section 15 of RFC4960 *recommends* as initial values ...
>
>    RTO.Initial - 3 seconds
>    RTO.Min - 1 second
>    RTO.Max - 60 seconds
>
> ... which we have as constants in [1] and are assigned to globals
> initially in [2,3] with those recommended values. That's all good.
>
> But still [not *directly* related to your patch though], where does
> 86400000 come from? I expect that's for the max SCTP heartbeat
> interval or max cookie lifetime?
>
Sorry, I don't know 86400000 come form as well.

> Hence, timer_max is used in multiple contexts for timers here,
> which seems a bit confusing. If it really makes sense to have such
> a huge RTO max though, then it at least needs a comment explaining
> so. ;)
>
> Wang, just a minor nitpick, I would much rather name them those
> defines like ...
>
>    SCTP_RTO_MIN_LIMIT
>    SCTP_RTO_MAX_LIMIT
>
> ... just so that we have recommended and upper limit constants with
> a more clear naming as obviously SCTP_ONE doesn't make much sense.
>
>   [1] include/net/sctp/constants.h +269
>   [2] include/net/netns/sctp.h +39
>   [3] net/sctp/protocol.c +1169
>
> More below ...
>
>>   include/net/sctp/constants.h |  3 ++
>>   net/sctp/socket.c            |  5 +++
>>   net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 2f0a565..d276978 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>   #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>   #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>
>> +#define SCTP_ONE                1        /* 1 ms */
>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>> +
>>   /* Maximum number of new data packets that can be sent in a burst.  */
>>   #define SCTP_DEFAULT_MAX_BURST        4
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 72046b9..13411ad 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>       if (copy_from_user(&rtoinfo, optval, optlen))
>>           return -EFAULT;
>>
>> +    if (rtoinfo.srto_min < SCTP_ONE ||
>> +        rtoinfo.srto_max > SCTP_TIMER_MAX ||
>> +        rtoinfo.srto_max < rtoinfo.srto_min)
>> +        return -EINVAL;
>> +
>>       asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>
>>       /* Set the values to the specific association */
>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>> index 6b36561..33c56c6 100644
>> --- a/net/sctp/sysctl.c
>> +++ b/net/sctp/sysctl.c
>> @@ -40,8 +40,8 @@
>>   #include <linux/sysctl.h>
>>
>>   static int zero = 0;
>> -static int one = 1;
>> -static int timer_max = 86400000; /* ms in one day */
>> +static int one = SCTP_ONE;
>> +static int timer_max = SCTP_TIMER_MAX;
>
> So here, I'd do something like this ...
>
> static int one = 1;              (leaving this as is)
> static int timer_max = 86400000; /* Max 1 day for HB interval, cookie life-time*/
>
> static int rto_timer_min = SCTP_RTO_MIN_LIMIT;
> static int rto_timer_max = SCTP_RTO_MAX_LIMIT;
>
> ... so that we have sack_timer_{min,max} and rto_timer_{min,max}.
>
> Opinions, thoughts?
>
Agree. Your Opinions is very good.

Thanks a lot, you help me so much.  I think I should wait for the
explanation of the 86400000, then send the v6.

Regards.
Wang

>>   static int int_max = INT_MAX;
>>   static int sack_timer_min = 1;
>>   static int sack_timer_max = 500;
>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>                   void __user *buffer, size_t *lenp,
>>
>>                   loff_t *ppos);
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +                void __user *buffer, size_t *lenp,
>> +                loff_t *ppos);
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +                void __user *buffer, size_t *lenp,
>> +                loff_t *ppos);
>> +
>>   static struct ctl_table sctp_table[] = {
>>       {
>>           .procname    = "sctp_mem",
>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>           .data        = &init_net.sctp.rto_min,
>>           .maxlen        = sizeof(unsigned int),
>>           .mode        = 0644,
>> -        .proc_handler    = proc_dointvec_minmax,
>> +        .proc_handler    = proc_sctp_do_rto_min,
>>           .extra1         = &one,
>> -        .extra2         = &timer_max
>> +        .extra2         = &init_net.sctp.rto_max
>>       },
>>       {
>>           .procname    = "rto_max",
>>           .data        = &init_net.sctp.rto_max,
>>           .maxlen        = sizeof(unsigned int),
>>           .mode        = 0644,
>> -        .proc_handler    = proc_dointvec_minmax,
>> -        .extra1         = &one,
>> +        .proc_handler    = proc_sctp_do_rto_max,
>> +        .extra1         = &init_net.sctp.rto_min,
>>           .extra2         = &timer_max
>>       },
>>       {
>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>       return ret;
>>   }
>>
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +                void __user*buffer, size_t *lenp,
>> +                loff_t *ppos)
>> +{
>> +    struct net *net = current->nsproxy->net_ns;
>> +    int new_value;
>> +    struct ctl_table tbl;
>> +    unsigned int min = *(unsigned int *) ctl->extra1;
>> +    unsigned int max = *(unsigned int *) ctl->extra2;
>> +    int ret;
>> +
>> +    memset(&tbl, 0, sizeof(struct ctl_table));
>> +    tbl.maxlen = sizeof(unsigned int);
>> +
>> +    if (write)
>> +        tbl.data = &new_value;
>> +    else
>> +        tbl.data = &net->sctp.rto_min;
>> +    ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +    if (write) {
>> +        if (ret || new_value > max || new_value < min)
>> +            return -EINVAL;
>> +        net->sctp.rto_min = new_value;
>> +    }
>> +    return ret;
>> +}
>> +
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +                void __user*buffer, size_t *lenp,
>> +                loff_t *ppos)
>> +{
>> +    struct net *net = current->nsproxy->net_ns;
>> +    int new_value;
>> +    struct ctl_table tbl;
>> +    unsigned int min = *(unsigned int *) ctl->extra1;
>> +    unsigned int max = *(unsigned int *) ctl->extra2;
>> +    int ret;
>> +
>> +    memset(&tbl, 0, sizeof(struct ctl_table));
>> +    tbl.maxlen = sizeof(unsigned int);
>> +
>> +    if (write)
>> +        tbl.data = &new_value;
>> +    else
>> +        tbl.data = &net->sctp.rto_max;
>> +    ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +    if (write) {
>> +                if (ret || new_value > max || new_value < min)
>> +            return -EINVAL;
>> +        net->sctp.rto_max = new_value;
>> +    }
>> +    return ret;
>> +}
>> +
>>   int sctp_sysctl_net_register(struct net *net)
>>   {
>>       struct ctl_table *table;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/07/2013 02:17 AM, Wang Weidong wrote:
> rto_min should be smaller than rto_max while rto_max should be larger
> than rto_min. Add two proc_handler for the checking. Add the check in
> sctp_setsockopt_rtoinfo.
> 
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>  include/net/sctp/constants.h |  3 ++
>  net/sctp/socket.c            |  5 +++
>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 75 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
> index 2f0a565..d276978 100644
> --- a/include/net/sctp/constants.h
> +++ b/include/net/sctp/constants.h
> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>  
> +#define SCTP_ONE                1        /* 1 ms */
> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
> +
>  /* Maximum number of new data packets that can be sent in a burst.  */
>  #define SCTP_DEFAULT_MAX_BURST		4
>  
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 72046b9..13411ad 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>  	if (copy_from_user(&rtoinfo, optval, optlen))
>  		return -EFAULT;
>  
> +	if (rtoinfo.srto_min < SCTP_ONE ||
> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
> +	    rtoinfo.srto_max < rtoinfo.srto_min)
> +		return -EINVAL;
> +

You can not do the check for srto_min < 1.  The following is the text
from the spec:
   All times are given in milliseconds.  A value of 0, when modifying
   the parameters, indicates that the current value should not be
   changed.

So, it is valid for a user to pass in a value of 0.  Also, I am not sure
if it makes sense to bind the upper limit here, as well.

-vlad

>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>  
>  	/* Set the values to the specific association */
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 6b36561..33c56c6 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -40,8 +40,8 @@
>  #include <linux/sysctl.h>
>  
>  static int zero = 0;
> -static int one = 1;
> -static int timer_max = 86400000; /* ms in one day */
> +static int one = SCTP_ONE;
> +static int timer_max = SCTP_TIMER_MAX;
>  static int int_max = INT_MAX;
>  static int sack_timer_min = 1;
>  static int sack_timer_max = 500;
> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>  				void __user *buffer, size_t *lenp,
>  
>  				loff_t *ppos);
> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
> +				void __user *buffer, size_t *lenp,
> +				loff_t *ppos);
> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
> +				void __user *buffer, size_t *lenp,
> +				loff_t *ppos);
> +
>  static struct ctl_table sctp_table[] = {
>  	{
>  		.procname	= "sctp_mem",
> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>  		.data		= &init_net.sctp.rto_min,
>  		.maxlen		= sizeof(unsigned int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> +		.proc_handler	= proc_sctp_do_rto_min,
>  		.extra1         = &one,
> -		.extra2         = &timer_max
> +		.extra2         = &init_net.sctp.rto_max
>  	},
>  	{
>  		.procname	= "rto_max",
>  		.data		= &init_net.sctp.rto_max,
>  		.maxlen		= sizeof(unsigned int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> -		.extra1         = &one,
> +		.proc_handler	= proc_sctp_do_rto_max,
> +		.extra1         = &init_net.sctp.rto_min,
>  		.extra2         = &timer_max
>  	},
>  	{
> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>  	return ret;
>  }
>  
> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
> +				void __user*buffer, size_t *lenp,
> +				loff_t *ppos)
> +{
> +	struct net *net = current->nsproxy->net_ns;
> +	int new_value;
> +	struct ctl_table tbl;
> +	unsigned int min = *(unsigned int *) ctl->extra1;
> +	unsigned int max = *(unsigned int *) ctl->extra2;
> +	int ret;
> +
> +	memset(&tbl, 0, sizeof(struct ctl_table));
> +	tbl.maxlen = sizeof(unsigned int);
> +
> +	if (write)
> +		tbl.data = &new_value;
> +	else
> +		tbl.data = &net->sctp.rto_min;
> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +	if (write) {
> +		if (ret || new_value > max || new_value < min)
> +			return -EINVAL;
> +		net->sctp.rto_min = new_value;
> +	}
> +	return ret;
> +}
> +
> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
> +				void __user*buffer, size_t *lenp,
> +				loff_t *ppos)
> +{
> +	struct net *net = current->nsproxy->net_ns;
> +	int new_value;
> +	struct ctl_table tbl;
> +	unsigned int min = *(unsigned int *) ctl->extra1;
> +	unsigned int max = *(unsigned int *) ctl->extra2;
> +	int ret;
> +
> +	memset(&tbl, 0, sizeof(struct ctl_table));
> +	tbl.maxlen = sizeof(unsigned int);
> +
> +	if (write)
> +		tbl.data = &new_value;
> +	else
> +		tbl.data = &net->sctp.rto_max;
> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
> +	if (write) {
> +                if (ret || new_value > max || new_value < min)
> +			return -EINVAL;
> +		net->sctp.rto_max = new_value;
> +	}
> +	return ret;
> +}
> +
>  int sctp_sysctl_net_register(struct net *net)
>  {
>  	struct ctl_table *table;
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/07/2013 07:43 AM, Daniel Borkmann wrote:
> On 12/07/2013 08:17 AM, Wang Weidong wrote:
>> rto_min should be smaller than rto_max while rto_max should be larger
>> than rto_min. Add two proc_handler for the checking. Add the check in
>> sctp_setsockopt_rtoinfo.
>>
>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
> 
> Thanks Wang, also for your second patch.
> 
> Second one looks good to me, thanks for the cleanup!
> 
> I was wondering where 86400000 comes from? Looking through the git
> history didn't give much clues and the RFC4960 neither. Clearly,
> section 15 of RFC4960 *recommends* as initial values ...
> 
>   RTO.Initial - 3 seconds
>   RTO.Min - 1 second
>   RTO.Max - 60 seconds
> 
> ... which we have as constants in [1] and are assigned to globals
> initially in [2,3] with those recommended values. That's all good.
> 
> But still [not *directly* related to your patch though], where does
> 86400000 come from? I expect that's for the max SCTP heartbeat
> interval or max cookie lifetime?

No, initially it was defined as rto_timer_max and was the upper bound
for the rto timer.   When you think about it, it's a bit ridiculous
really.  What you are saying is that your rto timer is allowed to
grow as long as 1 day, so you would at an absolute maximum retransmit
one packet per day :)

I don't think this limit is specified anywhere as is though.  It
was something that's been there since the 2.5 days.

-vlad

> 
> Hence, timer_max is used in multiple contexts for timers here,
> which seems a bit confusing. If it really makes sense to have such
> a huge RTO max though, then it at least needs a comment explaining
> so. ;)
> 
> Wang, just a minor nitpick, I would much rather name them those
> defines like ...
> 
>   SCTP_RTO_MIN_LIMIT
>   SCTP_RTO_MAX_LIMIT
> 
> ... just so that we have recommended and upper limit constants with
> a more clear naming as obviously SCTP_ONE doesn't make much sense.
> 
>  [1] include/net/sctp/constants.h +269
>  [2] include/net/netns/sctp.h +39
>  [3] net/sctp/protocol.c +1169
> 
> More below ...
> 
>>   include/net/sctp/constants.h |  3 ++
>>   net/sctp/socket.c            |  5 +++
>>   net/sctp/sysctl.c            | 73
>> ++++++++++++++++++++++++++++++++++++++++----
>>   3 files changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 2f0a565..d276978 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>   #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right
>> shifts. */
>>   #define SCTP_RTO_BETA           2   /* 1/4 when converted to right
>> shifts. */
>>
>> +#define SCTP_ONE                1        /* 1 ms */
>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>> +
>>   /* Maximum number of new data packets that can be sent in a burst.  */
>>   #define SCTP_DEFAULT_MAX_BURST        4
>>
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 72046b9..13411ad 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock
>> *sk, char __user *optval, unsigne
>>       if (copy_from_user(&rtoinfo, optval, optlen))
>>           return -EFAULT;
>>
>> +    if (rtoinfo.srto_min < SCTP_ONE ||
>> +        rtoinfo.srto_max > SCTP_TIMER_MAX ||
>> +        rtoinfo.srto_max < rtoinfo.srto_min)
>> +        return -EINVAL;
>> +
>>       asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>
>>       /* Set the values to the specific association */
>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>> index 6b36561..33c56c6 100644
>> --- a/net/sctp/sysctl.c
>> +++ b/net/sctp/sysctl.c
>> @@ -40,8 +40,8 @@
>>   #include <linux/sysctl.h>
>>
>>   static int zero = 0;
>> -static int one = 1;
>> -static int timer_max = 86400000; /* ms in one day */
>> +static int one = SCTP_ONE;
>> +static int timer_max = SCTP_TIMER_MAX;
> 
> So here, I'd do something like this ...
> 
> static int one = 1;              (leaving this as is)
> static int timer_max = 86400000; /* Max 1 day for HB interval, cookie
> life-time*/
> 
> static int rto_timer_min = SCTP_RTO_MIN_LIMIT;
> static int rto_timer_max = SCTP_RTO_MAX_LIMIT;
> 
> ... so that we have sack_timer_{min,max} and rto_timer_{min,max}.
> 
> Opinions, thoughts?
> 
>>   static int int_max = INT_MAX;
>>   static int sack_timer_min = 1;
>>   static int sack_timer_max = 500;
>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table
>> *ctl,
>>                   void __user *buffer, size_t *lenp,
>>
>>                   loff_t *ppos);
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +                void __user *buffer, size_t *lenp,
>> +                loff_t *ppos);
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +                void __user *buffer, size_t *lenp,
>> +                loff_t *ppos);
>> +
>>   static struct ctl_table sctp_table[] = {
>>       {
>>           .procname    = "sctp_mem",
>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>           .data        = &init_net.sctp.rto_min,
>>           .maxlen        = sizeof(unsigned int),
>>           .mode        = 0644,
>> -        .proc_handler    = proc_dointvec_minmax,
>> +        .proc_handler    = proc_sctp_do_rto_min,
>>           .extra1         = &one,
>> -        .extra2         = &timer_max
>> +        .extra2         = &init_net.sctp.rto_max
>>       },
>>       {
>>           .procname    = "rto_max",
>>           .data        = &init_net.sctp.rto_max,
>>           .maxlen        = sizeof(unsigned int),
>>           .mode        = 0644,
>> -        .proc_handler    = proc_dointvec_minmax,
>> -        .extra1         = &one,
>> +        .proc_handler    = proc_sctp_do_rto_max,
>> +        .extra1         = &init_net.sctp.rto_min,
>>           .extra2         = &timer_max
>>       },
>>       {
>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table
>> *ctl,
>>       return ret;
>>   }
>>
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +                void __user*buffer, size_t *lenp,
>> +                loff_t *ppos)
>> +{
>> +    struct net *net = current->nsproxy->net_ns;
>> +    int new_value;
>> +    struct ctl_table tbl;
>> +    unsigned int min = *(unsigned int *) ctl->extra1;
>> +    unsigned int max = *(unsigned int *) ctl->extra2;
>> +    int ret;
>> +
>> +    memset(&tbl, 0, sizeof(struct ctl_table));
>> +    tbl.maxlen = sizeof(unsigned int);
>> +
>> +    if (write)
>> +        tbl.data = &new_value;
>> +    else
>> +        tbl.data = &net->sctp.rto_min;
>> +    ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +    if (write) {
>> +        if (ret || new_value > max || new_value < min)
>> +            return -EINVAL;
>> +        net->sctp.rto_min = new_value;
>> +    }
>> +    return ret;
>> +}
>> +
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +                void __user*buffer, size_t *lenp,
>> +                loff_t *ppos)
>> +{
>> +    struct net *net = current->nsproxy->net_ns;
>> +    int new_value;
>> +    struct ctl_table tbl;
>> +    unsigned int min = *(unsigned int *) ctl->extra1;
>> +    unsigned int max = *(unsigned int *) ctl->extra2;
>> +    int ret;
>> +
>> +    memset(&tbl, 0, sizeof(struct ctl_table));
>> +    tbl.maxlen = sizeof(unsigned int);
>> +
>> +    if (write)
>> +        tbl.data = &new_value;
>> +    else
>> +        tbl.data = &net->sctp.rto_max;
>> +    ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +    if (write) {
>> +                if (ret || new_value > max || new_value < min)
>> +            return -EINVAL;
>> +        net->sctp.rto_max = new_value;
>> +    }
>> +    return ret;
>> +}
>> +
>>   int sctp_sysctl_net_register(struct net *net)
>>   {
>>       struct ctl_table *table;
>>

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Daniel Borkmann]

On 12/07/2013 08:01 PM, Vlad Yasevich wrote:
> On 12/07/2013 07:43 AM, Daniel Borkmann wrote:
>> On 12/07/2013 08:17 AM, Wang Weidong wrote:
>>> rto_min should be smaller than rto_max while rto_max should be larger
>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>> sctp_setsockopt_rtoinfo.
>>>
>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>> ---
>>
>> Thanks Wang, also for your second patch.
>>
>> Second one looks good to me, thanks for the cleanup!
>>
>> I was wondering where 86400000 comes from? Looking through the git
>> history didn't give much clues and the RFC4960 neither. Clearly,
>> section 15 of RFC4960 *recommends* as initial values ...
>>
>>    RTO.Initial - 3 seconds
>>    RTO.Min - 1 second
>>    RTO.Max - 60 seconds
>>
>> ... which we have as constants in [1] and are assigned to globals
>> initially in [2,3] with those recommended values. That's all good.
>>
>> But still [not *directly* related to your patch though], where does
>> 86400000 come from? I expect that's for the max SCTP heartbeat
>> interval or max cookie lifetime?
>
> No, initially it was defined as rto_timer_max and was the upper bound
> for the rto timer.   When you think about it, it's a bit ridiculous
> really.  What you are saying is that your rto timer is allowed to
> grow as long as 1 day, so you would at an absolute maximum retransmit
> one packet per day :)

Exactly, maybe initial SCTP implementors already took into account
we could have SCTP connections to other galaxies, but clearly untested
so far? :)

I think we should be absolutely fine with a max configurable upper
limit of twice the recommended RTO.Max value from the RFC.

> I don't think this limit is specified anywhere as is though.  It
> was something that's been there since the 2.5 days.
>
> -vlad

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/8 2:54, Vlad Yasevich wrote:
> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>> rto_min should be smaller than rto_max while rto_max should be larger
>> than rto_min. Add two proc_handler for the checking. Add the check in
>> sctp_setsockopt_rtoinfo.
>>
>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>  include/net/sctp/constants.h |  3 ++
>>  net/sctp/socket.c            |  5 +++
>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>> index 2f0a565..d276978 100644
>> --- a/include/net/sctp/constants.h
>> +++ b/include/net/sctp/constants.h
>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>  
>> +#define SCTP_ONE                1        /* 1 ms */
>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>> +
>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>  #define SCTP_DEFAULT_MAX_BURST		4
>>  
>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>> index 72046b9..13411ad 100644
>> --- a/net/sctp/socket.c
>> +++ b/net/sctp/socket.c
>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>  		return -EFAULT;
>>  
>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>> +		return -EINVAL;
>> +
> 
> You can not do the check for srto_min < 1.  The following is the text
> from the spec:
>    All times are given in milliseconds.  A value of 0, when modifying
>    the parameters, indicates that the current value should not be
>    changed.
> 
Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
Thanks.

> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
> if it makes sense to bind the upper limit here, as well.
> 
> -vlad
> 
Here, I am not sure as well. I think it should like what we do to the
init_net.sctp.rto_max when set larger than timer_max. Just not change the value.


Regards.
Wang

>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>  
>>  	/* Set the values to the specific association */
>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>> index 6b36561..33c56c6 100644
>> --- a/net/sctp/sysctl.c
>> +++ b/net/sctp/sysctl.c
>> @@ -40,8 +40,8 @@
>>  #include <linux/sysctl.h>
>>  
>>  static int zero = 0;
>> -static int one = 1;
>> -static int timer_max = 86400000; /* ms in one day */
>> +static int one = SCTP_ONE;
>> +static int timer_max = SCTP_TIMER_MAX;
>>  static int int_max = INT_MAX;
>>  static int sack_timer_min = 1;
>>  static int sack_timer_max = 500;
>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>  				void __user *buffer, size_t *lenp,
>>  
>>  				loff_t *ppos);
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +				void __user *buffer, size_t *lenp,
>> +				loff_t *ppos);
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +				void __user *buffer, size_t *lenp,
>> +				loff_t *ppos);
>> +
>>  static struct ctl_table sctp_table[] = {
>>  	{
>>  		.procname	= "sctp_mem",
>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>  		.data		= &init_net.sctp.rto_min,
>>  		.maxlen		= sizeof(unsigned int),
>>  		.mode		= 0644,
>> -		.proc_handler	= proc_dointvec_minmax,
>> +		.proc_handler	= proc_sctp_do_rto_min,
>>  		.extra1         = &one,
>> -		.extra2         = &timer_max
>> +		.extra2         = &init_net.sctp.rto_max
>>  	},
>>  	{
>>  		.procname	= "rto_max",
>>  		.data		= &init_net.sctp.rto_max,
>>  		.maxlen		= sizeof(unsigned int),
>>  		.mode		= 0644,
>> -		.proc_handler	= proc_dointvec_minmax,
>> -		.extra1         = &one,
>> +		.proc_handler	= proc_sctp_do_rto_max,
>> +		.extra1         = &init_net.sctp.rto_min,
>>  		.extra2         = &timer_max
>>  	},
>>  	{
>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>  	return ret;
>>  }
>>  
>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>> +				void __user*buffer, size_t *lenp,
>> +				loff_t *ppos)
>> +{
>> +	struct net *net = current->nsproxy->net_ns;
>> +	int new_value;
>> +	struct ctl_table tbl;
>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>> +	int ret;
>> +
>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>> +	tbl.maxlen = sizeof(unsigned int);
>> +
>> +	if (write)
>> +		tbl.data = &new_value;
>> +	else
>> +		tbl.data = &net->sctp.rto_min;
>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +	if (write) {
>> +		if (ret || new_value > max || new_value < min)
>> +			return -EINVAL;
>> +		net->sctp.rto_min = new_value;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>> +				void __user*buffer, size_t *lenp,
>> +				loff_t *ppos)
>> +{
>> +	struct net *net = current->nsproxy->net_ns;
>> +	int new_value;
>> +	struct ctl_table tbl;
>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>> +	int ret;
>> +
>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>> +	tbl.maxlen = sizeof(unsigned int);
>> +
>> +	if (write)
>> +		tbl.data = &new_value;
>> +	else
>> +		tbl.data = &net->sctp.rto_max;
>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>> +	if (write) {
>> +                if (ret || new_value > max || new_value < min)
>> +			return -EINVAL;
>> +		net->sctp.rto_max = new_value;
>> +	}
>> +	return ret;
>> +}
>> +
>>  int sctp_sysctl_net_register(struct net *net)
>>  {
>>  	struct ctl_table *table;
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/08/2013 08:53 PM, Wang Weidong wrote:
> On 2013/12/8 2:54, Vlad Yasevich wrote:
>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>> rto_min should be smaller than rto_max while rto_max should be larger
>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>> sctp_setsockopt_rtoinfo.
>>>
>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>> ---
>>>  include/net/sctp/constants.h |  3 ++
>>>  net/sctp/socket.c            |  5 +++
>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>> index 2f0a565..d276978 100644
>>> --- a/include/net/sctp/constants.h
>>> +++ b/include/net/sctp/constants.h
>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>  
>>> +#define SCTP_ONE                1        /* 1 ms */
>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>> +
>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>  
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index 72046b9..13411ad 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>  		return -EFAULT;
>>>  
>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>> +		return -EINVAL;
>>> +
>>
>> You can not do the check for srto_min < 1.  The following is the text
>> from the spec:
>>    All times are given in milliseconds.  A value of 0, when modifying
>>    the parameters, indicates that the current value should not be
>>    changed.
>>
> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
> Thanks.
> 
>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>> if it makes sense to bind the upper limit here, as well.
>>
>> -vlad
>>
> Here, I am not sure as well. I think it should like what we do to the
> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
> 

So, the basic reason that sysctl is limited is that it is a default
for all sctp association on the system.  It makes some sense to limit
what the max value here could be.  Limiting it to double suggested
RTO.MAX would only make it 2 minutes and may be insufficient for some
of the high latency low-throughput wireless links.  Making it about an
hour should be fine...  This would be a separate patch though...

Limiting the user-supplied value is not as appropriate since the
assumption is that user application may know better what it's
requirements are and it is not up to the stack to limit those.  As
long as the user value is withing the usable range (and the kernel
will already knows how and does limit this range), we should not
limit this further.

-vlad

> 
> Regards.
> Wang
> 
>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>  
>>>  	/* Set the values to the specific association */
>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>> index 6b36561..33c56c6 100644
>>> --- a/net/sctp/sysctl.c
>>> +++ b/net/sctp/sysctl.c
>>> @@ -40,8 +40,8 @@
>>>  #include <linux/sysctl.h>
>>>  
>>>  static int zero = 0;
>>> -static int one = 1;
>>> -static int timer_max = 86400000; /* ms in one day */
>>> +static int one = SCTP_ONE;
>>> +static int timer_max = SCTP_TIMER_MAX;
>>>  static int int_max = INT_MAX;
>>>  static int sack_timer_min = 1;
>>>  static int sack_timer_max = 500;
>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>  				void __user *buffer, size_t *lenp,
>>>  
>>>  				loff_t *ppos);
>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>> +				void __user *buffer, size_t *lenp,
>>> +				loff_t *ppos);
>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>> +				void __user *buffer, size_t *lenp,
>>> +				loff_t *ppos);
>>> +
>>>  static struct ctl_table sctp_table[] = {
>>>  	{
>>>  		.procname	= "sctp_mem",
>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>  		.data		= &init_net.sctp.rto_min,
>>>  		.maxlen		= sizeof(unsigned int),
>>>  		.mode		= 0644,
>>> -		.proc_handler	= proc_dointvec_minmax,
>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>  		.extra1         = &one,
>>> -		.extra2         = &timer_max
>>> +		.extra2         = &init_net.sctp.rto_max
>>>  	},
>>>  	{
>>>  		.procname	= "rto_max",
>>>  		.data		= &init_net.sctp.rto_max,
>>>  		.maxlen		= sizeof(unsigned int),
>>>  		.mode		= 0644,
>>> -		.proc_handler	= proc_dointvec_minmax,
>>> -		.extra1         = &one,
>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>> +		.extra1         = &init_net.sctp.rto_min,
>>>  		.extra2         = &timer_max
>>>  	},
>>>  	{
>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>  	return ret;
>>>  }
>>>  
>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>> +				void __user*buffer, size_t *lenp,
>>> +				loff_t *ppos)
>>> +{
>>> +	struct net *net = current->nsproxy->net_ns;
>>> +	int new_value;
>>> +	struct ctl_table tbl;
>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>> +	int ret;
>>> +
>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>> +	tbl.maxlen = sizeof(unsigned int);
>>> +
>>> +	if (write)
>>> +		tbl.data = &new_value;
>>> +	else
>>> +		tbl.data = &net->sctp.rto_min;
>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>> +	if (write) {
>>> +		if (ret || new_value > max || new_value < min)
>>> +			return -EINVAL;
>>> +		net->sctp.rto_min = new_value;
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>> +				void __user*buffer, size_t *lenp,
>>> +				loff_t *ppos)
>>> +{
>>> +	struct net *net = current->nsproxy->net_ns;
>>> +	int new_value;
>>> +	struct ctl_table tbl;
>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>> +	int ret;
>>> +
>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>> +	tbl.maxlen = sizeof(unsigned int);
>>> +
>>> +	if (write)
>>> +		tbl.data = &new_value;
>>> +	else
>>> +		tbl.data = &net->sctp.rto_max;
>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>> +	if (write) {
>>> +                if (ret || new_value > max || new_value < min)
>>> +			return -EINVAL;
>>> +		net->sctp.rto_max = new_value;
>>> +	}
>>> +	return ret;
>>> +}
>>> +
>>>  int sctp_sysctl_net_register(struct net *net)
>>>  {
>>>  	struct ctl_table *table;
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> .
>>
> 
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/9 10:19, Vlad Yasevich wrote:
> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>> sctp_setsockopt_rtoinfo.
>>>>
>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>> ---
>>>>  include/net/sctp/constants.h |  3 ++
>>>>  net/sctp/socket.c            |  5 +++
>>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>> index 2f0a565..d276978 100644
>>>> --- a/include/net/sctp/constants.h
>>>> +++ b/include/net/sctp/constants.h
>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>  
>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>> +
>>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>>  
>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>> index 72046b9..13411ad 100644
>>>> --- a/net/sctp/socket.c
>>>> +++ b/net/sctp/socket.c
>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>  		return -EFAULT;
>>>>  
>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>> +		return -EINVAL;
>>>> +
>>>
>>> You can not do the check for srto_min < 1.  The following is the text
>>> from the spec:
>>>    All times are given in milliseconds.  A value of 0, when modifying
>>>    the parameters, indicates that the current value should not be
>>>    changed.
>>>
>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>> Thanks.
>>
>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>> if it makes sense to bind the upper limit here, as well.
>>>
>>> -vlad
>>>
>> Here, I am not sure as well. I think it should like what we do to the
>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>
> 
> So, the basic reason that sysctl is limited is that it is a default
> for all sctp association on the system.  It makes some sense to limit
> what the max value here could be.  Limiting it to double suggested
> RTO.MAX would only make it 2 minutes and may be insufficient for some
> of the high latency low-throughput wireless links.  Making it about an
> hour should be fine...  This would be a separate patch though...
> 
Here, you mean that we should use 3600*1000 rather than 86400000? So
we should use another patch to fix that after my patchs?

> Limiting the user-supplied value is not as appropriate since the
> assumption is that user application may know better what it's
> requirements are and it is not up to the stack to limit those.  As
> long as the user value is withing the usable range (and the kernel
> will already knows how and does limit this range), we should not
> limit this further.
> 
> -vlad
> 
Agree, So I should check like this:
!srto_min || !srto_max || srto_min > srto_max ?
And no need to add macros for checking.

Thanks.

>>
>> Regards.
>> Wang
>>
>>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>  
>>>>  	/* Set the values to the specific association */
>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>> index 6b36561..33c56c6 100644
>>>> --- a/net/sctp/sysctl.c
>>>> +++ b/net/sctp/sysctl.c
>>>> @@ -40,8 +40,8 @@
>>>>  #include <linux/sysctl.h>
>>>>  
>>>>  static int zero = 0;
>>>> -static int one = 1;
>>>> -static int timer_max = 86400000; /* ms in one day */
>>>> +static int one = SCTP_ONE;
>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>  static int int_max = INT_MAX;
>>>>  static int sack_timer_min = 1;
>>>>  static int sack_timer_max = 500;
>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>  				void __user *buffer, size_t *lenp,
>>>>  
>>>>  				loff_t *ppos);
>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>> +				void __user *buffer, size_t *lenp,
>>>> +				loff_t *ppos);
>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>> +				void __user *buffer, size_t *lenp,
>>>> +				loff_t *ppos);
>>>> +
>>>>  static struct ctl_table sctp_table[] = {
>>>>  	{
>>>>  		.procname	= "sctp_mem",
>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>  		.data		= &init_net.sctp.rto_min,
>>>>  		.maxlen		= sizeof(unsigned int),
>>>>  		.mode		= 0644,
>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>  		.extra1         = &one,
>>>> -		.extra2         = &timer_max
>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>  	},
>>>>  	{
>>>>  		.procname	= "rto_max",
>>>>  		.data		= &init_net.sctp.rto_max,
>>>>  		.maxlen		= sizeof(unsigned int),
>>>>  		.mode		= 0644,
>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>> -		.extra1         = &one,
>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>  		.extra2         = &timer_max
>>>>  	},
>>>>  	{
>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>> +				void __user*buffer, size_t *lenp,
>>>> +				loff_t *ppos)
>>>> +{
>>>> +	struct net *net = current->nsproxy->net_ns;
>>>> +	int new_value;
>>>> +	struct ctl_table tbl;
>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>> +	int ret;
>>>> +
>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>> +
>>>> +	if (write)
>>>> +		tbl.data = &new_value;
>>>> +	else
>>>> +		tbl.data = &net->sctp.rto_min;
>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>> +	if (write) {
>>>> +		if (ret || new_value > max || new_value < min)
>>>> +			return -EINVAL;
>>>> +		net->sctp.rto_min = new_value;
>>>> +	}
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>> +				void __user*buffer, size_t *lenp,
>>>> +				loff_t *ppos)
>>>> +{
>>>> +	struct net *net = current->nsproxy->net_ns;
>>>> +	int new_value;
>>>> +	struct ctl_table tbl;
>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>> +	int ret;
>>>> +
>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>> +
>>>> +	if (write)
>>>> +		tbl.data = &new_value;
>>>> +	else
>>>> +		tbl.data = &net->sctp.rto_max;
>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>> +	if (write) {
>>>> +                if (ret || new_value > max || new_value < min)
>>>> +			return -EINVAL;
>>>> +		net->sctp.rto_max = new_value;
>>>> +	}
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  int sctp_sysctl_net_register(struct net *net)
>>>>  {
>>>>  	struct ctl_table *table;
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> .
>>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/07/2013 04:12 PM, Daniel Borkmann wrote:
> On 12/07/2013 08:01 PM, Vlad Yasevich wrote:
>> On 12/07/2013 07:43 AM, Daniel Borkmann wrote:
>>> On 12/07/2013 08:17 AM, Wang Weidong wrote:
>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>> sctp_setsockopt_rtoinfo.
>>>>
>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>> ---
>>>
>>> Thanks Wang, also for your second patch.
>>>
>>> Second one looks good to me, thanks for the cleanup!
>>>
>>> I was wondering where 86400000 comes from? Looking through the git
>>> history didn't give much clues and the RFC4960 neither. Clearly,
>>> section 15 of RFC4960 *recommends* as initial values ...
>>>
>>>    RTO.Initial - 3 seconds
>>>    RTO.Min - 1 second
>>>    RTO.Max - 60 seconds
>>>
>>> ... which we have as constants in [1] and are assigned to globals
>>> initially in [2,3] with those recommended values. That's all good.
>>>
>>> But still [not *directly* related to your patch though], where does
>>> 86400000 come from? I expect that's for the max SCTP heartbeat
>>> interval or max cookie lifetime?
>>
>> No, initially it was defined as rto_timer_max and was the upper bound
>> for the rto timer.   When you think about it, it's a bit ridiculous
>> really.  What you are saying is that your rto timer is allowed to
>> grow as long as 1 day, so you would at an absolute maximum retransmit
>> one packet per day :)
> 
> Exactly, maybe initial SCTP implementors already took into account
> we could have SCTP connections to other galaxies, but clearly untested
> so far? :)
> 
> I think we should be absolutely fine with a max configurable upper
> limit of twice the recommended RTO.Max value from the RFC.

That would only put at at 2 min...  That may not be enough in certain
deployments.  Yes, they always have the option to set it per socket,
but that becomes a pain and harder to tune the same app to different
links.  I think 2 hours might be a safer maximum RTO.Max, but I find
it hard to justify shrinking this value.  It isn't really broken
and if someone does set it to 6 hours, they really are shooting
themselves in the foot :)

-vlad

> 
>> I don't think this limit is specified anywhere as is though.  It
>> was something that's been there since the 2.5 days.
>>
>> -vlad

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/08/2013 09:28 PM, Wang Weidong wrote:
> On 2013/12/9 10:19, Vlad Yasevich wrote:
>> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>>> sctp_setsockopt_rtoinfo.
>>>>>
>>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>>> ---
>>>>>  include/net/sctp/constants.h |  3 ++
>>>>>  net/sctp/socket.c            |  5 +++
>>>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>> index 2f0a565..d276978 100644
>>>>> --- a/include/net/sctp/constants.h
>>>>> +++ b/include/net/sctp/constants.h
>>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>>  
>>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>>> +
>>>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>>>  
>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>> index 72046b9..13411ad 100644
>>>>> --- a/net/sctp/socket.c
>>>>> +++ b/net/sctp/socket.c
>>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>>  		return -EFAULT;
>>>>>  
>>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>>> +		return -EINVAL;
>>>>> +
>>>>
>>>> You can not do the check for srto_min < 1.  The following is the text
>>>> from the spec:
>>>>    All times are given in milliseconds.  A value of 0, when modifying
>>>>    the parameters, indicates that the current value should not be
>>>>    changed.
>>>>
>>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>>> Thanks.
>>>
>>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>>> if it makes sense to bind the upper limit here, as well.
>>>>
>>>> -vlad
>>>>
>>> Here, I am not sure as well. I think it should like what we do to the
>>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>>
>>
>> So, the basic reason that sysctl is limited is that it is a default
>> for all sctp association on the system.  It makes some sense to limit
>> what the max value here could be.  Limiting it to double suggested
>> RTO.MAX would only make it 2 minutes and may be insufficient for some
>> of the high latency low-throughput wireless links.  Making it about an
>> hour should be fine...  This would be a separate patch though...
>>
> Here, you mean that we should use 3600*1000 rather than 86400000? So
> we should use another patch to fix that after my patchs?
> 
>> Limiting the user-supplied value is not as appropriate since the
>> assumption is that user application may know better what it's
>> requirements are and it is not up to the stack to limit those.  As
>> long as the user value is withing the usable range (and the kernel
>> will already knows how and does limit this range), we should not
>> limit this further.
>>
>> -vlad
>>
> Agree, So I should check like this:
> !srto_min || !srto_max || srto_min > srto_max ?
> And no need to add macros for checking.

No, I think this would have to be a little more complicated :(
Remember it's ok to have srto_min == 0 and srto_max == 0.  It just
means that no change happens.

You may need to do something like

  unsigned long rto_max, rto_min;

  if (rtoinfo.srto_max)
     rto_max = msecs_to_jiffies(rtoinfo.srto_max);
  else
     rto_max = asoc ? asoc->rto_max : sp->rto_max;

  if (rtoinfo.srto_min)
     rto_min = msecs_to_jiffies(rtoinfo.srto_min);
  else
     rto_min = asoc ? asoc->rto_min : sp->rto_min;

  if (rto_min > rto_max)
      return -EINVAL;

  if (asoc) {
      asoc->rto_min = rto_min;
      asoc->rto_max = rto_max;
   ...
   etc....

This way we make sure that the user that supplied just rto_min or just
rto_max didn't set them so that min > max.

-vlad
> 
> Thanks.
> 
>>>
>>> Regards.
>>> Wang
>>>
>>>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>>  
>>>>>  	/* Set the values to the specific association */
>>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>>> index 6b36561..33c56c6 100644
>>>>> --- a/net/sctp/sysctl.c
>>>>> +++ b/net/sctp/sysctl.c
>>>>> @@ -40,8 +40,8 @@
>>>>>  #include <linux/sysctl.h>
>>>>>  
>>>>>  static int zero = 0;
>>>>> -static int one = 1;
>>>>> -static int timer_max = 86400000; /* ms in one day */
>>>>> +static int one = SCTP_ONE;
>>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>>  static int int_max = INT_MAX;
>>>>>  static int sack_timer_min = 1;
>>>>>  static int sack_timer_max = 500;
>>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>  				void __user *buffer, size_t *lenp,
>>>>>  
>>>>>  				loff_t *ppos);
>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>> +				void __user *buffer, size_t *lenp,
>>>>> +				loff_t *ppos);
>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>> +				void __user *buffer, size_t *lenp,
>>>>> +				loff_t *ppos);
>>>>> +
>>>>>  static struct ctl_table sctp_table[] = {
>>>>>  	{
>>>>>  		.procname	= "sctp_mem",
>>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>>  		.data		= &init_net.sctp.rto_min,
>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>  		.mode		= 0644,
>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>>  		.extra1         = &one,
>>>>> -		.extra2         = &timer_max
>>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>>  	},
>>>>>  	{
>>>>>  		.procname	= "rto_max",
>>>>>  		.data		= &init_net.sctp.rto_max,
>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>  		.mode		= 0644,
>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>> -		.extra1         = &one,
>>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>>  		.extra2         = &timer_max
>>>>>  	},
>>>>>  	{
>>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>> +				void __user*buffer, size_t *lenp,
>>>>> +				loff_t *ppos)
>>>>> +{
>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>> +	int new_value;
>>>>> +	struct ctl_table tbl;
>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>> +	int ret;
>>>>> +
>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>> +
>>>>> +	if (write)
>>>>> +		tbl.data = &new_value;
>>>>> +	else
>>>>> +		tbl.data = &net->sctp.rto_min;
>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>> +	if (write) {
>>>>> +		if (ret || new_value > max || new_value < min)
>>>>> +			return -EINVAL;
>>>>> +		net->sctp.rto_min = new_value;
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>> +				void __user*buffer, size_t *lenp,
>>>>> +				loff_t *ppos)
>>>>> +{
>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>> +	int new_value;
>>>>> +	struct ctl_table tbl;
>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>> +	int ret;
>>>>> +
>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>> +
>>>>> +	if (write)
>>>>> +		tbl.data = &new_value;
>>>>> +	else
>>>>> +		tbl.data = &net->sctp.rto_max;
>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>> +	if (write) {
>>>>> +                if (ret || new_value > max || new_value < min)
>>>>> +			return -EINVAL;
>>>>> +		net->sctp.rto_max = new_value;
>>>>> +	}
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>  int sctp_sysctl_net_register(struct net *net)
>>>>>  {
>>>>>  	struct ctl_table *table;
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>> .
>>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> .
>>
> 
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/9 10:40, Vlad Yasevich wrote:
> On 12/08/2013 09:28 PM, Wang Weidong wrote:
>> On 2013/12/9 10:19, Vlad Yasevich wrote:
>>> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>>>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>>>> sctp_setsockopt_rtoinfo.
>>>>>>
>>>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>>>> ---
>>>>>>  include/net/sctp/constants.h |  3 ++
>>>>>>  net/sctp/socket.c            |  5 +++
>>>>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>>> index 2f0a565..d276978 100644
>>>>>> --- a/include/net/sctp/constants.h
>>>>>> +++ b/include/net/sctp/constants.h
>>>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>>>  
>>>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>>>> +
>>>>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>>>>  
>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>> index 72046b9..13411ad 100644
>>>>>> --- a/net/sctp/socket.c
>>>>>> +++ b/net/sctp/socket.c
>>>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>>>  		return -EFAULT;
>>>>>>  
>>>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>
>>>>> You can not do the check for srto_min < 1.  The following is the text
>>>>> from the spec:
>>>>>    All times are given in milliseconds.  A value of 0, when modifying
>>>>>    the parameters, indicates that the current value should not be
>>>>>    changed.
>>>>>
>>>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>>>> Thanks.
>>>>
>>>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>>>> if it makes sense to bind the upper limit here, as well.
>>>>>
>>>>> -vlad
>>>>>
>>>> Here, I am not sure as well. I think it should like what we do to the
>>>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>>>
>>>
>>> So, the basic reason that sysctl is limited is that it is a default
>>> for all sctp association on the system.  It makes some sense to limit
>>> what the max value here could be.  Limiting it to double suggested
>>> RTO.MAX would only make it 2 minutes and may be insufficient for some
>>> of the high latency low-throughput wireless links.  Making it about an
>>> hour should be fine...  This would be a separate patch though...
>>>
>> Here, you mean that we should use 3600*1000 rather than 86400000? So
>> we should use another patch to fix that after my patchs?
>>
>>> Limiting the user-supplied value is not as appropriate since the
>>> assumption is that user application may know better what it's
>>> requirements are and it is not up to the stack to limit those.  As
>>> long as the user value is withing the usable range (and the kernel
>>> will already knows how and does limit this range), we should not
>>> limit this further.
>>>
>>> -vlad
>>>
>> Agree, So I should check like this:
>> !srto_min || !srto_max || srto_min > srto_max ?
>> And no need to add macros for checking.
> 
> No, I think this would have to be a little more complicated :(
> Remember it's ok to have srto_min == 0 and srto_max == 0.  It just
> means that no change happens.
> 
> You may need to do something like
> 
>   unsigned long rto_max, rto_min;
> 
>   if (rtoinfo.srto_max)
>      rto_max = msecs_to_jiffies(rtoinfo.srto_max);
>   else
>      rto_max = asoc ? asoc->rto_max : sp->rto_max;
> 
>   if (rtoinfo.srto_min)
>      rto_min = msecs_to_jiffies(rtoinfo.srto_min);
>   else
>      rto_min = asoc ? asoc->rto_min : sp->rto_min;
> 
>   if (rto_min > rto_max)
>       return -EINVAL;
> 
>   if (asoc) {
>       asoc->rto_min = rto_min;
>       asoc->rto_max = rto_max;
>    ...
>    etc....
> 
> This way we make sure that the user that supplied just rto_min or just
> rto_max didn't set them so that min > max.
> 
> -vlad

Nice.

Thanks. 

>>
>> Thanks.
>>
>>>>
>>>> Regards.
>>>> Wang
>>>>
>>>>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>>>  
>>>>>>  	/* Set the values to the specific association */
>>>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>>>> index 6b36561..33c56c6 100644
>>>>>> --- a/net/sctp/sysctl.c
>>>>>> +++ b/net/sctp/sysctl.c
>>>>>> @@ -40,8 +40,8 @@
>>>>>>  #include <linux/sysctl.h>
>>>>>>  
>>>>>>  static int zero = 0;
>>>>>> -static int one = 1;
>>>>>> -static int timer_max = 86400000; /* ms in one day */
>>>>>> +static int one = SCTP_ONE;
>>>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>>>  static int int_max = INT_MAX;
>>>>>>  static int sack_timer_min = 1;
>>>>>>  static int sack_timer_max = 500;
>>>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>  				void __user *buffer, size_t *lenp,
>>>>>>  
>>>>>>  				loff_t *ppos);
>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>> +				loff_t *ppos);
>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>> +				loff_t *ppos);
>>>>>> +
>>>>>>  static struct ctl_table sctp_table[] = {
>>>>>>  	{
>>>>>>  		.procname	= "sctp_mem",
>>>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>>>  		.data		= &init_net.sctp.rto_min,
>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>  		.mode		= 0644,
>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>>>  		.extra1         = &one,
>>>>>> -		.extra2         = &timer_max
>>>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>>>  	},
>>>>>>  	{
>>>>>>  		.procname	= "rto_max",
>>>>>>  		.data		= &init_net.sctp.rto_max,
>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>  		.mode		= 0644,
>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>> -		.extra1         = &one,
>>>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>>>  		.extra2         = &timer_max
>>>>>>  	},
>>>>>>  	{
>>>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>  	return ret;
>>>>>>  }
>>>>>>  
>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>> +				loff_t *ppos)
>>>>>> +{
>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>> +	int new_value;
>>>>>> +	struct ctl_table tbl;
>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>> +
>>>>>> +	if (write)
>>>>>> +		tbl.data = &new_value;
>>>>>> +	else
>>>>>> +		tbl.data = &net->sctp.rto_min;
>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>> +	if (write) {
>>>>>> +		if (ret || new_value > max || new_value < min)
>>>>>> +			return -EINVAL;
>>>>>> +		net->sctp.rto_min = new_value;
>>>>>> +	}
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>> +				loff_t *ppos)
>>>>>> +{
>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>> +	int new_value;
>>>>>> +	struct ctl_table tbl;
>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>> +
>>>>>> +	if (write)
>>>>>> +		tbl.data = &new_value;
>>>>>> +	else
>>>>>> +		tbl.data = &net->sctp.rto_max;
>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>> +	if (write) {
>>>>>> +                if (ret || new_value > max || new_value < min)
>>>>>> +			return -EINVAL;
>>>>>> +		net->sctp.rto_max = new_value;
>>>>>> +	}
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>>  int sctp_sysctl_net_register(struct net *net)
>>>>>>  {
>>>>>>  	struct ctl_table *table;
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> .
>>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

On 2013/12/9 10:40, Vlad Yasevich wrote:
> On 12/08/2013 09:28 PM, Wang Weidong wrote:
>> On 2013/12/9 10:19, Vlad Yasevich wrote:
>>> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>>>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>>>> sctp_setsockopt_rtoinfo.
>>>>>>
>>>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>>>> ---
>>>>>>  include/net/sctp/constants.h |  3 ++
>>>>>>  net/sctp/socket.c            |  5 +++
>>>>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>>> index 2f0a565..d276978 100644
>>>>>> --- a/include/net/sctp/constants.h
>>>>>> +++ b/include/net/sctp/constants.h
>>>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>>>  
>>>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>>>> +
>>>>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>>>>  
>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>> index 72046b9..13411ad 100644
>>>>>> --- a/net/sctp/socket.c
>>>>>> +++ b/net/sctp/socket.c
>>>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>>>  		return -EFAULT;
>>>>>>  
>>>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>
>>>>> You can not do the check for srto_min < 1.  The following is the text
>>>>> from the spec:
>>>>>    All times are given in milliseconds.  A value of 0, when modifying
>>>>>    the parameters, indicates that the current value should not be
>>>>>    changed.
>>>>>
>>>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>>>> Thanks.
>>>>
>>>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>>>> if it makes sense to bind the upper limit here, as well.
>>>>>
>>>>> -vlad
>>>>>
>>>> Here, I am not sure as well. I think it should like what we do to the
>>>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>>>
>>>
>>> So, the basic reason that sysctl is limited is that it is a default
>>> for all sctp association on the system.  It makes some sense to limit
>>> what the max value here could be.  Limiting it to double suggested
>>> RTO.MAX would only make it 2 minutes and may be insufficient for some
>>> of the high latency low-throughput wireless links.  Making it about an
>>> hour should be fine...  This would be a separate patch though...
>>>
>> Here, you mean that we should use 3600*1000 rather than 86400000? So
>> we should use another patch to fix that after my patchs?
>>
>>> Limiting the user-supplied value is not as appropriate since the
>>> assumption is that user application may know better what it's
>>> requirements are and it is not up to the stack to limit those.  As
>>> long as the user value is withing the usable range (and the kernel
>>> will already knows how and does limit this range), we should not
>>> limit this further.
>>>
>>> -vlad
>>>
>> Agree, So I should check like this:
>> !srto_min || !srto_max || srto_min > srto_max ?
>> And no need to add macros for checking.
> 
> No, I think this would have to be a little more complicated :(
> Remember it's ok to have srto_min == 0 and srto_max == 0.  It just
> means that no change happens.
> 
> You may need to do something like
> 
>   unsigned long rto_max, rto_min;
> 
>   if (rtoinfo.srto_max)
>      rto_max = msecs_to_jiffies(rtoinfo.srto_max);
>   else
>      rto_max = asoc ? asoc->rto_max : sp->rto_max;
> 
>   if (rtoinfo.srto_min)
>      rto_min = msecs_to_jiffies(rtoinfo.srto_min);
>   else
>      rto_min = asoc ? asoc->rto_min : sp->rto_min;
> 
>   if (rto_min > rto_max)
>       return -EINVAL;
> 
>   if (asoc) {
>       asoc->rto_min = rto_min;
>       asoc->rto_max = rto_max;
>    ...
>    etc....
> 
> This way we make sure that the user that supplied just rto_min or just
> rto_max didn't set them so that min > max.
> 
> -vlad

Hi vald,

I found that we had checked the value of 0 in sctp_setsockopt_rtoinfo.
So I only do this:

if (asoc) {
+	if (msecs_to_jiffies(rtoinfo.srto_min) >
+                   msecs_to_jiffies(rtoinfo.srto_max))
+                       return -EINVAL;
	...
} else {
	...
+	if (rtoinfo.srto_min > rtoinfo.srto_max)
+                       return -EINVAL;
	...
}

There because we set value to asoc and sp is not same. So I add the
check into two path.

Regards.
Wang

>>
>> Thanks.
>>
>>>>
>>>> Regards.
>>>> Wang
>>>>
>>>>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>>>  
>>>>>>  	/* Set the values to the specific association */
>>>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>>>> index 6b36561..33c56c6 100644
>>>>>> --- a/net/sctp/sysctl.c
>>>>>> +++ b/net/sctp/sysctl.c
>>>>>> @@ -40,8 +40,8 @@
>>>>>>  #include <linux/sysctl.h>
>>>>>>  
>>>>>>  static int zero = 0;
>>>>>> -static int one = 1;
>>>>>> -static int timer_max = 86400000; /* ms in one day */
>>>>>> +static int one = SCTP_ONE;
>>>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>>>  static int int_max = INT_MAX;
>>>>>>  static int sack_timer_min = 1;
>>>>>>  static int sack_timer_max = 500;
>>>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>  				void __user *buffer, size_t *lenp,
>>>>>>  
>>>>>>  				loff_t *ppos);
>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>> +				loff_t *ppos);
>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>> +				loff_t *ppos);
>>>>>> +
>>>>>>  static struct ctl_table sctp_table[] = {
>>>>>>  	{
>>>>>>  		.procname	= "sctp_mem",
>>>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>>>  		.data		= &init_net.sctp.rto_min,
>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>  		.mode		= 0644,
>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>>>  		.extra1         = &one,
>>>>>> -		.extra2         = &timer_max
>>>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>>>  	},
>>>>>>  	{
>>>>>>  		.procname	= "rto_max",
>>>>>>  		.data		= &init_net.sctp.rto_max,
>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>  		.mode		= 0644,
>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>> -		.extra1         = &one,
>>>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>>>  		.extra2         = &timer_max
>>>>>>  	},
>>>>>>  	{
>>>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>  	return ret;
>>>>>>  }
>>>>>>  
>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>> +				loff_t *ppos)
>>>>>> +{
>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>> +	int new_value;
>>>>>> +	struct ctl_table tbl;
>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>> +
>>>>>> +	if (write)
>>>>>> +		tbl.data = &new_value;
>>>>>> +	else
>>>>>> +		tbl.data = &net->sctp.rto_min;
>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>> +	if (write) {
>>>>>> +		if (ret || new_value > max || new_value < min)
>>>>>> +			return -EINVAL;
>>>>>> +		net->sctp.rto_min = new_value;
>>>>>> +	}
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>> +				loff_t *ppos)
>>>>>> +{
>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>> +	int new_value;
>>>>>> +	struct ctl_table tbl;
>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>> +
>>>>>> +	if (write)
>>>>>> +		tbl.data = &new_value;
>>>>>> +	else
>>>>>> +		tbl.data = &net->sctp.rto_max;
>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>> +	if (write) {
>>>>>> +                if (ret || new_value > max || new_value < min)
>>>>>> +			return -EINVAL;
>>>>>> +		net->sctp.rto_max = new_value;
>>>>>> +	}
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>>  int sctp_sysctl_net_register(struct net *net)
>>>>>>  {
>>>>>>  	struct ctl_table *table;
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> .
>>>
>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Vlad Yasevich]

On 12/08/2013 10:28 PM, Wang Weidong wrote:
> On 2013/12/9 10:40, Vlad Yasevich wrote:
>> On 12/08/2013 09:28 PM, Wang Weidong wrote:
>>> On 2013/12/9 10:19, Vlad Yasevich wrote:
>>>> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>>>>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>>>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>>>>> sctp_setsockopt_rtoinfo.
>>>>>>>
>>>>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>>>>> ---
>>>>>>>  include/net/sctp/constants.h |  3 ++
>>>>>>>  net/sctp/socket.c            |  5 +++
>>>>>>>  net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>>>>  3 files changed, 75 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>>>> index 2f0a565..d276978 100644
>>>>>>> --- a/include/net/sctp/constants.h
>>>>>>> +++ b/include/net/sctp/constants.h
>>>>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>>>  #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>>>>  #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>>>>  
>>>>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>>>>> +
>>>>>>>  /* Maximum number of new data packets that can be sent in a burst.  */
>>>>>>>  #define SCTP_DEFAULT_MAX_BURST		4
>>>>>>>  
>>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>>> index 72046b9..13411ad 100644
>>>>>>> --- a/net/sctp/socket.c
>>>>>>> +++ b/net/sctp/socket.c
>>>>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>>>>  	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>>>>  		return -EFAULT;
>>>>>>>  
>>>>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>>>>> +		return -EINVAL;
>>>>>>> +
>>>>>>
>>>>>> You can not do the check for srto_min < 1.  The following is the text
>>>>>> from the spec:
>>>>>>    All times are given in milliseconds.  A value of 0, when modifying
>>>>>>    the parameters, indicates that the current value should not be
>>>>>>    changed.
>>>>>>
>>>>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>>>>> Thanks.
>>>>>
>>>>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>>>>> if it makes sense to bind the upper limit here, as well.
>>>>>>
>>>>>> -vlad
>>>>>>
>>>>> Here, I am not sure as well. I think it should like what we do to the
>>>>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>>>>
>>>>
>>>> So, the basic reason that sysctl is limited is that it is a default
>>>> for all sctp association on the system.  It makes some sense to limit
>>>> what the max value here could be.  Limiting it to double suggested
>>>> RTO.MAX would only make it 2 minutes and may be insufficient for some
>>>> of the high latency low-throughput wireless links.  Making it about an
>>>> hour should be fine...  This would be a separate patch though...
>>>>
>>> Here, you mean that we should use 3600*1000 rather than 86400000? So
>>> we should use another patch to fix that after my patchs?
>>>
>>>> Limiting the user-supplied value is not as appropriate since the
>>>> assumption is that user application may know better what it's
>>>> requirements are and it is not up to the stack to limit those.  As
>>>> long as the user value is withing the usable range (and the kernel
>>>> will already knows how and does limit this range), we should not
>>>> limit this further.
>>>>
>>>> -vlad
>>>>
>>> Agree, So I should check like this:
>>> !srto_min || !srto_max || srto_min > srto_max ?
>>> And no need to add macros for checking.
>>
>> No, I think this would have to be a little more complicated :(
>> Remember it's ok to have srto_min == 0 and srto_max == 0.  It just
>> means that no change happens.
>>
>> You may need to do something like
>>
>>   unsigned long rto_max, rto_min;
>>
>>   if (rtoinfo.srto_max)
>>      rto_max = msecs_to_jiffies(rtoinfo.srto_max);
>>   else
>>      rto_max = asoc ? asoc->rto_max : sp->rto_max;
>>
>>   if (rtoinfo.srto_min)
>>      rto_min = msecs_to_jiffies(rtoinfo.srto_min);
>>   else
>>      rto_min = asoc ? asoc->rto_min : sp->rto_min;
>>
>>   if (rto_min > rto_max)
>>       return -EINVAL;
>>
>>   if (asoc) {
>>       asoc->rto_min = rto_min;
>>       asoc->rto_max = rto_max;
>>    ...
>>    etc....
>>
>> This way we make sure that the user that supplied just rto_min or just
>> rto_max didn't set them so that min > max.
>>
>> -vlad
> 
> Hi vald,
> 
> I found that we had checked the value of 0 in sctp_setsockopt_rtoinfo.
> So I only do this:
> 
> if (asoc) {
> +	if (msecs_to_jiffies(rtoinfo.srto_min) >
> +                   msecs_to_jiffies(rtoinfo.srto_max))
> +                       return -EINVAL;
> 	...

What if the value in rtoinfo is 0?  Right now, the doesn't do any
comparisons and just assigns values into the assoc or sp as long
as the user provided a non-0 value.

Now imagine the user did this:

    rtoinfo.srto_min = 0
    rtoinfo.srto_max = 5;
    setsockopt();

    ....  later on...

    rtoinfo.srto_min = 8;
    rtoinfo.srto_max = 0;
    setsockopt();

No you have a situation where min > max. However both calls were valid.

My suggestion to you, split the sysctl change into a separate patch and
and do socket option handling in its own patch.  Also, please be sure
to test it with different variants of the calls.

-vlad

> } else {
> 	...
> +	if (rtoinfo.srto_min > rtoinfo.srto_max)
> +                       return -EINVAL;
> 	...
> }
> 
> There because we set value to asoc and sp is not same. So I add the
> check into two path.
> 
> Regards.
> Wang
> 
>>>
>>> Thanks.
>>>
>>>>>
>>>>> Regards.
>>>>> Wang
>>>>>
>>>>>>>  	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>>>>  
>>>>>>>  	/* Set the values to the specific association */
>>>>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>>>>> index 6b36561..33c56c6 100644
>>>>>>> --- a/net/sctp/sysctl.c
>>>>>>> +++ b/net/sctp/sysctl.c
>>>>>>> @@ -40,8 +40,8 @@
>>>>>>>  #include <linux/sysctl.h>
>>>>>>>  
>>>>>>>  static int zero = 0;
>>>>>>> -static int one = 1;
>>>>>>> -static int timer_max = 86400000; /* ms in one day */
>>>>>>> +static int one = SCTP_ONE;
>>>>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>>>>  static int int_max = INT_MAX;
>>>>>>>  static int sack_timer_min = 1;
>>>>>>>  static int sack_timer_max = 500;
>>>>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>>  				void __user *buffer, size_t *lenp,
>>>>>>>  
>>>>>>>  				loff_t *ppos);
>>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>>> +				loff_t *ppos);
>>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>>> +				loff_t *ppos);
>>>>>>> +
>>>>>>>  static struct ctl_table sctp_table[] = {
>>>>>>>  	{
>>>>>>>  		.procname	= "sctp_mem",
>>>>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>>>>  		.data		= &init_net.sctp.rto_min,
>>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>>  		.mode		= 0644,
>>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>>>>  		.extra1         = &one,
>>>>>>> -		.extra2         = &timer_max
>>>>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>>>>  	},
>>>>>>>  	{
>>>>>>>  		.procname	= "rto_max",
>>>>>>>  		.data		= &init_net.sctp.rto_max,
>>>>>>>  		.maxlen		= sizeof(unsigned int),
>>>>>>>  		.mode		= 0644,
>>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>>> -		.extra1         = &one,
>>>>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>>>>  		.extra2         = &timer_max
>>>>>>>  	},
>>>>>>>  	{
>>>>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>>  	return ret;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>>> +				loff_t *ppos)
>>>>>>> +{
>>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>>> +	int new_value;
>>>>>>> +	struct ctl_table tbl;
>>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>>> +
>>>>>>> +	if (write)
>>>>>>> +		tbl.data = &new_value;
>>>>>>> +	else
>>>>>>> +		tbl.data = &net->sctp.rto_min;
>>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>>> +	if (write) {
>>>>>>> +		if (ret || new_value > max || new_value < min)
>>>>>>> +			return -EINVAL;
>>>>>>> +		net->sctp.rto_min = new_value;
>>>>>>> +	}
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>>> +				loff_t *ppos)
>>>>>>> +{
>>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>>> +	int new_value;
>>>>>>> +	struct ctl_table tbl;
>>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>>> +
>>>>>>> +	if (write)
>>>>>>> +		tbl.data = &new_value;
>>>>>>> +	else
>>>>>>> +		tbl.data = &net->sctp.rto_max;
>>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>>> +	if (write) {
>>>>>>> +                if (ret || new_value > max || new_value < min)
>>>>>>> +			return -EINVAL;
>>>>>>> +		net->sctp.rto_max = new_value;
>>>>>>> +	}
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>  int sctp_sysctl_net_register(struct net *net)
>>>>>>>  {
>>>>>>>  	struct ctl_table *table;
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>> .
>>>>
>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> .
>>
> 
> 

Re: [PATCH v5 1/2] sctp: check the rto_min and rto_max

[Wang Weidong]

 From Wang Weidong <wangweidong1@huawei.com>

On 2013/12/9 22:55, Vlad Yasevich wrote:
> On 12/08/2013 10:28 PM, Wang Weidong wrote:
>> On 2013/12/9 10:40, Vlad Yasevich wrote:
>>> On 12/08/2013 09:28 PM, Wang Weidong wrote:
>>>> On 2013/12/9 10:19, Vlad Yasevich wrote:
>>>>> On 12/08/2013 08:53 PM, Wang Weidong wrote:
>>>>>> On 2013/12/8 2:54, Vlad Yasevich wrote:
>>>>>>> On 12/07/2013 02:17 AM, Wang Weidong wrote:
>>>>>>>> rto_min should be smaller than rto_max while rto_max should be larger
>>>>>>>> than rto_min. Add two proc_handler for the checking. Add the check in
>>>>>>>> sctp_setsockopt_rtoinfo.
>>>>>>>>
>>>>>>>> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
>>>>>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>>>>>> ---
>>>>>>>>   include/net/sctp/constants.h |  3 ++
>>>>>>>>   net/sctp/socket.c            |  5 +++
>>>>>>>>   net/sctp/sysctl.c            | 73 ++++++++++++++++++++++++++++++++++++++++----
>>>>>>>>   3 files changed, 75 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>>>>>>> index 2f0a565..d276978 100644
>>>>>>>> --- a/include/net/sctp/constants.h
>>>>>>>> +++ b/include/net/sctp/constants.h
>>>>>>>> @@ -279,6 +279,9 @@ enum { SCTP_MAX_GABS = 16 };
>>>>>>>>   #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>>>>>>>   #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>>>>>>>
>>>>>>>> +#define SCTP_ONE                1        /* 1 ms */
>>>>>>>> +#define SCTP_TIMER_MAX          86400000 /* ms in one day */
>>>>>>>> +
>>>>>>>>   /* Maximum number of new data packets that can be sent in a burst.  */
>>>>>>>>   #define SCTP_DEFAULT_MAX_BURST		4
>>>>>>>>
>>>>>>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>>>>>>> index 72046b9..13411ad 100644
>>>>>>>> --- a/net/sctp/socket.c
>>>>>>>> +++ b/net/sctp/socket.c
>>>>>>>> @@ -2818,6 +2818,11 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>>>>>>   	if (copy_from_user(&rtoinfo, optval, optlen))
>>>>>>>>   		return -EFAULT;
>>>>>>>>
>>>>>>>> +	if (rtoinfo.srto_min < SCTP_ONE ||
>>>>>>>> +	    rtoinfo.srto_max > SCTP_TIMER_MAX ||
>>>>>>>> +	    rtoinfo.srto_max < rtoinfo.srto_min)
>>>>>>>> +		return -EINVAL;
>>>>>>>> +
>>>>>>>
>>>>>>> You can not do the check for srto_min < 1.  The following is the text
>>>>>>> from the spec:
>>>>>>>     All times are given in milliseconds.  A value of 0, when modifying
>>>>>>>     the parameters, indicates that the current value should not be
>>>>>>>     changed.
>>>>>>>
>>>>>> Yes, Your are right, I found it in draft-ietf-tsvwg-sctpsocket-14.txt.
>>>>>> Thanks.
>>>>>>
>>>>>>> So, it is valid for a user to pass in a value of 0.  Also, I am not sure
>>>>>>> if it makes sense to bind the upper limit here, as well.
>>>>>>>
>>>>>>> -vlad
>>>>>>>
>>>>>> Here, I am not sure as well. I think it should like what we do to the
>>>>>> init_net.sctp.rto_max when set larger than timer_max. Just not change the value.
>>>>>>
>>>>>
>>>>> So, the basic reason that sysctl is limited is that it is a default
>>>>> for all sctp association on the system.  It makes some sense to limit
>>>>> what the max value here could be.  Limiting it to double suggested
>>>>> RTO.MAX would only make it 2 minutes and may be insufficient for some
>>>>> of the high latency low-throughput wireless links.  Making it about an
>>>>> hour should be fine...  This would be a separate patch though...
>>>>>
>>>> Here, you mean that we should use 3600*1000 rather than 86400000? So
>>>> we should use another patch to fix that after my patchs?
>>>>
>>>>> Limiting the user-supplied value is not as appropriate since the
>>>>> assumption is that user application may know better what it's
>>>>> requirements are and it is not up to the stack to limit those.  As
>>>>> long as the user value is withing the usable range (and the kernel
>>>>> will already knows how and does limit this range), we should not
>>>>> limit this further.
>>>>>
>>>>> -vlad
>>>>>
>>>> Agree, So I should check like this:
>>>> !srto_min || !srto_max || srto_min > srto_max ?
>>>> And no need to add macros for checking.
>>>
>>> No, I think this would have to be a little more complicated :(
>>> Remember it's ok to have srto_min == 0 and srto_max == 0.  It just
>>> means that no change happens.
>>>
>>> You may need to do something like
>>>
>>>    unsigned long rto_max, rto_min;
>>>
>>>    if (rtoinfo.srto_max)
>>>       rto_max = msecs_to_jiffies(rtoinfo.srto_max);
>>>    else
>>>       rto_max = asoc ? asoc->rto_max : sp->rto_max;
>>>
>>>    if (rtoinfo.srto_min)
>>>       rto_min = msecs_to_jiffies(rtoinfo.srto_min);
>>>    else
>>>       rto_min = asoc ? asoc->rto_min : sp->rto_min;
>>>
>>>    if (rto_min > rto_max)
>>>        return -EINVAL;
>>>
>>>    if (asoc) {
>>>        asoc->rto_min = rto_min;
>>>        asoc->rto_max = rto_max;
>>>     ...
>>>     etc....
>>>
>>> This way we make sure that the user that supplied just rto_min or just
>>> rto_max didn't set them so that min > max.
>>>
>>> -vlad
>>
>> Hi vald,
>>
>> I found that we had checked the value of 0 in sctp_setsockopt_rtoinfo.
>> So I only do this:
>>
>> if (asoc) {
>> +	if (msecs_to_jiffies(rtoinfo.srto_min) >
>> +                   msecs_to_jiffies(rtoinfo.srto_max))
>> +                       return -EINVAL;
>> 	...
>
> What if the value in rtoinfo is 0?  Right now, the doesn't do any
> comparisons and just assigns values into the assoc or sp as long
> as the user provided a non-0 value.
>
> Now imagine the user did this:
>
>      rtoinfo.srto_min = 0
>      rtoinfo.srto_max = 5;
>      setsockopt();
>
>      ....  later on...
>
>      rtoinfo.srto_min = 8;
>      rtoinfo.srto_max = 0;
>      setsockopt();
>
> No you have a situation where min > max. However both calls were valid.
>
> My suggestion to you, split the sysctl change into a separate patch and
> and do socket option handling in its own patch.  Also, please be sure
> to test it with different variants of the calls.
>
> -vlad

Yes, You are right. I get it. I just see I can do it with a smaller change while
I not test it. Thanks for your suggestion. I will send these out after I test them.

Regards.
Wang

>
>> } else {
>> 	...
>> +	if (rtoinfo.srto_min > rtoinfo.srto_max)
>> +                       return -EINVAL;
>> 	...
>> }
>>
>> There because we set value to asoc and sp is not same. So I add the
>> check into two path.
>>
>> Regards.
>> Wang
>>
>>>>
>>>> Thanks.
>>>>
>>>>>>
>>>>>> Regards.
>>>>>> Wang
>>>>>>
>>>>>>>>   	asoc = sctp_id2assoc(sk, rtoinfo.srto_assoc_id);
>>>>>>>>
>>>>>>>>   	/* Set the values to the specific association */
>>>>>>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>>>>>>> index 6b36561..33c56c6 100644
>>>>>>>> --- a/net/sctp/sysctl.c
>>>>>>>> +++ b/net/sctp/sysctl.c
>>>>>>>> @@ -40,8 +40,8 @@
>>>>>>>>   #include <linux/sysctl.h>
>>>>>>>>
>>>>>>>>   static int zero = 0;
>>>>>>>> -static int one = 1;
>>>>>>>> -static int timer_max = 86400000; /* ms in one day */
>>>>>>>> +static int one = SCTP_ONE;
>>>>>>>> +static int timer_max = SCTP_TIMER_MAX;
>>>>>>>>   static int int_max = INT_MAX;
>>>>>>>>   static int sack_timer_min = 1;
>>>>>>>>   static int sack_timer_max = 500;
>>>>>>>> @@ -61,6 +61,13 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>>>   				void __user *buffer, size_t *lenp,
>>>>>>>>
>>>>>>>>   				loff_t *ppos);
>>>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>>>> +				loff_t *ppos);
>>>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>>>> +				void __user *buffer, size_t *lenp,
>>>>>>>> +				loff_t *ppos);
>>>>>>>> +
>>>>>>>>   static struct ctl_table sctp_table[] = {
>>>>>>>>   	{
>>>>>>>>   		.procname	= "sctp_mem",
>>>>>>>> @@ -102,17 +109,17 @@ static struct ctl_table sctp_net_table[] = {
>>>>>>>>   		.data		= &init_net.sctp.rto_min,
>>>>>>>>   		.maxlen		= sizeof(unsigned int),
>>>>>>>>   		.mode		= 0644,
>>>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>>>> +		.proc_handler	= proc_sctp_do_rto_min,
>>>>>>>>   		.extra1         = &one,
>>>>>>>> -		.extra2         = &timer_max
>>>>>>>> +		.extra2         = &init_net.sctp.rto_max
>>>>>>>>   	},
>>>>>>>>   	{
>>>>>>>>   		.procname	= "rto_max",
>>>>>>>>   		.data		= &init_net.sctp.rto_max,
>>>>>>>>   		.maxlen		= sizeof(unsigned int),
>>>>>>>>   		.mode		= 0644,
>>>>>>>> -		.proc_handler	= proc_dointvec_minmax,
>>>>>>>> -		.extra1         = &one,
>>>>>>>> +		.proc_handler	= proc_sctp_do_rto_max,
>>>>>>>> +		.extra1         = &init_net.sctp.rto_min,
>>>>>>>>   		.extra2         = &timer_max
>>>>>>>>   	},
>>>>>>>>   	{
>>>>>>>> @@ -342,6 +349,60 @@ static int proc_sctp_do_hmac_alg(struct ctl_table *ctl,
>>>>>>>>   	return ret;
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +static int proc_sctp_do_rto_min(struct ctl_table *ctl, int write,
>>>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>>>> +				loff_t *ppos)
>>>>>>>> +{
>>>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>>>> +	int new_value;
>>>>>>>> +	struct ctl_table tbl;
>>>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>>>> +
>>>>>>>> +	if (write)
>>>>>>>> +		tbl.data = &new_value;
>>>>>>>> +	else
>>>>>>>> +		tbl.data = &net->sctp.rto_min;
>>>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>>>> +	if (write) {
>>>>>>>> +		if (ret || new_value > max || new_value < min)
>>>>>>>> +			return -EINVAL;
>>>>>>>> +		net->sctp.rto_min = new_value;
>>>>>>>> +	}
>>>>>>>> +	return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int proc_sctp_do_rto_max(struct ctl_table *ctl, int write,
>>>>>>>> +				void __user*buffer, size_t *lenp,
>>>>>>>> +				loff_t *ppos)
>>>>>>>> +{
>>>>>>>> +	struct net *net = current->nsproxy->net_ns;
>>>>>>>> +	int new_value;
>>>>>>>> +	struct ctl_table tbl;
>>>>>>>> +	unsigned int min = *(unsigned int *) ctl->extra1;
>>>>>>>> +	unsigned int max = *(unsigned int *) ctl->extra2;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	memset(&tbl, 0, sizeof(struct ctl_table));
>>>>>>>> +	tbl.maxlen = sizeof(unsigned int);
>>>>>>>> +
>>>>>>>> +	if (write)
>>>>>>>> +		tbl.data = &new_value;
>>>>>>>> +	else
>>>>>>>> +		tbl.data = &net->sctp.rto_max;
>>>>>>>> +	ret = proc_dointvec(&tbl, write, buffer, lenp, ppos);
>>>>>>>> +	if (write) {
>>>>>>>> +                if (ret || new_value > max || new_value < min)
>>>>>>>> +			return -EINVAL;
>>>>>>>> +		net->sctp.rto_max = new_value;
>>>>>>>> +	}
>>>>>>>> +	return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   int sctp_sysctl_net_register(struct net *net)
>>>>>>>>   {
>>>>>>>>   	struct ctl_table *table;
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>> .
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>