diff mbox

[v2,4/4] param: convert some "on"/"off" users to strtobool

Message ID 1454619643-14444-5-git-send-email-keescook@chromium.org (mailing list archive)
State Not Applicable
Headers show

Commit Message

Kees Cook Feb. 4, 2016, 9 p.m. UTC
This changes several users of manual "on"/"off" parsing to use strtobool.
(Which means they will now parse y/n/1/0 meaningfully too.)

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: x86@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
---
 arch/powerpc/kernel/rtasd.c                  |  9 ++-------
 arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
 arch/s390/kernel/time.c                      |  8 ++------
 arch/s390/kernel/topology.c                  |  7 ++-----
 arch/x86/kernel/aperture_64.c                | 12 ++----------
 include/linux/tick.h                         |  2 +-
 kernel/time/hrtimer.c                        | 10 ++--------
 kernel/time/tick-sched.c                     | 10 ++--------
 8 files changed, 15 insertions(+), 53 deletions(-)

Comments

Andy Shevchenko Feb. 4, 2016, 11:04 p.m. UTC | #1
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
> This changes several users of manual "on"/"off" parsing to use strtobool.
> (Which means they will now parse y/n/1/0 meaningfully too.)
>

I like this change, but can you carefully check the acceptance of the
returned value?
Briefly I saw 1 or 0 as okay in different places.


> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: x86@kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> ---
>  arch/powerpc/kernel/rtasd.c                  |  9 ++-------
>  arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
>  arch/s390/kernel/time.c                      |  8 ++------
>  arch/s390/kernel/topology.c                  |  7 ++-----
>  arch/x86/kernel/aperture_64.c                | 12 ++----------
>  include/linux/tick.h                         |  2 +-
>  kernel/time/hrtimer.c                        | 10 ++--------
>  kernel/time/tick-sched.c                     | 10 ++--------
>  8 files changed, 15 insertions(+), 53 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
> index 5a2c049c1c61..567ed5a2f43a 100644
> --- a/arch/powerpc/kernel/rtasd.c
> +++ b/arch/powerpc/kernel/rtasd.c
> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
>  static unsigned int event_scan;
>  static unsigned int rtas_event_scan_rate;
>
> -static int full_rtas_msgs = 0;
> +static bool full_rtas_msgs;
>
>  /* Stop logging to nvram after first fatal error */
>  static int logging_enabled; /* Until we initialize everything,
> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>
>  static int __init rtasmsgs_setup(char *str)
>  {
> -       if (strcmp(str, "on") == 0)
> -               full_rtas_msgs = 1;
> -       else if (strcmp(str, "off") == 0)
> -               full_rtas_msgs = 0;
> -
> -       return 1;
> +       return kstrtobool(str, 0, &full_rtas_msgs);
>  }
>  __setup("rtasmsgs=", rtasmsgs_setup);
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 32274f72fe3f..b9787cae4108 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>
>  static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>
> -static int cede_offline_enabled __read_mostly = 1;
> +static bool cede_offline_enabled __read_mostly = true;
>
>  /*
>   * Enable/disable cede_offline when available.
>   */
>  static int __init setup_cede_offline(char *str)
>  {
> -       if (!strcmp(str, "off"))
> -               cede_offline_enabled = 0;
> -       else if (!strcmp(str, "on"))
> -               cede_offline_enabled = 1;
> -       else
> -               return 0;
> -       return 1;
> +       return kstrtobool(str, 0, &cede_offline_enabled);
>  }
>
>  __setup("cede_offline=", setup_cede_offline);
> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 99f84ac31307..dff6ce1b84b2 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
>  /*
>   * Server Time Protocol (STP) code.
>   */
> -static int stp_online;
> +static bool stp_online;
>  static struct stp_sstpi stp_info;
>  static void *stp_page;
>
> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>
>  static int __init early_parse_stp(char *p)
>  {
> -       if (strncmp(p, "off", 3) == 0)
> -               stp_online = 0;
> -       else if (strncmp(p, "on", 2) == 0)
> -               stp_online = 1;
> -       return 0;
> +       return kstrtobool(p, 0, &stp_online);
>  }
>  early_param("stp", early_parse_stp);
>
> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
> index 40b8102fdadb..5d8a80651f61 100644
> --- a/arch/s390/kernel/topology.c
> +++ b/arch/s390/kernel/topology.c
> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
>  static void topology_work_fn(struct work_struct *work);
>  static struct sysinfo_15_1_x *tl_info;
>
> -static int topology_enabled = 1;
> +static bool topology_enabled = true;
>  static DECLARE_WORK(topology_work, topology_work_fn);
>
>  /*
> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>
>  static int __init early_parse_topology(char *p)
>  {
> -       if (strncmp(p, "off", 3))
> -               return 0;
> -       topology_enabled = 0;
> -       return 0;
> +       return kstrtobool(p, 0, &topology_enabled);
>  }
>  early_param("topology", early_parse_topology);
>
> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
> index 6e85f713641d..6b423754083a 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
>         return 0;
>  }
>
> -static int gart_fix_e820 __initdata = 1;
> +static bool gart_fix_e820 __initdata = true;
>
>  static int __init parse_gart_mem(char *p)
>  {
> -       if (!p)
> -               return -EINVAL;
> -
> -       if (!strncmp(p, "off", 3))
> -               gart_fix_e820 = 0;
> -       else if (!strncmp(p, "on", 2))
> -               gart_fix_e820 = 1;
> -
> -       return 0;
> +       return kstrtobool(p, 0, &gart_fix_e820);
>  }
>  early_param("gart_fix_e820", parse_gart_mem);
>
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index 97fd4e543846..0ecdf0e248f4 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
>  }
>
>  #ifdef CONFIG_NO_HZ_COMMON
> -extern int tick_nohz_enabled;
> +extern bool tick_nohz_enabled;
>  extern int tick_nohz_tick_stopped(void);
>  extern void tick_nohz_idle_enter(void);
>  extern void tick_nohz_idle_exit(void);
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 435b8850dd80..456f066148d5 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>  /*
>   * High resolution timer enabled ?
>   */
> -static int hrtimer_hres_enabled __read_mostly  = 1;
> +static bool hrtimer_hres_enabled __read_mostly  = true;
>  unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
>  EXPORT_SYMBOL_GPL(hrtimer_resolution);
>
> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
>   */
>  static int __init setup_hrtimer_hres(char *str)
>  {
> -       if (!strcmp(str, "off"))
> -               hrtimer_hres_enabled = 0;
> -       else if (!strcmp(str, "on"))
> -               hrtimer_hres_enabled = 1;
> -       else
> -               return 0;
> -       return 1;
> +       return kstrtobool(str, 0, &hrtimer_hres_enabled);
>  }
>
>  __setup("highres=", setup_hrtimer_hres);
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 9d7a053545f5..b57f822c2069 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
>  /*
>   * NO HZ enabled ?
>   */
> -int tick_nohz_enabled __read_mostly = 1;
> +bool tick_nohz_enabled __read_mostly  = true;
>  unsigned long tick_nohz_active  __read_mostly;
>  /*
>   * Enable / Disable tickless mode
>   */
>  static int __init setup_tick_nohz(char *str)
>  {
> -       if (!strcmp(str, "off"))
> -               tick_nohz_enabled = 0;
> -       else if (!strcmp(str, "on"))
> -               tick_nohz_enabled = 1;
> -       else
> -               return 0;
> -       return 1;
> +       return kstrtobool(str, 0, &tick_nohz_enabled);
>  }
>
>  __setup("nohz=", setup_tick_nohz);
> --
> 2.6.3
>
Kees Cook Feb. 5, 2016, 12:11 a.m. UTC | #2
On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>> This changes several users of manual "on"/"off" parsing to use strtobool.
>> (Which means they will now parse y/n/1/0 meaningfully too.)
>>
>
> I like this change, but can you carefully check the acceptance of the
> returned value?
> Briefly I saw 1 or 0 as okay in different places.

Maybe I missed something, but I think this is actually a bug fix. The
two cases are early_param and __setup:

