[PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Jagdish Gediya]

At many places in kernel, It is necessary to convert sysfs input
to corrosponding bool value e.g. "false" or "0" need to be converted
to bool false, "true" or "1" need to be converted to bool true,
places where such conversion is needed currently check the input
string manually, kstrtobool() can be utilized at such places but
currently it doesn't have support to accept "false"/"true".

Add support to accept "false"/"true" as valid string in kstrtobool().

Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
Chnages in v2:
- kstrtobool to kstrtobool() in commit message.
- Split single patch into 2
- Remove strcmp usage from kstrtobool() and instead compare 1st
  character only.

Changes in v3:
- Covert -> Convert in patch 2 subject
- Collected Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

 lib/kstrtox.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 886510d248e5..465e31e4d70d 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -340,7 +340,7 @@ EXPORT_SYMBOL(kstrtos8);
  * @s: input string
  * @res: result
  *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
+ * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or
  * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
  * pointed to by res is updated upon finding a match.
  */
@@ -353,11 +353,15 @@ int kstrtobool(const char *s, bool *res)
 	switch (s[0]) {
 	case 'y':
 	case 'Y':
+	case 't':
+	case 'T':
 	case '1':
 		*res = true;
 		return 0;
 	case 'n':
 	case 'N':
+	case 'f':
+	case 'F':
 	case '0':
 		*res = false;
 		return 0;
-- 
2.35.1

[PATCH v3 2/2] mm: Convert sysfs input to bool using kstrtobool()

[Jagdish Gediya]

Sysfs input conversion to corrosponding bool value e.g. "false" or "0"
to false, "true" or "1" to true are currently handled through strncmp
at multiple places. Use kstrtobool() to convert sysfs input to bool
value.

Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
Chnages in v2:
- kstrtobool to kstrtobool() in commit message.
- Split single patch into 2
- Remove strcmp usage from kstrtobool() and instead compare 1st
  character only.

Changes in v3:
- Covert -> Convert in patch 2 subject
- Collected Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

 mm/migrate.c    | 6 +-----
 mm/swap_state.c | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 6c31ee1e1c9b..1de39bbfd6f9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -2523,11 +2523,7 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj,
 					   struct kobj_attribute *attr,
 					   const char *buf, size_t count)
 {
-	if (!strncmp(buf, "true", 4) || !strncmp(buf, "1", 1))
-		numa_demotion_enabled = true;
-	else if (!strncmp(buf, "false", 5) || !strncmp(buf, "0", 1))
-		numa_demotion_enabled = false;
-	else
+	if (kstrtobool(buf, &numa_demotion_enabled))
 		return -EINVAL;
 
 	return count;
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 013856004825..dba10045a825 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -865,11 +865,7 @@ static ssize_t vma_ra_enabled_store(struct kobject *kobj,
 				      struct kobj_attribute *attr,
 				      const char *buf, size_t count)
 {
-	if (!strncmp(buf, "true", 4) || !strncmp(buf, "1", 1))
-		enable_vma_readahead = true;
-	else if (!strncmp(buf, "false", 5) || !strncmp(buf, "0", 1))
-		enable_vma_readahead = false;
-	else
+	if (kstrtobool(buf, &enable_vma_readahead))
 		return -EINVAL;
 
 	return count;
-- 
2.35.1

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Matthew Wilcox]

On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

HEY!  You still have the buggy IFF -> IF change.  FIX IT.
My R-b was very clearly conditional on you fixing it.

> - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or

RE: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[David Laight]

From: Matthew Wilcox
> Sent: 26 April 2022 20:15
 ()
> 
> On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> HEY!  You still have the buggy IFF -> IF change.  FIX IT.
> My R-b was very clearly conditional on you fixing it.
> 
> > - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> > + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or

my 2c

Iff doesn't really go with an 'or' clause.
(With a maths degree I know what it means!)

so it probably reads better as 'if'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Jagdish Gediya]

On Tue, Apr 26, 2022 at 08:14:48PM +0100, Matthew Wilcox wrote:
> On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> HEY!  You still have the buggy IFF -> IF change.  FIX IT.
> My R-b was very clearly conditional on you fixing it.

My bad, I forgot to correct it. Andrew has corrected it.

> > - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> > + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> At many places in kernel, It is necessary to convert sysfs input
> to corrosponding bool value e.g. "false" or "0" need to be converted

corresponding

