Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
On Thu, Feb 4, 2016 at 4:11 PM, Kees Cook wrote:
> On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
> wrote:
>> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook 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
>>> Acked-by: Heiko Carstens
>>> Acked-by: Michael Ellerman
>>> Cc: x...@kernel.org
>>> Cc: linuxppc-...@lists.ozlabs.org
>>> Cc: linux-s...@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/kerne
Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook 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
>> Acked-by: Heiko Carstens
>> Acked-by: Michael Ellerman
>> Cc: x...@kernel.org
>> Cc: linuxppc-...@lists.ozlabs.org
>> Cc: linux-s...@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;
>> stati
Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook 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
> Acked-by: Heiko Carstens
> Acked-by: Michael Ellerman
> Cc: x...@kernel.org
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: linux-s...@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;
>
[PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
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
Acked-by: Heiko Carstens
Acked-by: Michael Ellerman
Cc: x...@kernel.org
Cc: linuxppc-...@lists.ozlabs.org
Cc: linux-s...@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/l