For early_param, the functions are called when walking the command
line in do_early_param via parse_args in parse_early_options. Any
non-zero return values produce a warning (in do_early_param not
parse_args). So this is a bug fix, since the function I touched would
(almost) always return 0, even with bad values (i.e. fixes unreported
bad arguments):

early_param early_parse_stp always 0
early_param early_parse_topology always 0
early_param parse_gart_mem always 0 unless !p (then -EINVAL)

For __setup, these are handled by obsolete_checksetup via
unknown_bootoption via parse_args in start_kernel, as a way to merge
__setup calls that should really be in param (i.e. non-early __setup).
Return values are bubbled up into parse_args and hit:

                default:
                        pr_err("%s: `%s' invalid for parameter `%s'\n",
                               doing, val ?: "", param);
                        break;

So this is also a bug fix, since these __setup functions returned inverted
values or always failed:

__setup rtasmsgs_setup always 1
__setup setup_cede_offline 1 on success, otherwise 0
__setup setup_hrtimer_hres 1 on success, otherwise 0
__setup setup_tick_nohz 1 on success, otherwise 0

So if you specified any of these, they would trigger a bogus "invalid
parameter" report.

I will double-check...

-Kees

>
>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: x86@kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-s390@vger.kernel.org
>> ---
>>  arch/powerpc/kernel/rtasd.c                  |  9 ++-------
>>  arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
>>  arch/s390/kernel/time.c                      |  8 ++------
>>  arch/s390/kernel/topology.c                  |  7 ++-----
>>  arch/x86/kernel/aperture_64.c                | 12 ++----------
>>  include/linux/tick.h                         |  2 +-
>>  kernel/time/hrtimer.c                        | 10 ++--------
>>  kernel/time/tick-sched.c                     | 10 ++--------
>>  8 files changed, 15 insertions(+), 53 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
>> index 5a2c049c1c61..567ed5a2f43a 100644
>> --- a/arch/powerpc/kernel/rtasd.c
>> +++ b/arch/powerpc/kernel/rtasd.c
>> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
>>  static unsigned int event_scan;
>>  static unsigned int rtas_event_scan_rate;
>>
>> -static int full_rtas_msgs = 0;
>> +static bool full_rtas_msgs;
>>
>>  /* Stop logging to nvram after first fatal error */
>>  static int logging_enabled; /* Until we initialize everything,
>> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>>
>>  static int __init rtasmsgs_setup(char *str)
>>  {
>> -       if (strcmp(str, "on") == 0)
>> -               full_rtas_msgs = 1;
>> -       else if (strcmp(str, "off") == 0)
>> -               full_rtas_msgs = 0;
>> -
>> -       return 1;
>> +       return kstrtobool(str, 0, &full_rtas_msgs);
>>  }
>>  __setup("rtasmsgs=", rtasmsgs_setup);
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 32274f72fe3f..b9787cae4108 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>>
>>  static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>>
>> -static int cede_offline_enabled __read_mostly = 1;
>> +static bool cede_offline_enabled __read_mostly = true;
>>
>>  /*
>>   * Enable/disable cede_offline when available.
>>   */
>>  static int __init setup_cede_offline(char *str)
>>  {
>> -       if (!strcmp(str, "off"))
>> -               cede_offline_enabled = 0;
>> -       else if (!strcmp(str, "on"))
>> -               cede_offline_enabled = 1;
>> -       else
>> -               return 0;
>> -       return 1;
>> +       return kstrtobool(str, 0, &cede_offline_enabled);
>>  }
>>
>>  __setup("cede_offline=", setup_cede_offline);
>> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
>> index 99f84ac31307..dff6ce1b84b2 100644
>> --- a/arch/s390/kernel/time.c
>> +++ b/arch/s390/kernel/time.c
>> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
>>  /*
>>   * Server Time Protocol (STP) code.
>>   */
>> -static int stp_online;
>> +static bool stp_online;
>>  static struct stp_sstpi stp_info;
>>  static void *stp_page;
>>
>> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>>
>>  static int __init early_parse_stp(char *p)
>>  {
>> -       if (strncmp(p, "off", 3) == 0)
>> -               stp_online = 0;
>> -       else if (strncmp(p, "on", 2) == 0)
>> -               stp_online = 1;
>> -       return 0;
>> +       return kstrtobool(p, 0, &stp_online);
>>  }
>>  early_param("stp", early_parse_stp);
>>
>> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
>> index 40b8102fdadb..5d8a80651f61 100644
>> --- a/arch/s390/kernel/topology.c
>> +++ b/arch/s390/kernel/topology.c
>> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
>>  static void topology_work_fn(struct work_struct *work);
>>  static struct sysinfo_15_1_x *tl_info;
>>
>> -static int topology_enabled = 1;
>> +static bool topology_enabled = true;
>>  static DECLARE_WORK(topology_work, topology_work_fn);
>>
>>  /*
>> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>>
>>  static int __init early_parse_topology(char *p)
>>  {
>> -       if (strncmp(p, "off", 3))
>> -               return 0;
>> -       topology_enabled = 0;
>> -       return 0;
>> +       return kstrtobool(p, 0, &topology_enabled);
>>  }
>>  early_param("topology", early_parse_topology);
>>
>> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
>> index 6e85f713641d..6b423754083a 100644
>> --- a/arch/x86/kernel/aperture_64.c
>> +++ b/arch/x86/kernel/aperture_64.c
>> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
>>         return 0;
>>  }
>>
>> -static int gart_fix_e820 __initdata = 1;
>> +static bool gart_fix_e820 __initdata = true;
>>
>>  static int __init parse_gart_mem(char *p)
>>  {
>> -       if (!p)
>> -               return -EINVAL;
>> -
>> -       if (!strncmp(p, "off", 3))
>> -               gart_fix_e820 = 0;
>> -       else if (!strncmp(p, "on", 2))
>> -               gart_fix_e820 = 1;
>> -
>> -       return 0;
>> +       return kstrtobool(p, 0, &gart_fix_e820);
>>  }
>>  early_param("gart_fix_e820", parse_gart_mem);
>>
>> diff --git a/include/linux/tick.h b/include/linux/tick.h
>> index 97fd4e543846..0ecdf0e248f4 100644
>> --- a/include/linux/tick.h
>> +++ b/include/linux/tick.h
>> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
>>  }
>>
>>  #ifdef CONFIG_NO_HZ_COMMON
>> -extern int tick_nohz_enabled;
>> +extern bool tick_nohz_enabled;
>>  extern int tick_nohz_tick_stopped(void);
>>  extern void tick_nohz_idle_enter(void);
>>  extern void tick_nohz_idle_exit(void);
>> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
>> index 435b8850dd80..456f066148d5 100644
>> --- a/kernel/time/hrtimer.c
>> +++ b/kernel/time/hrtimer.c
>> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>>  /*
>>   * High resolution timer enabled ?
>>   */
>> -static int hrtimer_hres_enabled __read_mostly  = 1;
>> +static bool hrtimer_hres_enabled __read_mostly  = true;
>>  unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
>>  EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>
>> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>   */
>>  static int __init setup_hrtimer_hres(char *str)
>>  {
>> -       if (!strcmp(str, "off"))
>> -               hrtimer_hres_enabled = 0;
>> -       else if (!strcmp(str, "on"))
>> -               hrtimer_hres_enabled = 1;
>> -       else
>> -               return 0;
>> -       return 1;
>> +       return kstrtobool(str, 0, &hrtimer_hres_enabled);
>>  }
>>
>>  __setup("highres=", setup_hrtimer_hres);
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index 9d7a053545f5..b57f822c2069 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
>>  /*
>>   * NO HZ enabled ?
>>   */
>> -int tick_nohz_enabled __read_mostly = 1;
>> +bool tick_nohz_enabled __read_mostly  = true;
>>  unsigned long tick_nohz_active  __read_mostly;
>>  /*
>>   * Enable / Disable tickless mode
>>   */
>>  static int __init setup_tick_nohz(char *str)
>>  {
>> -       if (!strcmp(str, "off"))
>> -               tick_nohz_enabled = 0;
>> -       else if (!strcmp(str, "on"))
>> -               tick_nohz_enabled = 1;
>> -       else
>> -               return 0;
>> -       return 1;
>> +       return kstrtobool(str, 0, &tick_nohz_enabled);
>>  }
>>
>>  __setup("nohz=", setup_tick_nohz);
>> --
>> 2.6.3
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
Kees Cook Feb. 5, 2016, 2:12 a.m. UTC | #3
On Thu, Feb 4, 2016 at 4:11 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>>> This changes several users of manual "on"/"off" parsing to use strtobool.
>>> (Which means they will now parse y/n/1/0 meaningfully too.)
>>>
>>
>> I like this change, but can you carefully check the acceptance of the
>> returned value?
>> Briefly I saw 1 or 0 as okay in different places.
>
> Maybe I missed something, but I think this is actually a bug fix. The
> two cases are early_param and __setup:
>
> For early_param, the functions are called when walking the command
> line in do_early_param via parse_args in parse_early_options. Any
> non-zero return values produce a warning (in do_early_param not
> parse_args). So this is a bug fix, since the function I touched would
> (almost) always return 0, even with bad values (i.e. fixes unreported
> bad arguments):
>
> early_param early_parse_stp always 0
> early_param early_parse_topology always 0
> early_param parse_gart_mem always 0 unless !p (then -EINVAL)
>
> For __setup, these are handled by obsolete_checksetup via
> unknown_bootoption via parse_args in start_kernel, as a way to merge
> __setup calls that should really be in param (i.e. non-early __setup).
> Return values are bubbled up into parse_args and hit:
>
>                 default:
>                         pr_err("%s: `%s' invalid for parameter `%s'\n",
>                                doing, val ?: "", param);
>                         break;
>
> So this is also a bug fix, since these __setup functions returned inverted
> values or always failed:
>
> __setup rtasmsgs_setup always 1
> __setup setup_cede_offline 1 on success, otherwise 0
> __setup setup_hrtimer_hres 1 on success, otherwise 0
> __setup setup_tick_nohz 1 on success, otherwise 0
>
> So if you specified any of these, they would trigger a bogus "invalid
> parameter" report.
>
> I will double-check...

