Re: [PATCH v2 2/2] reboot: fix parsing of reboot cpu number

[Matteo Croce]

On Tue, Nov 3, 2020 at 3:25 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Tue 2020-11-03 12:43:32, Matteo Croce wrote:
> > On Mon, Nov 2, 2020 at 12:01 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > >
> > > On Sun 2020-11-01 02:57:40, Matteo Croce wrote:
> > > > On Fri, Oct 30, 2020 at 3:30 PM Petr Mladek <pmladek@xxxxxxxx> wrote:
> > > > >
> > > > > On Tue 2020-10-27 14:35:45, Matteo Croce wrote:
> > > > > > From: Matteo Croce <mcroce@xxxxxxxxxxxxx>
> > > > > >
> > > > > > The kernel cmdline reboot= argument allows to specify the CPU used
> > > > > > for rebooting, with the syntax `s####` among the other flags, e.g.
> > > > > >
> > > > > >   reboot=soft,s4
> > > > > >   reboot=warm,s31,force
> > > > > >
> > > > > > In the early days the parsing was done with simple_strtoul(), later
> > > > > > deprecated in favor of the safer kstrtoint() which handles overflow.
> > > > > >
> > > > > > But kstrtoint() returns -EINVAL if there are non-digit characters
> > > > > > in a string, so if this flag is not the last given, it's silently
> > > > > > ignored as well as the subsequent ones.
> > > > > >
> > > > > > To fix it, revert the usage of simple_strtoul(), which is no longer
> > > > > > deprecated, and restore the old behaviour.
> > > > > >
> > > > > > While at it, merge two identical code blocks into one.
> > > > >
> > > > > > --- a/kernel/reboot.c
> > > > > > +++ b/kernel/reboot.c
> > > > > > @@ -552,25 +552,19 @@ static int __init reboot_setup(char *str)
> > > > > >
> > > > > >               case 's':
> > > > > >               {
> > > > > > -                     int rc;
> > > > > > -
> > > > > > -                     if (isdigit(*(str+1))) {
> > > > > > -                             rc = kstrtoint(str+1, 0, &reboot_cpu);
> > > > > > -                             if (rc)
> > > > > > -                                     return rc;
> > > > > > -                             if (reboot_cpu >= num_possible_cpus()) {
> > > > > > -                                     reboot_cpu = 0;
> > > > > > -                                     return -ERANGE;
> > > > > > -                             }
> > > > > > -                     } else if (str[1] == 'm' && str[2] == 'p' &&
> > > > > > -                                isdigit(*(str+3))) {
> > > > > > -                             rc = kstrtoint(str+3, 0, &reboot_cpu);
> > > > > > -                             if (rc)
> > > > > > -                                     return rc;
> > > > > > -                             if (reboot_cpu >= num_possible_cpus()) {
> > > > > > -                                     reboot_cpu = 0;
> > > > >
> > > > >                                                      ^^^^^^
> > > > >
> > > > > > +                     int cpu;
> > > > > > +
> > > > > > +                     /*
> > > > > > +                      * reboot_cpu is s[mp]#### with #### being the processor
> > > > > > +                      * to be used for rebooting. Skip 's' or 'smp' prefix.
> > > > > > +                      */
> > > > > > +                     str += str[1] == 'm' && str[2] == 'p' ? 3 : 1;
> > > > > > +
> > > > > > +                     if (isdigit(str[0])) {
> > > > > > +                             cpu = simple_strtoul(str, NULL, 0);
> > > > > > +                             if (cpu >= num_possible_cpus())
> > > > > >                                       return -ERANGE;
> > > > > > -                             }
> > > > > > +                             reboot_cpu = cpu;
> > > > >
> > > > > The original value stays when the new one is out of range. It is
> > > > > small functional change that should get mentioned in the commit
> > > > > message or better fixed separately.
> > >
> > > Ah, I see. From some reason, I assumed that it was defined as
> > > module_param() or core_param(). Then it would be possible to modify
> > > it later via /sys.
> > >
> > > I am sorry for the noise.
> > >
> >
> > Never mind :)
> >
> > So, is this an ack? Or I need to prepare a v3 with the revert as first patch?
>
> Good question ;-) It would be nice to do it the cleaner way but I do
> not resist on it. Feel free to use:
>
> Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
>
> Now, the question is who would actually push this upstream. These
> patches often go via Andrew Morton. He actually committed both
> changes that are fixed here.
>
> I suggest to resend the patchset with my Reviewed-by and
> Cc: stable@xxxxxxxxxxxxxxx lines. And put Andrew and Greg into Cc.
>

I see that by doing the revert first, makes the patch very small.
It's worth it.

I'm thinking, what should be the right action to do when the supplied
cpu number is too big?
With my patch I stop the parsing, while the previous behavior (other
than setting a wrong cpu number) was to continue parsing other fields.
Maybe I should just continue the loop and continue the parsing?
Maybe with a pr_warn "cpu X exceeds possible cpu number Y" etc.

-- 
per aspera ad upstream