| Message ID | 1386400651-21744-2-git-send-email-wangweidong1@huawei.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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; > -- 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
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 -- 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
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; > -- 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
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; >> -- 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
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 -- 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
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 > > . > -- 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
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 >> >> . >> > > -- 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
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 > > . > -- 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
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 -- 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
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 >> >> . >> > > -- 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
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 > > . > -- 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
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 > > . > -- 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
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 >> >> . >> > > -- 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
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 > -- 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
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;
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(-)