linux-kernel - Re: [PATCH] panic: Use atomic_try_cmpxchg in panic() and 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: <ZPb5kt_XD6Ta2X6n@FVFF77S0Q05N>
Date:   Tue, 5 Sep 2023 10:49:33 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Uros Bizjak <ubizjak@...il.com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] panic: Use atomic_try_cmpxchg in panic() and nmi_panic()

On Mon, Sep 04, 2023 at 05:21:01PM +0200, Uros Bizjak wrote:
> Use atomic_try_cmpxchg instead of atomic_cmpxchg (*ptr, old, new) == old
> in panic() and nmi_panic().  x86 CMPXCHG instruction returns success in
> ZF flag, so this change saves a compare after cmpxchg (and related move
> instruction in front of cmpxchg).
> 
> Also, rename cpu variable to this_cpu in nmi_panic() and try to unify
> logic flow between panic() and nmi_panic().
> 
> No functional change intended.

Do we really need to save a compare here? A panic isn't exactly a fast path,
and robustness and code clarity is far more important than performance here.

> 
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Uros Bizjak <ubizjak@...il.com>
> ---
>  kernel/panic.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 07239d4ad81e..8740ac65cb2c 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -192,14 +192,15 @@ atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
>   */
>  void nmi_panic(struct pt_regs *regs, const char *msg)
>  {
> -	int old_cpu, cpu;
> +	int old_cpu, this_cpu;
>  
> -	cpu = raw_smp_processor_id();
> -	old_cpu = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, cpu);
> +	old_cpu = PANIC_CPU_INVALID;
> +	this_cpu = raw_smp_processor_id();
>  
> -	if (old_cpu == PANIC_CPU_INVALID)
> +	/* atomic_try_cmpxchg updates old_cpu on failure */
> +	if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu))
>  		panic("%s", msg);
> -	else if (old_cpu != cpu)
> +	else if (old_cpu != this_cpu)
>  		nmi_panic_self_stop(regs);
>  }
>  EXPORT_SYMBOL(nmi_panic);
> @@ -311,15 +312,18 @@ void panic(const char *fmt, ...)
>  	 * stop themself or will wait until they are stopped by the 1st CPU
>  	 * with smp_send_stop().
>  	 *
> -	 * `old_cpu == PANIC_CPU_INVALID' means this is the 1st CPU which
> -	 * comes here, so go ahead.
> +	 * cmpxchg success means this is the 1st CPU which comes here,
> +	 * so go ahead.
>  	 * `old_cpu == this_cpu' means we came from nmi_panic() which sets
>  	 * panic_cpu to this CPU.  In this case, this is also the 1st CPU.
>  	 */
> +	old_cpu = PANIC_CPU_INVALID;
>  	this_cpu = raw_smp_processor_id();
> -	old_cpu  = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, this_cpu);
>  
> -	if (old_cpu != PANIC_CPU_INVALID && old_cpu != this_cpu)
> +	/* atomic_try_cmpxchg updates old_cpu on failure */
> +	if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu))
> +		;
> +	else if (old_cpu != this_cpu)
>  		panic_smp_self_stop();

That empty statement is quite painful to read and would be easy to break in
future with other changes. It'd be better to either avoid that entirely, or use
braces, e.g.

	if (!atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu) &&
	    old_cpu != this_cpu)
		 panic_smp_self_stop();

... or:

	if (atomic_try_cmpxchg(&panic_cpu, &old_cpu, this_cpu)) {
		/* do nothing */
	} else if (old_cpu != this_cpu) {
		panic_smp_self_stop();
	}

The former is closer to the existing logic, so that's probably best.

Mark.