On 21.10.22 18:01, Peter Xu wrote:
On Fri, Oct 21, 2022 at 05:17:08PM +0200, David Hildenbrand wrote:
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 ;)
Very interesting to learn this, thanks for the pointer, David. :)
Definitely helpful to me on knowing better on the vma security model.
Though note that it'll be a different topic if we go back to the original
problem we're discussing - the fake-read approach of shmem will still keep
the hole in file which will still change the behavior of missing messages
from generating.
Said that, I don't really know whether there can be a real impact on any
uffd users, or anything else that similarly access the file cache.
One odd behavior I could think of is if one would have someone a process
A that uses uffd on a MAP_SHARED shmem and someone other process B
(e.g., with read-only permissions) have a MAP_PRIVATE mapping on the
same file.
A read (or a write) from process B to the private mapping would result
in process A not receiving uffd events.
Of course, the same would happen if you have multiple MAP_SHARED
mappings as well ... but it feels a bit weird being able to do that
without write permissions to the file.
--
Thanks,
David / dhildenb
