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

[Peter Xu <peterx@redhat.com>]

On Mon, Aug 10, 2020 at 01:51:49PM -0700, Linus Torvalds wrote:
> On Mon, Aug 10, 2020 at 12:15 PM Peter Xu <peterx@redhat.com> 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!

Disregarding the name that we'd prefer, I thought the new flag should only be
used internally in GUP just like FOLL_COW, or am I wrong?

Now FOLL_BREAK_COW (or whatever we'd like to call it) is applied as long as for
FOLL_PIN and FOLL_GET irrelevant to who calls __get_user_pages().  Shouldn't
that be enough that the new flag will be applied correctly always?...

I'd be fine if you want me to rename the flag into FOLL_READ_WRONG_SIDE_COW_OK.
It's just a bit awkward to read and check, because otherwise for the COW path
we'll need to use "(FAULT_FLAG_WRITE || !FAULT_FLAG_READ_WRONG_SIDE_COW_OK)".

> 
> 
> > + * @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.

Yeah, that's why I totally agree we need to do enforced COW even for a read gup
as long as the page can be further referenced (GET|PIN).  However frankly
speaking I didn't follow the rest on what's wrong with "Userfaultfd-wp should
not care about this because it's not a write operation" that I mentiond.  Is
that the major part of the objection?

I'm afraid I still think that's the right thing to do...

Could you help elaborate more?  It would be better if you can help to point out
the code changes that are wrong or suspecious, maybe that'll help me to
understand what I might have missed too.

Thanks,

-- 
Peter Xu