netdev - Re: [PATCH 04/30] firmware: google: Convert regular spinlock into trylock on panic path

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: <c23f408f-d030-b599-19b4-8561d511ef73@igalia.com>
Date:   Tue, 10 May 2022 10:04:08 -0300
From:   "Guilherme G. Piccoli" <gpiccoli@...lia.com>
To:     Petr Mladek <pmladek@...e.com>, Evan Green <evgreen@...omium.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, bhe@...hat.com,
        kexec@...ts.infradead.org, LKML <linux-kernel@...r.kernel.org>,
        bcm-kernel-feedback-list@...adcom.com,
        linuxppc-dev@...ts.ozlabs.org, linux-alpha@...r.kernel.org,
        linux-edac@...r.kernel.org, linux-hyperv@...r.kernel.org,
        linux-leds@...r.kernel.org, linux-mips@...r.kernel.org,
        linux-parisc@...r.kernel.org, Linux PM <linux-pm@...r.kernel.org>,
        linux-remoteproc@...r.kernel.org, linux-s390@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-um@...ts.infradead.org,
        linux-xtensa@...ux-xtensa.org, netdev@...r.kernel.org,
        openipmi-developer@...ts.sourceforge.net, rcu@...r.kernel.org,
        sparclinux@...r.kernel.org, xen-devel@...ts.xenproject.org,
        x86@...nel.org, kernel-dev@...lia.com, kernel@...ccoli.net,
        halves@...onical.com, fabiomirmar@...il.com,
        alejandro.j.jimenez@...cle.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...en8.de>,
        Jonathan Corbet <corbet@....net>, d.hatayama@...fujitsu.com,
        dave.hansen@...ux.intel.com, dyoung@...hat.com,
        feng.tang@...el.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        mikelley@...rosoft.com, hidehiro.kawai.ez@...achi.com,
        jgross@...e.com, john.ogness@...utronix.de,
        Kees Cook <keescook@...omium.org>, luto@...nel.org,
        mhiramat@...nel.org, mingo@...hat.com, paulmck@...nel.org,
        peterz@...radead.org, rostedt@...dmis.org,
        senozhatsky@...omium.org, Alan Stern <stern@...land.harvard.edu>,
        Thomas Gleixner <tglx@...utronix.de>, vgoyal@...hat.com,
        vkuznets@...hat.com, Will Deacon <will@...nel.org>,
        Ard Biesheuvel <ardb@...nel.org>,
        David Gow <davidgow@...gle.com>,
        Julius Werner <jwerner@...omium.org>
Subject: Re: [PATCH 04/30] firmware: google: Convert regular spinlock into
 trylock on panic path

On 10/05/2022 08:38, Petr Mladek wrote:
> [...]
> I see two more alternative solutions:
> 
> 1st variant is a trick already used in console write() callbacks.
> They do trylock() when oops_in_progress is set. They remember
> the result to prevent double unlock when printing Oops messages and
> the system will try to continue working. For example:
> 
> pl011_console_write(struct console *co, const char *s, unsigned int count)
> {
> [...]
> 	int locked = 1;
> [...]
> 	if (uap->port.sysrq)
> 		locked = 0;
> 	else if (oops_in_progress)
> 		locked = spin_trylock(&uap->port.lock);
> 	else
> 		spin_lock(&uap->port.lock);
> 
> [...]
> 
> 	if (locked)
> 		spin_unlock(&uap->port.lock);
> }
> 
> 
> 2nd variant is to check panic_cpu variable. It is used in printk.c.
> We might move the function to panic.h:
> 
> static bool panic_in_progress(void)
> {
> 	return unlikely(atomic_read(&panic_cpu) != PANIC_CPU_INVALID);
> }
> 
> and then do:
> 
> 	if (panic_in_progress()) {
> 		...

Thanks for the review Petr! I feel alternative two is way better, it
checks for panic - the oops_in_progress isn't really enough, since we
can call panic() directly, not necessarily through an oops path, correct?

For me, we could stick with the lock check, but I'll defer to Evan - I
didn't work the V2 patch yet, what do you prefer Evan?


> [...]
> As already mentioned in the other reply, panic() sometimes stops
> the other CPUs using NMI, for example, see kdump_nmi_shootdown_cpus().
> 
> Another situation is when the CPU using the lock ends in some
> infinite loop because something went wrong. The system is in
> an unpredictable state during panic().
> 
> I am not sure if this is possible with the code under gsmi_dev.lock
> but such things really happen during panic() in other subsystems.
> Using trylock in the panic() code path is a good practice.
> 
> Best Regards,
> Petr

Makes total sense, thanks for confirming!
Cheers,


Guilherme