[PATCH -next v5 0/3] minor improvements for x86 mce processing

[PATCH -next v5 0/3] minor improvements for x86 mce processing

[Tong Tiangen]

In this patchset, we remove the unused macro EX_TYPE_COPY and centralize
the processing of memory-failure to do_machine_check() to avoid calling
memory_failure_queue() separately for different MC-Safe scenarios. In
addition, MCE_IN_KERNEL_COPYIN is renamed MCE_IN_KERNEL_COPY_MC to expand
its usage scope.

[1]https://lore.kernel.org/linux-mm/20230526063242.133656-1-wangkefeng.wang@huawei.com/

since v4:
  1. Patch1, suggested by Petkov, changes as follows:
    a.Improve the commit msg.
    b.Directly delete the unused fixup_type EX_TYPE_COPY instead of
      disrupting the overall layout.
  2. Patch2/3, improve the commit msg suggested by Petkov.
  3. To better describe the problem, reorder the patch 2/3.

since v3:
  1. Rebased on linux-next tag next-20240111.
  2. Delete duplicate commit references on patch 3.

since v2:
  1. remove redundant fixup type EX_TYPE_COPY.
  2. rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC.
  3. update patch3's commit message and the processing logic of
     EX_TYPE_DEFAULT_MCE_SAFE based on the discussion of [1].

Kefeng Wang (1):
  x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

Tong Tiangen (2):
  x86/mce: remove redundant fixup type EX_TYPE_COPY
  x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

 arch/x86/include/asm/asm.h                 |  3 ---
 arch/x86/include/asm/extable_fixup_types.h |  2 +-
 arch/x86/include/asm/mce.h                 |  8 ++++----
 arch/x86/kernel/cpu/mce/core.c             |  2 +-
 arch/x86/kernel/cpu/mce/severity.c         |  7 +++----
 arch/x86/mm/extable.c                      |  9 ---------
 mm/ksm.c                                   |  1 -
 mm/memory.c                                | 13 ++++---------
 8 files changed, 13 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH -next v5 1/3] x86/mce: remove redundant fixup type EX_TYPE_COPY

[Tong Tiangen]

Since commit 034ff37d3407 ("x86: rewrite '__copy_user_nocache' function")
rewrited __copy_user_nocache() uses EX_TYPE_UACCESS instead of
EX_TYPE_COPY, there is no user for EX_TYPE_COPY, so remove it.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/include/asm/asm.h                 | 3 ---
 arch/x86/include/asm/extable_fixup_types.h | 2 +-
 arch/x86/kernel/cpu/mce/severity.c         | 1 -
 arch/x86/mm/extable.c                      | 9 ---------
 4 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index fbcfec4dc4cc..692409ea0c37 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -215,9 +215,6 @@ register unsigned long current_stack_pointer asm(_ASM_SP);
 #define _ASM_EXTABLE_UA(from, to)				\
 	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_UACCESS)
 
-#define _ASM_EXTABLE_CPY(from, to)				\
-	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_COPY)
-
 #define _ASM_EXTABLE_FAULT(from, to)				\
 	_ASM_EXTABLE_TYPE(from, to, EX_TYPE_FAULT)
 
diff --git a/arch/x86/include/asm/extable_fixup_types.h b/arch/x86/include/asm/extable_fixup_types.h
index 7acf0383be80..906b0d5541e8 100644
--- a/arch/x86/include/asm/extable_fixup_types.h
+++ b/arch/x86/include/asm/extable_fixup_types.h
@@ -36,7 +36,7 @@
 #define	EX_TYPE_DEFAULT			 1
 #define	EX_TYPE_FAULT			 2
 #define	EX_TYPE_UACCESS			 3