> to bool false, "true" or "1" need to be converted to bool true,
> places where such conversion is needed currently check the input
> string manually, kstrtobool() can be utilized at such places but
> currently it doesn't have support to accept "false"/"true".
> 
> Add support to accept "false"/"true" as valid string in kstrtobool().

Andrew, this patch still needs a bit of work.

> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> Chnages in v2:
> - kstrtobool to kstrtobool() in commit message.
> - Split single patch into 2
> - Remove strcmp usage from kstrtobool() and instead compare 1st
>   character only.
> 
> Changes in v3:
> - Covert -> Convert in patch 2 subject
> - Collected Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
>  lib/kstrtox.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 886510d248e5..465e31e4d70d 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -340,7 +340,7 @@ EXPORT_SYMBOL(kstrtos8);
>   * @s: input string
>   * @res: result
>   *
> - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or

s/if/iff/ please. It's _not_ a typo!

>   * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
>   * pointed to by res is updated upon finding a match.
>   */
> @@ -353,11 +353,15 @@ int kstrtobool(const char *s, bool *res)
>  	switch (s[0]) {
>  	case 'y':
>  	case 'Y':
> +	case 't':
> +	case 'T':
>  	case '1':
>  		*res = true;
>  		return 0;
>  	case 'n':
>  	case 'N':
> +	case 'f':
> +	case 'F':
>  	case '0':
>  		*res = false;
>  		return 0;
> -- 
> 2.35.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andrew Morton]

On Wed, 27 Apr 2022 18:13:43 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > At many places in kernel, It is necessary to convert sysfs input
> > to corrosponding bool value e.g. "false" or "0" need to be converted
> 
> corresponding
> 
> > to bool false, "true" or "1" need to be converted to bool true,
> > places where such conversion is needed currently check the input
> > string manually, kstrtobool() can be utilized at such places but
> > currently it doesn't have support to accept "false"/"true".
> > 
> > Add support to accept "false"/"true" as valid string in kstrtobool().
> 
> Andrew, this patch still needs a bit of work.

I fixed the two issues you identified in this email.  Is there more to do?

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Wed, Apr 27, 2022 at 8:06 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 27 Apr 2022 18:13:43 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > At many places in kernel, It is necessary to convert sysfs input
> > > to corrosponding bool value e.g. "false" or "0" need to be converted
> >
> > corresponding
> >
> > > to bool false, "true" or "1" need to be converted to bool true,
> > > places where such conversion is needed currently check the input
> > > string manually, kstrtobool() can be utilized at such places but
> > > currently it doesn't have support to accept "false"/"true".
> > >
> > > Add support to accept "false"/"true" as valid string in kstrtobool().
> >
> > Andrew, this patch still needs a bit of work.
>
> I fixed the two issues you identified in this email.  Is there more to do?

I think the rest is fine, esp. taking into account Matthew's review.
Feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

(It might be better to check the second character as well to be sure
we avoid typos like 'talse' and 'frue', anyone to comment?)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Wed, Apr 27, 2022 at 8:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Apr 27, 2022 at 8:06 PM Andrew Morton <akpm@linux-foundation.org> wrote:

...

> (It might be better to check the second character as well to be sure
> we avoid typos like 'talse' and 'frue', anyone to comment?)

Note that T and F on the keyboard are quite close to each other, so
there is non-zero probability of the above mentioned typo(s).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Mark Rutland]

On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> At many places in kernel, It is necessary to convert sysfs input
> to corrosponding bool value e.g. "false" or "0" need to be converted
> to bool false, "true" or "1" need to be converted to bool true,
> places where such conversion is needed currently check the input
> string manually, kstrtobool() can be utilized at such places but
> currently it doesn't have support to accept "false"/"true".
> 
> Add support to accept "false"/"true" as valid string in kstrtobool().
> 
> Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

I've just spotted that this broke arm64's "rodata=full" command line option,
since "full" gets parsed as 'f' = FALSE, when previously that would have been
rejected. So anyone passing "rodata=full" on the command line will have rodata
disabled, which is not what they wanted.

The current state of things is a bit messy (we prase the option twice because
arch code needs it early), and we can probably fix that with some refactoring,
but I do wonder if we actually want to open up the sysfs parsing to accept
anything *beginning* with [tTfF] rather than the full "true" and "false"
strings as previously, or whether it's worth reverting this for now in case
anything else is affected.

Mark.