I am wrong! __setup functions (as handled by unknown_bootoption) need
to return 1, or they end up in the init environment. I will send a
fix...

-Kees

>
> -Kees
>
>>
>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: x86@kernel.org
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Cc: linux-s390@vger.kernel.org
>>> ---
>>>  arch/powerpc/kernel/rtasd.c                  |  9 ++-------
>>>  arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
>>>  arch/s390/kernel/time.c                      |  8 ++------
>>>  arch/s390/kernel/topology.c                  |  7 ++-----
>>>  arch/x86/kernel/aperture_64.c                | 12 ++----------
>>>  include/linux/tick.h                         |  2 +-
>>>  kernel/time/hrtimer.c                        | 10 ++--------
>>>  kernel/time/tick-sched.c                     | 10 ++--------
>>>  8 files changed, 15 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
>>> index 5a2c049c1c61..567ed5a2f43a 100644
>>> --- a/arch/powerpc/kernel/rtasd.c
>>> +++ b/arch/powerpc/kernel/rtasd.c
>>> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
>>>  static unsigned int event_scan;
>>>  static unsigned int rtas_event_scan_rate;
>>>
>>> -static int full_rtas_msgs = 0;
>>> +static bool full_rtas_msgs;
>>>
>>>  /* Stop logging to nvram after first fatal error */
>>>  static int logging_enabled; /* Until we initialize everything,
>>> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>>>
>>>  static int __init rtasmsgs_setup(char *str)
>>>  {
>>> -       if (strcmp(str, "on") == 0)
>>> -               full_rtas_msgs = 1;
>>> -       else if (strcmp(str, "off") == 0)
>>> -               full_rtas_msgs = 0;
>>> -
>>> -       return 1;
>>> +       return kstrtobool(str, 0, &full_rtas_msgs);
>>>  }
>>>  __setup("rtasmsgs=", rtasmsgs_setup);
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> index 32274f72fe3f..b9787cae4108 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>>>
>>>  static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>>>
>>> -static int cede_offline_enabled __read_mostly = 1;
>>> +static bool cede_offline_enabled __read_mostly = true;
>>>
>>>  /*
>>>   * Enable/disable cede_offline when available.
>>>   */
>>>  static int __init setup_cede_offline(char *str)
>>>  {
>>> -       if (!strcmp(str, "off"))
>>> -               cede_offline_enabled = 0;
>>> -       else if (!strcmp(str, "on"))
>>> -               cede_offline_enabled = 1;
>>> -       else
>>> -               return 0;
>>> -       return 1;
>>> +       return kstrtobool(str, 0, &cede_offline_enabled);
>>>  }
>>>
>>>  __setup("cede_offline=", setup_cede_offline);
>>> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
>>> index 99f84ac31307..dff6ce1b84b2 100644
>>> --- a/arch/s390/kernel/time.c
>>> +++ b/arch/s390/kernel/time.c
>>> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
>>>  /*
>>>   * Server Time Protocol (STP) code.
>>>   */
>>> -static int stp_online;
>>> +static bool stp_online;
>>>  static struct stp_sstpi stp_info;
>>>  static void *stp_page;
>>>
>>> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>>>
>>>  static int __init early_parse_stp(char *p)
>>>  {
>>> -       if (strncmp(p, "off", 3) == 0)
>>> -               stp_online = 0;
>>> -       else if (strncmp(p, "on", 2) == 0)
>>> -               stp_online = 1;
>>> -       return 0;
>>> +       return kstrtobool(p, 0, &stp_online);
>>>  }
>>>  early_param("stp", early_parse_stp);
>>>
>>> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
>>> index 40b8102fdadb..5d8a80651f61 100644
>>> --- a/arch/s390/kernel/topology.c
>>> +++ b/arch/s390/kernel/topology.c
>>> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
>>>  static void topology_work_fn(struct work_struct *work);
>>>  static struct sysinfo_15_1_x *tl_info;
>>>
>>> -static int topology_enabled = 1;
>>> +static bool topology_enabled = true;
>>>  static DECLARE_WORK(topology_work, topology_work_fn);
>>>
>>>  /*
>>> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>>>
>>>  static int __init early_parse_topology(char *p)
>>>  {
>>> -       if (strncmp(p, "off", 3))
>>> -               return 0;
>>> -       topology_enabled = 0;
>>> -       return 0;
>>> +       return kstrtobool(p, 0, &topology_enabled);
>>>  }
>>>  early_param("topology", early_parse_topology);
>>>
>>> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
>>> index 6e85f713641d..6b423754083a 100644
>>> --- a/arch/x86/kernel/aperture_64.c
>>> +++ b/arch/x86/kernel/aperture_64.c
>>> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
>>>         return 0;
>>>  }
>>>
>>> -static int gart_fix_e820 __initdata = 1;
>>> +static bool gart_fix_e820 __initdata = true;
>>>
>>>  static int __init parse_gart_mem(char *p)
>>>  {
>>> -       if (!p)
>>> -               return -EINVAL;
>>> -
>>> -       if (!strncmp(p, "off", 3))
>>> -               gart_fix_e820 = 0;
>>> -       else if (!strncmp(p, "on", 2))
>>> -               gart_fix_e820 = 1;
>>> -
>>> -       return 0;
>>> +       return kstrtobool(p, 0, &gart_fix_e820);
>>>  }
>>>  early_param("gart_fix_e820", parse_gart_mem);
>>>
>>> diff --git a/include/linux/tick.h b/include/linux/tick.h
>>> index 97fd4e543846..0ecdf0e248f4 100644
>>> --- a/include/linux/tick.h
>>> +++ b/include/linux/tick.h
>>> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
>>>  }
>>>
>>>  #ifdef CONFIG_NO_HZ_COMMON
>>> -extern int tick_nohz_enabled;
>>> +extern bool tick_nohz_enabled;
>>>  extern int tick_nohz_tick_stopped(void);
>>>  extern void tick_nohz_idle_enter(void);
>>>  extern void tick_nohz_idle_exit(void);
>>> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
>>> index 435b8850dd80..456f066148d5 100644
>>> --- a/kernel/time/hrtimer.c
>>> +++ b/kernel/time/hrtimer.c
>>> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>>>  /*
>>>   * High resolution timer enabled ?
>>>   */
>>> -static int hrtimer_hres_enabled __read_mostly  = 1;
>>> +static bool hrtimer_hres_enabled __read_mostly  = true;
>>>  unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
>>>  EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>>
>>> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>>   */
>>>  static int __init setup_hrtimer_hres(char *str)
>>>  {
>>> -       if (!strcmp(str, "off"))
>>> -               hrtimer_hres_enabled = 0;
>>> -       else if (!strcmp(str, "on"))
>>> -               hrtimer_hres_enabled = 1;
>>> -       else
>>> -               return 0;
>>> -       return 1;
>>> +       return kstrtobool(str, 0, &hrtimer_hres_enabled);
>>>  }
>>>
>>>  __setup("highres=", setup_hrtimer_hres);
>>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>>> index 9d7a053545f5..b57f822c2069 100644
>>> --- a/kernel/time/tick-sched.c
>>> +++ b/kernel/time/tick-sched.c
>>> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
>>>  /*
>>>   * NO HZ enabled ?
>>>   */
>>> -int tick_nohz_enabled __read_mostly = 1;
>>> +bool tick_nohz_enabled __read_mostly  = true;
>>>  unsigned long tick_nohz_active  __read_mostly;
>>>  /*
>>>   * Enable / Disable tickless mode
>>>   */
>>>  static int __init setup_tick_nohz(char *str)
>>>  {
>>> -       if (!strcmp(str, "off"))
>>> -               tick_nohz_enabled = 0;
>>> -       else if (!strcmp(str, "on"))
>>> -               tick_nohz_enabled = 1;
>>> -       else
>>> -               return 0;
>>> -       return 1;
>>> +       return kstrtobool(str, 0, &tick_nohz_enabled);
>>>  }
>>>
>>>  __setup("nohz=", setup_tick_nohz);
>>> --
>>> 2.6.3
>>>
>>
>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security
diff mbox

