| Message ID | 1997375e-815d-137f-20c9-0829a8587ee9@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net] ipvs: Improve robustness to the ipvs sysctl | expand |
hujunwei <hujunwei4@huawei.com> wrote: [ trimmed CC list ] > The ipvs module parse the user buffer and save it to sysctl, > then check if the value is valid. invalid value occurs > over a period of time. > Here, I add a variable, struct ctl_table tmp, used to read > the value from the user buffer, and save only when it is valid. Does this cause any problems? If so, what are those? > Fixes: f73181c8288f ("ipvs: add support for sync threads") > Signed-off-by: Junwei Hu <hujunwei4@huawei.com> > --- > net/netfilter/ipvs/ip_vs_ctl.c | 61 +++++++++++++++++++++++----------- > 1 file changed, 42 insertions(+), 19 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > index 741d91aa4a8d..e78fd05f108b 100644 > --- a/net/netfilter/ipvs/ip_vs_ctl.c > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > @@ -1680,12 +1680,18 @@ proc_do_defense_mode(struct ctl_table *table, int write, > int val = *valp; > int rc; > > - rc = proc_dointvec(table, write, buffer, lenp, ppos); > + struct ctl_table tmp = { > + .data = &val, > + .maxlen = sizeof(int), > + .mode = table->mode, > + }; > + > + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); Wouldn't it be better do use proc_dointvec_minmax and set the constraints via .extra1,2 in the sysctl knob definition?
Hi Julian, thank you for replay. On 2019/7/29 8:49, Florian Westphal wrote: > hujunwei <hujunwei4@huawei.com> wrote: > > [ trimmed CC list ] > >> The ipvs module parse the user buffer and save it to sysctl, >> then check if the value is valid. invalid value occurs >> over a period of time. >> Here, I add a variable, struct ctl_table tmp, used to read >> the value from the user buffer, and save only when it is valid. > > Does this cause any problems? If so, what are those? > For example, when a negative number value occurs over a period of time, the func such as ip_vs_sync_conn_v0() will get invalid number by sysctl_sync_threshold(), casue judge abnormal in ip_vs_sync_conn_needed(). >> Fixes: f73181c8288f ("ipvs: add support for sync threads") >> Signed-off-by: Junwei Hu <hujunwei4@huawei.com> >> --- >> net/netfilter/ipvs/ip_vs_ctl.c | 61 +++++++++++++++++++++++----------- >> 1 file changed, 42 insertions(+), 19 deletions(-) >> >> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c >> index 741d91aa4a8d..e78fd05f108b 100644 >> --- a/net/netfilter/ipvs/ip_vs_ctl.c >> +++ b/net/netfilter/ipvs/ip_vs_ctl.c >> @@ -1680,12 +1680,18 @@ proc_do_defense_mode(struct ctl_table *table, int write, >> int val = *valp; >> int rc; >> >> - rc = proc_dointvec(table, write, buffer, lenp, ppos); >> + struct ctl_table tmp = { >> + .data = &val, >> + .maxlen = sizeof(int), >> + .mode = table->mode, >> + }; >> + >> + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); > > Wouldn't it be better do use proc_dointvec_minmax and set the > constraints via .extra1,2 in the sysctl knob definition? > You are right, proc_dointvec_minmax seems like a better choice, I will update the patch. Regards, Junwei
Hello, On Mon, 29 Jul 2019, Florian Westphal wrote: > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > > index 741d91aa4a8d..e78fd05f108b 100644 > > --- a/net/netfilter/ipvs/ip_vs_ctl.c > > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > > @@ -1680,12 +1680,18 @@ proc_do_defense_mode(struct ctl_table *table, int write, > > int val = *valp; > > int rc; > > > > - rc = proc_dointvec(table, write, buffer, lenp, ppos); > > + struct ctl_table tmp = { > > + .data = &val, > > + .maxlen = sizeof(int), > > + .mode = table->mode, > > + }; > > + > > + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); > > Wouldn't it be better do use proc_dointvec_minmax and set the > constraints via .extra1,2 in the sysctl knob definition? We store the 'ipvs' back-ptr in extra2, so may be we can not use it in the table for proc_do_defense_mode, only for tmp. proc_do_sync_mode may use extra1/2 in table for the proc_dointvec_minmax call. Regards -- Julian Anastasov <ja@ssi.bg>
Hello, Julian On 2019/7/30 4:20, Julian Anastasov wrote: > > Hello, > > On Mon, 29 Jul 2019, Florian Westphal wrote: > >>> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c >>> index 741d91aa4a8d..e78fd05f108b 100644 >>> --- a/net/netfilter/ipvs/ip_vs_ctl.c >>> +++ b/net/netfilter/ipvs/ip_vs_ctl.c >>> @@ -1680,12 +1680,18 @@ proc_do_defense_mode(struct ctl_table *table, int write, >>> int val = *valp; >>> int rc; >>> >>> - rc = proc_dointvec(table, write, buffer, lenp, ppos); >>> + struct ctl_table tmp = { >>> + .data = &val, >>> + .maxlen = sizeof(int), >>> + .mode = table->mode, >>> + }; >>> + >>> + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); >> >> Wouldn't it be better do use proc_dointvec_minmax and set the >> constraints via .extra1,2 in the sysctl knob definition? > > We store the 'ipvs' back-ptr in extra2, so may be we > can not use it in the table for proc_do_defense_mode, only for > tmp. proc_do_sync_mode may use extra1/2 in table for the > proc_dointvec_minmax call. > > Regards > > -- > Julian Anastasov <ja@ssi.bg> > > . > I agree with you, in these four function, only proc_do_sync_mode can use extra1/2 in table for the proc_dointvec_minmax, i will update it in patch v2. Thank you for reply. Regards, Junwei
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 741d91aa4a8d..e78fd05f108b 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -1680,12 +1680,18 @@ proc_do_defense_mode(struct ctl_table *table, int write, int val = *valp; int rc; - rc = proc_dointvec(table, write, buffer, lenp, ppos); + struct ctl_table tmp = { + .data = &val, + .maxlen = sizeof(int), + .mode = table->mode, + }; + + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); if (write && (*valp != val)) { - if ((*valp < 0) || (*valp > 3)) { - /* Restore the correct value */ + if (val < 0 || val > 3) + rc = -EINVAL; + else { *valp = val; - } else { update_defense_level(ipvs); } } @@ -1699,15 +1705,20 @@ proc_do_sync_threshold(struct ctl_table *table, int write, int *valp = table->data; int val[2]; int rc; + struct ctl_table tmp = { + .data = &val, + .maxlen = table->maxlen, + .mode = table->mode, + }; - /* backup the value first */ memcpy(val, valp, sizeof(val)); - - rc = proc_dointvec(table, write, buffer, lenp, ppos); - if (write && (valp[0] < 0 || valp[1] < 0 || - (valp[0] >= valp[1] && valp[1]))) { - /* Restore the correct value */ - memcpy(valp, val, sizeof(val)); + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); + if (write) { + if (val[0] < 0 || val[1] < 0 || + (val[0] >= val[1] && val[1])) + rc = -EINVAL; + else + memcpy(valp, val, sizeof(val)); } return rc; } @@ -1720,12 +1731,18 @@ proc_do_sync_mode(struct ctl_table *table, int write, int val = *valp; int rc; - rc = proc_dointvec(table, write, buffer, lenp, ppos); + struct ctl_table tmp = { + .data = &val, + .maxlen = sizeof(int), + .mode = table->mode, + }; + + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); if (write && (*valp != val)) { - if ((*valp < 0) || (*valp > 1)) { - /* Restore the correct value */ + if (val < 0 || val > 1) + rc = -EINVAL; + else *valp = val; - } } return rc; } @@ -1738,12 +1755,18 @@ proc_do_sync_ports(struct ctl_table *table, int write, int val = *valp; int rc; - rc = proc_dointvec(table, write, buffer, lenp, ppos); + struct ctl_table tmp = { + .data = &val, + .maxlen = sizeof(int), + .mode = table->mode, + }; + + rc = proc_dointvec(&tmp, write, buffer, lenp, ppos); if (write && (*valp != val)) { - if (*valp < 1 || !is_power_of_2(*valp)) { - /* Restore the correct value */ + if (val < 1 || !is_power_of_2(val)) + rc = -EINVAL; + else *valp = val; - } } return rc; }