linux-kernel - Re: [PATCH v5 13/18] watchdog/hardlockup: Have the perf hardlockup use _

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]

Date:   Wed, 24 May 2023 15:59:15 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        kgdb-bugreport@...ts.sourceforge.net,
        Stephane Eranian <eranian@...gle.com>, mpe@...erman.id.au,
        Tzung-Bi Shih <tzungbi@...omium.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        linuxppc-dev@...ts.ozlabs.org, Sumit Garg <sumit.garg@...aro.org>,
        npiggin@...il.com, davem@...emloft.net,
        Marc Zyngier <maz@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>, sparclinux@...r.kernel.org,
        christophe.leroy@...roup.eu,
        Catalin Marinas <catalin.marinas@....com>,
        ravi.v.shankar@...el.com, Randy Dunlap <rdunlap@...radead.org>,
        Pingfan Liu <kernelfans@...il.com>,
        Guenter Roeck <groeck@...omium.org>,
        Lecopzer Chen <lecopzer.chen@...iatek.com>,
        Ian Rogers <irogers@...gle.com>, ito-yuichi@...itsu.com,
        ricardo.neri@...el.com, linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, Will Deacon <will@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>, linux-kernel@...r.kernel.org,
        Masayoshi Mizuma <msys.mizuma@...il.com>,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v5 13/18] watchdog/hardlockup: Have the perf hardlockup
 use __weak functions more cleanly

On Fri 2023-05-19 10:18:37, Douglas Anderson wrote:
> The fact that there watchdog_hardlockup_enable(),
> watchdog_hardlockup_disable(), and watchdog_hardlockup_probe() are
> declared __weak means that the configured hardlockup detector can
> define non-weak versions of those functions if it needs to. Instead of
> doing this, the perf hardlockup detector hooked itself into the
> default __weak implementation, which was a bit awkward. Clean this up.
> 
> >From comments, it looks as if the original design was done because the
> __weak function were expected to implemented by the architecture and
> not by the configured hardlockup detector. This got awkward when we
> tried to add the buddy lockup detector which was not arch-specific but
> wanted to hook into those same functions.
> 
> This is not expected to have any functional impact.
>
> @@ -187,27 +187,33 @@ static inline void watchdog_hardlockup_kick(void) { }
>  #endif /* !CONFIG_HARDLOCKUP_DETECTOR_PERF */
>  
>  /*
> - * These functions can be overridden if an architecture implements its
> - * own hardlockup detector.
> + * These functions can be overridden based on the configured hardlockdup detector.
>   *
>   * watchdog_hardlockup_enable/disable can be implemented to start and stop when
> - * softlockup watchdog start and stop. The arch must select the
> + * softlockup watchdog start and stop. The detector must select the
>   * SOFTLOCKUP_DETECTOR Kconfig.
>   */
> -void __weak watchdog_hardlockup_enable(unsigned int cpu)
> -{
> -	hardlockup_detector_perf_enable();
> -}
> +void __weak watchdog_hardlockup_enable(unsigned int cpu) { }
>  
> -void __weak watchdog_hardlockup_disable(unsigned int cpu)
> -{
> -	hardlockup_detector_perf_disable();
> -}
> +void __weak watchdog_hardlockup_disable(unsigned int cpu) { }
>  
>  /* Return 0, if a hardlockup watchdog is available. Error code otherwise */
>  int __weak __init watchdog_hardlockup_probe(void)
>  {
> -	return hardlockup_detector_perf_init();
> +	/*
> +	 * If CONFIG_HAVE_NMI_WATCHDOG is defined then an architecture
> +	 * is assumed to have the hard watchdog available and we return 0.
> +	 */
> +	if (IS_ENABLED(CONFIG_HAVE_NMI_WATCHDOG))
> +		return 0;
> +
> +	/*
> +	 * Hardlockup detectors other than those using CONFIG_HAVE_NMI_WATCHDOG
> +	 * are required to implement a non-weak version of this probe function
> +	 * to tell whether they are available. If they don't override then
> +	 * we'll return -ENODEV.
> +	 */
> +	return -ENODEV;
>  }

When thinking more about it. It is weird that we need to handle
CONFIG_HAVE_NMI_WATCHDOG in this default week function.

It should be handled in watchdog_hardlockup_probe() implemented
in kernel/watchdog_perf.c.

IMHO, the default __weak function could always return -ENODEV;

Would it make sense, please?

Best Regards,
Petr