Patch

diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index 5a2c049c1c61..567ed5a2f43a 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -49,7 +49,7 @@  static unsigned int rtas_error_log_buffer_max;
 static unsigned int event_scan;
 static unsigned int rtas_event_scan_rate;
 
-static int full_rtas_msgs = 0;
+static bool full_rtas_msgs;
 
 /* Stop logging to nvram after first fatal error */
 static int logging_enabled; /* Until we initialize everything,
@@ -592,11 +592,6 @@  __setup("surveillance=", surveillance_setup);
 
 static int __init rtasmsgs_setup(char *str)
 {
-	if (strcmp(str, "on") == 0)
-		full_rtas_msgs = 1;
-	else if (strcmp(str, "off") == 0)
-		full_rtas_msgs = 0;
-
-	return 1;
+	return kstrtobool(str, 0, &full_rtas_msgs);
 }
 __setup("rtasmsgs=", rtasmsgs_setup);
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 32274f72fe3f..b9787cae4108 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -47,20 +47,14 @@  static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
 
 static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
 
-static int cede_offline_enabled __read_mostly = 1;
+static bool cede_offline_enabled __read_mostly = true;
 
 /*
  * Enable/disable cede_offline when available.
  */
 static int __init setup_cede_offline(char *str)
 {
-	if (!strcmp(str, "off"))
-		cede_offline_enabled = 0;
-	else if (!strcmp(str, "on"))
-		cede_offline_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return kstrtobool(str, 0, &cede_offline_enabled);
 }
 
 __setup("cede_offline=", setup_cede_offline);
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 99f84ac31307..dff6ce1b84b2 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -1433,7 +1433,7 @@  device_initcall(etr_init_sysfs);
 /*
  * Server Time Protocol (STP) code.
  */
