Re: [PATCH v2] mm: hwpoison: coredump: support recovery from dump_user_range()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2023/4/26 23:45, Luck, Tony wrote:
>>>> Thanks for your confirm, and what your option about add
>>>> MCE_IN_KERNEL_COPYIN to EX_TYPE_DEFAULT_MCE_SAFE/FAULT_MCE_SAFE type
>>>> to let do_machine_check call queue_task_work(&m, msg, kill_me_never),
>>>> which kill every call memory_failure_queue() after mc safe copy return?
>>>
>>> I haven't been following this thread closely. Can you give a link to the e-mail
>>> where you posted a patch that does this? Or just repost that patch if easier.
>>
>> The major diff changes is [1], I will post a formal patch when -rc1 out,
>> thanks.
>>
>> [1]
>> https://lore.kernel.org/linux-mm/6dc1b117-020e-be9e-7e5e-a349ffb7d00a@huawei.com/
> 
> There seem to be a few misconceptions in that message. Not sure if all of them
> were resolved.  Here are some pertinent points:
> 
>>>> In my understanding, an MCE should not be triggered when MC-safe copy
>>>> tries
>>>> to access to a memory error.  So I feel that we might be talking about
>>>> different scenarios.
> 
> This is wrong. There is still a machine check when a MC-safe copy does a read
> from a location that has a memory error.
> 
> The recovery flow in this case does not involve queue_task_work(). That is only
> useful for machine check exceptions taken in user context. The queued work will
> be executed to call memory_failure() from the kernel, but in process context (not
> from the machine check exception stack) to handle the error.
> 
> For machine checks taken by kernel code (MC-safe copy functions) the recovery
> path is here:
> 
>                  if (m.kflags & MCE_IN_KERNEL_RECOV) {
>                          if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
>                                  mce_panic("Failed kernel mode recovery", &m, msg);
>                  }
> 
>                  if (m.kflags & MCE_IN_KERNEL_COPYIN)
>                          queue_task_work(&m, msg, kill_me_never);
> 
> The "fixup_exception()" ensures that on return from the machine check handler
> code returns to the extable[] fixup location instead of the instruction that was
> loading from the memory error location.
> 
> When the exception was from one of the copy_from_user() variants it makes
> sense to also do the queue_task_work() because the kernel is going to return
> to the user context (with an EFAULT error code from whatever system call was
> attempting the copy_from_user()).
> 
> But in the core dump case there is no return to user. The process is being
> terminated by the signal that leads to this core dump. So even though you
> may consider the page being accessed to be a "user" page, you can't fix
> it by queueing work to run on return to user.

For coredump，the task work will be called too, see following code,

get_signal
	sig_kernel_coredump
		elf_core_dump
			dump_user_range
				_copy_from_iter // with MC-safe copy, return without panic
	do_group_exit(ksig->info.si_signo);
		do_exit
			exit_task_work
				task_work_run
					kill_me_never
						memory_failure

I also add debug print to check the memory_failure() processing after
add MCE_IN_KERNEL_COPYIN to MCE_SAFE exception type, also tested CoW of
normal page and huge page, it works too.

> 
> I don't have an well thought out suggestion on how to make sure that memory_failure()
> is called for the page in this case. Maybe the core dump code can check for the
> return from the MC-safe copy it is using and handle it in the error path?
> 

So we could safely add MCE_IN_KERNEL_COPYIN to all MCE_SAFE exception type?

The kernel diff based on next-20230425 shown bellow

diff --git a/arch/x86/kernel/cpu/mce/severity.c 
b/arch/x86/kernel/cpu/mce/severity.c
index c4477162c07d..63e94484c5d6 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -293,12 +293,11 @@ static noinstr int error_context(struct mce *m, 
struct pt_regs *regs)
  	case EX_TYPE_COPY:
  		if (!copy_user)
  			return IN_KERNEL;
-		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;
+		m->kflags |= MCE_IN_KERNEL_RECOV | MCE_IN_KERNEL_COPYIN;
  		return IN_KERNEL_RECOV;

  	default:
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 4509a566fe6c..59a7afe3dfce 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3667,23 +3667,19 @@ enum mf_flags {
  int mf_dax_kill_procs(struct address_space *mapping, pgoff_t index,
  		      unsigned long count, int mf_flags);
  extern int memory_failure(unsigned long pfn, int flags);
+extern void memory_failure_queue(unsigned long pfn, int flags);
  extern void memory_failure_queue_kick(int cpu);
  extern int unpoison_memory(unsigned long pfn);
  extern void shake_page(struct page *p);
  extern atomic_long_t num_poisoned_pages __read_mostly;
  extern int soft_offline_page(unsigned long pfn, int flags);
  #ifdef CONFIG_MEMORY_FAILURE
-extern void memory_failure_queue(unsigned long pfn, int flags);
  extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
  					bool *migratable_cleared);
  void num_poisoned_pages_inc(unsigned long pfn);
  void num_poisoned_pages_sub(unsigned long pfn, long i);
  struct task_struct *task_early_kill(struct task_struct *tsk, int 
force_early);
  #else
-static inline void memory_failure_queue(unsigned long pfn, int flags)
-{
-}
-
  static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int 
flags,
  					bool *migratable_cleared)
  {
diff --git a/mm/ksm.c b/mm/ksm.c
index 0156bded3a66..7abdf4892387 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2794,7 +2794,6 @@ struct page *ksm_might_need_to_copy(struct page *page,
  	if (new_page) {
  		if (copy_mc_user_highpage(new_page, page, address, vma)) {
  			put_page(new_page);
-			memory_failure_queue(page_to_pfn(page), 0);
  			return ERR_PTR(-EHWPOISON);
  		}
  		SetPageDirty(new_page);
diff --git a/mm/memory.c b/mm/memory.c
index 5e2c6b1fc00e..c0f586257017 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2814,10 +2814,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;
  	}

@@ -5852,10 +5850,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;
  }
@@ -5871,10 +5867,8 @@ static int copy_subpage(unsigned long addr, int 
idx, void *arg)
  	struct copy_subpage_arg *copy_arg = arg;

  	if (copy_mc_user_highpage(copy_arg->dst + idx, copy_arg->src + idx,
-				  addr, copy_arg->vma)) {
-		memory_failure_queue(page_to_pfn(copy_arg->src + idx), 0);
+				  addr, copy_arg->vma))
  		return -EHWPOISON;
-	}
  	return 0;
  }

-- 
2.35.3


Thanks,

> -Tony