| Message ID | 20230120021304.5773-1-lilingfeng3@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-next,v3] lib: parser: optimize match_NUMER apis to use local array | expand |
On Fri, Jan 20, 2023 at 10:13:04AM +0800, Li Lingfeng wrote: > Memory will be allocated to store substring_t in match_strdup(), which means > the caller of match_strdup() may need to be scheduled out to wait for reclaiming > memory. > > Using local array to store substring_t to remove the restriction. > > Link: https://lore.kernel.org/all/20221104023938.2346986-5-yukuai1@huaweicloud.com/ > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> Acked-by: Tejun Heo <tj@kernel.org> This fixes a sleep-while-atomic splat in blk-iocost, so it'd be a good idea to add: Fixes: 2c0647988433 ("blk-iocost: don't release 'ioc->lock' while updating params"). The mm tree likely is the best fit but given the splat the block tree can work too. Andrew, Jens, what do you think? Thanks.
On Fri, Jan 20, 2023 at 10:13:04AM +0800, Li Lingfeng wrote: > [PATCH-next v3] lib: parser: optimize match_NUMER apis to use NUMER => NUMBER > Memory will be allocated to store substring_t in match_strdup(), which means > the caller of match_strdup() may need to be scheduled out to wait for reclaiming > memory. Text in commit messages should be wrapped at 72 columns. > @@ -163,18 +169,16 @@ static int match_number(substring_t *s, int *result, int base) > */ > static int match_u64int(substring_t *s, u64 *result, int base) > { > - char *buf; > + char buf[NUMBER_BUF_LEN]; > int ret; > u64 val; > > - buf = match_strdup(s); > - if (!buf) > - return -ENOMEM; > - > + if ((s->to - s->from) >= NUMBER_BUF_LEN) > + return -ERANGE; > + match_strlcpy(buf, s, NUMBER_BUF_LEN); As I requested on v2, the return value of match_strlcpy() should be used instead of checking '((s->to - s->from) >= NUMBER_BUF_LEN'. - Eric
On 1/19/23 7:00?PM, Tejun Heo wrote: > On Fri, Jan 20, 2023 at 10:13:04AM +0800, Li Lingfeng wrote: >> Memory will be allocated to store substring_t in match_strdup(), which means >> the caller of match_strdup() may need to be scheduled out to wait for reclaiming >> memory. >> >> Using local array to store substring_t to remove the restriction. >> >> Link: https://lore.kernel.org/all/20221104023938.2346986-5-yukuai1@huaweicloud.com/ >> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> > > Acked-by: Tejun Heo <tj@kernel.org> > > This fixes a sleep-while-atomic splat in blk-iocost, so it'd be a good idea to add: > > Fixes: 2c0647988433 ("blk-iocost: don't release 'ioc->lock' while updating params"). > > The mm tree likely is the best fit but given the splat the block tree can > work too. Andrew, Jens, what do you think? I can take it through the block tree once folks are happy with it, as the buggy patch came through there. Doesn't really matter to me, however. Why is it marked for-next though, seems like this is a regression in this series?
diff --git a/lib/parser.c b/lib/parser.c index bcb23484100e..e3525a2522ea 100644 --- a/lib/parser.c +++ b/lib/parser.c @@ -11,6 +11,15 @@ #include <linux/slab.h> #include <linux/string.h> +/* + * max size needed by diffrent bases to express U64 + * HEX: "0xFFFFFFFFFFFFFFFF" --> 18 + * DEC: "18446744073709551615" --> 20 + * OCT: "01777777777777777777777" --> 23 + * pick the max one to define NUMBER_BUF_LEN + */ +#define NUMBER_BUF_LEN 24 + /** * match_one - Determines if a string matches a simple pattern * @s: the string to examine for presence of the pattern @@ -129,14 +138,12 @@ EXPORT_SYMBOL(match_token); static int match_number(substring_t *s, int *result, int base) { char *endp; - char *buf; + char buf[NUMBER_BUF_LEN]; int ret; long val; - buf = match_strdup(s); - if (!buf) - return -ENOMEM; - + if (match_strlcpy(buf, s, NUMBER_BUF_LEN) >= NUMBER_BUF_LEN) + return -ERANGE; ret = 0; val = simple_strtol(buf, &endp, base); if (endp == buf) @@ -145,7 +152,6 @@ static int match_number(substring_t *s, int *result, int base) ret = -ERANGE; else *result = (int) val; - kfree(buf); return ret; } @@ -163,18 +169,16 @@ static int match_number(substring_t *s, int *result, int base) */ static int match_u64int(substring_t *s, u64 *result, int base) { - char *buf; + char buf[NUMBER_BUF_LEN]; int ret; u64 val; - buf = match_strdup(s); - if (!buf) - return -ENOMEM; - + if ((s->to - s->from) >= NUMBER_BUF_LEN) + return -ERANGE; + match_strlcpy(buf, s, NUMBER_BUF_LEN); ret = kstrtoull(buf, base, &val); if (!ret) *result = val; - kfree(buf); return ret; } @@ -206,14 +210,13 @@ EXPORT_SYMBOL(match_int); */ int match_uint(substring_t *s, unsigned int *result) { - int err = -ENOMEM; - char *buf = match_strdup(s); + char buf[NUMBER_BUF_LEN]; - if (buf) { - err = kstrtouint(buf, 10, result); - kfree(buf); - } - return err; + if ((s->to - s->from) >= NUMBER_BUF_LEN) + return -ERANGE; + match_strlcpy(buf, s, NUMBER_BUF_LEN); + + return kstrtouint(buf, 10, result); } EXPORT_SYMBOL(match_uint);
Memory will be allocated to store substring_t in match_strdup(), which means the caller of match_strdup() may need to be scheduled out to wait for reclaiming memory. Using local array to store substring_t to remove the restriction. Link: https://lore.kernel.org/all/20221104023938.2346986-5-yukuai1@huaweicloud.com/ Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> --- v1->v2: change the name of buffer's length use match_strlcpy() to copy string and keep string length check v2->v3: judge whether the length to be copied exceeds the limit by the return value of match_strlcpy() lib/parser.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-)