-static int stp_online;
+static bool stp_online;
 static struct stp_sstpi stp_info;
 static void *stp_page;
 
@@ -1444,11 +1444,7 @@  static struct timer_list stp_timer;
 
 static int __init early_parse_stp(char *p)
 {
-	if (strncmp(p, "off", 3) == 0)
-		stp_online = 0;
-	else if (strncmp(p, "on", 2) == 0)
-		stp_online = 1;
-	return 0;
+	return kstrtobool(p, 0, &stp_online);
 }
 early_param("stp", early_parse_stp);
 
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 40b8102fdadb..5d8a80651f61 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -37,7 +37,7 @@  static void set_topology_timer(void);
 static void topology_work_fn(struct work_struct *work);
 static struct sysinfo_15_1_x *tl_info;
 
-static int topology_enabled = 1;
+static bool topology_enabled = true;
 static DECLARE_WORK(topology_work, topology_work_fn);
 
 /*
@@ -444,10 +444,7 @@  static const struct cpumask *cpu_book_mask(int cpu)
 
 static int __init early_parse_topology(char *p)
 {
-	if (strncmp(p, "off", 3))
-		return 0;
-	topology_enabled = 0;
-	return 0;
+	return kstrtobool(p, 0, &topology_enabled);
 }
 early_param("topology", early_parse_topology);
 
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 6e85f713641d..6b423754083a 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -227,19 +227,11 @@  static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
 	return 0;
 }
 
-static int gart_fix_e820 __initdata = 1;
+static bool gart_fix_e820 __initdata = true;
 
 static int __init parse_gart_mem(char *p)
 {
-	if (!p)
-		return -EINVAL;
-
-	if (!strncmp(p, "off", 3))
-		gart_fix_e820 = 0;
-	else if (!strncmp(p, "on", 2))
-		gart_fix_e820 = 1;
-
-	return 0;
+	return kstrtobool(p, 0, &gart_fix_e820);
 }
 early_param("gart_fix_e820", parse_gart_mem);
 
diff --git a/include/linux/tick.h b/include/linux/tick.h
index 97fd4e543846..0ecdf0e248f4 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -98,7 +98,7 @@  static inline void tick_broadcast_exit(void)
 }
 
 #ifdef CONFIG_NO_HZ_COMMON
-extern int tick_nohz_enabled;
+extern bool tick_nohz_enabled;
 extern int tick_nohz_tick_stopped(void);
 extern void tick_nohz_idle_enter(void);
 extern void tick_nohz_idle_exit(void);
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 435b8850dd80..456f066148d5 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -515,7 +515,7 @@  static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
 /*
  * High resolution timer enabled ?
  */
