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

[PATCH-next] 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.

Introducing a helper which use 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>
---
 lib/parser.c | 56 +++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/lib/parser.c b/lib/parser.c
index bcb23484100e..d4f4c81ff653 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -11,6 +11,30 @@
 #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 U64_MAX_SIZE
+ */
+#define U64_MAX_SIZE 23
+
+static int match_strdup_local(const substring_t *s, char *buf)
+{
+	size_t len = s->to - s->from;
+
+	if (!s->from)
+		return -EINVAL;
+
+	if (len > U64_MAX_SIZE)
+		return -ERANGE;
+
+	memcpy(buf, s->from, len);
+	buf[len] = '\0';
+	return 0;
+}
+
 /**
  * match_one - Determines if a string matches a simple pattern
  * @s: the string to examine for presence of the pattern
@@ -129,15 +153,13 @@ EXPORT_SYMBOL(match_token);
 static int match_number(substring_t *s, int *result, int base)
 {
 	char *endp;
-	char *buf;
+	char buf[U64_MAX_SIZE + 1];
 	int ret;
 	long val;
 
-	buf = match_strdup(s);
-	if (!buf)
-		return -ENOMEM;
-
-	ret = 0;
+	ret = match_strdup_local(s, buf);
+	if (ret)
+		return ret;
 	val = simple_strtol(buf, &endp, base);
 	if (endp == buf)
 		ret = -EINVAL;
@@ -145,7 +167,6 @@ static int match_number(substring_t *s, int *result, int base)
 		ret = -ERANGE;
 	else
 		*result = (int) val;
-	kfree(buf);
 	return ret;
 }
 
@@ -163,18 +184,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[U64_MAX_SIZE + 1];
 	int ret;
 	u64 val;
 
-	buf = match_strdup(s);
-	if (!buf)
-		return -ENOMEM;
-
+	ret = match_strdup_local(s, buf);
+	if (ret)
+		return ret;
 	ret = kstrtoull(buf, base, &val);
 	if (!ret)
 		*result = val;
-	kfree(buf);
 	return ret;
 }
 
@@ -207,12 +226,13 @@ EXPORT_SYMBOL(match_int);
 int match_uint(substring_t *s, unsigned int *result)
 {
 	int err = -ENOMEM;
-	char *buf = match_strdup(s);
+	char buf[U64_MAX_SIZE + 1];
 
-	if (buf) {
-		err = kstrtouint(buf, 10, result);
-		kfree(buf);
-	}
+	err = match_strdup_local(s, buf);
+	if (err)
+		return err;
+
+	err = kstrtouint(buf, 10, result);
 	return err;
 }
 EXPORT_SYMBOL(match_uint);
-- 
2.31.1

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

[Tejun Heo]

Hello,

In general, I think this is a great idea. Some nits below:

On Fri, Dec 09, 2022 at 02:34:34PM +0800, Li Lingfeng wrote:
> +/*
> + * max size needed by diffrent bases to express U64
> + * HEX: "0xFFFFFFFFFFFFFFFF" --> 18
> + * DEC: "18446744073709551615" --> 20
> + * OCT: "01777777777777777777777" --> 23
> + * pick the max one to define U64_MAX_SIZE
> + */
> +#define U64_MAX_SIZE 23

Bikeshedding but how about naming it like NUMBER_BUF_LEN and including the
space for '\0'? Or just give it some extra space and make it 32 bytes.

> +static int match_strdup_local(const substring_t *s, char *buf)

I find it weird to name this as generic as match_strdup_local() and make it
assume that the buffer length is U64_MAX_SIZE + 1. Maybe just let the caller
pass in the buffer length as a parameter? Then, it's just strcpy and there
already is match_strlcpy() so we don't need this at all.

> +{
> +	size_t len = s->to - s->from;
> +
> +	if (!s->from)
> +		return -EINVAL;

If we use match_strlcpy() we lose the above null check but given that other
match_*() functions aren't doing it, this likely shouldn't matter.

Thanks.

-- 
tejun

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

[lilingfeng (A)]

在 2022/12/10 0:57, Tejun Heo 写道:
> Hello,
>
> In general, I think this is a great idea. Some nits below:
>
> On Fri, Dec 09, 2022 at 02:34:34PM +0800, Li Lingfeng wrote:
>> +/*
>> + * max size needed by diffrent bases to express U64
>> + * HEX: "0xFFFFFFFFFFFFFFFF" --> 18
>> + * DEC: "18446744073709551615" --> 20
>> + * OCT: "01777777777777777777777" --> 23
>> + * pick the max one to define U64_MAX_SIZE
>> + */
>> +#define U64_MAX_SIZE 23
> Bikeshedding but how about naming it like NUMBER_BUF_LEN and including the
> space for '\0'? Or just give it some extra space and make it 32 bytes.
Yes, it's my mistake, I'll send a new patch soon.
>> +static int match_strdup_local(const substring_t *s, char *buf)
> I find it weird to name this as generic as match_strdup_local() and make it
> assume that the buffer length is U64_MAX_SIZE + 1. Maybe just let the caller
> pass in the buffer length as a parameter? Then, it's just strcpy and there
> already is match_strlcpy() so we don't need this at all.

Thank you for your advice. But I think match_number() is aimed to turn the
string to num, so maybe it's better to return an error code rather than
using match_stlcpy() to truncate it to give a wrong num when the string
is too long to store.

>> +{
>> +	size_t len = s->to - s->from;
>> +
>> +	if (!s->from)
>> +		return -EINVAL;
> If we use match_strlcpy() we lose the above null check but given that other
> match_*() functions aren't doing it, this likely shouldn't matter.

Like this:
match_strdup
  kmemdup_nul
   if (!s) // null check has been done here
    return NULL
So I think null check may be necessary.

Thanks.

> Thanks.
>

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

[Tejun Heo]

Hello,

On Sat, Dec 10, 2022 at 10:51:11AM +0800, lilingfeng (A) wrote:
> Thank you for your advice. But I think match_number() is aimed to turn the
> string to num, so maybe it's better to return an error code rather than
> using match_stlcpy() to truncate it to give a wrong num when the string
> is too long to store.

Yeah, so, you check the the returned length and return an error code if the
returned value is too long for the buffer. That's how this family of
functions get error-checked.

> > > +{
> > > +	size_t len = s->to - s->from;
> > > +
> > > +	if (!s->from)
> > > +		return -EINVAL;
> > If we use match_strlcpy() we lose the above null check but given that other
> > match_*() functions aren't doing it, this likely shouldn't matter.
> 
> Like this:
> match_strdup
>  kmemdup_nul
>   if (!s) // null check has been done here
>    return NULL
> So I think null check may be necessary.

I mean, it's there now but other match functions don't, so it's unlikely
that the NULL check is necessary unless we're saying "parsing this type
string can encounter NULL inputs but these don't". That said, it doesn't
really matter. If you wanna keep the NULL check, do so before calling
strlcpy.

Thanks.

-- 
tejun