Re: [RFC v2 PATCH 00/17] variable-order, large folios for anonymous memory

David Hildenbrand
2023-04-19T10:51:12Z

> I'm looking to fix this problem in my code, but am struggling to see how the
> current code is safe. I'm thinking about the following scenario:
>

Let's see :)

> - A page is CoW mapped into processes A and B.
> - The page takes a fault in process A, and do_wp_page() determines that it is
> "maybe-shared" and therefore must copy. So drops the PTL and calls
> wp_page_copy().

Note that before calling wp_page_copy(), we do a folio_get(folio);
Further, the page table reference is only dropped once we actually
replace the page in the page table. So while in wp_page_copy(), the
folio should have at least 2 references if the page is still mapped.

> - Process B exits.
> - Another thread in process A faults on the page. This time dw_wp_page()
> determines that the page is exclusive (due to the ref count), and reuses it,
> marking it exclusive along the way.

The refcount should not be 1 (other reference from the wp_page_copy()
caller), so A won't be able to reuse it, and ...

> - wp_page_copy() from the original thread in process A retakes the PTL and
> copies the _now exclusive_ page.
>
> Having typed it up, I guess this can't happen, because wp_page_copy() will only
> do the copy if the PTE hasn't changed and it will have changed because it is now
> writable? So this is safe?

this applies as well. If the pte changed (when reusing due to a write
failt it's now writable, or someone else broke COW), we back off. For
FAULT_FLAG_UNSHARE, however, the PTE may not change. But the additional
reference should make it work.

I think it works as intended. It would be clearer if we'd also recheck
in wp_page_copy() whether we still don't have an exclusive anon page
under PT lock -- and if we would, back off.

>
> To make things more convoluted, what happens if the second thread does an
> mprotect() to make the page RO after its write fault was handled? I think
> mprotect() will serialize on the mmap write lock so this is safe too?

Yes, mprotect() synchronizes that. There are other mechanisms to
write-protect a page, though, under mmap lock in read mode (uffd-wp). So
it's a valid concern.

In all of these cases, reuse should be prevented due to the additional
reference on the folio when entering wp_page_copy() right from the
start, not turning the page exclusive but instead replacing it by a
copy. An additional sanity check sounds like the right thing to do.

>
> Sorry if this is a bit rambly, just trying to make sure I've understood
> everything correctly.

It's a very interesting corner case, thanks for bringing that up. I
think the old mapcount based approach could have suffered from this
theoretical issue, but I might be wrong.

--
Thanks,

David / dhildenb