Re: Avoiding allocation of unused shmem page

David Hildenbrand
2022-10-21T15:17:18Z

On 21.10.22 17:08, Peter Xu wrote:
> On Fri, Oct 21, 2022 at 04:45:27PM +0200, David Hildenbrand wrote:
>> On 21.10.22 16:28, Peter Xu wrote:
>>> On Fri, Oct 21, 2022 at 04:10:41PM +0200, David Hildenbrand wrote:
>>>> On 21.10.22 16:01, Peter Xu wrote:
>>>>> On Fri, Oct 21, 2022 at 09:23:00AM +0200, David Hildenbrand wrote:
>>>>>> On 20.10.22 23:10, Peter Xu wrote:
>>>>>>> On Thu, Oct 20, 2022 at 09:14:09PM +0100, Matthew Wilcox wrote:
>>>>>>>> In yesterday's call, David brought up the case where we fallocate a file
>>>>>>>> in shmem, call mmap(MAP_PRIVATE) and then store to a page which is over
>>>>>>>> a hole. That currently causes shmem to allocate a page, zero-fill it,
>>>>>>>> then COW it, resulting in two pages being allocated when only the
>>>>>>>> COW page really needs to be allocated.
>>>>>>>>
>>>>>>>> The path we currently take through the MM when we take the page fault
>>>>>>>> looks like this (correct me if I'm wrong ...):
>>>>>>>>
>>>>>>>> handle_mm_fault()
>>>>>>>> __handle_mm_fault()
>>>>>>>> handle_pte_fault()
>>>>>>>> do_fault()
>>>>>>>> do_cow_fault()
>>>>>>>> __do_fault()
>>>>>>>> vm_ops->fault()
>>>>>>>>
>>>>>>>> ... which is where we come into shmem_fault(). Apart from the
>>>>>>>> horrendous hole-punch handling case, shmem_fault() is quite simple:
>>>>>>>>
>>>>>>>> err = shmem_get_folio_gfp(inode, vmf->pgoff, &folio, SGP_CACHE,
>>>>>>>> gfp, vma, vmf, &ret);
>>>>>>>> if (err)
>>>>>>>> return vmf_error(err);
>>>>>>>> vmf->page = folio_file_page(folio, vmf->pgoff);
>>>>>>>> return ret;
>>>>>>>>
>>>>>>>> What we could do here is detect this case. Something like:
>>>>>>>>
>>>>>>>> enum sgp_type sgp = SGP_CACHE;
>>>>>>>>
>>>>>>>> if ((vmf->flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED))
>>>>>>>> sgp = SGP_READ;
>>>>>>>
>>>>>>> Yes this will start to save the space, but just to mention this may start
>>>>>>> to break anything that will still depend on the pagecache to work. E.g.,
>>>>>>> it'll change behavior if the vma is registered with uffd missing mode;
>>>>>>> we'll start to lose MISSING events for these private mappings. Not sure
>>>>>>> whether there're other side effects.
>>>>>>
>>>>>> I don't follow, can you elaborate?
>>>>>>
>>>>>> hugetlb doesn't perform this kind of unnecessary allocation and should be fine in regards to uffd. Why should it matter here and how exactly would a problematic sequence look like?
>>>>>
>>>>> Hugetlb is special because hugetlb detects pte first and relies on pte at
>>>>> least for uffd. shmem is not.
>>>>>
>>>>> Feel free to also reference the recent fix which relies on the stable
>>>>> hugetlb pte with commit 2ea7ff1e39cbe375.
>>>>
>>>> Sorry to be dense here, but I don't follow how that relates.
>>>>
>>>> Assume we have a MAP_PRIVATE shmem mapping and someone registers uffd
>>>> missing events on that mapping.
>>>>
>>>> Assume we get a page fault on a hole. We detect no page is mapped and check
>>>> if the page cache has a page mapped -- which is also not the case, because
>>>> there is a hole.
>>>>
>>>> So we notify uffd.
>>>>
>>>> Uffd will place a page. It should *not* touch the page cache and only insert
>>>> that page into the page table -- otherwise we'd be violating MAP_PRIVATE
>>>> semantics.
>>>
>>> That's actually exactly what we do right now... we insert into page cache
>>> for the shmem. See shmem_mfill_atomic_pte().
>>>
>>> Why it violates MAP_PRIVATE? Private pages only guarantee the exclusive
>>> ownership of pages, I don't see why it should restrict uffd behavior. Uffd
>>> missing mode (afaiu) is defined to resolve page cache missings in this
>>> case. Hugetlb is special but not shmem IMO comparing to most of the rest
>>> of the file systems.
>>
>> If a write (or uffd placement) via a MAP_PRIVATE mapping results in other
>> shared/private mappings from observing these modifications, you have a clear
>> violation of MAP_PRIVATE semantics.
>
> I think I understand what you meant, but just to mention again that I think
> it's a matter of how we defined the uffd missing semantics when shmem
> missing mode was introduced years ago. It does not need to be the same
> semantic as writting directly to a private mapping.
>

I think uffd does exactly the right thing in mfill_atomic_pte:

/*
* The normal page fault path for a shmem will invoke the
* fault, fill the hole in the file and COW it right away. The
* result generates plain anonymous memory. So when we are
* asked to fill an hole in a MAP_PRIVATE shmem mapping, we'll
* generate anonymous memory directly without actually filling
* the hole. For the MAP_PRIVATE case the robustness check
* only happens in the pagetable (to verify it's still none)
* and not in the radix tree.
*/
if (!(dst_vma->vm_flags & VM_SHARED)) {
if (mode == MCOPY_ATOMIC_NORMAL)
err = mcopy_atomic_pte(dst_mm, dst_pmd, dst_vma,
dst_addr, src_addr, page,
wp_copy);
else
err = mfill_zeropage_pte(dst_mm, dst_pmd,
dst_vma, dst_addr);
} else {
err = shmem_mfill_atomic_pte(dst_mm, dst_pmd, dst_vma,
dst_addr, src_addr,
mode != MCOPY_ATOMIC_NORMAL,
wp_copy, page);
}

Unless we have a writable shared mapping, we end up not touching the pagecache.

After what I understand from your last message (maybe I understood it wrong),
I tried exploiting uffd behavior by writing into a hole of a file without
write permissions using uffd. I failed because it does the right thing ;)

--
Thanks,

David / dhildenb