linux-kernel - Re: [PATCH 1/7] riscv: Implement cmpxchg32/64() using Zacas

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: Tue, 28 May 2024 16:34:48 +0100
From: Conor Dooley <conor@...nel.org>
To: Alexandre Ghiti <alexghiti@...osinc.com>
Cc: Jonathan Corbet <corbet@....net>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>,
	Arnd Bergmann <arnd@...db.de>, Leonardo Bras <leobras@...hat.com>,
	Guo Ren <guoren@...nel.org>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-arch@...r.kernel.org
Subject: Re: [PATCH 1/7] riscv: Implement cmpxchg32/64() using Zacas

On Tue, May 28, 2024 at 05:10:46PM +0200, Alexandre Ghiti wrote:
> This adds runtime support for Zacas in cmpxchg operations.
> 
> Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> ---
>  arch/riscv/Kconfig               | 17 +++++++++++++++++
>  arch/riscv/Makefile              | 11 +++++++++++
>  arch/riscv/include/asm/cmpxchg.h | 23 ++++++++++++++++++++---
>  3 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 8a0f403432e8..b443def70139 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -579,6 +579,23 @@ config RISCV_ISA_V_PREEMPTIVE
>  	  preemption. Enabling this config will result in higher memory
>  	  consumption due to the allocation of per-task's kernel Vector context.
>  
> +config TOOLCHAIN_HAS_ZACAS
> +	bool
> +	default y
> +	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zacas)
> +	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zacas)
> +	depends on AS_HAS_OPTION_ARCH
> +
> +config RISCV_ISA_ZACAS
> +	bool "Zacas extension support for atomic CAS"
> +	depends on TOOLCHAIN_HAS_ZACAS
> +	default y
> +	help
> +	  Adds support to use atomic CAS instead of LR/SC to implement kernel
> +	  atomic cmpxchg operation.

If you were a person compiling a kernel, would you be able to read this
and realise that this is safe to enable when their system does not
support atomic CAS? Please take a look at other how other extensions
handle this, or the patch that I have been sending that tries to make
things clearer:
https://patchwork.kernel.org/project/linux-riscv/patch/20240528-varnish-status-9c22973093a0@spud/

> +
> +	  If you don't know what to do here, say Y.
> +
>  config TOOLCHAIN_HAS_ZBB
>  	bool
>  	default y
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 5b3115a19852..d5b60b87998c 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -78,6 +78,17 @@ endif
>  # Check if the toolchain supports Zihintpause extension
>  riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
>  
> +# Check if the toolchain supports Zacas
> +ifdef CONFIG_AS_IS_LLVM
> +# Support for experimental Zacas was merged in LLVM 17, but the removal of
> +# the "experimental" was merged in LLVM 19.
> +KBUILD_CFLAGS += -menable-experimental-extensions
> +KBUILD_AFLAGS += -menable-experimental-extensions
> +riscv-march-y := $(riscv-march-y)_zacas1p0
> +else
> +riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas
> +endif

I'm almost certain that we discussed this before for vector and it was
decided to not enable experimental extensions (particularly as it is a
global option), and instead require the non-experimental versions.
This isn't even consistent with your TOOLCHAIN_HAS_ZACAS checks, that
will only enable the option for the ratified version. I think we should
continue to avoid enabling experimental extensions, even if that imposes
a requirement of having a bleeding edge toolchain to actually use the
extension.

Thanks,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)