On Sat, Jun 18, 2022 at 05:27:45PM +0800, Tong Tiangen wrote:

在 2022/6/17 17:06, Mark Rutland 写道:

quoted

On Sat, May 28, 2022 at 06:50:55AM +0000, Tong Tiangen wrote:

quoted

If user access fail due to hardware memory error, only the relevant
processes are affected, so killing the user process and isolate the
error page with hardware memory errors is a more reasonable choice
than kernel panic.

Signed-off-by: Tong Tiangen <redacted>

quoted

---
  arch/arm64/lib/copy_from_user.S | 8 ++++----
  arch/arm64/lib/copy_to_user.S   | 8 ++++----

All of these changes are to the *kernel* accesses performed as part of copy
to/from user, and have nothing to do with userspace, so it does not make sense
to mark these as UACCESS.

You have a point. so there is no need to modify copy_from/to_user.S in this
patch set.

Cool, thanks. If this patch just has the extable change, that's fine by me.

quoted

Do we *actually* need to recover from failues on these accesses? Looking at
_copy_from_user(), the kernel will immediately follow this up with a memset()
to the same address which will be fatal anyway, so this is only punting the
failure for a few instructions.

If recovery success, The task will be killed and there will be no subsequent
memset().

I don't think that's true.

IIUC per the last patch, in the exception handler we'll apply the fixup then
force a signal. That doesn't kill the task immediately, and we'll return from
the exception handler back into the original context (with the fixup applied).

The structure of copy_from_user() is 

	copy_from_user(to, from, n) {
		_copy_from_user(to, from, n) {
			res = n;
			res = raw_copy_from_user(to, from, n);
			if (res) 
				memset(to + (n - res), 0, res);
		}
	}

So when the fixup is applied and res indicates that the copy terminated early,
there is an unconditinal memset() before the fatal signal is handled in the
return to userspace path.

quoted

If we really need to recover from certain accesses to kernel memory we should
add a new EX_TYPE_KACCESS_ERR_ZERO_MC or similar, but we need a strong
rationale as to why that's useful. As things stand I do not beleive it makes
sense for copy to/from user specifically.

[...]

quoted

quoted

diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
index c301dcf6335f..8ca8d9639f9f 100644
--- a/arch/arm64/mm/extable.c
+++ b/arch/arm64/mm/extable.c

@@ -86,10 +86,10 @@ bool fixup_exception_mc(struct pt_regs *regs)
  	if (!ex)
  		return false;
-	/*
-	 * This is not complete, More Machine check safe extable type can
-	 * be processed here.
-	 */
+	switch (ex->type) {
+	case EX_TYPE_UACCESS_ERR_ZERO:
+		return ex_handler_uaccess_err_zero(ex, regs);
+	}

This addition specifically makes sense to me, so can you split this into a separate patch?

According to my understanding of the above, only the modification of
extable.c is retained.

So what do you mean which part is made into a separate patch?

As above, if you just retain the extable.c changes, that's fine by me.

Thanks,
Mark.