Re: [PATCH] riscv: Fix spurious errors from __get/put_kernel_nofault

From: Charlie Jenkins <charlie@rivosinc.com>
To: Samuel Holland <samuel.holland@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH] riscv: Fix spurious errors from __get/put_kernel_nofault
Date: Tue, 12 Mar 2024 19:53:41 -0700	[thread overview]
Message-ID: <ZfEVNbt9AMeVJS0k@ghost> (raw)
In-Reply-To: <20240312022030.320789-1-samuel.holland@sifive.com>

On Mon, Mar 11, 2024 at 07:19:13PM -0700, Samuel Holland wrote:
> These macros did not initialize __kr_err, so they could fail even if
> the access did not fault.
> 
> Cc: stable@vger.kernel.org
> Fixes: d464118cdc41 ("riscv: implement __get_kernel_nofault and __put_user_nofault")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> Found while testing the unaligned access speed series[1]. The observed
> behavior was that with RISCV_EFFICIENT_UNALIGNED_ACCESS=y, the
> copy_from_kernel_nofault() in prepend_copy() failed every time when
> filling out /proc/self/mounts, so all of the mount points were "xxx".
> 
> I'm surprised this hasn't been seen before. For reference, I'm compiling
> with clang 18.
> 
> [1]: https://lore.kernel.org/linux-riscv/20240308-disable_misaligned_probe_config-v9-0-a388770ba0ce@rivosinc.com/
> 
>  arch/riscv/include/asm/uaccess.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index ec0cab9fbddd..72ec1d9bd3f3 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -319,7 +319,7 @@ unsigned long __must_check clear_user(void __user *to, unsigned long n)
>  
>  #define __get_kernel_nofault(dst, src, type, err_label)			\
>  do {									\
> -	long __kr_err;							\
> +	long __kr_err = 0;						\
>  									\
>  	__get_user_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
>  	if (unlikely(__kr_err))						\
> @@ -328,7 +328,7 @@ do {									\
>  
>  #define __put_kernel_nofault(dst, src, type, err_label)			\
>  do {									\
> -	long __kr_err;							\
> +	long __kr_err = 0;						\
>  									\
>  	__put_user_nocheck(*((type *)(src)), (type *)(dst), __kr_err);	\
>  	if (unlikely(__kr_err))						\
> -- 
> 2.43.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

I am not able to reproduce this using Clang 18 with
RISCV_EFFICIENT_UNALIGNED_ACCESS=y on 6.8. However I can see how this
could be an issue.

Going down the rabbit hold of macros here, I end up at
arch/riscv/include/asm/asm-extable.h where the register that hold 'err'
is written into the __ex_table section:

#define EX_DATA_REG(reg, gpr)						\
	"((.L__gpr_num_" #gpr ") << " __stringify(EX_DATA_REG_##reg##_SHIFT) ")"

#define _ASM_EXTABLE_UACCESS_ERR_ZERO(insn, fixup, err, zero)		\
	__DEFINE_ASM_GPR_NUMS						\
	__ASM_EXTABLE_RAW(#insn, #fixup, 				\
			  __stringify(EX_TYPE_UACCESS_ERR_ZERO),	\
			  "("						\
			    EX_DATA_REG(ERR, err) " | "			\
			    EX_DATA_REG(ZERO, zero)			\
			  ")")

I am wondering if setting this value to zero solves the problem by
hiding another issue. It seems like this shouldn't need to be
initialized to zero, however I am lost as to how this extable setup
works so perhaps this is the proper solution.

- Charlie