> ---
> Chnages in v2:
> - kstrtobool to kstrtobool() in commit message.
> - Split single patch into 2
> - Remove strcmp usage from kstrtobool() and instead compare 1st
>   character only.
> 
> Changes in v3:
> - Covert -> Convert in patch 2 subject
> - Collected Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
>  lib/kstrtox.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 886510d248e5..465e31e4d70d 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -340,7 +340,7 @@ EXPORT_SYMBOL(kstrtos8);
>   * @s: input string
>   * @res: result
>   *
> - * This routine returns 0 iff the first character is one of 'Yy1Nn0', or
> + * This routine returns 0 if the first character is one of 'YyTt1NnFf0', or
>   * [oO][NnFf] for "on" and "off". Otherwise it will return -EINVAL.  Value
>   * pointed to by res is updated upon finding a match.
>   */
> @@ -353,11 +353,15 @@ int kstrtobool(const char *s, bool *res)
>  	switch (s[0]) {
>  	case 'y':
>  	case 'Y':
> +	case 't':
> +	case 'T':
>  	case '1':
>  		*res = true;
>  		return 0;
>  	case 'n':
>  	case 'N':
> +	case 'f':
> +	case 'F':
>  	case '0':
>  		*res = false;
>  		return 0;
> -- 
> 2.35.1
> 
> 

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Matthew Wilcox]

On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > At many places in kernel, It is necessary to convert sysfs input
> > to corrosponding bool value e.g. "false" or "0" need to be converted
> > to bool false, "true" or "1" need to be converted to bool true,
> > places where such conversion is needed currently check the input
> > string manually, kstrtobool() can be utilized at such places but
> > currently it doesn't have support to accept "false"/"true".
> > 
> > Add support to accept "false"/"true" as valid string in kstrtobool().
> > 
> > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> I've just spotted that this broke arm64's "rodata=full" command line option,

That isn't a documented option.

        rodata=         [KNL]
                on      Mark read-only kernel memory as read-only (default).
                off     Leave read-only kernel memory writable for debugging.

Hopefully this is an object lesson in why you need to update the
documentation when you extend a feature.

> since "full" gets parsed as 'f' = FALSE, when previously that would have been
> rejected. So anyone passing "rodata=full" on the command line will have rodata
> disabled, which is not what they wanted.
> 
> The current state of things is a bit messy (we prase the option twice because
> arch code needs it early), and we can probably fix that with some refactoring,
> but I do wonder if we actually want to open up the sysfs parsing to accept
> anything *beginning* with [tTfF] rather than the full "true" and "false"
> strings as previously, or whether it's worth reverting this for now in case
> anything else is affected.

Well, that's going to break people who've started using the new option.
As a quick fix, how about only allowing either "f\0" or "fa"?

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Mark Rutland]

On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > At many places in kernel, It is necessary to convert sysfs input
> > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > to bool false, "true" or "1" need to be converted to bool true,
> > > places where such conversion is needed currently check the input
> > > string manually, kstrtobool() can be utilized at such places but
> > > currently it doesn't have support to accept "false"/"true".
> > > 
> > > Add support to accept "false"/"true" as valid string in kstrtobool().
> > > 
> > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > I've just spotted that this broke arm64's "rodata=full" command line option,

> > since "full" gets parsed as 'f' = FALSE, when previously that would have been
> > rejected. So anyone passing "rodata=full" on the command line will have rodata
> > disabled, which is not what they wanted.
> > 
> > The current state of things is a bit messy (we prase the option twice because
> > arch code needs it early), and we can probably fix that with some refactoring,
> > but I do wonder if we actually want to open up the sysfs parsing to accept
> > anything *beginning* with [tTfF] rather than the full "true" and "false"
> > strings as previously, or whether it's worth reverting this for now in case
> > anything else is affected.
> 
> Well, that's going to break people who've started using the new option.

Ah; I had mistakenly thought this was new in v5.19, and so a revert was fine. I
see that it made it in for v5.18.

> As a quick fix, how about only allowing either "f\0" or "fa"?

TBH I reckon it's best to go for reworking the "rodata=" parsing, and
backporting that to stable, since people could be relying on any "f*" string by
now...

I'll have a go at that rework...

