Message ID | 20170830184211.12471-4-bart.vanassche@wdc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> Il giorno 30 ago 2017, alle ore 20:42, Bart Van Assche <bart.vanassche@wdc.com> ha scritto: > > Make sysfs writes fail for invalid numbers instead of storing > uninitialized data copied from the stack. This patch removes > all uninitialized_var() occurrences from the BFQ source code. > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Paolo Valente <paolo.valente@linaro.org> Acked-by: Paolo Valente <paolo.valente@linaro.org> > --- > block/bfq-iosched.c | 52 +++++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 37 insertions(+), 15 deletions(-) > > diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c > index 8c11c2e827a5..cf92f16eb5f2 100644 > --- a/block/bfq-iosched.c > +++ b/block/bfq-iosched.c > @@ -4802,13 +4802,15 @@ static ssize_t bfq_var_show(unsigned int var, char *page) > return sprintf(page, "%u\n", var); > } > > -static void bfq_var_store(unsigned long *var, const char *page) > +static int bfq_var_store(unsigned long *var, const char *page) > { > unsigned long new_val; > int ret = kstrtoul(page, 10, &new_val); > > - if (ret == 0) > - *var = new_val; > + if (ret) > + return ret; > + *var = new_val; > + return 0; > } > > #define SHOW_FUNCTION(__FUNC, __VAR, __CONV) \ > @@ -4849,8 +4851,12 @@ static ssize_t \ > __FUNC(struct elevator_queue *e, const char *page, size_t count) \ > { \ > struct bfq_data *bfqd = e->elevator_data; \ > - unsigned long uninitialized_var(__data); \ > - bfq_var_store(&__data, (page)); \ > + unsigned long __data; \ > + int ret; \ > + \ > + ret = bfq_var_store(&__data, (page)); \ > + if (ret) \ > + return ret; \ > if (__data < (MIN)) \ > __data = (MIN); \ > else if (__data > (MAX)) \ > @@ -4877,8 +4883,12 @@ STORE_FUNCTION(bfq_slice_idle_store, &bfqd->bfq_slice_idle, 0, INT_MAX, 2); > static ssize_t __FUNC(struct elevator_queue *e, const char *page, size_t count)\ > { \ > struct bfq_data *bfqd = e->elevator_data; \ > - unsigned long uninitialized_var(__data); \ > - bfq_var_store(&__data, (page)); \ > + unsigned long __data; \ > + int ret; \ > + \ > + ret = bfq_var_store(&__data, (page)); \ > + if (ret) \ > + return ret; \ > if (__data < (MIN)) \ > __data = (MIN); \ > else if (__data > (MAX)) \ > @@ -4894,9 +4904,12 @@ static ssize_t bfq_max_budget_store(struct elevator_queue *e, > const char *page, size_t count) > { > struct bfq_data *bfqd = e->elevator_data; > - unsigned long uninitialized_var(__data); > + unsigned long __data; > + int ret; > > - bfq_var_store(&__data, (page)); > + ret = bfq_var_store(&__data, (page)); > + if (ret) > + return ret; > > if (__data == 0) > bfqd->bfq_max_budget = bfq_calc_max_budget(bfqd); > @@ -4919,9 +4932,12 @@ static ssize_t bfq_timeout_sync_store(struct elevator_queue *e, > const char *page, size_t count) > { > struct bfq_data *bfqd = e->elevator_data; > - unsigned long uninitialized_var(__data); > + unsigned long __data; > + int ret; > > - bfq_var_store(&__data, (page)); > + ret = bfq_var_store(&__data, (page)); > + if (ret) > + return ret; > > if (__data < 1) > __data = 1; > @@ -4939,9 +4955,12 @@ static ssize_t bfq_strict_guarantees_store(struct elevator_queue *e, > const char *page, size_t count) > { > struct bfq_data *bfqd = e->elevator_data; > - unsigned long uninitialized_var(__data); > + unsigned long __data; > + int ret; > > - bfq_var_store(&__data, (page)); > + ret = bfq_var_store(&__data, (page)); > + if (ret) > + return ret; > > if (__data > 1) > __data = 1; > @@ -4958,9 +4977,12 @@ static ssize_t bfq_low_latency_store(struct elevator_queue *e, > const char *page, size_t count) > { > struct bfq_data *bfqd = e->elevator_data; > - unsigned long uninitialized_var(__data); > + unsigned long __data; > + int ret; > > - bfq_var_store(&__data, (page)); > + ret = bfq_var_store(&__data, (page)); > + if (ret) > + return ret; > > if (__data > 1) > __data = 1; > -- > 2.14.1 >
2017-09-02 1:14 GMT+08:00 Paolo Valente <paolo.valente@linaro.org>: > >> Il giorno 30 ago 2017, alle ore 20:42, Bart Van Assche <bart.vanassche@wdc.com> ha scritto: >> >> Make sysfs writes fail for invalid numbers instead of storing >> uninitialized data copied from the stack. This patch removes >> all uninitialized_var() occurrences from the BFQ source code. >> >> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> >> Cc: Paolo Valente <paolo.valente@linaro.org> > > Acked-by: Paolo Valente <paolo.valente@linaro.org> > Hi Bart, how about using simple_strtoul which was used in cfq/mq-iosched.c *var = simple_strtoul(p, &p, 10); if invalid string came from sysfs, this function just return 0, and there are validations after every calling bfq_var_store. Thanks
On Sat, 2017-09-02 at 01:37 +0800, weiping zhang wrote: > 2017-09-02 1:14 GMT+08:00 Paolo Valente <paolo.valente@linaro.org>: > > Il giorno 30 ago 2017, alle ore 20:42, Bart Van Assche <bart.vanassche@wdc.com> ha scritto: > > > > > > Make sysfs writes fail for invalid numbers instead of storing > > > uninitialized data copied from the stack. This patch removes > > > all uninitialized_var() occurrences from the BFQ source code. > > > > > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > > > Cc: Paolo Valente <paolo.valente@linaro.org> > > > > Acked-by: Paolo Valente <paolo.valente@linaro.org> > > how about using simple_strtoul which was used in cfq/mq-iosched.c > *var = simple_strtoul(p, &p, 10); > > if invalid string came from sysfs, this function just return 0, > and there are validations after every calling bfq_var_store. Hello Weiping, Sorry but I do not like this proposal because: * If invalid input is provided writing into a sysfs attribute should fail instead of ignoring the invalid input silently. * simple_strtoul() is considered obsolete and must not be used in new code. From include/linux/kernel.h: /* Obsolete, do not use. Use kstrto<foo> instead */ extern unsigned long simple_strtoul(const char *,char **,unsigned int); extern long simple_strtol(const char *,char **,unsigned int); extern unsigned long long simple_strtoull(const char *,char **,unsigned int); extern long long simple_strtoll(const char *,char **,unsigned int); Bart.
> Sorry but I do not like this proposal because: > * If invalid input is provided writing into a sysfs attribute should fail > instead of ignoring the invalid input silently. > * simple_strtoul() is considered obsolete and must not be used in new code. > From include/linux/kernel.h: > > /* Obsolete, do not use. Use kstrto<foo> instead */ > > extern unsigned long simple_strtoul(const char *,char **,unsigned int); > extern long simple_strtol(const char *,char **,unsigned int); > extern unsigned long long simple_strtoull(const char *,char **,unsigned int); > extern long long simple_strtoll(const char *,char **,unsigned int); > Hello Bart, Agree with you, it seems more reasonable to give error message to user. Thanks
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 8c11c2e827a5..cf92f16eb5f2 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4802,13 +4802,15 @@ static ssize_t bfq_var_show(unsigned int var, char *page) return sprintf(page, "%u\n", var); } -static void bfq_var_store(unsigned long *var, const char *page) +static int bfq_var_store(unsigned long *var, const char *page) { unsigned long new_val; int ret = kstrtoul(page, 10, &new_val); - if (ret == 0) - *var = new_val; + if (ret) + return ret; + *var = new_val; + return 0; } #define SHOW_FUNCTION(__FUNC, __VAR, __CONV) \ @@ -4849,8 +4851,12 @@ static ssize_t \ __FUNC(struct elevator_queue *e, const char *page, size_t count) \ { \ struct bfq_data *bfqd = e->elevator_data; \ - unsigned long uninitialized_var(__data); \ - bfq_var_store(&__data, (page)); \ + unsigned long __data; \ + int ret; \ + \ + ret = bfq_var_store(&__data, (page)); \ + if (ret) \ + return ret; \ if (__data < (MIN)) \ __data = (MIN); \ else if (__data > (MAX)) \ @@ -4877,8 +4883,12 @@ STORE_FUNCTION(bfq_slice_idle_store, &bfqd->bfq_slice_idle, 0, INT_MAX, 2); static ssize_t __FUNC(struct elevator_queue *e, const char *page, size_t count)\ { \ struct bfq_data *bfqd = e->elevator_data; \ - unsigned long uninitialized_var(__data); \ - bfq_var_store(&__data, (page)); \ + unsigned long __data; \ + int ret; \ + \ + ret = bfq_var_store(&__data, (page)); \ + if (ret) \ + return ret; \ if (__data < (MIN)) \ __data = (MIN); \ else if (__data > (MAX)) \ @@ -4894,9 +4904,12 @@ static ssize_t bfq_max_budget_store(struct elevator_queue *e, const char *page, size_t count) { struct bfq_data *bfqd = e->elevator_data; - unsigned long uninitialized_var(__data); + unsigned long __data; + int ret; - bfq_var_store(&__data, (page)); + ret = bfq_var_store(&__data, (page)); + if (ret) + return ret; if (__data == 0) bfqd->bfq_max_budget = bfq_calc_max_budget(bfqd); @@ -4919,9 +4932,12 @@ static ssize_t bfq_timeout_sync_store(struct elevator_queue *e, const char *page, size_t count) { struct bfq_data *bfqd = e->elevator_data; - unsigned long uninitialized_var(__data); + unsigned long __data; + int ret; - bfq_var_store(&__data, (page)); + ret = bfq_var_store(&__data, (page)); + if (ret) + return ret; if (__data < 1) __data = 1; @@ -4939,9 +4955,12 @@ static ssize_t bfq_strict_guarantees_store(struct elevator_queue *e, const char *page, size_t count) { struct bfq_data *bfqd = e->elevator_data; - unsigned long uninitialized_var(__data); + unsigned long __data; + int ret; - bfq_var_store(&__data, (page)); + ret = bfq_var_store(&__data, (page)); + if (ret) + return ret; if (__data > 1) __data = 1; @@ -4958,9 +4977,12 @@ static ssize_t bfq_low_latency_store(struct elevator_queue *e, const char *page, size_t count) { struct bfq_data *bfqd = e->elevator_data; - unsigned long uninitialized_var(__data); + unsigned long __data; + int ret; - bfq_var_store(&__data, (page)); + ret = bfq_var_store(&__data, (page)); + if (ret) + return ret; if (__data > 1) __data = 1;
Make sysfs writes fail for invalid numbers instead of storing uninitialized data copied from the stack. This patch removes all uninitialized_var() occurrences from the BFQ source code. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Paolo Valente <paolo.valente@linaro.org> --- block/bfq-iosched.c | 52 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 15 deletions(-)