Re: [PATCH v12 2/6] arm64: add support for ARCH_HAS_COPY_MC

[Tong Tiangen <tongtiangen@huawei.com>]

在 2024/8/20 17:12, Mark Rutland 写道:
> On Tue, Aug 20, 2024 at 10:11:45AM +0800, Tong Tiangen wrote:
>> 在 2024/8/20 1:29, Mark Rutland 写道:
>>> Hi Tong,
>>>
>>> On Tue, May 28, 2024 at 04:59:11PM +0800, Tong Tiangen wrote:
>>>> For the arm64 kernel, when it processes hardware memory errors for
>>>> synchronize notifications(do_sea()), if the errors is consumed within the
>>>> kernel, the current processing is panic. However, it is not optimal.
>>>>
>>>> Take copy_from/to_user for example, If ld* triggers a memory error, even in
>>>> kernel mode, only the associated process is affected. Killing the user
>>>> process and isolating the corrupt page is a better choice.
>>>>
>>>> New fixup type EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE is added to identify insn
>>>> that can recover from memory errors triggered by access to kernel memory.
>>>>
>>>> Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
> 
> [...]
> 
>>>> diff --git a/arch/arm64/include/asm/asm-extable.h b/arch/arm64/include/asm/asm-extable.h
>>>> index 980d1dd8e1a3..9c0664fe1eb1 100644
>>>> --- a/arch/arm64/include/asm/asm-extable.h
>>>> +++ b/arch/arm64/include/asm/asm-extable.h
>>>> @@ -5,11 +5,13 @@
>>>>    #include <linux/bits.h>
>>>>    #include <asm/gpr-num.h>
>>>> -#define EX_TYPE_NONE			0
>>>> -#define EX_TYPE_BPF			1
>>>> -#define EX_TYPE_UACCESS_ERR_ZERO	2
>>>> -#define EX_TYPE_KACCESS_ERR_ZERO	3
>>>> -#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD	4
>>>> +#define EX_TYPE_NONE				0
>>>> +#define EX_TYPE_BPF				1
>>>> +#define EX_TYPE_UACCESS_ERR_ZERO		2
>>>> +#define EX_TYPE_KACCESS_ERR_ZERO		3
>>>> +#define EX_TYPE_LOAD_UNALIGNED_ZEROPAD		4
>>>> +/* kernel access memory error safe */
>>>> +#define EX_TYPE_KACCESS_ERR_ZERO_ME_SAFE	5
>>>
>>> Could we please use 'MEM_ERR', and likewise for the macros below? That's
>>> more obvious than 'ME_SAFE', and we wouldn't need the comment here.
>>> Likewise elsewhere in this patch and the series.
>>>
>>> To Jonathan's comment, I do prefer these numbers are aligned, so aside
>>> from the naming, the diff above looks good.
>>
>> OK, I also modified other locations to use 'MEM_ERR'.
> 
> Thanks!
> 
> [...]
> 
>>>> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
>>>> index 802231772608..2ac716c0d6d8 100644
>>>> --- a/arch/arm64/lib/copy_to_user.S
>>>> +++ b/arch/arm64/lib/copy_to_user.S
>>>> @@ -20,7 +20,7 @@
>>>>     *	x0 - bytes not copied
>>>>     */
>>>>    	.macro ldrb1 reg, ptr, val
>>>> -	ldrb  \reg, [\ptr], \val
>>>> +	KERNEL_ME_SAFE(9998f, ldrb  \reg, [\ptr], \val)
>>>>    	.endm
>>>>    	.macro strb1 reg, ptr, val
>>>> @@ -28,7 +28,7 @@
>>>>    	.endm
>>>>    	.macro ldrh1 reg, ptr, val
>>>> -	ldrh  \reg, [\ptr], \val
>>>> +	KERNEL_ME_SAFE(9998f, ldrh  \reg, [\ptr], \val)
>>>>    	.endm
>>>>    	.macro strh1 reg, ptr, val
>>>> @@ -36,7 +36,7 @@
>>>>    	.endm
>>>>    	.macro ldr1 reg, ptr, val
>>>> -	ldr \reg, [\ptr], \val
>>>> +	KERNEL_ME_SAFE(9998f, ldr \reg, [\ptr], \val)
>>>>    	.endm
>>>>    	.macro str1 reg, ptr, val
>>>> @@ -44,7 +44,7 @@
>>>>    	.endm
>>>>    	.macro ldp1 reg1, reg2, ptr, val
>>>> -	ldp \reg1, \reg2, [\ptr], \val
>>>> +	KERNEL_ME_SAFE(9998f, ldp \reg1, \reg2, [\ptr], \val)
>>>>    	.endm
>>>>    	.macro stp1 reg1, reg2, ptr, val
>>>
>>> These changes mean that regular copy_to_user() will handle kernel memory
>>> errors, rather than only doing that in copy_mc_to_user(). If that's
>>> intentional, please call that out explicitly in the commit message.
>>
>> Yes. This is the purpose of the modification. If the copy_to_user()
>> function encounters a memory error, this uaccess affects only the
>> current process. and only need to kill the current process instead of
>> the entire kernel panic. Do not add copy_mc_to_user() so that
>> copy_to_user() can process memory errors.
>>
>> I'll add a description in the commit msg next version.
> 
> Ok; why do powerpc and x86 have separate copy_mc_to_user()
> implementations, then?