Mark.

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > At many places in kernel, It is necessary to convert sysfs input
> > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > to bool false, "true" or "1" need to be converted to bool true,
> > > places where such conversion is needed currently check the input
> > > string manually, kstrtobool() can be utilized at such places but
> > > currently it doesn't have support to accept "false"/"true".
> > > 
> > > Add support to accept "false"/"true" as valid string in kstrtobool().
> > > 
> > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > 
> > I've just spotted that this broke arm64's "rodata=full" command line option,
> 
> That isn't a documented option.
> 
>         rodata=         [KNL]
>                 on      Mark read-only kernel memory as read-only (default).
>                 off     Leave read-only kernel memory writable for debugging.
> 
> Hopefully this is an object lesson in why you need to update the
> documentation when you extend a feature.
> 
> > since "full" gets parsed as 'f' = FALSE, when previously that would have been
> > rejected. So anyone passing "rodata=full" on the command line will have rodata
> > disabled, which is not what they wanted.
> > 
> > The current state of things is a bit messy (we prase the option twice because
> > arch code needs it early), and we can probably fix that with some refactoring,
> > but I do wonder if we actually want to open up the sysfs parsing to accept
> > anything *beginning* with [tTfF] rather than the full "true" and "false"
> > strings as previously, or whether it's worth reverting this for now in case
> > anything else is affected.
> 
> Well, that's going to break people who've started using the new option.
> As a quick fix, how about only allowing either "f\0" or "fa"?

I think we need to be more strict in kstrtobool(), i.e. 'f\0' ('t\0') and 'fal'
('tru') perhaps?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 05:35:26PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> > On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> > > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > > At many places in kernel, It is necessary to convert sysfs input
> > > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > > to bool false, "true" or "1" need to be converted to bool true,
> > > > places where such conversion is needed currently check the input
> > > > string manually, kstrtobool() can be utilized at such places but
> > > > currently it doesn't have support to accept "false"/"true".
> > > > 
> > > > Add support to accept "false"/"true" as valid string in kstrtobool().
> > > > 
> > > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > 
> > > I've just spotted that this broke arm64's "rodata=full" command line option,
> > 
> > That isn't a documented option.
> > 
> >         rodata=         [KNL]
> >                 on      Mark read-only kernel memory as read-only (default).
> >                 off     Leave read-only kernel memory writable for debugging.
> > 
> > Hopefully this is an object lesson in why you need to update the
> > documentation when you extend a feature.
> > 
> > > since "full" gets parsed as 'f' = FALSE, when previously that would have been
> > > rejected. So anyone passing "rodata=full" on the command line will have rodata
> > > disabled, which is not what they wanted.
> > > 
> > > The current state of things is a bit messy (we prase the option twice because
> > > arch code needs it early), and we can probably fix that with some refactoring,
> > > but I do wonder if we actually want to open up the sysfs parsing to accept
> > > anything *beginning* with [tTfF] rather than the full "true" and "false"
> > > strings as previously, or whether it's worth reverting this for now in case
> > > anything else is affected.
> > 
> > Well, that's going to break people who've started using the new option.
> > As a quick fix, how about only allowing either "f\0" or "fa"?
> 
> I think we need to be more strict in kstrtobool(), i.e. 'f\0' ('t\0') and 'fal'
> ('tru') perhaps?

Actually kstrtobool() has been designed as a generic parser that should have
lowest priority. It means that the code that uses it should take care of any
other custom cases _before_ calling for kstrtobool().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Alexey Dobriyan]

On Fri, Jul 29, 2022 at 05:35:26PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> > On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> > > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > > At many places in kernel, It is necessary to convert sysfs input
> > > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > > to bool false, "true" or "1" need to be converted to bool true,
> > > > places where such conversion is needed currently check the input
> > > > string manually, kstrtobool() can be utilized at such places but
> > > > currently it doesn't have support to accept "false"/"true".
> > > > 
> > > > Add support to accept "false"/"true" as valid string in kstrtobool().
> > > > 
> > > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > 
> > > I've just spotted that this broke arm64's "rodata=full" command line option,
> > 
> > That isn't a documented option.
> > 
> >         rodata=         [KNL]
> >                 on      Mark read-only kernel memory as read-only (default).
> >                 off     Leave read-only kernel memory writable for debugging.
> > 
> > Hopefully this is an object lesson in why you need to update the
> > documentation when you extend a feature.
> > 
> > > since "full" gets parsed as 'f' = FALSE, when previously that would have been
> > > rejected. So anyone passing "rodata=full" on the command line will have rodata
> > > disabled, which is not what they wanted.
> > > 
> > > The current state of things is a bit messy (we prase the option twice because
> > > arch code needs it early), and we can probably fix that with some refactoring,
> > > but I do wonder if we actually want to open up the sysfs parsing to accept
> > > anything *beginning* with [tTfF] rather than the full "true" and "false"
> > > strings as previously, or whether it's worth reverting this for now in case
> > > anything else is affected.
> > 
> > Well, that's going to break people who've started using the new option.
> > As a quick fix, how about only allowing either "f\0" or "fa"?
> 
> I think we need to be more strict in kstrtobool(), i.e. 'f\0' ('t\0') and 'fal'
> ('tru') perhaps?

