Re: [PATCH v2] mm/gup: Allow real explicit breaking of COW

[Linus Torvalds]

On Mon, Aug 10, 2020 at 12:15 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> My previous understanding was that although COW is always safe, we should still
> avoid it when unnecessary because it's still expensive.  Currently we will do
> enforced COW only if should_force_cow_break() returns true, which seems to be a
> good justification of when to ask for the enforcement.

It's not the _enforcement_ I worry about.

It's the _users_.

So COW is always the safe thing to do, but as you say, it can be
expensive (although we actually seemed to get a robot that claimed it
sped up some test benchmark, it wasn't clear why).

But because not breaking COW can be very subtly unsafe - considering
that we had this behavior for over two decades - I think the users
that decide to not break COW need to explain why it is safe for them.

See what I'm saying?

That's why an explicit  FOLL_READ_WRONG_SIDE_COW_OK flag would be
good, because it would force people to *think* about what they are
doing, and explain why it's ok to do that unsafe thing, and get a page
that may actually end up not being your page at all!


> + * @FAULT_FLAG_BREAK_COW: Do COW explicitly for the fault (even for read).
> + *                        Please read FOLL_BREAK_COW for more information.

This comment is useless - because it's aimed at the wrong people.

It's aimed at the people who already know they want to break COW. They
understand, and they are doing the safe thing.

The case I worry about is the people who do NOT say "break COW". Not
because they are smart and know what they are doing, but because they
don't think about the issue.

> Userfaultfd-wp should not care about this because it's not a write operation,

Ok, I will *definitely* not be applying tyhis patch, beause you don't
even understand what the problem is.

The fact is, A READ operation that doesn't break COW can GET THE WRONG ANSWER.

Why?

If you have two (threaded) processes A and B, who have that shared COW
page, what can happen is:

 - A does a READ operation using get_follow_page, and gets the shared page

 - another thread in A ends up doing an unmap operation

 - B does write to the page, and causes a COW, but now doesn't see the
A mapping at all any more, so takes over the page and writes to it

 - the original get_follow_page() thread in A is now holding a page
that contains data that wasn't written by A

See? That's the whole point. It doesn't _matter_ if you're only
reading the data, without the COW you may be reading the _wrong_ data.

So no. I will not be applying the patch, because you apparently didn't
understand why reading needs to do a COW.

                     Linus