Taking x86 as an example:

unsigned long __must_check copy_mc_to_user(...)
{
	unsigned long ret;

	if (copy_mc_fragile_enabled) {
		instrument_copy_to_user(dst, src, len);
		__uaccess_begin();
		ret = copy_mc_fragile((__force void *)dst, src, len);
		__uaccess_end();
		return ret;
	}

	if (static_cpu_has(X86_FEATURE_ERMS)) {
		instrument_copy_to_user(dst, src, len);
		__uaccess_begin();
		ret = copy_mc_enhanced_fast_string((__force void *)dst, src, len);
		__uaccess_end();
		return ret;
	}

	return copy_user_generic((__force void *)dst, src, len);
}

Through checking the source code, I found that "copy_mc_fragile_enabled"
and "X86_FEATURE_ERMS" both rely on the hardware features of x86. I
cannot explain the reasons for the details, but I feel that these are
related to the hardware implementation.


Dan Williams should be able to explain the reason.

Hi Dan:

We need your help:)

Compared to copy_to_user(), copy_mc_to_user() added memory error
handling. My question is why the error handling is not directly
implemented on copy_to_user(), but instead the copy_mc_to_user()
function is added?  Related to hardware features or performance
considerations ?


Thanks,
Tong.

> 
> [...]
> 
>>>> +/*
>>>> + * APEI claimed this as a firmware-first notification.
>>>> + * Some processing deferred to task_work before ret_to_user().
>>>> + */
>>>> +static bool do_apei_claim_sea(struct pt_regs *regs)
>>>> +{
>>>> +	if (user_mode(regs)) {
>>>> +		if (!apei_claim_sea(regs))
>>>> +			return true;
>>>> +	} else if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC)) {
>>>> +		if (fixup_exception_me(regs) && !apei_claim_sea(regs))
>>>> +			return true;
>>>> +	}
>>>> +
>>>> +	return false;
>>>> +}
>>>
>>> Hmm... that'll fixup the exception even if we don't manage to claim a
>>> the SEA. I suspect this should probably be:
>>>
>>> static bool do_apei_claim_sea(struct pt_regs *regs)
>>> {
>>> 	if (apei_claim_sea(regs))
>>> 		return false;
>>> 	if (user_mode(regs))
>>> 		return true;
>>> 	if (IS_ENABLED(CONFIG_ARCH_HAS_COPY_MC))
>>> 		return !fixup_excepton_mem_err(regs);
>>> 	
>>> 	return false;
>>> }
>>>
>>> ... unless we *don't* want to claim the SEA in the case we don't have a
>>> fixup?
>>>
>>> Mark.
>>>
>>
>> Yes. My original meaning here is that if not have fixup, panic is
>> performed in do_sea() according to the original logic, and claim sea is
>> not required.
> 
> AFAICT my suggestion doesn't change that; if we don't have a fixup the
> proprosed do_apei_claim_sea() would return false, and so do_sea() would
> caryy on to arm64_notify_die(...).
> 
> I'm specifically asking if we need to avoid calling apei_claim_sea()
> when we don't have a fixup handler, or if calling that would be fine.
> 
> One important thing is that if apei_claim_sea() fails to claim the SEA,
> we'd like to panic(), and in that case it'd be good to have not applied
> the fixup handler, so that the pt_regs::pc shows where the fault was
> taken from.
> 
> Mark.

I roughly understand what you mean. The prerequisite of fixup is sea 
claimed succeed. But the fixup here actually just set the regs->pc, and 
not applied the fixup handler here. If claim sea fails, it will directly 
panic() here without applying the fixup handler.

Thanks,
Tong.

> 
> .