Tony Luck wrote:
> If the kernel is copying a page as the result of a copy-on-write
> fault and runs into an uncorrectable error, Linux will crash because
> it does not have recovery code for this case where poison is consumed
> by the kernel.
>
> It is easy to set up a test case. Just inject an error into a private
> page, fork(2), and have the child process write to the page.
>
> I wrapped that neatly into a test at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/aegl/ras-tools.git
>
> just enable ACPI error injection and run:
>
> # ./einj_mem-uc -f copy-on-write
>
> Add a new copy_user_highpage_mc() function that uses copy_mc_to_kernel()
> on architectures where that is available (currently x86 and powerpc).
> When an error is detected during the page copy, return VM_FAULT_HWPOISON
> to caller of wp_page_copy(). This propagates up the call stack. Both x86
> and powerpc have code in their fault handler to deal with this code by
> sending a SIGBUS to the application.
>
> Note that this patch avoids a system crash and signals the process that
> triggered the copy-on-write action. It does not take any action for the
> memory error that is still in the shared page. To handle that a call to
> memory_failure() is needed. But this cannot be done from wp_page_copy()
> because it holds mmap_lock(). Perhaps the architecture fault handlers
> can deal with this loose end in a subsequent patch?
>
> On Intel/x86 this loose end will often be handled automatically because
> the memory controller provides an additional notification of the h/w
> poison in memory, the handler for this will call memory_failure(). This
> isn't a 100% solution. If there are multiple errors, not all may be
> logged in this way.
>
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
Just some minor comments below, but you can add:
Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>
> ---
> Changes in V2:
> Naoya Horiguchi:
> 1) Use -EHWPOISON error code instead of minus one.
> 2) Poison path needs also to deal with old_page
> Tony Luck:
> Rewrote commit message
> Added some powerpc folks to Cc: list
> ---
> include/linux/highmem.h | 19 +++++++++++++++++++
> mm/memory.c | 28 +++++++++++++++++++---------
> 2 files changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/highmem.h b/include/linux/highmem.h
> index e9912da5441b..5967541fbf0e 100644
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -319,6 +319,25 @@ static inline void copy_user_highpage(struct page *to, struct page *from,
>
> #endif
>
> +static inline int copy_user_highpage_mc(struct page *to, struct page *from,
> + unsigned long vaddr, struct vm_area_struct *vma)
> +{
> + unsigned long ret = 0;
> +#ifdef copy_mc_to_kernel
> + char *vfrom, *vto;
> +
> + vfrom = kmap_local_page(from);
> + vto = kmap_local_page(to);
> + ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
> + kunmap_local(vto);
> + kunmap_local(vfrom);
> +#else
> + copy_user_highpage(to, from, vaddr, vma);
> +#endif
> +
> + return ret;
> +}
> +
There is likely some small benefit of doing this the idiomatic way and
let grep see that there are multiple definitions of
copy_user_highpage_mc() with an organization like:
#ifdef copy_mc_to_kernel
static inline int copy_user_highpage_mc(struct page *to, struct page *from,
unsigned long vaddr,
struct vm_area_struct *vma)
{
unsigned long ret = 0;
char *vfrom, *vto;
vfrom = kmap_local_page(from);
vto = kmap_local_page(to);
ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
kunmap_local(vto);
kunmap_local(vfrom);
return ret;
}
#else
static inline int copy_user_highpage_mc(struct page *to, struct page *from,
unsigned long vaddr,
struct vm_area_struct *vma)
{
copy_user_highpage(to, from, vaddr, vma);
return 0;
}
#endif
Per the copy_mc* discussion with Linus I would have called this function
copy_mc_to_user_highpage() to clarify that hwpoison is handled from the
source buffer of the copy.
> #ifndef __HAVE_ARCH_COPY_HIGHPAGE
>
> static inline void copy_highpage(struct page *to, struct page *from)
> diff --git a/mm/memory.c b/mm/memory.c
> index f88c351aecd4..a32556c9b689 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2848,8 +2848,14 @@ static inline int pte_unmap_same(struct vm_fault *vmf)
> return same;
> }
>
> -static inline bool __wp_page_copy_user(struct page *dst, struct page *src,
> - struct vm_fault *vmf)
> +/*
> + * Return:
> + * -EHWPOISON: copy failed due to hwpoison in source page
> + * 0: copied failed (some other reason)
> + * 1: copied succeeded
> + */
> +static inline int __wp_page_copy_user(struct page *dst, struct page *src,
> + struct vm_fault *vmf)
> {
> bool ret;
> void *kaddr;
> @@ -2860,8 +2866,9 @@ static inline bool __wp_page_copy_user(struct page *dst, struct page *src,
> unsigned long addr = vmf->address;
>
> if (likely(src)) {
> - copy_user_highpage(dst, src, addr, vma);
> - return true;
> + if (copy_user_highpage_mc(dst, src, addr, vma))
> + return -EHWPOISON;
Given there is no use case for the residue value returned by
copy_mc_to_kernel() perhaps just return EHWPOISON directly from
copyuser_highpage_mc() in the short-copy case?
> + return 1;
> }
>
> /*
> @@ -2888,7 +2895,7 @@ static inline bool __wp_page_copy_user(struct page *dst, struct page *src,
> * and update local tlb only
> */
> update_mmu_tlb(vma, addr, vmf->pte);
> - ret = false;
> + ret = 0;
What do you think about just making these 'false' cases also return a
negative errno? (rationale below...)
> goto pte_unlock;
> }
>
> @@ -2913,7 +2920,7 @@ static inline bool __wp_page_copy_user(struct page *dst, struct page *src,
> if (!likely(pte_same(*vmf->pte, vmf->orig_pte))) {
> /* The PTE changed under us, update local tlb */
> update_mmu_tlb(vma, addr, vmf->pte);
> - ret = false;
> + ret = 0;
> goto pte_unlock;
> }
>
> @@ -2932,7 +2939,7 @@ static inline bool __wp_page_copy_user(struct page *dst, struct page *src,
> }
> }
>
> - ret = true;
> + ret = 1;
>
> pte_unlock:
> if (locked)
> @@ -3104,6 +3111,7 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
> pte_t entry;
> int page_copied = 0;
> struct mmu_notifier_range range;
> + int ret;
>
> delayacct_wpcopy_start();
>
> @@ -3121,19 +3129,21 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
> if (!new_page)
> goto oom;
>
> - if (!__wp_page_copy_user(new_page, old_page, vmf)) {
> + ret = __wp_page_copy_user(new_page, old_page, vmf);
> + if (ret <= 0) {
...this would become a typical '0 == success' and 'negative errno ==
failure', where all but EHWPOISON are retried.
> /*
> * COW failed, if the fault was solved by other,
> * it's fine. If not, userspace would re-fault on
> * the same address and we will handle the fault
> * from the second attempt.
> + * The -EHWPOISON case will not be retried.
> */
> put_page(new_page);
> if (old_page)
> put_page(old_page);
>
> delayacct_wpcopy_end();
> - return 0;
> + return ret == -EHWPOISON ? VM_FAULT_HWPOISON : 0;
