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

[Jiaqi Yan <jiaqiyan@google.com>]

On Tue, Feb 28, 2023 at 5:40 AM <kirill@shutemov.name> wrote:
>
> On Wed, Feb 08, 2023 at 03:00:59PM -0800, Jiaqi Yan wrote:
> > On Wed, Feb 8, 2023 at 3:45 AM Alexander Potapenko <glider@google.com> wrote:
> > >
> > > On Tue, Feb 7, 2023 at 7:20 PM Jiaqi Yan <jiaqiyan@google.com> wrote:
> > > >
> > > > Pinging KMSAN experts, for the general guidance of
> > > > kmsan_copy_page_meta vs kmsan_unpoison_memory
> > >
> > > Oh, sorry, I've missed the previous email.
> >
> > NP, thanks for jumping in :)
> >
> > >
> > > copy_mc_user_highpage() is expected to copy data from the user page,
> > > for which no metadata is ever allocated.
> > > Therefore we just initialize the destination shadow with zeros instead
> > > of copying anything.
> > >
> > > kmsan_copy_page_meta() is used when the metadata is copied between two
> > > kernel pages, therefore it handles the cases when page->kmsan_shadow
> > > is NULL for the source and destination pages.
> > >
> > > It might be a good idea to use kmsan_copy_page_meta() in both cases,
> > > but to do that I want to better understand what happens when
> > > kmap_local_page(from) is called in copy_mc_user_highpage().
> > > Where does the corresponding struct page come from?
> >
> > Kirill can correct me, but I think khugepaged always copies user pages
> > because it is trying to convert raw pages to THP for better userspace
> > application performance. Therefore khugepaged should only need
> > copy_mc_user_highpage(), for both file-backed and anonymous memory
> > pages.
> >
> > However, copy_mc_user_highpage() needs both vaddr and vma, so it is a
> > little bit hard to use it in collapse_file (i.e. in the file-backed
> > case):
> > 1. vma is not carried over to collapse_file from khugepaged_scan_mm_slot
> > 2. collapse_file is not directly iterating with vaddrs of pages to be copied
> >
> > (Although both vaddr and vma are unused auguments in
> > copy_mc_user_highpage, I think for cleanness, the caller e.g.
> > khugepaged should feed valid values).
>
> It is not unused for !copy_mc_to_kernel case (basically everything but x86
> and power). And it is used to flush caches. The fact that you don't have
> it in your implementation *may* indicate a problem.

ah, I agree this looks problematic. Maybe I should give up on unifying
the copying routine used by anon and file memory:
* for anon, switch to copy_mc_user_highpage, which "works" for all
architectures, but #MC is only recoverable on x86 and powerpc.
* for shmem, extend copy_highpage to copy_mc_highpage, like how Tony
makes copy_mc_user_highpage.

>
> > So my patchset uses copy_mc_page(and kmsan_copy_page_meta) for both
> > file-backed and anon memory pages. I guess as long as
> > kmsan_copy_page_meta doesn't do anything unexpected for user pages (at
> > least from my reading), we are good?
> >
>
> --
>   Kiryl Shutsemau / Kirill A. Shutemov