From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757377AbcBEALo (ORCPT ); Thu, 4 Feb 2016 19:11:44 -0500 Received: from mail-ig0-f173.google.com ([209.85.213.173]:34933 "EHLO mail-ig0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753392AbcBEALi (ORCPT ); Thu, 4 Feb 2016 19:11:38 -0500 MIME-Version: 1.0 In-Reply-To: References: <1454619643-14444-1-git-send-email-keescook@chromium.org> <1454619643-14444-5-git-send-email-keescook@chromium.org> Date: Thu, 4 Feb 2016 16:11:35 -0800 X-Google-Sender-Auth: qZERiV_Zx6wI4mVGMJvUcDPnln0 Message-ID: Subject: Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool From: Kees Cook To: Andy Shevchenko Cc: Andrew Morton , "x86@kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-s390@vger.kernel.org" , Joe Perches , Rasmus Villemoes , Daniel Borkmann , Amitkumar Karwar , Nishant Sarmukadam , Kalle Valo , Steve French , Michael Ellerman , Heiko Carstens , Martin Schwidefsky , "open list:TI WILINK WIRELES..." , netdev , linux-cifs@vger.kernel.org, "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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: 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