Re: [PATCH v1 06/11] mm: support GUP-triggered unsharing via FAULT_FLAG_UNSHARE (!hugetlb)

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Dec 17, 2021 at 1:47 PM David Hildenbrand <david@redhat.com> wrote:
>
> For now I have not heard a compelling argument why the mapcount is
> dubious, I repeat:
>
> * mapcount can only increase due to fork()
> * mapcount can decrease due to unmap / zap

And to answer the "why is this dubious", let' sjust look at your
actual code that I reacted to:

+       vmf->page = vm_normal_page(vmf->vma, vmf->address, vmf->orig_pte);
+       if (vmf->page && PageAnon(vmf->page) && !PageKsm(vmf->page) &&
+           page_mapcount(vmf->page) > 1) {

Note how you don't just check page_mapcount(). Why not? Because
mapcount is completely immaterial if it's not a PageAnon page, so you
test for that.

So even when you do the mapcount read as one atomic thing, it's one
atomic thing that depends on _other_ things, and all these checks are
not atomic.

But a PageAnon() page can actually become a swap-backed page, and as
far as I can tell, your code doesn't have any locking to protect
against that.

So now you need not only the mmap_sem (to protect against fork), you
also need the page lock (to protect against rmap changing the type of
page).

I don't see you taking the page lock anywhere. Maybe the page table
lock ends up serializing sufficiently with the rmap code that it ends
up working

In the do_wp_page() path, we currently do those kinds of racy checks
too, but then we do a trylock_page, and re-do them. And at any time
there is any question about things, we fall back to copying - because
a copy is always safe.

Well, it's always safe if we have the rule that "once we've pinned
things, we don't cause them to be COW again".

But that "it's safe if" was exactly my (b) case.

That's why I much prefer the model I'm trying to push - it's
conceptually quite simple. I can literally explain mine at a
conceptual level with that "break pre-existing COW, make sure no
future COW" model.

In contrast, I look at your page_mapcount() code, and I go "there is
no conceptual rules here, and the actual implementation details look
dodgy".

I personally like having clear conceptual rules - as opposed to random
implementation details.

             Linus