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

["HORIGUCHI NAOYA(堀口 直也)" <naoya.horiguchi@nec.com>]

On Fri, Apr 21, 2023 at 01:43:39PM +0800, Kefeng Wang wrote:
...
> > > > > > 
> > > > > > Another question, other copy_mc_to_kernel() callers, eg,
> > > > > > nvdimm/dm-writecache/dax, there are not call memory_failure_queue(),
> > > > > > should they need a memory_failure_queue(), if so, why not add it into
> > > > > > do_machine_check() ?
> > > > > 
> > > > 
> > > > What I mean is that EX_TYPE_DEFAULT_MCE_SAFE/EX_TYPE_FAULT_MCE_SAFE
> > > > is designed to identify fixups which allow in kernel #MC recovery,
> > > > that is, the caller of copy_mc_to_kernel() must know the source
> > > > is a user address, so we could add a MCE_IN_KERNEL_COPYIN fro
> > > > the MCE_SAFE type.
> > > 
> > > And I think we need the following change for MCE_SAFE copy to set
> > > MCE_IN_KERNEL_COPYIN.
> > > 
> > > > 
> > > > 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;
> > 
> > This change seems to not related to what you try to fix.
> > Could this break some other workloads like copying from user address?
> > 
> 
> Yes, this move MCE_IN_KERNEL_COPYIN set into next case, both COPY and
> MCE_SAFE type will set MCE_IN_KERNEL_COPYIN, for EX_TYPE_COPY, we don't
> break it.
> 
> 
> > > >                   fallthrough;

Sorry, I overlooked this fallthrough. So this change is fine to me.

> > > > 
> > > >           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:
> > > > 
> > > > then we could drop memory_failure_queue(pfn, flags) from cow/ksm copy,
> > > > or every Machine Check safe memory copy will need a memory_failure_xx()
> > > > call.
> > > 
> > > which help use to kill unneeded memory_failure_queue() call, any comments?
> > 
> > I'm not 100% sure that we can safely use queue_task_work() instead of
> > memory_failure_queue() (due to the difference between workqueue and task
> > work, which should be recently discussed in thread [1]).  So I prefer to
> > keep the approach of memory_failure_queue() to keep the impact minimum.
> > 
> 
> +tony for x86 mce
> 
> The x86 call queue_task_work() for EX_TYPE_COPY, so EX_TYPE_FAULT_MCE_SAFE
> and EX_TYPE_DEFAULT_MCE_SAFE should be similar to EX_TYPE_COPY,
> memcpy_mc_xxx return bytes not copied, let the task to decide
> what to do next, and call memory_failure(pfn, 0) to isolate
> the poisoned page.
> 
> 1) queue_task_work() will make the memory_failure() called before
> return-to-user
> 2) memory_failure_queue() called in COW will put the work on a specific
> cpu(current task is running), and memory_failure() will be called in
> the work. see more from commit d302c2398ba2 ("mm, hwpoison: when copy-
> on-write hits poison, take page offline"),  "It is important, but not
> urgent, to mark the source page as h/w poisoned and unmap it from other
> tasks."
> 
> Both of them just wants to isolate memory, they shouldn't add action,
> they set flag=0 for memory_failure(). so preliminarily, there are not
> different.

Thanks, sounds good to me.

- Naoya Horiguchi

> 
> 
> 
> > [1] https://lore.kernel.org/lkml/20230417011407.58319-1-xueshuai@linux.alibaba.com/T/#u
> > 
> 
> The COPY_MC support on arm64 is still under review[1],  xueshuai's patch
> is only trying to fix the uncorrected si_code of synchronous exceptions
> when memory error occurred, so I think it is not involved the COPY_MC.