-#define	EX_TYPE_COPY			 4
+/* unused, was: #define EX_TYPE_COPY	 4 */
 #define	EX_TYPE_CLEAR_FS		 5
 #define	EX_TYPE_FPU_RESTORE		 6
 #define	EX_TYPE_BPF			 7
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index c4477162c07d..bca780fa5e57 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -290,7 +290,6 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 
 	switch (fixup_type) {
 	case EX_TYPE_UACCESS:
-	case EX_TYPE_COPY:
 		if (!copy_user)
 			return IN_KERNEL;
 		m->kflags |= MCE_IN_KERNEL_COPYIN;
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index b522933bfa56..51986e8a9d35 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -164,13 +164,6 @@ static bool ex_handler_uaccess(const struct exception_table_entry *fixup,
 	return ex_handler_default(fixup, regs);
 }
 
-static bool ex_handler_copy(const struct exception_table_entry *fixup,
-			    struct pt_regs *regs, int trapnr)
-{
-	WARN_ONCE(trapnr == X86_TRAP_GP, "General protection fault in user access. Non-canonical address?");
-	return ex_handler_fault(fixup, regs, trapnr);
-}
-
 static bool ex_handler_msr(const struct exception_table_entry *fixup,
 			   struct pt_regs *regs, bool wrmsr, bool safe, int reg)
 {
@@ -341,8 +334,6 @@ int fixup_exception(struct pt_regs *regs, int trapnr, unsigned long error_code,
 		return ex_handler_fault(e, regs, trapnr);
 	case EX_TYPE_UACCESS:
 		return ex_handler_uaccess(e, regs, trapnr, fault_addr);
-	case EX_TYPE_COPY:
-		return ex_handler_copy(e, regs, trapnr);
 	case EX_TYPE_CLEAR_FS:
 		return ex_handler_clear_fs(e, regs);
 	case EX_TYPE_FPU_RESTORE:
-- 
2.25.1

[PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

From: Kefeng Wang <wangkefeng.wang@huawei.com>

Currently, some kernel memory copy scenarios[1][2][3] which use
copy_mc_{user_}highpage() to safely abort copy and report 'bytes not copied'
when accessing the poisoned source page, a recoverable synchronous exception
generated in the processing and the fixup type EX_TYPE_DEFAULT_MCE_SAFE is
used to distinguish from other exceptions, and an asynchronous
memory_failure_queue() is called to handle memory failure of the source page
, but scheduling someone else to handle it at some future point is
unpredictable and risky.

The better way is immediately deal with it during current context,
fortunately, there is already a framework to synchronously call
memory_failure(), see kill_me_never() in do_machine_check(), a task work is
triggered once MCE_IN_KERNEL_COPYIN is set, in order to fix above issue,
setting MCE_IN_KERNEL_COPYIN for EX_TYPE_DEFAULT_MCE_SAFE case too.

[1] commit d302c2398ba2 ("mm, hwpoison: when copy-on-write hits poison, take page offline")
[2] commit 1cb9dc4b475c ("mm: hwpoison: support recovery from HugePage copy-on-write faults")
[3] commit 6b970599e807 ("mm: hwpoison: support recovery from ksm_might_need_to_copy()")

Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/kernel/cpu/mce/severity.c |  4 ++--
 mm/ksm.c                           |  1 -
 mm/memory.c                        | 13 ++++---------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index bca780fa5e57..b2cce1b6c96d 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 	case EX_TYPE_UACCESS:
 		if (!copy_user)
 			return IN_KERNEL;
+		fallthrough;
+	case EX_TYPE_DEFAULT_MCE_SAFE:
 		m->kflags |= MCE_IN_KERNEL_COPYIN;
 		fallthrough;
-
 	case EX_TYPE_FAULT_MCE_SAFE:
-	case EX_TYPE_DEFAULT_MCE_SAFE:
 		m->kflags |= MCE_IN_KERNEL_RECOV;
 		return IN_KERNEL_RECOV;
 
diff --git a/mm/ksm.c b/mm/ksm.c
index 8c001819cf10..ba9d324ea1c6 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -3084,7 +3084,6 @@ struct folio *ksm_might_need_to_copy(struct folio *folio,
 		if (copy_mc_user_highpage(folio_page(new_folio, 0), page,
 								addr, vma)) {
 			folio_put(new_folio);
-			memory_failure_queue(folio_pfn(folio), 0);
 			return ERR_PTR(-EHWPOISON);
 		}
 		folio_set_dirty(new_folio);
diff --git a/mm/memory.c b/mm/memory.c
index 8d14ba440929..ee06a8f766ab 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2846,10 +2846,8 @@ static inline int __wp_page_copy_user(struct page *dst, struct page *src,
 	unsigned long addr = vmf->address;
 
 	if (likely(src)) {
-		if (copy_mc_user_highpage(dst, src, addr, vma)) {
-			memory_failure_queue(page_to_pfn(src), 0);
+		if (copy_mc_user_highpage(dst, src, addr, vma))
 			return -EHWPOISON;
-		}
 		return 0;
 	}
 
@@ -6179,10 +6177,8 @@ static int copy_user_gigantic_page(struct folio *dst, struct folio *src,
 
 		cond_resched();
 		if (copy_mc_user_highpage(dst_page, src_page,
-					  addr + i*PAGE_SIZE, vma)) {
-			memory_failure_queue(page_to_pfn(src_page), 0);
+					  addr + i*PAGE_SIZE, vma))
 			return -EHWPOISON;
-		}
 	}
 	return 0;
 }
@@ -6199,10 +6195,9 @@ static int copy_subpage(unsigned long addr, int idx, void *arg)
 	struct page *dst = nth_page(copy_arg->dst, idx);
 	struct page *src = nth_page(copy_arg->src, idx);
 
-	if (copy_mc_user_highpage(dst, src, addr, copy_arg->vma)) {
-		memory_failure_queue(page_to_pfn(src), 0);
+	if (copy_mc_user_highpage(dst, src, addr, copy_arg->vma))
 		return -EHWPOISON;
-	}
+
 	return 0;
 }
 
-- 
2.25.1

[PATCH -next v5 3/3] x86/mce: rename MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC

[Tong Tiangen]

In the x86 mce processing, error_context() use MCE_IN_KERNEL_COPYIN for
user-to-kernel copy(EX_TYPE_UACCESS) and kernel-to-kernel copy
(EX_TYPE_DEFAULT_MCE_SAFE).

"COPYIN" only stands for user-to-kernel copy and can't stands for
kernel-to-kernel copy, this can cause some misunderstandings. So rename
MCE_IN_KERNEL_COPYIN to MCE_IN_KERNEL_COPY_MC.

Signed-off-by: Tong Tiangen <tongtiangen@huawei.com>
---
 arch/x86/include/asm/mce.h         | 8 ++++----
 arch/x86/kernel/cpu/mce/core.c     | 2 +-
 arch/x86/kernel/cpu/mce/severity.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index de3118305838..cb628ab2f32f 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -151,11 +151,11 @@
 
 /*
  * Indicates an MCE that happened in kernel space while copying data
- * from user. In this case fixup_exception() gets the kernel to the
- * error exit for the copy function. Machine check handler can then
- * treat it like a fault taken in user mode.
+ * from user or kernel. In this case fixup_exception() gets the kernel
+ * to the error exit for the copy function. Machine check handler can
+ * then treat it like a fault taken in user or kernel mode.
  */
-#define MCE_IN_KERNEL_COPYIN	BIT_ULL(7)
+#define MCE_IN_KERNEL_COPY_MC	BIT_ULL(7)
 
 /*
  * This structure contains all data related to the MCE log.  Also
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 04acdc3534c8..644748a5d98e 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1608,7 +1608,7 @@ noinstr void do_machine_check(struct pt_regs *regs)
 				mce_panic("Failed kernel mode recovery", &m, msg);
 		}
 
-		if (m.kflags & MCE_IN_KERNEL_COPYIN)
+		if (m.kflags & MCE_IN_KERNEL_COPY_MC)
 			queue_task_work(&m, msg, kill_me_never);
 	}
 
diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
index b2cce1b6c96d..b4b1d028cbb3 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -294,7 +294,7 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
 			return IN_KERNEL;
 		fallthrough;
 	case EX_TYPE_DEFAULT_MCE_SAFE:
-		m->kflags |= MCE_IN_KERNEL_COPYIN;
+		m->kflags |= MCE_IN_KERNEL_COPY_MC;
 		fallthrough;
 	case EX_TYPE_FAULT_MCE_SAFE:
 		m->kflags |= MCE_IN_KERNEL_RECOV;
-- 
2.25.1

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Borislav Petkov]

On Sun, Feb 04, 2024 at 04:26:26PM +0800, Tong Tiangen wrote:
> diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
> index bca780fa5e57..b2cce1b6c96d 100644
> --- a/arch/x86/kernel/cpu/mce/severity.c
> +++ b/arch/x86/kernel/cpu/mce/severity.c
> @@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
>  	case EX_TYPE_UACCESS:
>  		if (!copy_user)
>  			return IN_KERNEL;
> +		fallthrough;
> +	case EX_TYPE_DEFAULT_MCE_SAFE:
>  		m->kflags |= MCE_IN_KERNEL_COPYIN;
>  		fallthrough;

I knew something was still bugging me here and this is still wrong.

Let's imagine this flow:

copy_mc_to_user() - note *src is kernel memory
|-> copy_mc_enhanced_fast_string or copy_mc_fragile - it's the same thing
  |-> -#MC, exception type EX_TYPE_DEFAULT_MCE_SAFE
    |-> error_context():
       case EX_TYPE_DEFAULT_MCE_SAFE:
                m->kflags |= MCE_IN_KERNEL_COPYIN;

MCE_IN_KERNEL_COPYIN does kill_me_never():

	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);

but that's reading from kernel memory!

IOW, I *think* that switch statement should be this:

	switch (fixup_type) {
	case EX_TYPE_UACCESS:
	case EX_TYPE_DEFAULT_MCE_SAFE:
		if (!copy_user)
			return IN_KERNEL;

		m->kflags |= MCE_IN_KERNEL_COPYIN;
		fallthrough;

	case EX_TYPE_FAULT_MCE_SAFE:
		m->kflags |= MCE_IN_KERNEL_RECOV;
		return IN_KERNEL_RECOV;

	default:
		return IN_KERNEL;
	}

Provided I'm not missing a case and provided is_copy_from_user() really
detects all cases properly.

And then patch 3 is wrong because we only can handle "copy in" - not
just any copy.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

在 2024/2/7 20:29, Borislav Petkov 写道:
> On Sun, Feb 04, 2024 at 04:26:26PM +0800, Tong Tiangen wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
>> index bca780fa5e57..b2cce1b6c96d 100644
>> --- a/arch/x86/kernel/cpu/mce/severity.c
>> +++ b/arch/x86/kernel/cpu/mce/severity.c
>> @@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
>>   	case EX_TYPE_UACCESS:
>>   		if (!copy_user)
>>   			return IN_KERNEL;
>> +		fallthrough;
>> +	case EX_TYPE_DEFAULT_MCE_SAFE:
>>   		m->kflags |= MCE_IN_KERNEL_COPYIN;
>>   		fallthrough;
> 
> I knew something was still bugging me here and this is still wrong.
> 
> Let's imagine this flow:
> 
> copy_mc_to_user() - note *src is kernel memory
> |-> copy_mc_enhanced_fast_string or copy_mc_fragile - it's the same thing
>    |-> -#MC, exception type EX_TYPE_DEFAULT_MCE_SAFE
>      |-> error_context():
>         case EX_TYPE_DEFAULT_MCE_SAFE:
>                  m->kflags |= MCE_IN_KERNEL_COPYIN;
> 
> MCE_IN_KERNEL_COPYIN does kill_me_never():
> 
> 	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
> 
> but that's reading from kernel memory!
> 
> IOW, I *think* that switch statement should be this:
> 
> 	switch (fixup_type) {
> 	case EX_TYPE_UACCESS:
> 	case EX_TYPE_DEFAULT_MCE_SAFE:
> 		if (!copy_user)
> 			return IN_KERNEL;
> 
> 		m->kflags |= MCE_IN_KERNEL_COPYIN;
> 		fallthrough;
> 
> 	case EX_TYPE_FAULT_MCE_SAFE:
> 		m->kflags |= MCE_IN_KERNEL_RECOV;
> 		return IN_KERNEL_RECOV;
> 
> 	default:
> 		return IN_KERNEL;
> 	}
> 
> Provided I'm not missing a case and provided is_copy_from_user() really
> detects all cases properly.
> 
> And then patch 3 is wrong because we only can handle "copy in" - not
> just any copy.

That makes sense. I'll check that point out later.

Many thanks.
Tong.
> 
> Thx.
> 

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

在 2024/2/7 20:29, Borislav Petkov 写道:
> On Sun, Feb 04, 2024 at 04:26:26PM +0800, Tong Tiangen wrote:
>> diff --git a/arch/x86/kernel/cpu/mce/severity.c b/arch/x86/kernel/cpu/mce/severity.c
>> index bca780fa5e57..b2cce1b6c96d 100644
>> --- a/arch/x86/kernel/cpu/mce/severity.c
>> +++ b/arch/x86/kernel/cpu/mce/severity.c
>> @@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, struct pt_regs *regs)
>>   	case EX_TYPE_UACCESS:
>>   		if (!copy_user)
>>   			return IN_KERNEL;
>> +		fallthrough;
>> +	case EX_TYPE_DEFAULT_MCE_SAFE:
>>   		m->kflags |= MCE_IN_KERNEL_COPYIN;
>>   		fallthrough;
> 
> I knew something was still bugging me here and this is still wrong.
> 
> Let's imagine this flow:
> 
> copy_mc_to_user() - note *src is kernel memory
> |-> copy_mc_enhanced_fast_string or copy_mc_fragile - it's the same thing
>    |-> -#MC, exception type EX_TYPE_DEFAULT_MCE_SAFE
>      |-> error_context():
>         case EX_TYPE_DEFAULT_MCE_SAFE:
>                  m->kflags |= MCE_IN_KERNEL_COPYIN;
> 
> MCE_IN_KERNEL_COPYIN does kill_me_never():
> 
> 	pr_err("Kernel accessed poison in user space at %llx\n", p->mce_addr);
> 
> but that's reading from kernel memory!

Hi:

1. The copy_mc_to_kernel() is used in the coredump, KSM, and COW
scenarios, in these scenarios, the src mem stores the user data and the
kernel use kernel address to access the src mem(using kmap()).

2. the src mem of copy_mc_to_user() is currently only used by the DAX:

   dax_iomap_iter()
     -> dax_copy_to_iter()
       -> _copy_mc_to_iter
         -> copy_to_user_iter_mc()
           -> copy_mc_to_user()

DAX is also used to store user data，such as pmem，pmem uses the kernel
address to access src mem(memremap_pages()):

   pmem_attach_disk()
     -> devm_memremap_pages()
       -> memremap_pages()

3. EX_TYPE_DEFAULT_MCE_SAFE is only used in
copy_mc_to_user()/copy_mc_to_kernel()。

4. Therefore, for EX_TYPE_DEFAULT_MCE_SAFE, the memory page where the
hardware error occurs stores user data, these page can be securely
isolated. This is different from UACCESS, which can be securely isolated
only COPYIN(the src mem is user data) is checked.

Based on the above understanding, I think the original logic should be
fine, except for the pr_err() in kill_me_never().

Thanks.
Tong.

> 
> IOW, I *think* that switch statement should be this:
> 
> 	switch (fixup_type) {
> 	case EX_TYPE_UACCESS:
> 	case EX_TYPE_DEFAULT_MCE_SAFE:
> 		if (!copy_user)
> 			return IN_KERNEL;
> 
> 		m->kflags |= MCE_IN_KERNEL_COPYIN;
> 		fallthrough;
> 
> 	case EX_TYPE_FAULT_MCE_SAFE:
> 		m->kflags |= MCE_IN_KERNEL_RECOV;
> 		return IN_KERNEL_RECOV;
> 
> 	default:
> 		return IN_KERNEL;
> 	}
> 
> Provided I'm not missing a case and provided is_copy_from_user() really
> detects all cases properly.
> 
> And then patch 3 is wrong because we only can handle "copy in" - not
> just any copy.
> 
> Thx.
> 

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

Hi Petkov：

	Kindly ping...

Thanks,
Tong.

在 2024/2/18 18:08, Tong Tiangen 写道:
> 
> 
> 在 2024/2/7 20:29, Borislav Petkov 写道:
>> On Sun, Feb 04, 2024 at 04:26:26PM +0800, Tong Tiangen wrote:
>>> diff --git a/arch/x86/kernel/cpu/mce/severity.c 
>>> b/arch/x86/kernel/cpu/mce/severity.c
>>> index bca780fa5e57..b2cce1b6c96d 100644
>>> --- a/arch/x86/kernel/cpu/mce/severity.c
>>> +++ b/arch/x86/kernel/cpu/mce/severity.c
>>> @@ -292,11 +292,11 @@ static noinstr int error_context(struct mce *m, 
>>> struct pt_regs *regs)
>>>       case EX_TYPE_UACCESS:
>>>           if (!copy_user)
>>>               return IN_KERNEL;
>>> +        fallthrough;
>>> +    case EX_TYPE_DEFAULT_MCE_SAFE:
>>>           m->kflags |= MCE_IN_KERNEL_COPYIN;
>>>           fallthrough;
>>
>> I knew something was still bugging me here and this is still wrong.
>>
>> Let's imagine this flow:
>>
>> copy_mc_to_user() - note *src is kernel memory
>> |-> copy_mc_enhanced_fast_string or copy_mc_fragile - it's the same thing
>>    |-> -#MC, exception type EX_TYPE_DEFAULT_MCE_SAFE
>>      |-> error_context():
>>         case EX_TYPE_DEFAULT_MCE_SAFE:
>>                  m->kflags |= MCE_IN_KERNEL_COPYIN;
>>
>> MCE_IN_KERNEL_COPYIN does kill_me_never():
>>
>>     pr_err("Kernel accessed poison in user space at %llx\n", 
>> p->mce_addr);
>>
>> but that's reading from kernel memory!
> 
> Hi:
> 
> 1. The copy_mc_to_kernel() is used in the coredump, KSM, and COW
> scenarios, in these scenarios, the src mem stores the user data and the
> kernel use kernel address to access the src mem(using kmap()).
> 
> 2. the src mem of copy_mc_to_user() is currently only used by the DAX:
> 
>    dax_iomap_iter()
>      -> dax_copy_to_iter()
>        -> _copy_mc_to_iter
>          -> copy_to_user_iter_mc()
>            -> copy_mc_to_user()
> 
> DAX is also used to store user data，such as pmem，pmem uses the kernel
> address to access src mem(memremap_pages()):
> 
>    pmem_attach_disk()
>      -> devm_memremap_pages()
>        -> memremap_pages()
> 
> 3. EX_TYPE_DEFAULT_MCE_SAFE is only used in
> copy_mc_to_user()/copy_mc_to_kernel()。
> 
> 4. Therefore, for EX_TYPE_DEFAULT_MCE_SAFE, the memory page where the
> hardware error occurs stores user data, these page can be securely
> isolated. This is different from UACCESS, which can be securely isolated
> only COPYIN(the src mem is user data) is checked.
> 
> Based on the above understanding, I think the original logic should be
> fine, except for the pr_err() in kill_me_never().
> 
> Thanks.
> Tong.
> 
>>
>> IOW, I *think* that switch statement should be this:
>>
>>     switch (fixup_type) {
>>     case EX_TYPE_UACCESS:
>>     case EX_TYPE_DEFAULT_MCE_SAFE:
>>         if (!copy_user)
>>             return IN_KERNEL;
>>
>>         m->kflags |= MCE_IN_KERNEL_COPYIN;
>>         fallthrough;
>>
>>     case EX_TYPE_FAULT_MCE_SAFE:
>>         m->kflags |= MCE_IN_KERNEL_RECOV;
>>         return IN_KERNEL_RECOV;
>>
>>     default:
>>         return IN_KERNEL;
>>     }
>>
>> Provided I'm not missing a case and provided is_copy_from_user() really
>> detects all cases properly.
>>
>> And then patch 3 is wrong because we only can handle "copy in" - not
>> just any copy.
>>
>> Thx.
>>

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Borislav Petkov]

On Sun, Feb 18, 2024 at 06:08:14PM +0800, Tong Tiangen wrote:
> 1. The copy_mc_to_kernel() is used in the coredump, KSM, and COW
> scenarios, in these scenarios, the src mem stores the user data and the
> kernel use kernel address to access the src mem(using kmap()).
> 
> 2. the src mem of copy_mc_to_user() is currently only used by the DAX:

You mean just because it currently is used somewhere which probably is
ok - no clue what DAX does - and even if the source address is still
*kernel* memory and even at the danger that someone else might use it in
the future and think the handling on a potential #MC is ok, you're still
arguing that this is the right thing to do perhaps because it fits your
use case?!

Sorry Tiangen, not gonna happen.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH -next v5 2/3] x86/mce: set MCE_IN_KERNEL_COPYIN for DEFAULT_MCE_SAFE exception

[Tong Tiangen]

在 2024/3/28 6:05, Borislav Petkov 写道:
> On Sun, Feb 18, 2024 at 06:08:14PM +0800, Tong Tiangen wrote:
>> 1. The copy_mc_to_kernel() is used in the coredump, KSM, and COW
>> scenarios, in these scenarios, the src mem stores the user data and the
>> kernel use kernel address to access the src mem(using kmap()).
>>
>> 2. the src mem of copy_mc_to_user() is currently only used by the DAX:
> 
> You mean just because it currently is used somewhere which probably is
> ok - no clue what DAX does - and even if the source address is still
> *kernel* memory and even at the danger that someone else might use it in
> the future and think the handling on a potential #MC is ok, you're still
> arguing that this is the right thing to do perhaps because it fits your
> use case?!
> 
> Sorry Tiangen, not gonna happen.
> 

I left the office last week and felt sorry for the lateness of the
reply.

You are right. Our current processing is based on "experience" rather
than interface constraints.

is_copy_from_user() determines whether a user is a "copy user" based on
fault_in_kernel_space(). Therefore, it returns false for
copy_mc_to_kernel()/copy_mc_to_user(). As a result, MCE_IN_KERNEL_COPYIN
cannot be set in error_context().

Comprehensive consideration of all factors, it is better to manually
call memory_failure_queue() to handle this problem case by case.

Finally, do we consider accepting only the patch 1/3 ?

Thanks,
Tong.