linux-kernel - Re: [PATCH v3 1/1] ppc/crash: Reset spinlocks during crash

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: <20200401092618.GW20713@hirez.programming.kicks-ass.net>
Date:   Wed, 1 Apr 2020 11:26:18 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Leonardo Bras <leonardo@...ux.ibm.com>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Enrico Weigelt <info@...ux.net>,
        Alexios Zavras <alexios.zavras@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christophe Leroy <christophe.leroy@....fr>,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] ppc/crash: Reset spinlocks during crash

On Tue, Mar 31, 2020 at 09:00:21PM -0300, Leonardo Bras wrote:
> During a crash, there is chance that the cpus that handle the NMI IPI
> are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
> will cause a deadlock. (rtas.lock and printk logbuf_lock as of today)
> 
> This is a problem if the system has kdump set up, given if it crashes
> for any reason kdump may not be saved for crash analysis.
> 
> After NMI IPI is sent to all other cpus, force unlock all spinlocks
> needed for finishing crash routine.
> 
> Signed-off-by: Leonardo Bras <leonardo@...ux.ibm.com>

> @@ -129,6 +132,13 @@ static void crash_kexec_prepare_cpus(int cpu)
>  	/* Would it be better to replace the trap vector here? */
>  
>  	if (atomic_read(&cpus_in_crash) >= ncpus) {
> +		/*
> +		 * At this point no other CPU is running, and some of them may
> +		 * have been interrupted while holding one of the locks needed
> +		 * to complete crashing. Free them so there is no deadlock.
> +		 */
> +		arch_spin_unlock(&logbuf_lock.raw_lock);
> +		arch_spin_unlock(&rtas.lock);
>  		printk(KERN_EMERG "IPI complete\n");
>  		return;
>  	}

You might want to add a note to your asm/spinlock.h that you rely on
spin_unlock() unconditionally clearing a lock.

This isn't naturally true for all lock implementations. Consider ticket
locks, doing a surplus unlock will wreck your lock state in that case.
So anybody poking at the powerpc spinlock implementation had better know
you rely on this.