linux-kernel - Re: [PATCH v6 24/29] watchdog/hardlockup: Use parse_option_str() to handle "nmi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1652179333.fnu2fjiffn.astroid@bobo.none>
Date:   Tue, 10 May 2022 20:46:41 +1000
From:   Nicholas Piggin <npiggin@...il.com>
To:     Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org
Cc:     Andi Kleen <ak@...ux.intel.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lu Baolu <baolu.lu@...ux.intel.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Stephane Eranian <eranian@...gle.com>,
        iommu@...ts.linux-foundation.org, Joerg Roedel <joro@...tes.org>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Ricardo Neri <ricardo.neri@...el.com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v6 24/29] watchdog/hardlockup: Use parse_option_str() to
 handle "nmi_watchdog"

Excerpts from Ricardo Neri's message of May 6, 2022 10:00 am:
> Prepare hardlockup_panic_setup() to handle a comma-separated list of
> options. Thus, it can continue parsing its own command-line options while
> ignoring parameters that are relevant only to specific implementations of
> the hardlockup detector. Such implementations may use an early_param to
> parse their own options.

It can't really handle comma separated list though, until the next
patch. nmi_watchdog=panic,0 does not make sense, so you lost error
handling of that.

And is it kosher to double handle options like this? I'm sure it
happens but it's ugly.

Would you consider just add a new option for x86 and avoid changing
this? Less code and patches.

Thanks,
Nick

> 
> Cc: Andi Kleen <ak@...ux.intel.com>
> Cc: Nicholas Piggin <npiggin@...il.com>
> Cc: Stephane Eranian <eranian@...gle.com>
> Cc: "Ravi V. Shankar" <ravi.v.shankar@...el.com>
> Cc: iommu@...ts.linux-foundation.org
> Cc: linuxppc-dev@...ts.ozlabs.org
> Cc: x86@...nel.org
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
> ---
> Changes since v5:
>  * Corrected typo in commit message. (Tony)
> 
> Changes since v4:
>  * None
> 
> Changes since v3:
>  * None
> 
> Changes since v2:
>  * Introduced this patch.
> 
> Changes since v1:
>  * None
> ---
>  kernel/watchdog.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 9166220457bc..6443841a755f 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -73,13 +73,13 @@ void __init hardlockup_detector_disable(void)
>  
>  static int __init hardlockup_panic_setup(char *str)
>  {
> -	if (!strncmp(str, "panic", 5))
> +	if (parse_option_str(str, "panic"))
>  		hardlockup_panic = 1;
> -	else if (!strncmp(str, "nopanic", 7))
> +	else if (parse_option_str(str, "nopanic"))
>  		hardlockup_panic = 0;
> -	else if (!strncmp(str, "0", 1))
> +	else if (parse_option_str(str, "0"))
>  		nmi_watchdog_user_enabled = 0;
> -	else if (!strncmp(str, "1", 1))
> +	else if (parse_option_str(str, "1"))
>  		nmi_watchdog_user_enabled = 1;
>  	return 1;
>  }
> -- 
> 2.17.1
> 
>