[PATCH v2 0/4] lib: add "on" and "off" to strtobool

This consolidates logic for handling "on"/"off" parsing for bools into
the strtobool function, by way of moving it into kstrtobool (with helpers),
and updating various callers.

 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 -----
 drivers/net/wireless/marvell/mwifiex/debugfs.c |   10 +---
 fs/cifs/cifs_debug.c                           |   58 ++++++-------------------
 fs/cifs/cifs_debug.h                           |    2 
 fs/cifs/cifsfs.c                               |    6 +-
 fs/cifs/cifsglob.h                             |    4 -
 include/linux/kernel.h                         |    3 +
 include/linux/string.h                         |    6 ++
 include/linux/tick.h                           |    2 
 kernel/time/hrtimer.c                          |   10 ----
 kernel/time/tick-sched.c                       |   10 ----
 lib/kstrtox.c                                  |   49 +++++++++++++++++++++
 lib/string.c                                   |   29 ------------
 17 files changed, 98 insertions(+), 137 deletions(-)

-Kees

Add support for "on" and "off" when converting to boolean.

Signed-off-by: Kees Cook <redacted>
---
 lib/kstrtox.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index e18f088704d7..09e83a19a96d 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c

@@ -347,6 +347,20 @@ int kstrtobool(const char *s, unsigned int base, bool *res)
 	case '0':
 		*res = false;
 		return 0;
+	case 'o':
+	case 'O':
+		switch (s[1]) {
+		case 'n':
+		case 'N':
+			*res = true;
+			return 0;
+		case 'f':
+		case 'F':
+			*res = false;
+			return 0;
+		default:
+			break;
+		}
 	default:
 		break;
 	}

-- 
2.6.3

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] wrote:

Forgot update description?

        case '0':
                *res = false;
                return 0;
+       case 'o':
+       case 'O':
+               switch (s[1]) {
+               case 'n':
+               case 'N':
+                       *res = true;
+                       return 0;
+               case 'f':
+               case 'F':
+                       *res = false;
+                       return 0;
+               default:
+                       break;
+               }
        default:
                break;
        }
--
2.6.3

-- 
With Best Regards,
Andy Shevchenko

On Thu, Feb 4, 2016 at 3:00 PM, Andy Shevchenko
[off-list ref] wrote:

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] wrote:

quoted

Add support for "on" and "off" when converting to boolean.

Signed-off-by: Kees Cook <redacted>
---
 lib/kstrtox.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index e18f088704d7..09e83a19a96d 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c

@@ -347,6 +347,20 @@ int kstrtobool(const char *s, unsigned int base, bool *res)

Forgot update description?

Argh, thank you. Good eye. Sent another update.

-Kees

quoted

        case '0':
                *res = false;
                return 0;
+       case 'o':
+       case 'O':
+               switch (s[1]) {
+               case 'n':
+               case 'N':
+                       *res = true;
+                       return 0;
+               case 'f':
+               case 'F':
+                       *res = false;
+                       return 0;
+               default:
+                       break;
+               }
        default:
                break;
        }
--
2.6.3

--
With Best Regards,
Andy Shevchenko

-- 
Kees Cook
Chrome OS & Brillo Security

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 <redacted>
Acked-by: Heiko Carstens <redacted>
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

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] 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.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
[off-list ref] wrote:

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] wrote:

quoted

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

quoted

Signed-off-by: Kees Cook <redacted>
Acked-by: Heiko Carstens <redacted>
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

On Thu, Feb 4, 2016 at 4:11 PM, Kees Cook [off-list ref] wrote:

On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
[off-list ref] wrote:

quoted

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] wrote:

quoted

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

quoted

quoted

Signed-off-by: Kees Cook <redacted>
Acked-by: Heiko Carstens <redacted>
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

-- 
Kees Cook
Chrome OS & Brillo Security

Create the kstrtobool_from_user helper and moves strtobool logic into
the new kstrtobool (matching all the other kstrto* functions). Provides
an inline wrapper for existing strtobool callers.

Signed-off-by: Kees Cook <redacted>
---
 include/linux/kernel.h |  3 +++
 include/linux/string.h |  6 +++++-
 lib/kstrtox.c          | 35 +++++++++++++++++++++++++++++++++++
 lib/string.c           | 29 -----------------------------
 4 files changed, 43 insertions(+), 30 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c6e873..cdc25f47a23f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h

@@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
 int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
 int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
 int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
+int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
 
 int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
 int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);

@@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
 int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
 int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
 int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
+int __must_check kstrtobool_from_user(const char __user *s, size_t count,
+				      unsigned int base, bool *res);
 
 static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
 {

diff --git a/include/linux/string.h b/include/linux/string.h
index 9eebc66d957a..d2fb21b1081d 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
 extern void argv_free(char **argv);
 
 extern bool sysfs_streq(const char *s1, const char *s2);
-extern int strtobool(const char *s, bool *res);
+extern int kstrtobool(const char *s, unsigned int base, bool *res);
+static inline int strtobool(const char *s, bool *res)
+{
+	return kstrtobool(s, 0, res);
+}
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);

diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 94be244e8441..e18f088704d7 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c

@@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
 }
 EXPORT_SYMBOL(kstrtos8);
 
+/**
+ * kstrtobool - convert common user inputs into boolean values
+ * @s: input string
+ * @base: ignored
+ * @res: result
+ *
+ * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
+ * Otherwise it will return -EINVAL.  Value pointed to by res is
+ * updated upon finding a match.
+ */
+int kstrtobool(const char *s, unsigned int base, bool *res)
+{
+	if (!s)
+		return -EINVAL;
+
+	switch (s[0]) {
+	case 'y':
+	case 'Y':
+	case '1':
+		*res = true;
+		return 0;
+	case 'n':
+	case 'N':
+	case '0':
+		*res = false;
+		return 0;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(kstrtobool);
+
 #define kstrto_from_user(f, g, type)					\
 int f(const char __user *s, size_t count, unsigned int base, type *res)	\
 {									\

@@ -345,3 +379,4 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
 kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
 kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
 kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
+kstrto_from_user(kstrtobool_from_user,	kstrtobool,	bool);

diff --git a/lib/string.c b/lib/string.c
index 0323c0d5629a..1a90db9bc6e1 100644
--- a/lib/string.c
+++ b/lib/string.c

@@ -630,35 +630,6 @@ bool sysfs_streq(const char *s1, const char *s2)
 }
 EXPORT_SYMBOL(sysfs_streq);
 
-/**
- * strtobool - convert common user inputs into boolean values
- * @s: input string
- * @res: result
- *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
- * Otherwise it will return -EINVAL.  Value pointed to by res is
- * updated upon finding a match.
- */
-int strtobool(const char *s, bool *res)
-{
-	switch (s[0]) {
-	case 'y':
-	case 'Y':
-	case '1':
-		*res = true;
-		break;
-	case 'n':
-	case 'N':
-	case '0':
-		*res = false;
-		break;
-	default:
-		return -EINVAL;
-	}
-	return 0;
-}
-EXPORT_SYMBOL(strtobool);
-
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value

-- 
2.6.3

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook [off-list ref] wrote:

Create the kstrtobool_from_user helper and moves strtobool logic into
the new kstrtobool (matching all the other kstrto* functions). Provides
an inline wrapper for existing strtobool callers.

Signed-off-by: Kees Cook <redacted>

Reviewed-by: Andy Shevchenko <redacted>

One minor below.

+int __must_check kstrtobool_from_user(const char __user *s, size_t count,
+                                     unsigned int base, bool *res);

We already are using long lines here, perhaps do the same?

-- 
With Best Regards,
Andy Shevchenko