lkml.org 
[lkml]   [2024]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
SubjectRe: [Question] CoW on VM_PFNMAP vma during write fault
From
On 27.02.24 14:00, David Hildenbrand wrote:
> On 27.02.24 13:28, Wupeng Ma wrote:
>> We find that a warn will be produced during our test, the detail log is
>> shown in the end.
>>
>> The core problem of this warn is that the first pfn of this pfnmap vma is
>> cleared during memory-failure. Digging into the source we find that this
>> problem can be triggered as following:
>>
>> // mmap with MAP_PRIVATE and specific fd which hook mmap
>> mmap(MAP_PRIVATE, fd)
>> __mmap_region
>> remap_pfn_range
>> // set vma with pfnmap and the prot of pte is read only
>>
>
> Okay, so we get a MAP_PRIVATE VM_PFNMAP I assume.
>
> What fd is that exactly? Often, we disallow private mappings in the
> mmap() callback (for a good reason).
>
>> // memset this memory with trigger fault
>> handle_mm_fault
>> __handle_mm_fault
>> handle_pte_fault
>> // write fault and !pte_write(entry)
>> do_wp_page
>> wp_page_copy // this will alloc a new page with valid page struct
>> // for this pfnmap vma
>
> Here we replace the mapped PFNMAP thingy by a proper anon folio.
>
>>
>> // inject a hwpoison to the first page of this vma
>
> I assume this is an anon folio?
>
>> madvise_inject_error
>> memory_failure
>> hwpoison_user_mappings
>> try_to_unmap_one
>> // mark this pte as invalid (hwpoison)
>> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
>> address, range.end);
>>
>> // during unmap vma, the first pfn of this pfnmap vma is invalid
>> vm_mmap_pgoff
>> do_mmap
>> __do_mmap_mm
>> __mmap_region
>> __do_munmap
>> unmap_region
>> unmap_vmas
>> unmap_single_vma
>> untrack_pfn
>> follow_phys // pte is already invalidate, WARN_ON here
>
> unmap_single_vma()->...->zap_pte_range() should do the right thing when
> calling vm_normal_page().
>
> untrack_pfn() is the problematic part.
>
>>
>> CoW with a valid page for pfnmap vma is weird to us. Can we use
>> remap_pfn_range for private vma(read only)? Once CoW happens on a pfnmap
>> vma during write fault, this page is normal(page flag is valid) for most mm
>> subsystems, such as memory failure in thais case and extra should be done to
>> handle this special page.
>>
>> During unmap, if this vma is pfnmap, unmap shouldn't be done since page
>> should not be touched for pfnmap vma.
>>
>> But the root problem is that can we insert a valid page for pfnmap vma?
>>
>> Any thoughts to solve this warn?
>
> vm_normal_page() documentation explains how that magic is supposed to
> work. vm_normal_page() should be able to correctly identify whether we
> want to look at the struct page for an anon folio that was COWed.
>
>
> untrack_pfn() indeed does not seem to be well prepared for handling
> MAP_PRIVATE mappings where we end up having anon folios.
>
> I think it will already *completely mess up* simply when unmapping the
> range without the memory failure involved.
>
> See, follow_phys() would get the PFN of the anon folio and then
> untrack_pfn() would do some nonesense with that. Completely broken.
>
> The WARN is just a side-effect of the brokenness.
>
> In follow_phys(), we'd likely have to call vm_normal_page(). If we get a
> page back, we'd likely have to fail follow_phys() instead of returning a
> PFN of an anon folio.
>
> Now, how do we fix untrack_pfn() ? I really don't know. In theory, we
> might no longer have *any* PFNMAP PFN in there after COW'ing everything.
>
> Sounds like MAP_PRIVATE VM_PFNMAP + __HAVE_PFNMAP_TRACKING is some
> broken garbage (sorry). Can we disallow it?

Staring at track_pfn_copy(), it's maybe similarly broken?

I think we want to do:

diff --git a/mm/memory.c b/mm/memory.c
index 098356b8805ae..da5d1e37c5534 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6050,6 +6050,10 @@ int follow_phys(struct vm_area_struct *vma,
goto out;
pte = ptep_get(ptep);

+ /* Never return addresses of COW'ed anon folios. */
+ if (vm_normal_page(vma, address, pte))
+ goto unlock;
+
if ((flags & FOLL_WRITE) && !pte_write(pte))
goto unlock;

And then, just disallow it with PAT involved:

diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 0904d7e8e1260..e4d2b2e8c0281 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -997,6 +997,15 @@ int track_pfn_remap(struct vm_area_struct *vma, pgprot_t *prot,
&& size == (vma->vm_end - vma->vm_start))) {
int ret;

+ /*
+ * untrack_pfn() and friends cannot handl regions that suddenly
+ * contain anon folios after COW. In particular, follow_phys()
+ * will fail when we have an anon folio at the beginning og the
+ * VMA.
+ */
+ if (vma && is_cow_mapping(vma->vm_flags))
+ return -EINVAL;
+
ret = reserve_pfn_range(paddr, size, prot, 0);
if (ret == 0 && vma)
vm_flags_set(vma, VM_PAT);

I'm afraid that will break something. But well, it's already semi-broken.

As long as VM_PAT is not involved, it should work as expected.

In an ideal world, we'd get rid of follow_phys() completely and just
derive that information from the VMA?

--
Cheers,

David / dhildenb


\
 
 \ /
  Last update: 2024-05-27 15:24    [W:0.107 / U:0.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site