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

[David Hildenbrand]

On 18.12.21 20:52, Linus Torvalds wrote:
> On Sat, Dec 18, 2021 at 11:21 AM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> To recap:
>>  (1) is important, and page_count() is the only thing that guarantees
>> "you get full access to a page only when it's *obviously* exclusively
>> yours".
>>  (2) is NOT important, but could be a performance issue, but we have
>> real data from the past year that it isn't.
>>  (3) is important, and has a really spectacularly simple conceptual
>> fix with quite simple code too.
>>
>> In contrast, with the "mapcount" games you can't even explain why they
>> should work, and the patches I see are actively buggy because
>> everything is so subtle.
> 
> So to challenge you, please explain exactly how mapcount works to
> solve (1) and (3), and how it incidentally guarantees that (2) doesn't
> happen.

Oh, there is no need for additional challenges, I've been challenged
with this problem for way too long already ;)

And again, I appreciate this discussion and your feedback. I want to get
all of this fixed ASAP, even if it's not going to be the way I propose
as I raised. Any input is appreciated (as long as people don't scream at
me).


To get to your point: I thought about your remarks with the swapcount
and it makes sense. The mapcount itself is not expressive enough to
catch macpount == 1 vs mapcount > 1.

What *would* work to have precise COW I think is having the active and
inactive count instead of just the active (mapcount) part, whereby:

active: page tables mapping this page -> mapcount
inactive: page tables referencing this page via swap/migration entry

An alternative would be to just know that there are inactive users. We'd
have to read both values atomically in one shot.

There would be ways to store that information in the _mapcount but it
certainly adds a lot of complexity, and ...

> 
> And that really involves explaining the actual code too. I can explain
> the high-level concepts in literally a couple of sentences.
> 
> For (1), "the page_count()==1 guarantees you are the only owner, so a
> COW event can re-use the page" really explains it. And the code is
> pretty simple too. There's nothing subtle about "goto copy" when
> pagecount is not 1. And even the locking is simple: "we hold the page
> table lock, we found a page, it has only one ref to it, we own it"
> 
> Our VM is *incredibly* complicated. There really are serious
> advantages to having simple rules in place.

... you have a point here.

Having that said, I hope we can agree that the "page_count" is not the
perfect solution. I hope we can at least tweak it for now to get rid of
3) Wrong COW.

> 
> And for (2), the simple rule is "yeah, we can cause spurious cow
> events". That's not only simple to explain, it's simple to code for.
> Suddenly you don't need to worry. "Copying the page is always safe".
> That's a really really powerful statement.
> 
> Now, admittedly (3) is the one that ends up being more complicated,
> but the *concept* sure is simple. "If you don't want to COW this page,
> then don't mark it for COW".
> 
> The *code* for (3) is admittedly a bit more complicated. The "don't
> mark it for COW" is simple to say, but we do have that fairly odd
> locking thing with fork() doing a seqcount_write_begin/end, and then
> GIP does the read-seqcount thing with retry. So it's a bit unusual,
> and I don't think we have that particular pattern anywhere else, but
> it's one well-defined lock and while unusual it's not *complicated* as
> far as kernel locking rules go. It's unusual and perhaps not trivial,
> but in the end those seqcount code sequences are maybe 10 lines total,
> and they don't interact with anything else.
> 
> And yes, the "don't mark it for COW" means that write-protecting
> something is special, mainly because we sadly do not have extra bits
> in the page tables. It would be *really* easy if we could just hide
> this "don't COW this page" in the page table. Truly trivial. We don't,
> because of portability across different architectures ;(
> 
> So I'll freely give you that my (3) is somewhat painful, but it's
> painful with a really simple concept.

Thanks for admitting that!

I might have had an idea yesterday on how to fix most of the issues
without relying on the mapcount, doing it similar (but slightly
different) as you propose here. Let's call it a mixture of the unsharing
approach and your approach. I cannot promise anything, so ...

... I'll go playing with it and share some details ASAP. At least it
sounds comparatively simple in my head.

> 
> And the places that get (3) wrong are generally places that nobody has
> been able to care about. I didn't realize the problem with creating a
> swap page after the fact for a while, so that commit feb889fb40fa
> ("mm: don't put pinned pages into the swap cache") came later, but
> it's literally a very simple two-liner.
> 

Just to give you my perspective:


Personally I don't care too much about 2). The only reason why I somehow
care about "Unnecessary COW" are
* Challenging for hugetlb use as I explained. We might still want to use
  the mapcount there.
* It's mostly a symptom of our eventually too simple COW logic that
  effectively leads to 3).

While I do care about 1) (Missed CoW) for our customers, I *especially*
care about 3) (Wrong Cow) simply because silent memory corruptions in
user space are not acceptable.

As you say, fixing 1) the "page_count" way might be easy, at least for THP.

Simple example: Have swapping enabled and register a fixed io_uring
buffer at the wrong time. Fixed io_uring buffers are no a commodity
feature for unprivileged users space ...

So that's why I so deeply care about all of this.

-- 
Thanks,

David / dhildenb