-static int hrtimer_hres_enabled __read_mostly  = 1;
+static bool hrtimer_hres_enabled __read_mostly  = true;
 unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
 EXPORT_SYMBOL_GPL(hrtimer_resolution);
 
@@ -524,13 +524,7 @@  EXPORT_SYMBOL_GPL(hrtimer_resolution);
  */
 static int __init setup_hrtimer_hres(char *str)
 {
-	if (!strcmp(str, "off"))
-		hrtimer_hres_enabled = 0;
-	else if (!strcmp(str, "on"))
-		hrtimer_hres_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return kstrtobool(str, 0, &hrtimer_hres_enabled);
 }
 
 __setup("highres=", setup_hrtimer_hres);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9d7a053545f5..b57f822c2069 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -387,20 +387,14 @@  void __init tick_nohz_init(void)
 /*
  * NO HZ enabled ?
  */
-int tick_nohz_enabled __read_mostly = 1;
+bool tick_nohz_enabled __read_mostly  = true;
 unsigned long tick_nohz_active  __read_mostly;
 /*
  * Enable / Disable tickless mode
  */
 static int __init setup_tick_nohz(char *str)
 {
-	if (!strcmp(str, "off"))
-		tick_nohz_enabled = 0;
-	else if (!strcmp(str, "on"))
-		tick_nohz_enabled = 1;
-	else
-		return 0;
-	return 1;
+	return kstrtobool(str, 0, &tick_nohz_enabled);
 }
 
 __setup("nohz=", setup_tick_nohz);