Re: [PATCH v7 1/2] mm/khugepaged: recover from poisoned anonymous memory

[Jiaqi Yan <jiaqiyan@google.com>]

On Fri, Nov 18, 2022 at 8:46 AM Luck, Tony <tony.luck@intel.com> wrote:
>
> +/*
> + * Machine check exception handled version of copy_highpage.
> + * Return true if copying page content failed; otherwise false.
> + * Note handling #MC requires arch opt-in.
> + */
> +static inline bool copy_highpage_mc(struct page *to, struct page *from)
> +{
> +       char *vfrom, *vto;
> +       unsigned long ret;
> +
> +       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 > 0;
> +}
>
> Andrew Morton took my patches to recover from copy-on-write faults
> into the -mm tree. They are now sitting in linux-next (hopefully) ready
> for the next merge window.
>
> I had copied this function from an earlier version of your patch, but
> there was a suggestion that copy_mc_user_highpage() would be a
> more consistent name with other copy code that can handle machine
> checks. There was also an upstream change to copy_highpage()
> that needed to be reflected here.

Thanks for pointing this out, Tony. Is it the kmsan_copy_page_meta(to,
from) call in copy_highpage()? I will make sure to include it in
copy_highpage_mc(). If there is more, can you provide lore pointers?

>
> Net result, this version is already in flight:
>
> static inline int copy_mc_user_highpage(struct page *to, struct page *from,
>                                         unsigned long vaddr, struct vm_area_struct *vma)
> {
>         unsigned long ret;
>         char *vfrom, *vto;
>
>         vfrom = kmap_local_page(from);
>         vto = kmap_local_page(to);
>         ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
>         if (!ret)
>                 kmsan_unpoison_memory(page_address(to), PAGE_SIZE);

Does it make sense to kmsan_unpoison_memory in copy_highpage? (and in
copy_mc_highpage if no MC happens?)

>         kunmap_local(vto);
>         kunmap_local(vfrom);
>
>         return ret;
> }
>
> -Tony

While I am ok to switch to copy_mc_user_highpage for this commit
(given __collapse_huge_page_copy originally uses copy_user_highpage),
the question is, Yang Shi suggested in a previous version to unify the
copy routine used by both __collapse_huge_page_copy (using
copy_user_highpage) and collapse_file (using copy_highpage). I prefer
MC-aware copy_highpage because collapse_file may have a hard time to
adapt to the vaddr and vma in copy_user_highpage's interface, and
neither collapse_file nor __collapse_huge_page_copy gain anything from
passing vaddr and vma (correct me if I am wrong).

So I would still prefer copy_mc_highpage (after renaming and adding
kmsan_xxx calls) to be used by both __collapse_huge_page_copy and
collapse_file. What are your opinions, Tony and Yang?