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 2:29 PM David Hildenbrand <david@redhat.com> wrote:
>
> While I do care about future use cases, I cannot possibly see fork() not
> requiring the mmap_lock in the foreseeable future. Just so much depends
> on it as of now.

It's not that *fork()* depends on it.

Of course fork() takes the mmap_sem.

It's that fast-gup really really doesn't want it, and can't take it.

So any fast-gup user fundamentally cannot look at mapcount(), because
that would be fundamentally wrong and racy, and could race with fork.

And yet, as far as I can tell, that's *exactly* what your gup patches
do, with gup_pte_range() adding

+               if (!pte_write(pte) && gup_must_unshare(flags, page, false)) {
+                       put_compound_head(head, 1, flags);
+                       goto pte_unmap;
+               }

which looks at the page mapcount without holding the mmap sem at all.

And see my other email - I think there are other examples of your
patches looking at data that isn't stable because you don't hold the
right locks.

And you can't even do the optimistic case without taking the lock,
because in your world, a COW that optimistically copies in the case of
a race condition is fundamentally *wrong* and buggy. Because in your
world-view, GUP and COW are very different and have different rules,
but you need things to be *exact*, and they aren't.

And none of this is anything at least I can think about, because I
don't see what the "design" is.

I really have a hard time following what the rules actually are. You
seem to think that "page_mapcount()" is a really simple rule, and I
fundamentally disagree. It's a _very_ complicated thing indeed, with
locking issues, AND YOU ACTIVELY VIOLATE THE LOCKING RULES!

See why I'm so unhappy?

We *did* do the page_mapcount() thing. It was bad. It forced COW to
always take the page lock. There's a very real reason why I'm pushing
my "let's have a _design_ here", instead of your "let's look at
page_mapcount without even doing the locking".

And yes, I *know* that fork() takes the mmap_sem, and likely always
will. That really isn't the problem here. The problem is that your
page_mapcount() paths DO NOT take that lock.

Btw, maybe I'm misreading things. I looked at the individual patches,
I didn't apply them, maybe I missed something. But I don't think I am.

             Linus