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 <[email protected]>
Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
---
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) <[email protected]>
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
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 <[email protected]>
> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> 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) <[email protected]>
>
> 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
On Wed, 27 Apr 2022 18:13:43 +0300 Andy Shevchenko <[email protected]> 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?
On Wed, Apr 27, 2022 at 8:06 PM Andrew Morton <[email protected]> wrote:
>
> On Wed, 27 Apr 2022 18:13:43 +0300 Andy Shevchenko <[email protected]> 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 <[email protected]>
(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
On Wed, Apr 27, 2022 at 8:17 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Apr 27, 2022 at 8:06 PM Andrew Morton <[email protected]> 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
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 <[email protected]>
> Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
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) <[email protected]>
>
> 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
>
>
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 <[email protected]>
> > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
>
> 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"?
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 <[email protected]>
> > > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> >
> > 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.
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 <[email protected]>
> > > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> >
> > 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
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 <[email protected]>
> > > > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> > >
> > > 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
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 <[email protected]>
> > > > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> > >
> > > 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.
On Fri, Jul 29, 2022 at 5:21 PM Alexey Dobriyan <[email protected]> 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
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 <[email protected]>
> > > > > Reviewed-by: Matthew Wilcox (Oracle) <[email protected]>
> > > >
> > > > 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: [email protected] # v5.19+
Best Regards,
Petr
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: [email protected] # 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