lol what?

the only way to be strict is to accept "0" and "1" with optional
newline and delete all the rubbish entirely.

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 5:21 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Fri, Jul 29, 2022 at 05:35:26PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> > > On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:

...

> > > Well, that's going to break people who've started using the new option.
> > > As a quick fix, how about only allowing either "f\0" or "fa"?
> >
> > I think we need to be more strict in kstrtobool(), i.e. 'f\0' ('t\0') and 'fal'
> > ('tru') perhaps?
>
> lol what?
>
> the only way to be strict is to accept "0" and "1" with optional
> newline and delete all the rubbish entirely.

You have an anti-user mindset. Be more constructive, please.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Petr Mladek]

On Fri 2022-07-29 17:36:50, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 05:35:26PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:
> > > On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:
> > > > On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:
> > > > > At many places in kernel, It is necessary to convert sysfs input
> > > > > to corrosponding bool value e.g. "false" or "0" need to be converted
> > > > > to bool false, "true" or "1" need to be converted to bool true,
> > > > > places where such conversion is needed currently check the input
> > > > > string manually, kstrtobool() can be utilized at such places but
> > > > > currently it doesn't have support to accept "false"/"true".
> > > > > 
> > > > > Add support to accept "false"/"true" as valid string in kstrtobool().
> > > > > 
> > > > > Signed-off-by: Jagdish Gediya <jvgediya@linux.ibm.com>
> > > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > > 
> > > > I've just spotted that this broke arm64's "rodata=full" command line option,

The problem still seems to be in mainline.


> > > That isn't a documented option.
> > > 
> > >         rodata=         [KNL]
> > >                 on      Mark read-only kernel memory as read-only (default).
> > >                 off     Leave read-only kernel memory writable for debugging.
> > > 
> > > Hopefully this is an object lesson in why you need to update the
> > > documentation when you extend a feature.
> > > 
> > > > since "full" gets parsed as 'f' = FALSE, when previously that would have been
> > > > rejected. So anyone passing "rodata=full" on the command line will have rodata
> > > > disabled, which is not what they wanted.
> > > > 
> > > > The current state of things is a bit messy (we prase the option twice because
> > > > arch code needs it early), and we can probably fix that with some refactoring,
> > > > but I do wonder if we actually want to open up the sysfs parsing to accept
> > > > anything *beginning* with [tTfF] rather than the full "true" and "false"
> > > > strings as previously, or whether it's worth reverting this for now in case
> > > > anything else is affected.
> > > 
> > > Well, that's going to break people who've started using the new option.
> > > As a quick fix, how about only allowing either "f\0" or "fa"?
> > 
> > I think we need to be more strict in kstrtobool(), i.e. 'f\0' ('t\0') and 'fal'
> > ('tru') perhaps?
> 
> Actually kstrtobool() has been designed as a generic parser that should have
> lowest priority. It means that the code that uses it should take care of any
> other custom cases _before_ calling for kstrtobool().

Makes sense.

Jagdish, could you please send a patch fixing the "rodata" early_param
in arch/arm64/mm/mmu.c?

Please, also add:

Cc: stable@vger.kernel.org # v5.19+

Best Regards,
Petr

Re: [PATCH v3 1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool()

[Will Deacon]

On Thu, Aug 25, 2022 at 03:39:19PM +0200, Petr Mladek wrote:
> On Fri 2022-07-29 17:36:50, Andy Shevchenko wrote:
> > Actually kstrtobool() has been designed as a generic parser that should have
> > lowest priority. It means that the code that uses it should take care of any
> > other custom cases _before_ calling for kstrtobool().
> 
> Makes sense.
> 
> Jagdish, could you please send a patch fixing the "rodata" early_param
> in arch/arm64/mm/mmu.c?
> 
> Please, also add:
> 
> Cc: stable@vger.kernel.org # v5.19+

I have a fix already queued here:

https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?h=for-next/fixes&id=2e8cff0a0eee87b27f0cf87ad8310eb41b5886ab

Sadly, it's missing the stable tag, but we can always send it manually if
the robot doesn't grab it.

Will