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

On Fri 2022-07-29 17:36:50, Andy Shevchenko wrote:

On Fri, Jul 29, 2022 at 05:35:26PM +0300, Andy Shevchenko wrote:

quoted

On Mon, Jul 25, 2022 at 04:21:11PM +0100, Matthew Wilcox wrote:

quoted

On Mon, Jul 25, 2022 at 03:55:27PM +0100, Mark Rutland wrote:

quoted

On Tue, Apr 26, 2022 at 11:32:02PM +0530, Jagdish Gediya wrote:

quoted

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 <redacted>
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.

quoted

quoted

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.

quoted

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