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

[Mark Rutland <mark.rutland@arm.com>]

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?

[...]

> > > +/*
> > > + * 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.