Re: [PATCH-next v3] lib: parser: optimize match_NUMER apis to use local array

Re: [PATCH-next v3] lib: parser: optimize match_NUMER apis to use local array

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH-next v3] lib: parser: optimize match_NUMER apis to use local array

[Eric Biggers]

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

[PATCH-next v3] lib: parser: optimize match_NUMER apis to use local array

[Li Lingfeng]

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(-)

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);
 
-- 
2.31.1

Re: [PATCH-next v3] lib: parser: optimize match_NUMER apis to use local array

[Jens Axboe]

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?

-- 
Jens Axboe