| Message ID | 20220426180203.70782-1-jvgediya@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] lib/kstrtox.c: Add "false"/"true" support to kstrtobool() | expand |
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
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)
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
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 >
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?
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?)
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).
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 > >
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"?
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.
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?
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().
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.
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.
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
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
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;