* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-22 3:19 ` Andy Lutomirski
0 siblings, 0 replies; 141+ messages in thread
From: Andy Lutomirski @ 2020-12-22 3:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Andy Lutomirski, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 3:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Dec 21, 2020 at 2:30 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > AFAIU mprotect() is the only one who modifies the pte using the mmap write
> > lock. NUMA balancing is also using read mmap lock when changing pte
> > protections, while my understanding is mprotect() used write lock only because
> > it manipulates the address space itself (aka. vma layout) rather than modifying
> > the ptes, so it needs to.
>
> So it's ok to change the pte holding only the PTE lock, if it's a
> *one*way* conversion.
>
> That doesn't break the "re-check the PTE contents" model (which
> predates _all_ of the rest: NUMA, userfaultfd, everything - it's
> pretty much the original model for our page table operations, and goes
> back to the dark ages even before SMP and the existence of a page
> table lock).
>
> So for example, a COW will always create a different pte (not just
> because the page number itself changes - you could imagine a page
> getting re-used and changing back - but because it's always a RO->RW
> transition).
>
> So two COW operations cannot "undo" each other and fool us into
> thinking nothing changed.
>
> Anything that changes RW->RO - like fork(), for example - needs to
> take the mmap_lock.
Ugh, this is unpleasantly complicated. I will admit that any API that
takes an address and more-or-less-blindly marks it RO makes me quite
nervous even assuming all the relevant locks are held. At least
userfaultfd refuses to operate on VM_SHARED VMAs, but we have another
instance of this (with mmap_sem held for write) in x86:
mark_screen_rdonly(). Dare I ask how broken this is? We could likely
get away with deleting it entirely.
--Andy
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 3:19 ` Andy Lutomirski
@ 2020-12-22 4:16 ` Linus Torvalds
-1 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-22 4:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 7:19 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> Ugh, this is unpleasantly complicated.
I probably should have phrased it differently, because the case you
quote is actually a good one:
> I will admit that any API that
> takes an address and more-or-less-blindly marks it RO makes me quite
> nervous even assuming all the relevant locks are held. At least
> userfaultfd refuses to operate on VM_SHARED VMAs, but we have another
> instance of this (with mmap_sem held for write) in x86:
> mark_screen_rdonly(). Dare I ask how broken this is? We could likely
> get away with deleting it entirely.
So I think the basic rule is that "if you hold mmap_sem for writing,
you're always safe". And that really should be considered the
"default" locking.
ANY time you make a modification to the VM layer, you should basically
always treat it as a write operation, and get the mmap_sem for
writing.
Yeah, yeah, that's a bit simplified, and it ignores various special
cases (and the hardware page table walkers that obviously take no
locks at all), but if you hold the mmap_sem for writing you won't
really race with anything else - not page faults, and not other
"modify this VM".
So mark_screen_rdonly() looks trivially fine to me. I don't think it
really necessarily matters any more, and it's a legacy thing for a
legacy hardware issue, but I don't think there's any reason at all to
remove it either.
To a first approximation, everybody that changes the VM should take
the mmap_sem for writing, and the readers should just be just about
page fault handling (and I count GUP as "page fault handling" too -
it's kind of the same "look up page" rather than "modify vm" kind of
operation).
And there are just a _lot_ more page faults than there are things that
modify the page tables and the vma's.
So having that mental model of "lookup of pages in a VM take mmap_semn
for reading, any modification of the VM uses it for writing" makes
sense both from a performance angle and a logical standpoint. It's the
correct model.
And it's worth noting that COW is still "lookup of pages", even though
it might modify the page tables in the process. The same way lookup
can modify the page tables to mark things accessed or dirty.
So COW is still a lookup operation, in ways that "change the
writabiility of this range" very much is not. COW is "lookup for
write", and the magic we do to copy to make that write valid is still
all about the lookup of the page.
Which brings up another mental mistake I saw earlier in this thread:
you should not think "mmap_sem is for vma, and the page table lock is
for the page table changes".
mmap_sem is the primary lock for any modifications to the VM layout,
whether it be in the vma's or in the page tables.
Now, the page table lock does exist _in_addition_to_ the mmap_sem, but
it is partly because
(a) we have things that historically walked the page tables _without_
walking the vma's (notably the virtual memory scanning)
(b) we do allow concurrent page faults, so we then need a lower-level
lock to serialize the parallelism we _do_ have.
And yes, we have ended up allowing some of those "modify the VM state"
to then take the mmap_sem for reading, just because their
modifications are slight enough that we can then say "ok, this is a
write modification, but the writes it does are protected by the page
table lock, we'll just get the mmap_sem for reading". It's an
optimization, but it should be considered exactly that: not
fundamental, but just a clever trick and an optimization.
It's why I went "userfaultfd is buggy" immediately when I noticed. It
basically became clear that "oh, that's an optimization, but it's an
*invalid* one", in that it didn't actually work and give the same end
result.
So when I said:
> Anything that changes RW->RO - like fork(), for example - needs to
> take the mmap_lock.
I didn't mean that we should consider that RW->RO change to be this
complex semantic marker that we should honor and say "ok, because it's
a RW->RO change, we need to hold the mmap_sem".
I phrased it really badly, in other words.
What I *should* have said is that *because* userfaultfd is changing
the VM layout, it should always act as if it had to take the mmap_sem
for writing, and that the RW->RO change is an example of when
downgrading that write-lock to a read lock is simply not valid -
because it basically breaks the rules about what a lookup (ie a read)
can do.
A lookup can never turn a writable page non-writable. A lookup -
through COW - _can_ turn a read-only page writable. So that's why
"RO->RW" can be ok under the read lock, but RW->RO is not.
Does that clarify the situation when I phrase it that way instead?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-22 4:16 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-22 4:16 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 7:19 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> Ugh, this is unpleasantly complicated.
I probably should have phrased it differently, because the case you
quote is actually a good one:
> I will admit that any API that
> takes an address and more-or-less-blindly marks it RO makes me quite
> nervous even assuming all the relevant locks are held. At least
> userfaultfd refuses to operate on VM_SHARED VMAs, but we have another
> instance of this (with mmap_sem held for write) in x86:
> mark_screen_rdonly(). Dare I ask how broken this is? We could likely
> get away with deleting it entirely.
So I think the basic rule is that "if you hold mmap_sem for writing,
you're always safe". And that really should be considered the
"default" locking.
ANY time you make a modification to the VM layer, you should basically
always treat it as a write operation, and get the mmap_sem for
writing.
Yeah, yeah, that's a bit simplified, and it ignores various special
cases (and the hardware page table walkers that obviously take no
locks at all), but if you hold the mmap_sem for writing you won't
really race with anything else - not page faults, and not other
"modify this VM".
So mark_screen_rdonly() looks trivially fine to me. I don't think it
really necessarily matters any more, and it's a legacy thing for a
legacy hardware issue, but I don't think there's any reason at all to
remove it either.
To a first approximation, everybody that changes the VM should take
the mmap_sem for writing, and the readers should just be just about
page fault handling (and I count GUP as "page fault handling" too -
it's kind of the same "look up page" rather than "modify vm" kind of
operation).
And there are just a _lot_ more page faults than there are things that
modify the page tables and the vma's.
So having that mental model of "lookup of pages in a VM take mmap_semn
for reading, any modification of the VM uses it for writing" makes
sense both from a performance angle and a logical standpoint. It's the
correct model.
And it's worth noting that COW is still "lookup of pages", even though
it might modify the page tables in the process. The same way lookup
can modify the page tables to mark things accessed or dirty.
So COW is still a lookup operation, in ways that "change the
writabiility of this range" very much is not. COW is "lookup for
write", and the magic we do to copy to make that write valid is still
all about the lookup of the page.
Which brings up another mental mistake I saw earlier in this thread:
you should not think "mmap_sem is for vma, and the page table lock is
for the page table changes".
mmap_sem is the primary lock for any modifications to the VM layout,
whether it be in the vma's or in the page tables.
Now, the page table lock does exist _in_addition_to_ the mmap_sem, but
it is partly because
(a) we have things that historically walked the page tables _without_
walking the vma's (notably the virtual memory scanning)
(b) we do allow concurrent page faults, so we then need a lower-level
lock to serialize the parallelism we _do_ have.
And yes, we have ended up allowing some of those "modify the VM state"
to then take the mmap_sem for reading, just because their
modifications are slight enough that we can then say "ok, this is a
write modification, but the writes it does are protected by the page
table lock, we'll just get the mmap_sem for reading". It's an
optimization, but it should be considered exactly that: not
fundamental, but just a clever trick and an optimization.
It's why I went "userfaultfd is buggy" immediately when I noticed. It
basically became clear that "oh, that's an optimization, but it's an
*invalid* one", in that it didn't actually work and give the same end
result.
So when I said:
> Anything that changes RW->RO - like fork(), for example - needs to
> take the mmap_lock.
I didn't mean that we should consider that RW->RO change to be this
complex semantic marker that we should honor and say "ok, because it's
a RW->RO change, we need to hold the mmap_sem".
I phrased it really badly, in other words.
What I *should* have said is that *because* userfaultfd is changing
the VM layout, it should always act as if it had to take the mmap_sem
for writing, and that the RW->RO change is an example of when
downgrading that write-lock to a read lock is simply not valid -
because it basically breaks the rules about what a lookup (ie a read)
can do.
A lookup can never turn a writable page non-writable. A lookup -
through COW - _can_ turn a read-only page writable. So that's why
"RO->RW" can be ok under the read lock, but RW->RO is not.
Does that clarify the situation when I phrase it that way instead?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 4:16 ` Linus Torvalds
@ 2020-12-22 20:19 ` Andy Lutomirski
-1 siblings, 0 replies; 141+ messages in thread
From: Andy Lutomirski @ 2020-12-22 20:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 8:16 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Dec 21, 2020 at 7:19 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > Ugh, this is unpleasantly complicated.
>
> What I *should* have said is that *because* userfaultfd is changing
> the VM layout, it should always act as if it had to take the mmap_sem
> for writing, and that the RW->RO change is an example of when
> downgrading that write-lock to a read lock is simply not valid -
> because it basically breaks the rules about what a lookup (ie a read)
> can do.
>
> A lookup can never turn a writable page non-writable. A lookup -
> through COW - _can_ turn a read-only page writable. So that's why
> "RO->RW" can be ok under the read lock, but RW->RO is not.
>
> Does that clarify the situation when I phrase it that way instead?
It's certainly more clear, but I'm still not thrilled with a bunch of
functions in different files maintained by different people that
cooperate using an undocumented lockless protocol. It makes me
nervous from a maintainability standpoint even if the code is, in
fact, entirely correct.
But I didn't make my question clear either: when I asked about
mark_screen_rdonly(), I wasn't asking about locking or races at all.
mark_screen_rdonly() will happily mark *any* PTE readonly. I can
easily believe that writable mappings of non-shared mappings all
follow the same CoW rules in the kernel and that, assuming all the
locking is correct, marking them readonly is conceptually okay. But
shared mappings are a whole different story. Is it actually the case
that all, or even most, drivers and other sources of shared mappings
will function correctly if one of their PTEs becomes readonly behind
their back? Or maybe there are less bizarre ways for this to happen
without vm86 shenanigans and this is completely safe after all.
P.S. This whole mess reminds me that I need to take a closer look at
the upcoming SHSTK code. Shadow stack pages violate all common sense
about how the various PTE bits work.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-22 20:19 ` Andy Lutomirski
0 siblings, 0 replies; 141+ messages in thread
From: Andy Lutomirski @ 2020-12-22 20:19 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 8:16 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Dec 21, 2020 at 7:19 PM Andy Lutomirski <luto@kernel.org> wrote:
> >
> > Ugh, this is unpleasantly complicated.
>
> What I *should* have said is that *because* userfaultfd is changing
> the VM layout, it should always act as if it had to take the mmap_sem
> for writing, and that the RW->RO change is an example of when
> downgrading that write-lock to a read lock is simply not valid -
> because it basically breaks the rules about what a lookup (ie a read)
> can do.
>
> A lookup can never turn a writable page non-writable. A lookup -
> through COW - _can_ turn a read-only page writable. So that's why
> "RO->RW" can be ok under the read lock, but RW->RO is not.
>
> Does that clarify the situation when I phrase it that way instead?
It's certainly more clear, but I'm still not thrilled with a bunch of
functions in different files maintained by different people that
cooperate using an undocumented lockless protocol. It makes me
nervous from a maintainability standpoint even if the code is, in
fact, entirely correct.
But I didn't make my question clear either: when I asked about
mark_screen_rdonly(), I wasn't asking about locking or races at all.
mark_screen_rdonly() will happily mark *any* PTE readonly. I can
easily believe that writable mappings of non-shared mappings all
follow the same CoW rules in the kernel and that, assuming all the
locking is correct, marking them readonly is conceptually okay. But
shared mappings are a whole different story. Is it actually the case
that all, or even most, drivers and other sources of shared mappings
will function correctly if one of their PTEs becomes readonly behind
their back? Or maybe there are less bizarre ways for this to happen
without vm86 shenanigans and this is completely safe after all.
P.S. This whole mess reminds me that I need to take a closer look at
the upcoming SHSTK code. Shadow stack pages violate all common sense
about how the various PTE bits work.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 4:16 ` Linus Torvalds
(?)
(?)
@ 2021-01-05 15:37 ` Peter Zijlstra
2021-01-05 18:03 ` Andrea Arcangeli
2021-01-12 11:43 ` Vinayak Menon
-1 siblings, 2 replies; 141+ messages in thread
From: Peter Zijlstra @ 2021-01-05 15:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon
On Mon, Dec 21, 2020 at 08:16:11PM -0800, Linus Torvalds wrote:
> So I think the basic rule is that "if you hold mmap_sem for writing,
> you're always safe". And that really should be considered the
> "default" locking.
>
> ANY time you make a modification to the VM layer, you should basically
> always treat it as a write operation, and get the mmap_sem for
> writing.
>
> Yeah, yeah, that's a bit simplified, and it ignores various special
> cases (and the hardware page table walkers that obviously take no
> locks at all), but if you hold the mmap_sem for writing you won't
> really race with anything else - not page faults, and not other
> "modify this VM".
> To a first approximation, everybody that changes the VM should take
> the mmap_sem for writing, and the readers should just be just about
> page fault handling (and I count GUP as "page fault handling" too -
> it's kind of the same "look up page" rather than "modify vm" kind of
> operation).
>
> And there are just a _lot_ more page faults than there are things that
> modify the page tables and the vma's.
>
> So having that mental model of "lookup of pages in a VM take mmap_semn
> for reading, any modification of the VM uses it for writing" makes
> sense both from a performance angle and a logical standpoint. It's the
> correct model.
> And it's worth noting that COW is still "lookup of pages", even though
> it might modify the page tables in the process. The same way lookup
> can modify the page tables to mark things accessed or dirty.
>
> So COW is still a lookup operation, in ways that "change the
> writabiility of this range" very much is not. COW is "lookup for
> write", and the magic we do to copy to make that write valid is still
> all about the lookup of the page.
(your other email clarified this point; the COW needs to copy while
holding the PTL and we need TLBI under PTL if we're to change this)
> Which brings up another mental mistake I saw earlier in this thread:
> you should not think "mmap_sem is for vma, and the page table lock is
> for the page table changes".
>
> mmap_sem is the primary lock for any modifications to the VM layout,
> whether it be in the vma's or in the page tables.
>
> Now, the page table lock does exist _in_addition_to_ the mmap_sem, but
> it is partly because
>
> (a) we have things that historically walked the page tables _without_
> walking the vma's (notably the virtual memory scanning)
>
> (b) we do allow concurrent page faults, so we then need a lower-level
> lock to serialize the parallelism we _do_ have.
And I'm thinking the speculative page fault series steps right into all
this, it fundamentally avoids mmap_sem and entirely relies on the PTL.
Which opens it up to exactly these races explored here.
The range lock approach does not suffer this, but I'm still worried
about the actual performance of that thing.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-05 15:37 ` Peter Zijlstra
@ 2021-01-05 18:03 ` Andrea Arcangeli
2021-01-12 16:20 ` Peter Zijlstra
2021-01-12 11:43 ` Vinayak Menon
1 sibling, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2021-01-05 18:03 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Linus Torvalds, Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon
On Tue, Jan 05, 2021 at 04:37:27PM +0100, Peter Zijlstra wrote:
> (your other email clarified this point; the COW needs to copy while
> holding the PTL and we need TLBI under PTL if we're to change this)
The COW doesn't need to hold the PT lock, the TLBI broadcast doesn't
need to be delivered under PT lock either.
Simply there need to be a TLBI broadcast before the copy. The patch I
sent here https://lkml.kernel.org/r/X+QLr1WmGXMs33Ld@redhat.com that
needs to be cleaned up with some abstraction and better commentary
also misses a smp_mb() in the case flush_tlb_page is not called, but
that's a small detail.
> And I'm thinking the speculative page fault series steps right into all
> this, it fundamentally avoids mmap_sem and entirely relies on the PTL.
I thought about that but that only applies to some kind of "anon" page
fault.
Here the problem isn't just the page fault, the problem is not to
regress clear_refs to block on page fault I/O, and all
MAP_PRIVATE/MAP_SHARED filebacked faults bitting the disk to read
/usr/ will still prevent clear_refs from running (and the other way
around) if it has to take the mmap_sem for writing.
I don't look at the speculative page fault for a while but last I
checked there was nothing there that can tame the above major
regression from CPU speed to disk I/O speed that would be inflicted on
both clear_refs on huge mm and on uffd-wp.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-05 18:03 ` Andrea Arcangeli
@ 2021-01-12 16:20 ` Peter Zijlstra
0 siblings, 0 replies; 141+ messages in thread
From: Peter Zijlstra @ 2021-01-12 16:20 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Linus Torvalds, Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon
On Tue, Jan 05, 2021 at 01:03:48PM -0500, Andrea Arcangeli wrote:
> On Tue, Jan 05, 2021 at 04:37:27PM +0100, Peter Zijlstra wrote:
> > (your other email clarified this point; the COW needs to copy while
> > holding the PTL and we need TLBI under PTL if we're to change this)
>
> The COW doesn't need to hold the PT lock, the TLBI broadcast doesn't
> need to be delivered under PT lock either.
>
> Simply there need to be a TLBI broadcast before the copy. The patch I
> sent here https://lkml.kernel.org/r/X+QLr1WmGXMs33Ld@redhat.com that
> needs to be cleaned up with some abstraction and better commentary
> also misses a smp_mb() in the case flush_tlb_page is not called, but
> that's a small detail.
That's horrific crap. All of that tlb-pending stuff is batshit, and this
makes it worse.
> > And I'm thinking the speculative page fault series steps right into all
> > this, it fundamentally avoids mmap_sem and entirely relies on the PTL.
>
> I thought about that but that only applies to some kind of "anon" page
> fault.
That must be something new; it used to handle all faults. I specifically
spend quite a bit of time getting the file crud right (which Linus
initially fingered for being horrible broken).
SPF fundamentally elides the mmap_sem, which Linus said must serialize
faults.
> Here the problem isn't just the page fault, the problem is not to
> regress clear_refs to block on page fault I/O, and all
IIRC we do the actual reads without any locks held, just like
VM_FAULT_RETRY does today. You take the fault, find you need IO, drop
locks, do IO, retake fault.
> MAP_PRIVATE/MAP_SHARED filebacked faults bitting the disk to read
> /usr/ will still prevent clear_refs from running (and the other way
> around) if it has to take the mmap_sem for writing.
>
> I don't look at the speculative page fault for a while but last I
> checked there was nothing there that can tame the above major
> regression from CPU speed to disk I/O speed that would be inflicted on
> both clear_refs on huge mm and on uffd-wp.
All of the clear_refs nonsense is immaterial to SPF. Also, who again
cares about clear_refs? Why is it important?
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-05 15:37 ` Peter Zijlstra
2021-01-05 18:03 ` Andrea Arcangeli
@ 2021-01-12 11:43 ` Vinayak Menon
2021-01-12 15:47 ` Laurent Dufour
1 sibling, 1 reply; 141+ messages in thread
From: Vinayak Menon @ 2021-01-12 11:43 UTC (permalink / raw)
To: Peter Zijlstra, Linus Torvalds
Cc: Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, ldufour, surenb
On 1/5/2021 9:07 PM, Peter Zijlstra wrote:
> On Mon, Dec 21, 2020 at 08:16:11PM -0800, Linus Torvalds wrote:
>
>> So I think the basic rule is that "if you hold mmap_sem for writing,
>> you're always safe". And that really should be considered the
>> "default" locking.
>>
>> ANY time you make a modification to the VM layer, you should basically
>> always treat it as a write operation, and get the mmap_sem for
>> writing.
>>
>> Yeah, yeah, that's a bit simplified, and it ignores various special
>> cases (and the hardware page table walkers that obviously take no
>> locks at all), but if you hold the mmap_sem for writing you won't
>> really race with anything else - not page faults, and not other
>> "modify this VM".
>> To a first approximation, everybody that changes the VM should take
>> the mmap_sem for writing, and the readers should just be just about
>> page fault handling (and I count GUP as "page fault handling" too -
>> it's kind of the same "look up page" rather than "modify vm" kind of
>> operation).
>>
>> And there are just a _lot_ more page faults than there are things that
>> modify the page tables and the vma's.
>>
>> So having that mental model of "lookup of pages in a VM take mmap_semn
>> for reading, any modification of the VM uses it for writing" makes
>> sense both from a performance angle and a logical standpoint. It's the
>> correct model.
>> And it's worth noting that COW is still "lookup of pages", even though
>> it might modify the page tables in the process. The same way lookup
>> can modify the page tables to mark things accessed or dirty.
>>
>> So COW is still a lookup operation, in ways that "change the
>> writabiility of this range" very much is not. COW is "lookup for
>> write", and the magic we do to copy to make that write valid is still
>> all about the lookup of the page.
> (your other email clarified this point; the COW needs to copy while
> holding the PTL and we need TLBI under PTL if we're to change this)
>
>> Which brings up another mental mistake I saw earlier in this thread:
>> you should not think "mmap_sem is for vma, and the page table lock is
>> for the page table changes".
>>
>> mmap_sem is the primary lock for any modifications to the VM layout,
>> whether it be in the vma's or in the page tables.
>>
>> Now, the page table lock does exist _in_addition_to_ the mmap_sem, but
>> it is partly because
>>
>> (a) we have things that historically walked the page tables _without_
>> walking the vma's (notably the virtual memory scanning)
>>
>> (b) we do allow concurrent page faults, so we then need a lower-level
>> lock to serialize the parallelism we _do_ have.
> And I'm thinking the speculative page fault series steps right into all
> this, it fundamentally avoids mmap_sem and entirely relies on the PTL.
>
> Which opens it up to exactly these races explored here.
>
> The range lock approach does not suffer this, but I'm still worried
> about the actual performance of that thing.
Some thoughts on why there may not be an issue with speculative page fault.
Adding Laurent as well.
Possibility of race against other PTE modifiers
1) Fork - We have seen a case of SPF racing with fork marking PTEs RO
and that
is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
2) mprotect - change_protection in mprotect which does the deferred flush is
marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
on those VMAs.
3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
But SPF does not take UFFD faults.
4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
(2) above.
5) Concurrent faults - SPF does not handle all faults. Only anon page
faults.
Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
transitions without tlb flush. And I hope do_wp_page with RO->RW is fine
as well.
I could not see a case where speculative path cannot see a PTE update
done via
a fault on another CPU.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 11:43 ` Vinayak Menon
@ 2021-01-12 15:47 ` Laurent Dufour
2021-01-12 16:57 ` Peter Zijlstra
0 siblings, 1 reply; 141+ messages in thread
From: Laurent Dufour @ 2021-01-12 15:47 UTC (permalink / raw)
To: Vinayak Menon, Peter Zijlstra, Linus Torvalds
Cc: Andy Lutomirski, Peter Xu, Nadav Amit, Yu Zhao, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, surenb
Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
> On 1/5/2021 9:07 PM, Peter Zijlstra wrote:
>> On Mon, Dec 21, 2020 at 08:16:11PM -0800, Linus Torvalds wrote:
>>
>>> So I think the basic rule is that "if you hold mmap_sem for writing,
>>> you're always safe". And that really should be considered the
>>> "default" locking.
>>>
>>> ANY time you make a modification to the VM layer, you should basically
>>> always treat it as a write operation, and get the mmap_sem for
>>> writing.
>>>
>>> Yeah, yeah, that's a bit simplified, and it ignores various special
>>> cases (and the hardware page table walkers that obviously take no
>>> locks at all), but if you hold the mmap_sem for writing you won't
>>> really race with anything else - not page faults, and not other
>>> "modify this VM".
>>> To a first approximation, everybody that changes the VM should take
>>> the mmap_sem for writing, and the readers should just be just about
>>> page fault handling (and I count GUP as "page fault handling" too -
>>> it's kind of the same "look up page" rather than "modify vm" kind of
>>> operation).
>>>
>>> And there are just a _lot_ more page faults than there are things that
>>> modify the page tables and the vma's.
>>>
>>> So having that mental model of "lookup of pages in a VM take mmap_semn
>>> for reading, any modification of the VM uses it for writing" makes
>>> sense both from a performance angle and a logical standpoint. It's the
>>> correct model.
>>> And it's worth noting that COW is still "lookup of pages", even though
>>> it might modify the page tables in the process. The same way lookup
>>> can modify the page tables to mark things accessed or dirty.
>>>
>>> So COW is still a lookup operation, in ways that "change the
>>> writabiility of this range" very much is not. COW is "lookup for
>>> write", and the magic we do to copy to make that write valid is still
>>> all about the lookup of the page.
>> (your other email clarified this point; the COW needs to copy while
>> holding the PTL and we need TLBI under PTL if we're to change this)
>>
>>> Which brings up another mental mistake I saw earlier in this thread:
>>> you should not think "mmap_sem is for vma, and the page table lock is
>>> for the page table changes".
>>>
>>> mmap_sem is the primary lock for any modifications to the VM layout,
>>> whether it be in the vma's or in the page tables.
>>>
>>> Now, the page table lock does exist _in_addition_to_ the mmap_sem, but
>>> it is partly because
>>>
>>> (a) we have things that historically walked the page tables _without_
>>> walking the vma's (notably the virtual memory scanning)
>>>
>>> (b) we do allow concurrent page faults, so we then need a lower-level
>>> lock to serialize the parallelism we _do_ have.
>> And I'm thinking the speculative page fault series steps right into all
>> this, it fundamentally avoids mmap_sem and entirely relies on the PTL.
>>
>> Which opens it up to exactly these races explored here.
>>
>> The range lock approach does not suffer this, but I'm still worried
>> about the actual performance of that thing.
>
>
> Some thoughts on why there may not be an issue with speculative page fault.
> Adding Laurent as well.
>
> Possibility of race against other PTE modifiers
>
> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
> 2) mprotect - change_protection in mprotect which does the deferred flush is
> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults on those
> VMAs.
> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
> But SPF does not take UFFD faults.
> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
> (2) above.
> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
> I could not see a case where speculative path cannot see a PTE update done via
> a fault on another CPU.
>
Thanks Vinayak,
You explained it fine. Indeed SPF is handling deferred TLB invalidation by
marking the VMA through vm_write_begin/end(), as for the fork case you
mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
values read are valid.
Cheers,
Laurent.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 15:47 ` Laurent Dufour
@ 2021-01-12 16:57 ` Peter Zijlstra
2021-01-12 19:02 ` Laurent Dufour
0 siblings, 1 reply; 141+ messages in thread
From: Peter Zijlstra @ 2021-01-12 16:57 UTC (permalink / raw)
To: Laurent Dufour
Cc: Vinayak Menon, Linus Torvalds, Andy Lutomirski, Peter Xu,
Nadav Amit, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
> > Possibility of race against other PTE modifiers
> >
> > 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
> > is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
Right, that's exactly the kind of thing I was worried about.
> > 2) mprotect - change_protection in mprotect which does the deferred flush is
> > marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
> > on those VMAs.
Sure, mprotect also changes vm_flags, so it really needs that anyway.
> > 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
> > But SPF does not take UFFD faults.
> > 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
> > (2) above.
> > 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
What happened to shared/file-backed stuff? ISTR I had that working.
> > Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
> > transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
The tricky one is demotion, specifically write to non-write.
> > I could not see a case where speculative path cannot see a PTE update done via
> > a fault on another CPU.
One you didn't mention is the NUMA balancing scanning crud; although I
think that's fine, loosing a PTE update there is harmless. But I've not
thought overly hard on it.
> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
> marking the VMA through vm_write_begin/end(), as for the fork case you
> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
> values read are valid.
That should indeed work, but are we really sure we covered them all?
Should we invest in better TLBI APIs to make sure we can't get this
wrong?
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 16:57 ` Peter Zijlstra
@ 2021-01-12 19:02 ` Laurent Dufour
2021-01-12 19:15 ` Nadav Amit
0 siblings, 1 reply; 141+ messages in thread
From: Laurent Dufour @ 2021-01-12 19:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Vinayak Menon, Linus Torvalds, Andy Lutomirski, Peter Xu,
Nadav Amit, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
Le 12/01/2021 à 17:57, Peter Zijlstra a écrit :
> On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
>> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
>
>>> Possibility of race against other PTE modifiers
>>>
>>> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
>>> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
>
> Right, that's exactly the kind of thing I was worried about.
>
>>> 2) mprotect - change_protection in mprotect which does the deferred flush is
>>> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
>>> on those VMAs.
>
> Sure, mprotect also changes vm_flags, so it really needs that anyway.
>
>>> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
>>> But SPF does not take UFFD faults.
>>> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
>>> (2) above.
>
>>> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
>
> What happened to shared/file-backed stuff? ISTR I had that working.
File-backed mappings are not processed in a speculative way, there were options
to manage some of them depending on the underlying file system but that's still
not done.
Shared anonymous mapping, are also not yet handled in a speculative way (vm_ops
is not null).
>>> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
>>> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
>
> The tricky one is demotion, specifically write to non-write.
>
>>> I could not see a case where speculative path cannot see a PTE update done via
>>> a fault on another CPU.
>
> One you didn't mention is the NUMA balancing scanning crud; although I
> think that's fine, loosing a PTE update there is harmless. But I've not
> thought overly hard on it.
That's a good point, I need to double check on that side.
>> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
>> marking the VMA through vm_write_begin/end(), as for the fork case you
>> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
>> values read are valid.
>
> That should indeed work, but are we really sure we covered them all?
> Should we invest in better TLBI APIs to make sure we can't get this
> wrong?
That may be a good option to identify deferred TLB invalidation but I've no clue
on what this API would look like.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 19:02 ` Laurent Dufour
@ 2021-01-12 19:15 ` Nadav Amit
2021-01-12 19:56 ` Yu Zhao
0 siblings, 1 reply; 141+ messages in thread
From: Nadav Amit @ 2021-01-12 19:15 UTC (permalink / raw)
To: Laurent Dufour
Cc: Peter Zijlstra, Vinayak Menon, Linus Torvalds, Andy Lutomirski,
Peter Xu, Yu Zhao, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
> On Jan 12, 2021, at 11:02 AM, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote:
>
> Le 12/01/2021 à 17:57, Peter Zijlstra a écrit :
>> On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
>>> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
>>>> Possibility of race against other PTE modifiers
>>>>
>>>> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
>>>> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
>> Right, that's exactly the kind of thing I was worried about.
>>>> 2) mprotect - change_protection in mprotect which does the deferred flush is
>>>> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
>>>> on those VMAs.
>> Sure, mprotect also changes vm_flags, so it really needs that anyway.
>>>> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
>>>> But SPF does not take UFFD faults.
>>>> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
>>>> (2) above.
>>>> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
>> What happened to shared/file-backed stuff? ISTR I had that working.
>
> File-backed mappings are not processed in a speculative way, there were options to manage some of them depending on the underlying file system but that's still not done.
>
> Shared anonymous mapping, are also not yet handled in a speculative way (vm_ops is not null).
>
>>>> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
>>>> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
>> The tricky one is demotion, specifically write to non-write.
>>>> I could not see a case where speculative path cannot see a PTE update done via
>>>> a fault on another CPU.
>> One you didn't mention is the NUMA balancing scanning crud; although I
>> think that's fine, loosing a PTE update there is harmless. But I've not
>> thought overly hard on it.
>
> That's a good point, I need to double check on that side.
>
>>> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
>>> marking the VMA through vm_write_begin/end(), as for the fork case you
>>> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
>>> values read are valid.
>> That should indeed work, but are we really sure we covered them all?
>> Should we invest in better TLBI APIs to make sure we can't get this
>> wrong?
>
> That may be a good option to identify deferred TLB invalidation but I've no clue on what this API would look like.
I will send an RFC soon for per-table deferred TLB flushes tracking.
The basic idea is to save a generation in the page-struct that tracks
when deferred PTE change took place, and track whenever a TLB flush
completed. In addition, other users - such as mprotect - would use
the tlb_gather interface.
Unfortunately, due to limited space in page-struct this would only
be possible for 64-bit (and my implementation is only for x86-64).
It would still require to do the copying while holding the PTL though.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 19:15 ` Nadav Amit
@ 2021-01-12 19:56 ` Yu Zhao
2021-01-12 20:38 ` Nadav Amit
0 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2021-01-12 19:56 UTC (permalink / raw)
To: Nadav Amit
Cc: Laurent Dufour, Peter Zijlstra, Vinayak Menon, Linus Torvalds,
Andy Lutomirski, Peter Xu, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> > On Jan 12, 2021, at 11:02 AM, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote:
> >
> > Le 12/01/2021 à 17:57, Peter Zijlstra a écrit :
> >> On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
> >>> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
> >>>> Possibility of race against other PTE modifiers
> >>>>
> >>>> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
> >>>> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
> >> Right, that's exactly the kind of thing I was worried about.
> >>>> 2) mprotect - change_protection in mprotect which does the deferred flush is
> >>>> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
> >>>> on those VMAs.
> >> Sure, mprotect also changes vm_flags, so it really needs that anyway.
> >>>> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
> >>>> But SPF does not take UFFD faults.
> >>>> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
> >>>> (2) above.
> >>>> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
> >> What happened to shared/file-backed stuff? ISTR I had that working.
> >
> > File-backed mappings are not processed in a speculative way, there were options to manage some of them depending on the underlying file system but that's still not done.
> >
> > Shared anonymous mapping, are also not yet handled in a speculative way (vm_ops is not null).
> >
> >>>> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
> >>>> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
> >> The tricky one is demotion, specifically write to non-write.
> >>>> I could not see a case where speculative path cannot see a PTE update done via
> >>>> a fault on another CPU.
> >> One you didn't mention is the NUMA balancing scanning crud; although I
> >> think that's fine, loosing a PTE update there is harmless. But I've not
> >> thought overly hard on it.
> >
> > That's a good point, I need to double check on that side.
> >
> >>> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
> >>> marking the VMA through vm_write_begin/end(), as for the fork case you
> >>> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
> >>> values read are valid.
> >> That should indeed work, but are we really sure we covered them all?
> >> Should we invest in better TLBI APIs to make sure we can't get this
> >> wrong?
> >
> > That may be a good option to identify deferred TLB invalidation but I've no clue on what this API would look like.
>
> I will send an RFC soon for per-table deferred TLB flushes tracking.
> The basic idea is to save a generation in the page-struct that tracks
> when deferred PTE change took place, and track whenever a TLB flush
> completed. In addition, other users - such as mprotect - would use
> the tlb_gather interface.
>
> Unfortunately, due to limited space in page-struct this would only
> be possible for 64-bit (and my implementation is only for x86-64).
I don't want to discourage you but I don't think this would end up
well. PPC doesn't necessarily follow one-page-struct-per-table rule,
and I've run into problems with this before while trying to do
something similar.
I'd recommend per-vma and per-category (unmapping, clearing writable
and clearing dirty) tracking, which only rely on arch-independent data
structures, i.e., vm_area_struct and mm_struct.
> It would still require to do the copying while holding the PTL though.
IMO, this is unacceptable. Most archs don't support per-table PTL, and
even x86_64 can be configured to use per-mm PTL. What if we want to
support a larger page size in the feature?
It seems to me the only way to solve the problem with self-explanatory
code and without performance impact is to check mm_tlb_flush_pending
and the writable bit (and two other cases I mentioned above) at the
same time. Of course, this requires a lot of effort to audit the
existing uses, clean them up and properly wrap them up with new
primitives, BUG_ON all invalid cases and document the exact workflow
to prevent misuses.
I've mentioned the following before -- it only demonstrates the rough
idea.
diff --git a/mm/memory.c b/mm/memory.c
index 5e9ca612d7d7..af38c5ee327e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
goto unlock;
}
if (vmf->flags & FAULT_FLAG_WRITE) {
- if (!pte_write(entry))
+ if (!pte_write(entry)) {
+ if (mm_tlb_flush_pending(vmf->vma->vm_mm))
+ flush_tlb_page(vmf->vma, vmf->address);
return do_wp_page(vmf);
+ }
entry = pte_mkdirty(entry);
}
entry = pte_mkyoung(entry);
^ permalink raw reply related [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 19:56 ` Yu Zhao
@ 2021-01-12 20:38 ` Nadav Amit
2021-01-12 20:49 ` Yu Zhao
2021-01-12 21:43 ` Will Deacon
0 siblings, 2 replies; 141+ messages in thread
From: Nadav Amit @ 2021-01-12 20:38 UTC (permalink / raw)
To: Yu Zhao
Cc: Laurent Dufour, Peter Zijlstra, Vinayak Menon, Linus Torvalds,
Andy Lutomirski, Peter Xu, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>
> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>> On Jan 12, 2021, at 11:02 AM, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote:
>>>
>>> Le 12/01/2021 à 17:57, Peter Zijlstra a écrit :
>>>> On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
>>>>> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
>>>>>> Possibility of race against other PTE modifiers
>>>>>>
>>>>>> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
>>>>>> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
>>>> Right, that's exactly the kind of thing I was worried about.
>>>>>> 2) mprotect - change_protection in mprotect which does the deferred flush is
>>>>>> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
>>>>>> on those VMAs.
>>>> Sure, mprotect also changes vm_flags, so it really needs that anyway.
>>>>>> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
>>>>>> But SPF does not take UFFD faults.
>>>>>> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
>>>>>> (2) above.
>>>>>> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
>>>> What happened to shared/file-backed stuff? ISTR I had that working.
>>>
>>> File-backed mappings are not processed in a speculative way, there were options to manage some of them depending on the underlying file system but that's still not done.
>>>
>>> Shared anonymous mapping, are also not yet handled in a speculative way (vm_ops is not null).
>>>
>>>>>> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
>>>>>> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
>>>> The tricky one is demotion, specifically write to non-write.
>>>>>> I could not see a case where speculative path cannot see a PTE update done via
>>>>>> a fault on another CPU.
>>>> One you didn't mention is the NUMA balancing scanning crud; although I
>>>> think that's fine, loosing a PTE update there is harmless. But I've not
>>>> thought overly hard on it.
>>>
>>> That's a good point, I need to double check on that side.
>>>
>>>>> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
>>>>> marking the VMA through vm_write_begin/end(), as for the fork case you
>>>>> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
>>>>> values read are valid.
>>>> That should indeed work, but are we really sure we covered them all?
>>>> Should we invest in better TLBI APIs to make sure we can't get this
>>>> wrong?
>>>
>>> That may be a good option to identify deferred TLB invalidation but I've no clue on what this API would look like.
>>
>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>> The basic idea is to save a generation in the page-struct that tracks
>> when deferred PTE change took place, and track whenever a TLB flush
>> completed. In addition, other users - such as mprotect - would use
>> the tlb_gather interface.
>>
>> Unfortunately, due to limited space in page-struct this would only
>> be possible for 64-bit (and my implementation is only for x86-64).
>
> I don't want to discourage you but I don't think this would end up
> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> and I've run into problems with this before while trying to do
> something similar.
Discourage, discourage. Better now than later.
It will be relatively easy to extend the scheme to be per-VMA instead of
per-table for architectures that prefer it this way. It does require
TLB-generation tracking though, which Andy only implemented for x86, so I
will focus on x86-64 right now.
[ For per-VMA it would require an additional cmpxchg, I presume to save the
last deferred generation though. ]
> I'd recommend per-vma and per-category (unmapping, clearing writable
> and clearing dirty) tracking, which only rely on arch-independent data
> structures, i.e., vm_area_struct and mm_struct.
I think that tracking changes on “what was changed” granularity is harder
and more fragile.
Let me finish trying the clean up the mess first, since fullmm and
need_flush_all semantics were mixed; there are 3 different flushing schemes
for mprotect(), munmap() and try_to_unmap(); there are missing memory
barriers; mprotect() performs TLB flushes even when permissions are
promoted; etc.
There are also some optimizations that we discussed before, such on x86 -
RW->RO does not require a TLB flush if a PTE is not dirty, etc.
I am trying to finish something so you can say how terrible it is, so I will
not waste too much time. ;-)
>> It would still require to do the copying while holding the PTL though.
>
> IMO, this is unacceptable. Most archs don't support per-table PTL, and
> even x86_64 can be configured to use per-mm PTL. What if we want to
> support a larger page size in the feature?
>
> It seems to me the only way to solve the problem with self-explanatory
> code and without performance impact is to check mm_tlb_flush_pending
> and the writable bit (and two other cases I mentioned above) at the
> same time. Of course, this requires a lot of effort to audit the
> existing uses, clean them up and properly wrap them up with new
> primitives, BUG_ON all invalid cases and document the exact workflow
> to prevent misuses.
>
> I've mentioned the following before -- it only demonstrates the rough
> idea.
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 5e9ca612d7d7..af38c5ee327e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> goto unlock;
> }
> if (vmf->flags & FAULT_FLAG_WRITE) {
> - if (!pte_write(entry))
> + if (!pte_write(entry)) {
> + if (mm_tlb_flush_pending(vmf->vma->vm_mm))
> + flush_tlb_page(vmf->vma, vmf->address);
> return do_wp_page(vmf);
> + }
> entry = pte_mkdirty(entry);
> }
> entry = pte_mkyoung(entry);
I understand. This might be required, regardless of the deferred flushes
detection scheme. If we assume that no write-unprotect requires a COW (which
should be true in this case, since we take a reference on the page), your
proposal should be sufficient.
Still, I think that there are many unnecessary TLB flushes right now,
and others that might be missed due to the overly complicated invalidation
schemes.
Regardless, as Andrea pointed, this requires first to figure out the
semantics of mprotect() and friends when pages are pinned.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 20:38 ` Nadav Amit
@ 2021-01-12 20:49 ` Yu Zhao
2021-01-12 21:43 ` Will Deacon
1 sibling, 0 replies; 141+ messages in thread
From: Yu Zhao @ 2021-01-12 20:49 UTC (permalink / raw)
To: Nadav Amit
Cc: Laurent Dufour, Peter Zijlstra, Vinayak Menon, Linus Torvalds,
Andy Lutomirski, Peter Xu, Andrea Arcangeli, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, surenb
On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> > On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> >>> On Jan 12, 2021, at 11:02 AM, Laurent Dufour <ldufour@linux.vnet.ibm.com> wrote:
> >>>
> >>> Le 12/01/2021 à 17:57, Peter Zijlstra a écrit :
> >>>> On Tue, Jan 12, 2021 at 04:47:17PM +0100, Laurent Dufour wrote:
> >>>>> Le 12/01/2021 à 12:43, Vinayak Menon a écrit :
> >>>>>> Possibility of race against other PTE modifiers
> >>>>>>
> >>>>>> 1) Fork - We have seen a case of SPF racing with fork marking PTEs RO and that
> >>>>>> is described and fixed here https://lore.kernel.org/patchwork/patch/1062672/
> >>>> Right, that's exactly the kind of thing I was worried about.
> >>>>>> 2) mprotect - change_protection in mprotect which does the deferred flush is
> >>>>>> marked under vm_write_begin/vm_write_end, thus SPF bails out on faults
> >>>>>> on those VMAs.
> >>>> Sure, mprotect also changes vm_flags, so it really needs that anyway.
> >>>>>> 3) userfaultfd - mwriteprotect_range is not protected unlike in (2) above.
> >>>>>> But SPF does not take UFFD faults.
> >>>>>> 4) hugetlb - hugetlb_change_protection - called from mprotect and covered by
> >>>>>> (2) above.
> >>>>>> 5) Concurrent faults - SPF does not handle all faults. Only anon page faults.
> >>>> What happened to shared/file-backed stuff? ISTR I had that working.
> >>>
> >>> File-backed mappings are not processed in a speculative way, there were options to manage some of them depending on the underlying file system but that's still not done.
> >>>
> >>> Shared anonymous mapping, are also not yet handled in a speculative way (vm_ops is not null).
> >>>
> >>>>>> Of which do_anonymous_page and do_swap_page are NONE/NON-PRESENT->PRESENT
> >>>>>> transitions without tlb flush. And I hope do_wp_page with RO->RW is fine as well.
> >>>> The tricky one is demotion, specifically write to non-write.
> >>>>>> I could not see a case where speculative path cannot see a PTE update done via
> >>>>>> a fault on another CPU.
> >>>> One you didn't mention is the NUMA balancing scanning crud; although I
> >>>> think that's fine, loosing a PTE update there is harmless. But I've not
> >>>> thought overly hard on it.
> >>>
> >>> That's a good point, I need to double check on that side.
> >>>
> >>>>> You explained it fine. Indeed SPF is handling deferred TLB invalidation by
> >>>>> marking the VMA through vm_write_begin/end(), as for the fork case you
> >>>>> mentioned. Once the PTL is held, and the VMA's seqcount is checked, the PTE
> >>>>> values read are valid.
> >>>> That should indeed work, but are we really sure we covered them all?
> >>>> Should we invest in better TLBI APIs to make sure we can't get this
> >>>> wrong?
> >>>
> >>> That may be a good option to identify deferred TLB invalidation but I've no clue on what this API would look like.
> >>
> >> I will send an RFC soon for per-table deferred TLB flushes tracking.
> >> The basic idea is to save a generation in the page-struct that tracks
> >> when deferred PTE change took place, and track whenever a TLB flush
> >> completed. In addition, other users - such as mprotect - would use
> >> the tlb_gather interface.
> >>
> >> Unfortunately, due to limited space in page-struct this would only
> >> be possible for 64-bit (and my implementation is only for x86-64).
> >
> > I don't want to discourage you but I don't think this would end up
> > well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> > and I've run into problems with this before while trying to do
> > something similar.
>
> Discourage, discourage. Better now than later.
>
> It will be relatively easy to extend the scheme to be per-VMA instead of
> per-table for architectures that prefer it this way. It does require
> TLB-generation tracking though, which Andy only implemented for x86, so I
> will focus on x86-64 right now.
>
> [ For per-VMA it would require an additional cmpxchg, I presume to save the
> last deferred generation though. ]
>
> > I'd recommend per-vma and per-category (unmapping, clearing writable
> > and clearing dirty) tracking, which only rely on arch-independent data
> > structures, i.e., vm_area_struct and mm_struct.
>
> I think that tracking changes on “what was changed” granularity is harder
> and more fragile.
>
> Let me finish trying the clean up the mess first, since fullmm and
> need_flush_all semantics were mixed; there are 3 different flushing schemes
> for mprotect(), munmap() and try_to_unmap(); there are missing memory
> barriers; mprotect() performs TLB flushes even when permissions are
> promoted; etc.
>
> There are also some optimizations that we discussed before, such on x86 -
> RW->RO does not require a TLB flush if a PTE is not dirty, etc.
>
> I am trying to finish something so you can say how terrible it is, so I will
> not waste too much time. ;-)
>
> >> It would still require to do the copying while holding the PTL though.
> >
> > IMO, this is unacceptable. Most archs don't support per-table PTL, and
> > even x86_64 can be configured to use per-mm PTL. What if we want to
> > support a larger page size in the feature?
> >
> > It seems to me the only way to solve the problem with self-explanatory
> > code and without performance impact is to check mm_tlb_flush_pending
> > and the writable bit (and two other cases I mentioned above) at the
> > same time. Of course, this requires a lot of effort to audit the
> > existing uses, clean them up and properly wrap them up with new
> > primitives, BUG_ON all invalid cases and document the exact workflow
> > to prevent misuses.
> >
> > I've mentioned the following before -- it only demonstrates the rough
> > idea.
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 5e9ca612d7d7..af38c5ee327e 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> > goto unlock;
> > }
> > if (vmf->flags & FAULT_FLAG_WRITE) {
> > - if (!pte_write(entry))
> > + if (!pte_write(entry)) {
> > + if (mm_tlb_flush_pending(vmf->vma->vm_mm))
> > + flush_tlb_page(vmf->vma, vmf->address);
> > return do_wp_page(vmf);
> > + }
> > entry = pte_mkdirty(entry);
> > }
> > entry = pte_mkyoung(entry);
>
> I understand. This might be required, regardless of the deferred flushes
> detection scheme. If we assume that no write-unprotect requires a COW (which
> should be true in this case, since we take a reference on the page), your
> proposal should be sufficient.
>
> Still, I think that there are many unnecessary TLB flushes right now,
> and others that might be missed due to the overly complicated invalidation
> schemes.
>
> Regardless, as Andrea pointed, this requires first to figure out the
> semantics of mprotect() and friends when pages are pinned.
Thanks, I appreciate your effort. I'd be glad to review whatever you
come up with.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 20:38 ` Nadav Amit
2021-01-12 20:49 ` Yu Zhao
@ 2021-01-12 21:43 ` Will Deacon
2021-01-12 22:29 ` Nadav Amit
2021-01-17 4:41 ` Yu Zhao
1 sibling, 2 replies; 141+ messages in thread
From: Will Deacon @ 2021-01-12 21:43 UTC (permalink / raw)
To: Nadav Amit
Cc: Yu Zhao, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb
On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> > On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> > On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> >> I will send an RFC soon for per-table deferred TLB flushes tracking.
> >> The basic idea is to save a generation in the page-struct that tracks
> >> when deferred PTE change took place, and track whenever a TLB flush
> >> completed. In addition, other users - such as mprotect - would use
> >> the tlb_gather interface.
> >>
> >> Unfortunately, due to limited space in page-struct this would only
> >> be possible for 64-bit (and my implementation is only for x86-64).
> >
> > I don't want to discourage you but I don't think this would end up
> > well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> > and I've run into problems with this before while trying to do
> > something similar.
>
> Discourage, discourage. Better now than later.
>
> It will be relatively easy to extend the scheme to be per-VMA instead of
> per-table for architectures that prefer it this way. It does require
> TLB-generation tracking though, which Andy only implemented for x86, so I
> will focus on x86-64 right now.
Can you remind me of what we're missing on arm64 in this area, please? I'm
happy to help get this up and running once you have something I can build
on.
Will
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 21:43 ` Will Deacon
@ 2021-01-12 22:29 ` Nadav Amit
2021-01-12 22:46 ` Will Deacon
2021-01-13 0:31 ` Andy Lutomirski
2021-01-17 4:41 ` Yu Zhao
1 sibling, 2 replies; 141+ messages in thread
From: Nadav Amit @ 2021-01-12 22:29 UTC (permalink / raw)
To: Will Deacon
Cc: Yu Zhao, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb
> On Jan 12, 2021, at 1:43 PM, Will Deacon <will@kernel.org> wrote:
>
> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>>>> The basic idea is to save a generation in the page-struct that tracks
>>>> when deferred PTE change took place, and track whenever a TLB flush
>>>> completed. In addition, other users - such as mprotect - would use
>>>> the tlb_gather interface.
>>>>
>>>> Unfortunately, due to limited space in page-struct this would only
>>>> be possible for 64-bit (and my implementation is only for x86-64).
>>>
>>> I don't want to discourage you but I don't think this would end up
>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
>>> and I've run into problems with this before while trying to do
>>> something similar.
>>
>> Discourage, discourage. Better now than later.
>>
>> It will be relatively easy to extend the scheme to be per-VMA instead of
>> per-table for architectures that prefer it this way. It does require
>> TLB-generation tracking though, which Andy only implemented for x86, so I
>> will focus on x86-64 right now.
>
> Can you remind me of what we're missing on arm64 in this area, please? I'm
> happy to help get this up and running once you have something I can build
> on.
Let me first finish making something that we can use as a basis for a
discussion. I do not waste your time before I have something ready.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 22:29 ` Nadav Amit
@ 2021-01-12 22:46 ` Will Deacon
2021-01-13 0:31 ` Andy Lutomirski
1 sibling, 0 replies; 141+ messages in thread
From: Will Deacon @ 2021-01-12 22:46 UTC (permalink / raw)
To: Nadav Amit
Cc: Yu Zhao, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb
On Tue, Jan 12, 2021 at 02:29:51PM -0800, Nadav Amit wrote:
> > On Jan 12, 2021, at 1:43 PM, Will Deacon <will@kernel.org> wrote:
> >
> > On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> >>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> >>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> >>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
> >>>> The basic idea is to save a generation in the page-struct that tracks
> >>>> when deferred PTE change took place, and track whenever a TLB flush
> >>>> completed. In addition, other users - such as mprotect - would use
> >>>> the tlb_gather interface.
> >>>>
> >>>> Unfortunately, due to limited space in page-struct this would only
> >>>> be possible for 64-bit (and my implementation is only for x86-64).
> >>>
> >>> I don't want to discourage you but I don't think this would end up
> >>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> >>> and I've run into problems with this before while trying to do
> >>> something similar.
> >>
> >> Discourage, discourage. Better now than later.
> >>
> >> It will be relatively easy to extend the scheme to be per-VMA instead of
> >> per-table for architectures that prefer it this way. It does require
> >> TLB-generation tracking though, which Andy only implemented for x86, so I
> >> will focus on x86-64 right now.
> >
> > Can you remind me of what we're missing on arm64 in this area, please? I'm
> > happy to help get this up and running once you have something I can build
> > on.
>
> Let me first finish making something that we can use as a basis for a
> discussion. I do not waste your time before I have something ready.
Sure thing! Give me a shout when you're ready.
Will
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 22:29 ` Nadav Amit
2021-01-12 22:46 ` Will Deacon
@ 2021-01-13 0:31 ` Andy Lutomirski
1 sibling, 0 replies; 141+ messages in thread
From: Andy Lutomirski @ 2021-01-13 0:31 UTC (permalink / raw)
To: Nadav Amit
Cc: Will Deacon, Yu Zhao, Laurent Dufour, Peter Zijlstra,
Vinayak Menon, Linus Torvalds, Andy Lutomirski, Peter Xu,
Andrea Arcangeli, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, surenb
> On Jan 12, 2021, at 2:29 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
>
>
>>
>> On Jan 12, 2021, at 1:43 PM, Will Deacon <will@kernel.org> wrote:
>>
>> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
>>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>>>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>>>>>> The basic idea is to save a generation in the page-struct that tracks
>>>>>> when deferred PTE change took place, and track whenever a TLB flush
>>>>>> completed. In addition, other users - such as mprotect - would use
>>>>>> the tlb_gather interface.
>>>>>>
>>>>>> Unfortunately, due to limited space in page-struct this would only
>>>>>> be possible for 64-bit (and my implementation is only for x86-64).
>>>>>
>>>>> I don't want to discourage you but I don't think this would end up
>>>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
>>>>> and I've run into problems with this before while trying to do
>>>>> something similar.
>>>
>>> Discourage, discourage. Better now than later.
>>>
>>> It will be relatively easy to extend the scheme to be per-VMA instead of
>>> per-table for architectures that prefer it this way. It does require
>>> TLB-generation tracking though, which Andy only implemented for x86, so I
>>> will focus on x86-64 right now.
>>
>> Can you remind me of what we're missing on arm64 in this area, please? I'm
>> happy to help get this up and running once you have something I can build
>> on.
>
> Let me first finish making something that we can use as a basis for a
> discussion. I do not waste your time before I have something ready.
If you want a hand, let me know. I suspect you understand the x86 code as well as I do at this point, though :)
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-12 21:43 ` Will Deacon
2021-01-12 22:29 ` Nadav Amit
@ 2021-01-17 4:41 ` Yu Zhao
2021-01-17 7:32 ` Nadav Amit
1 sibling, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2021-01-17 4:41 UTC (permalink / raw)
To: Will Deacon
Cc: Nadav Amit, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb
On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> > > On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> > > On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> > >> I will send an RFC soon for per-table deferred TLB flushes tracking.
> > >> The basic idea is to save a generation in the page-struct that tracks
> > >> when deferred PTE change took place, and track whenever a TLB flush
> > >> completed. In addition, other users - such as mprotect - would use
> > >> the tlb_gather interface.
> > >>
> > >> Unfortunately, due to limited space in page-struct this would only
> > >> be possible for 64-bit (and my implementation is only for x86-64).
> > >
> > > I don't want to discourage you but I don't think this would end up
> > > well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> > > and I've run into problems with this before while trying to do
> > > something similar.
> >
> > Discourage, discourage. Better now than later.
> >
> > It will be relatively easy to extend the scheme to be per-VMA instead of
> > per-table for architectures that prefer it this way. It does require
> > TLB-generation tracking though, which Andy only implemented for x86, so I
> > will focus on x86-64 right now.
>
> Can you remind me of what we're missing on arm64 in this area, please? I'm
> happy to help get this up and running once you have something I can build
> on.
I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
Would it be something worth pursuing? Arm has been using mm_cpumask,
so it might not be too difficult I guess?
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-17 4:41 ` Yu Zhao
@ 2021-01-17 7:32 ` Nadav Amit
2021-01-17 9:16 ` Yu Zhao
0 siblings, 1 reply; 141+ messages in thread
From: Nadav Amit @ 2021-01-17 7:32 UTC (permalink / raw)
To: Yu Zhao
Cc: Will Deacon, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb, Mel Gorman
> On Jan 16, 2021, at 8:41 PM, Yu Zhao <yuzhao@google.com> wrote:
>
> On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
>> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
>>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>>>>> The basic idea is to save a generation in the page-struct that tracks
>>>>> when deferred PTE change took place, and track whenever a TLB flush
>>>>> completed. In addition, other users - such as mprotect - would use
>>>>> the tlb_gather interface.
>>>>>
>>>>> Unfortunately, due to limited space in page-struct this would only
>>>>> be possible for 64-bit (and my implementation is only for x86-64).
>>>>
>>>> I don't want to discourage you but I don't think this would end up
>>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
>>>> and I've run into problems with this before while trying to do
>>>> something similar.
>>>
>>> Discourage, discourage. Better now than later.
>>>
>>> It will be relatively easy to extend the scheme to be per-VMA instead of
>>> per-table for architectures that prefer it this way. It does require
>>> TLB-generation tracking though, which Andy only implemented for x86, so I
>>> will focus on x86-64 right now.
>>
>> Can you remind me of what we're missing on arm64 in this area, please? I'm
>> happy to help get this up and running once you have something I can build
>> on.
>
> I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
> Would it be something worth pursuing? Arm has been using mm_cpumask,
> so it might not be too difficult I guess?
[ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]
IIUC, there are at least two bugs in x86 implementation.
First, there is a missing memory barrier in tlbbatch_add_mm() between
inc_mm_tlb_gen() and the read of mm_cpumask().
Second, try_to_unmap_flush() clears flush_required after flushing. Another
thread can call set_tlb_ubc_flush_pending() after the flush and before
flush_required is cleared, and the indication that a TLB flush is pending
can be lost.
I am working on addressing these issues among others, but, as you already
saw, I am a bit slow.
On a different but related topic: Another thing that I noticed that Arm does
not do is batching TLB flushes across VMAs. Since Arm does not have its own
tlb_end_vma(), it uses the default tlb_end_vma(), which flushes each VMA
separately. Peter Zijlstra’s comment says that there are advantages in
flushing each VMA separately, but I am not sure it is better or intentional
(especially since x86 does not do so).
I am trying to remove the arch-specific tlb_end_vma() and have a config
option to control this behavior.
Again, sorry for being slow. I hope to send an RFC soon.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-17 7:32 ` Nadav Amit
@ 2021-01-17 9:16 ` Yu Zhao
2021-01-17 10:13 ` Nadav Amit
0 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2021-01-17 9:16 UTC (permalink / raw)
To: Nadav Amit
Cc: Will Deacon, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb, Mel Gorman
On Sat, Jan 16, 2021 at 11:32:22PM -0800, Nadav Amit wrote:
> > On Jan 16, 2021, at 8:41 PM, Yu Zhao <yuzhao@google.com> wrote:
> >
> > On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
> >> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> >>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> >>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> >>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
> >>>>> The basic idea is to save a generation in the page-struct that tracks
> >>>>> when deferred PTE change took place, and track whenever a TLB flush
> >>>>> completed. In addition, other users - such as mprotect - would use
> >>>>> the tlb_gather interface.
> >>>>>
> >>>>> Unfortunately, due to limited space in page-struct this would only
> >>>>> be possible for 64-bit (and my implementation is only for x86-64).
> >>>>
> >>>> I don't want to discourage you but I don't think this would end up
> >>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> >>>> and I've run into problems with this before while trying to do
> >>>> something similar.
> >>>
> >>> Discourage, discourage. Better now than later.
> >>>
> >>> It will be relatively easy to extend the scheme to be per-VMA instead of
> >>> per-table for architectures that prefer it this way. It does require
> >>> TLB-generation tracking though, which Andy only implemented for x86, so I
> >>> will focus on x86-64 right now.
> >>
> >> Can you remind me of what we're missing on arm64 in this area, please? I'm
> >> happy to help get this up and running once you have something I can build
> >> on.
> >
> > I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
> > Would it be something worth pursuing? Arm has been using mm_cpumask,
> > so it might not be too difficult I guess?
>
> [ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]
>
> IIUC, there are at least two bugs in x86 implementation.
>
> First, there is a missing memory barrier in tlbbatch_add_mm() between
> inc_mm_tlb_gen() and the read of mm_cpumask().
In arch_tlbbatch_add_mm()? inc_mm_tlb_gen() has builtin barrier as its
comment says -- atomic update ops that return values are also full
memory barriers.
> Second, try_to_unmap_flush() clears flush_required after flushing. Another
> thread can call set_tlb_ubc_flush_pending() after the flush and before
> flush_required is cleared, and the indication that a TLB flush is pending
> can be lost.
This isn't a problem either because flush_required is per thread.
> I am working on addressing these issues among others, but, as you already
> saw, I am a bit slow.
>
> On a different but related topic: Another thing that I noticed that Arm does
> not do is batching TLB flushes across VMAs. Since Arm does not have its own
> tlb_end_vma(), it uses the default tlb_end_vma(), which flushes each VMA
> separately. Peter Zijlstra’s comment says that there are advantages in
> flushing each VMA separately, but I am not sure it is better or intentional
> (especially since x86 does not do so).
>
> I am trying to remove the arch-specific tlb_end_vma() and have a config
> option to control this behavior.
One thing worth noting is not all arm/arm64 hw versions support ranges.
(system_supports_tlb_range()). But IIUC what you are trying to do, this
isn't a problem.
> Again, sorry for being slow. I hope to send an RFC soon.
No worries. I brought it up only because I noticed it and didn't want
it to slip away.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-17 9:16 ` Yu Zhao
@ 2021-01-17 10:13 ` Nadav Amit
2021-01-17 19:25 ` Yu Zhao
0 siblings, 1 reply; 141+ messages in thread
From: Nadav Amit @ 2021-01-17 10:13 UTC (permalink / raw)
To: Yu Zhao
Cc: Will Deacon, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb, Mel Gorman
> On Jan 17, 2021, at 1:16 AM, Yu Zhao <yuzhao@google.com> wrote:
>
> On Sat, Jan 16, 2021 at 11:32:22PM -0800, Nadav Amit wrote:
>>> On Jan 16, 2021, at 8:41 PM, Yu Zhao <yuzhao@google.com> wrote:
>>>
>>> On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
>>>> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
>>>>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>>>>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>>>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>>>>>>> The basic idea is to save a generation in the page-struct that tracks
>>>>>>> when deferred PTE change took place, and track whenever a TLB flush
>>>>>>> completed. In addition, other users - such as mprotect - would use
>>>>>>> the tlb_gather interface.
>>>>>>>
>>>>>>> Unfortunately, due to limited space in page-struct this would only
>>>>>>> be possible for 64-bit (and my implementation is only for x86-64).
>>>>>>
>>>>>> I don't want to discourage you but I don't think this would end up
>>>>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
>>>>>> and I've run into problems with this before while trying to do
>>>>>> something similar.
>>>>>
>>>>> Discourage, discourage. Better now than later.
>>>>>
>>>>> It will be relatively easy to extend the scheme to be per-VMA instead of
>>>>> per-table for architectures that prefer it this way. It does require
>>>>> TLB-generation tracking though, which Andy only implemented for x86, so I
>>>>> will focus on x86-64 right now.
>>>>
>>>> Can you remind me of what we're missing on arm64 in this area, please? I'm
>>>> happy to help get this up and running once you have something I can build
>>>> on.
>>>
>>> I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
>>> Would it be something worth pursuing? Arm has been using mm_cpumask,
>>> so it might not be too difficult I guess?
>>
>> [ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]
>>
>> IIUC, there are at least two bugs in x86 implementation.
>>
>> First, there is a missing memory barrier in tlbbatch_add_mm() between
>> inc_mm_tlb_gen() and the read of mm_cpumask().
>
> In arch_tlbbatch_add_mm()? inc_mm_tlb_gen() has builtin barrier as its
> comment says -- atomic update ops that return values are also full
> memory barriers.
Yes, you are correct.
>
>> Second, try_to_unmap_flush() clears flush_required after flushing. Another
>> thread can call set_tlb_ubc_flush_pending() after the flush and before
>> flush_required is cleared, and the indication that a TLB flush is pending
>> can be lost.
>
> This isn't a problem either because flush_required is per thread.
Sorry, I meant mm->tlb_flush_batched . It is not per-thread.
flush_tlb_batched_pending() clears it after flush and indications that
set_tlb_ubc_flush_pending() sets in between can be lost.
>> I am working on addressing these issues among others, but, as you already
>> saw, I am a bit slow.
>>
>> On a different but related topic: Another thing that I noticed that Arm does
>> not do is batching TLB flushes across VMAs. Since Arm does not have its own
>> tlb_end_vma(), it uses the default tlb_end_vma(), which flushes each VMA
>> separately. Peter Zijlstra’s comment says that there are advantages in
>> flushing each VMA separately, but I am not sure it is better or intentional
>> (especially since x86 does not do so).
>>
>> I am trying to remove the arch-specific tlb_end_vma() and have a config
>> option to control this behavior.
>
> One thing worth noting is not all arm/arm64 hw versions support ranges.
> (system_supports_tlb_range()). But IIUC what you are trying to do, this
> isn't a problem.
I just wanted to get rid of arch-specific tlb_start_vma() it in order to
cleanup the code (I am doing first the VMA-deferred tracking, as you asked).
While I was doing that, I noticed that Arm does per-VMA TLB flushes. I do
not know whether it is intentional, but it seems rather easy to get this
behavior unintentionally.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-17 10:13 ` Nadav Amit
@ 2021-01-17 19:25 ` Yu Zhao
2021-01-18 2:49 ` Nadav Amit
0 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2021-01-17 19:25 UTC (permalink / raw)
To: Nadav Amit
Cc: Will Deacon, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb, Mel Gorman
On Sun, Jan 17, 2021 at 02:13:43AM -0800, Nadav Amit wrote:
> > On Jan 17, 2021, at 1:16 AM, Yu Zhao <yuzhao@google.com> wrote:
> >
> > On Sat, Jan 16, 2021 at 11:32:22PM -0800, Nadav Amit wrote:
> >>> On Jan 16, 2021, at 8:41 PM, Yu Zhao <yuzhao@google.com> wrote:
> >>>
> >>> On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
> >>>> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
> >>>>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
> >>>>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
> >>>>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
> >>>>>>> The basic idea is to save a generation in the page-struct that tracks
> >>>>>>> when deferred PTE change took place, and track whenever a TLB flush
> >>>>>>> completed. In addition, other users - such as mprotect - would use
> >>>>>>> the tlb_gather interface.
> >>>>>>>
> >>>>>>> Unfortunately, due to limited space in page-struct this would only
> >>>>>>> be possible for 64-bit (and my implementation is only for x86-64).
> >>>>>>
> >>>>>> I don't want to discourage you but I don't think this would end up
> >>>>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
> >>>>>> and I've run into problems with this before while trying to do
> >>>>>> something similar.
> >>>>>
> >>>>> Discourage, discourage. Better now than later.
> >>>>>
> >>>>> It will be relatively easy to extend the scheme to be per-VMA instead of
> >>>>> per-table for architectures that prefer it this way. It does require
> >>>>> TLB-generation tracking though, which Andy only implemented for x86, so I
> >>>>> will focus on x86-64 right now.
> >>>>
> >>>> Can you remind me of what we're missing on arm64 in this area, please? I'm
> >>>> happy to help get this up and running once you have something I can build
> >>>> on.
> >>>
> >>> I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
> >>> Would it be something worth pursuing? Arm has been using mm_cpumask,
> >>> so it might not be too difficult I guess?
> >>
> >> [ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]
> >>
> >> IIUC, there are at least two bugs in x86 implementation.
> >>
> >> First, there is a missing memory barrier in tlbbatch_add_mm() between
> >> inc_mm_tlb_gen() and the read of mm_cpumask().
> >
> > In arch_tlbbatch_add_mm()? inc_mm_tlb_gen() has builtin barrier as its
> > comment says -- atomic update ops that return values are also full
> > memory barriers.
>
> Yes, you are correct.
>
> >
> >> Second, try_to_unmap_flush() clears flush_required after flushing. Another
> >> thread can call set_tlb_ubc_flush_pending() after the flush and before
> >> flush_required is cleared, and the indication that a TLB flush is pending
> >> can be lost.
> >
> > This isn't a problem either because flush_required is per thread.
>
> Sorry, I meant mm->tlb_flush_batched . It is not per-thread.
> flush_tlb_batched_pending() clears it after flush and indications that
> set_tlb_ubc_flush_pending() sets in between can be lost.
Hmm, the PTL argument above flush_tlb_batched_pending() doesn't seem
to hold when USE_SPLIT_PTE_PTLOCKS is set. Do you have a reproducer?
KCSAN might be able to help in this case.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2021-01-17 19:25 ` Yu Zhao
@ 2021-01-18 2:49 ` Nadav Amit
0 siblings, 0 replies; 141+ messages in thread
From: Nadav Amit @ 2021-01-18 2:49 UTC (permalink / raw)
To: Yu Zhao
Cc: Will Deacon, Laurent Dufour, Peter Zijlstra, Vinayak Menon,
Linus Torvalds, Andy Lutomirski, Peter Xu, Andrea Arcangeli,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, surenb, Mel Gorman, cai
> On Jan 17, 2021, at 11:25 AM, Yu Zhao <yuzhao@google.com> wrote:
>
> On Sun, Jan 17, 2021 at 02:13:43AM -0800, Nadav Amit wrote:
>>> On Jan 17, 2021, at 1:16 AM, Yu Zhao <yuzhao@google.com> wrote:
>>>
>>> On Sat, Jan 16, 2021 at 11:32:22PM -0800, Nadav Amit wrote:
>>>>> On Jan 16, 2021, at 8:41 PM, Yu Zhao <yuzhao@google.com> wrote:
>>>>>
>>>>> On Tue, Jan 12, 2021 at 09:43:38PM +0000, Will Deacon wrote:
>>>>>> On Tue, Jan 12, 2021 at 12:38:34PM -0800, Nadav Amit wrote:
>>>>>>>> On Jan 12, 2021, at 11:56 AM, Yu Zhao <yuzhao@google.com> wrote:
>>>>>>>> On Tue, Jan 12, 2021 at 11:15:43AM -0800, Nadav Amit wrote:
>>>>>>>>> I will send an RFC soon for per-table deferred TLB flushes tracking.
>>>>>>>>> The basic idea is to save a generation in the page-struct that tracks
>>>>>>>>> when deferred PTE change took place, and track whenever a TLB flush
>>>>>>>>> completed. In addition, other users - such as mprotect - would use
>>>>>>>>> the tlb_gather interface.
>>>>>>>>>
>>>>>>>>> Unfortunately, due to limited space in page-struct this would only
>>>>>>>>> be possible for 64-bit (and my implementation is only for x86-64).
>>>>>>>>
>>>>>>>> I don't want to discourage you but I don't think this would end up
>>>>>>>> well. PPC doesn't necessarily follow one-page-struct-per-table rule,
>>>>>>>> and I've run into problems with this before while trying to do
>>>>>>>> something similar.
>>>>>>>
>>>>>>> Discourage, discourage. Better now than later.
>>>>>>>
>>>>>>> It will be relatively easy to extend the scheme to be per-VMA instead of
>>>>>>> per-table for architectures that prefer it this way. It does require
>>>>>>> TLB-generation tracking though, which Andy only implemented for x86, so I
>>>>>>> will focus on x86-64 right now.
>>>>>>
>>>>>> Can you remind me of what we're missing on arm64 in this area, please? I'm
>>>>>> happy to help get this up and running once you have something I can build
>>>>>> on.
>>>>>
>>>>> I noticed arm/arm64 don't support ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH.
>>>>> Would it be something worth pursuing? Arm has been using mm_cpumask,
>>>>> so it might not be too difficult I guess?
>>>>
>>>> [ +Mel Gorman who implemented ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH ]
>>>>
>>>> IIUC, there are at least two bugs in x86 implementation.
>>>>
>>>> First, there is a missing memory barrier in tlbbatch_add_mm() between
>>>> inc_mm_tlb_gen() and the read of mm_cpumask().
>>>
>>> In arch_tlbbatch_add_mm()? inc_mm_tlb_gen() has builtin barrier as its
>>> comment says -- atomic update ops that return values are also full
>>> memory barriers.
>>
>> Yes, you are correct.
>>
>>>> Second, try_to_unmap_flush() clears flush_required after flushing. Another
>>>> thread can call set_tlb_ubc_flush_pending() after the flush and before
>>>> flush_required is cleared, and the indication that a TLB flush is pending
>>>> can be lost.
>>>
>>> This isn't a problem either because flush_required is per thread.
>>
>> Sorry, I meant mm->tlb_flush_batched . It is not per-thread.
>> flush_tlb_batched_pending() clears it after flush and indications that
>> set_tlb_ubc_flush_pending() sets in between can be lost.
>
> Hmm, the PTL argument above flush_tlb_batched_pending() doesn't seem
> to hold when USE_SPLIT_PTE_PTLOCKS is set. Do you have a reproducer?
> KCSAN might be able to help in this case.
I do not have a reproducer. It is just based on my understanding of this
code.
I will give a short try for building a reproducer, although for some reason
“you guys” complain that my reproducers do not work for you (is it PTI that
I disable? idle=poll? running in a VM?). It is also not likely to be too
easy to build a reproducer that actually triggers a memory corruption.
Anyhow, apparently KCSAN has already shouted about this code, causing Qian
Cai to add "data_race()" to avoid KCSAN from shouting (9c1177b62a8c
"mm/rmap: annotate a data race at tlb_flush_batched”).
Note that Andrea asked me not to hijack this thread and have a different one
on this issue.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 3:19 ` Andy Lutomirski
(?)
(?)
@ 2020-12-22 9:38 ` Nadav Amit
-1 siblings, 0 replies; 141+ messages in thread
From: Nadav Amit @ 2020-12-22 9:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Peter Xu, Yu Zhao, Andrea Arcangeli, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 21, 2020, at 7:19 PM, Andy Lutomirski <luto@kernel.org> wrote:
>
> On Mon, Dec 21, 2020 at 3:22 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Mon, Dec 21, 2020 at 2:30 PM Peter Xu <peterx@redhat.com> wrote:
>>> AFAIU mprotect() is the only one who modifies the pte using the mmap write
>>> lock. NUMA balancing is also using read mmap lock when changing pte
>>> protections, while my understanding is mprotect() used write lock only because
>>> it manipulates the address space itself (aka. vma layout) rather than modifying
>>> the ptes, so it needs to.
>>
>> So it's ok to change the pte holding only the PTE lock, if it's a
>> *one*way* conversion.
>>
>> That doesn't break the "re-check the PTE contents" model (which
>> predates _all_ of the rest: NUMA, userfaultfd, everything - it's
>> pretty much the original model for our page table operations, and goes
>> back to the dark ages even before SMP and the existence of a page
>> table lock).
>>
>> So for example, a COW will always create a different pte (not just
>> because the page number itself changes - you could imagine a page
>> getting re-used and changing back - but because it's always a RO->RW
>> transition).
>>
>> So two COW operations cannot "undo" each other and fool us into
>> thinking nothing changed.
>>
>> Anything that changes RW->RO - like fork(), for example - needs to
>> take the mmap_lock.
>
> Ugh, this is unpleasantly complicated. I will admit that any API that
> takes an address and more-or-less-blindly marks it RO makes me quite
> nervous even assuming all the relevant locks are held. At least
> userfaultfd refuses to operate on VM_SHARED VMAs, but we have another
> instance of this (with mmap_sem held for write) in x86:
> mark_screen_rdonly(). Dare I ask how broken this is? We could likely
> get away with deleting it entirely.
If you only look at the function in isolation, it seems broken. It should
have flushed the TLB before releasing the mmap_lock. After the
mmap_write_unlock() and before the actual flush, a #PF on another thread can
happen, and a similar scenario to the one that is mentioned in this thread
(copying while a stale PTE in the TLBs is not-writeprotected) might happen.
Having said that, I do not know this code and the context in which this
function is called, so I do not know whether there are other mitigating
factors.
Funny, I had a deja-vu and indeed you have already raised (other) TLB issues
with mark_screen_rdonly() 3 years ago. At the time you said "I'd like to
delete it.” [1]
[1] https://lore.kernel.org/patchwork/patch/782486/#976151
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 3:19 ` Andy Lutomirski
` (2 preceding siblings ...)
(?)
@ 2020-12-22 19:31 ` Andrea Arcangeli
2020-12-22 20:15 ` Matthew Wilcox
` (2 more replies)
-1 siblings, 3 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-22 19:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Linus Torvalds, Peter Xu, Nadav Amit, Yu Zhao, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Mon, Dec 21, 2020 at 07:19:35PM -0800, Andy Lutomirski wrote:
> instance of this (with mmap_sem held for write) in x86:
> mark_screen_rdonly(). Dare I ask how broken this is? We could likely
That one is buggy despite it takes the mmap_write_lock... inverting
the last two lines would fix it though.
- mmap_write_unlock(mm);
flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, PAGE_SHIFT, false);
+ mmap_write_unlock(mm);
The issue here is that rightfully wp_page_copy copies outside the PT
lock (good thing) and it correctly assumes during the copy nobody can
modify the page if the fault happened in a write access and the pte
had no _PAGE_WRITE bit set.
The bug is clearly in userfaultfd_writeprotect that doesn't enforce
the above invariant.
If the userfaultfd_wrprotect(mode_wp = false) run after the deferred
TLB flush this could never happen because the uffd_wp bit was still
set and the page fault would hit the handle_userfault dead end and
it'd have to be restarted from scratch.
Leaving stale tlb entries after dropping the PT lock and with only the
mmap_lock_read is only ok if the page fault has a "check" that catches
that specific case, so that specific case is fully serialized by the
PT lock alone and the "catch" path is fully aware about the stale TLB
entries that were left around.
So looking at the two cases that predates uffd-wp that already did a
RW->RO transition with the mmap_lock_read, they both comply with the
wp_page_copy invariant but with two different techniques:
1) change_protection is called by the scheduler with the
mmap_read_lock and with a deferred TLB flush, and things don't fall
apart completely there simply because we catch that in do_numa_page:
if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma))
+ /* catch it: no risk to end up in wp_page_copy even if the change_protection
+ is running concurrently and the tlb flush is still pending */
return do_numa_page(vmf);
do_numa_page doesn't issue any further tlb flush, but basically
restores the pte to the previous value, so then the pending flush
becomes a noop, since there was no modification to the pte in the
first place after do_numa_page runs.
2) ksm write_protect_page is also called with mmap_read_lock, and it
will simply not defer the flush. If the tlb flush is done inside
the PT lock it is ok to take mmap_read_lock since the page fault
code will not make any assumption about the TLB before reading the
pagetable content under PT lock.
In fact if mm_tlb_flush_pending(mm) is true, write_protect_page in
KSM has to issue an extra synchronous flush before dropping the PT
lock, even if it finds the _PAGE_WRITE bit is already clear,
precisely because there can be another deferred flush that cleared
the pte but didn't flush the TLB yet.
userfaultfd_writeprotect() is already using the technique in 1) to be
safe, except the un-wrprotect can break it if run while the tlb flush
is still pending...
The good thing is this guarantee must be provided not more granular
even than a vma, not per-mm and a vma cannot be registered on two
different uffd ctx at the same time, so the locking can happen all
internal to the uffd ctx.
My previous suggestion to use a mutex to serialize
userfaultfd_writeprotect with a mutex will still work, but we can run
as many wrprotect and un-wrprotect as we want in parallel, as long as
they're not simultaneous, we can do much better than a mutex.
Ideally we would need a new two_group_semaphore, where each group can
run as many parallel instances as it wants, but no instance of one
group can run in parallel with any instance of the other group. AFIK
such a kind of lock doesn't exist right now.
wrprotect if left alone never had an issue since we had a working
"catch" check for it well before wp_page_copy could run. unprotect
also if left alone was ok since it's a permission upgrade.
Alternatively, we can solve this with the same technique as in 2),
because all it matters is that the 4k pte modification doesn't take
the mmap_lock_write. A modification to a single 4k pte or a single 2m
hugepmd is very likely indication that there's going to be a flood of
those in parallel from all CPUs and likely there's also a flood of
page faults from all CPUs in parallel. In addition for a 4k wrprotect
or un-wrprotect there's not a significant benefit in deferring the TLB
flush.
I don't think we can take the mmap_write_lock unconditionally because
one of the main reasons to deal with the extra complexity of resolving
page faults in userland is to bypass mprotect entirely.
Example, JITs can use unprivileged uffd to eliminate the software-set
dirty bit every time it modifies memory, that would then require
frequent wrprotect/un-wrprotect during page faults and the less likely
we have to block for I/O during those CPU-only events, the
better. This is one of the potential use cases, but there's plenty
more.
So the alternative would be to take mmap_read_lock for "len <=
HPAGE_PMD_SIZE" and the mmap_write_lock otherwise, and to add a
parameter in change_protection to tell it to flush before dropping the
PT lock and not defer the flush. So this is going to be more intrusive
in the VM and it's overall unnecessary.
The below is mostly untested... but it'd be good to hear some feedback
before doing more work in this direction.
From 4ace4d1b53f5cb3b22a5c2dc33facc4150b112d6 Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Tue, 22 Dec 2020 14:30:16 -0500
Subject: [PATCH 1/1] mm: userfaultfd: avoid leaving stale TLB after
userfaultfd_writeprotect()
change_protection() is called by userfaultfd_writeprotect() with the
mmap_lock_read like in change_prot_numa().
The page fault code in wp_copy_page() rightfully assumes if the CPU
issued a write fault and the write bit in the pagetable is not set, no
CPU can write to the page. That's wrong assumption after
userfaultfd_writeprotect(). That's also wrong assumption after
change_prot_numa() where the regular page fault code also would assume
that if the present bit is not set and the page fault is running,
there should be no stale TLB entry, but there is still.
So to stay safe, the page fault code must be prevented to run as long
as long as the TLB flush remains pending. That is already achieved by
the do_numa_page() path for change_prot_numa() and by the
userfaultfd_pte_wp() path for userfaultfd_writeprotect().
The problem that needs fixing is that an un-wrprotect
(i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP not
set) could run in between the original wrprotect
(i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP set)
and wp_copy_page, while the TLB flush remains pending.
In such case the userfaultfd_pte_wp() check will stop being effective
to prevent the regular page fault code to run.
CPU0 CPU 1 CPU 2
------ -------- -------
userfaultfd_wrprotect(mode_wp = true)
PT lock
atomic set _PAGE_UFFD_WP and clear _PAGE_WRITE
PT unlock
do_page_fault FAULT_FLAG_WRITE
userfaultfd_wrprotect(mode_wp = false)
PT lock
ATOMIC clear _PAGE_UFFD_WP <- problem
/* _PAGE_WRITE not set */
PT unlock
XXXXXXXXXXXXXX BUG RACE window open here
PT lock
FAULT_FLAG_WRITE is set by CPU
_PAGE_WRITE is still clear in pte
PT unlock
wp_page_copy
copy_user_page runs with stale TLB
deferred tlb flush <- too late
XXXXXXXXXXXXXX BUG RACE window close here
================================================================================
If the userfaultfd_wrprotect(mode_wp = false) can only run after the
deferred TLB flush the above cannot happen either, because the uffd_wp
bit will remain set until after the TLB flush and the page fault would
reliably hit the handle_userfault() dead end as long as the TLB is
stale.
So to fix this we need to prevent any un-wrprotect to start until the
last outstanding wrprotect completed and to prevent any further
wrprotect until the last outstanding un-wrprotect completed. Then
wp_page_copy can still run in parallel but only with the un-wrprotect,
and that's fine since it's a permission promotion.
We would need a new two_group_semaphore, where each group can run as
many parallel instances as it wants, but no instance of one group can
run in parallel with any instance of the other group. The below rwsem
with two atomics approximates that kind lock.
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
fs/userfaultfd.c | 39 +++++++++++++++++++++++++++++++++++++++
mm/memory.c | 20 ++++++++++++++++++++
2 files changed, 59 insertions(+)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 894cc28142e7..3729ca99dae5 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -76,6 +76,20 @@ struct userfaultfd_ctx {
bool mmap_changing;
/* mm with one ore more vmas attached to this userfaultfd_ctx */
struct mm_struct *mm;
+ /*
+ * We cannot let userfaultd_writeprotect(mode_wp = false)
+ * clear _PAGE_UFFD_WP from the pgtable, until the last
+ * outstanding userfaultd_writeprotect(mode_wp = true)
+ * completes, because the TLB flush is deferred. The page
+ * fault must be forced to enter handle_userfault() while the
+ * TLB flush is deferred, and that's achieved with the
+ * _PAGE_UFFD_WP bit. The _PAGE_UFFD_WP can only be cleared
+ * after there are no stale TLB entries left, only then it'll
+ * be safe again for the page fault to continue and not hit
+ * the handle_userfault() dead end anymore.
+ */
+ struct rw_semaphore wrprotect_rwsem;
+ atomic64_t wrprotect_count[2];
};
struct userfaultfd_fork_ctx {
@@ -1783,6 +1797,7 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
struct uffdio_writeprotect __user *user_uffdio_wp;
struct userfaultfd_wake_range range;
bool mode_wp, mode_dontwake;
+ bool contention;
if (READ_ONCE(ctx->mmap_changing))
return -EAGAIN;
@@ -1808,9 +1823,30 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
if (mode_wp && mode_dontwake)
return -EINVAL;
+ VM_WARN_ON(atomic64_read(&ctx->wrprotect_count[0]) < 0);
+ VM_WARN_ON(atomic64_read(&ctx->wrprotect_count[1]) < 0);
+ atomic64_inc(&ctx->wrprotect_count[mode_wp ? 0 : 1]);
+ smp_mb__after_atomic();
+ contention = atomic64_read(&ctx->wrprotect_count[mode_wp ? 1 : 0]) > 0;
+ if (!contention)
+ down_read(&ctx->wrprotect_rwsem);
+ else {
+ down_write(&ctx->wrprotect_rwsem);
+ if (!atomic64_read(&ctx->wrprotect_count[mode_wp ? 1 : 0])) {
+ contention = false;
+ downgrade_write(&ctx->wrprotect_rwsem);
+ }
+
+ }
ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
uffdio_wp.range.len, mode_wp,
&ctx->mmap_changing);
+ if (!contention)
+ up_read(&ctx->wrprotect_rwsem);
+ else
+ up_write(&ctx->wrprotect_rwsem);
+ smp_mb__before_atomic();
+ atomic64_dec(&ctx->wrprotect_count[mode_wp ? 0 : 1]);
if (ret)
return ret;
@@ -1958,6 +1994,9 @@ static void init_once_userfaultfd_ctx(void *mem)
init_waitqueue_head(&ctx->fault_wqh);
init_waitqueue_head(&ctx->event_wqh);
init_waitqueue_head(&ctx->fd_wqh);
+ init_rwsem(&ctx->wrprotect_rwsem);
+ atomic64_set(&ctx->wrprotect_count[0], 0);
+ atomic64_set(&ctx->wrprotect_count[1], 0);
seqcount_spinlock_init(&ctx->refile_seq, &ctx->fault_pending_wqh.lock);
}
diff --git a/mm/memory.c b/mm/memory.c
index 7d608765932b..4ff9f21d5a7b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3085,6 +3085,12 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
{
struct vm_area_struct *vma = vmf->vma;
+ /*
+ * STALE_TLB_WARNING: while the uffd_wp bit is set, the TLB
+ * can be stale. We cannot allow do_wp_page to proceed or
+ * it'll wrongly assume that nobody can still be writing to
+ * the page if !pte_write.
+ */
if (userfaultfd_pte_wp(vma, *vmf->pte)) {
pte_unmap_unlock(vmf->pte, vmf->ptl);
return handle_userfault(vmf, VM_UFFD_WP);
@@ -4274,6 +4280,12 @@ static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf, pmd_t orig_pmd)
{
if (vma_is_anonymous(vmf->vma)) {
+ /*
+ * STALE_TLB_WARNING: while the uffd_wp bit is set,
+ * the TLB can be stale. We cannot allow wp_huge_pmd()
+ * to proceed or it'll wrongly assume that nobody can
+ * still be writing to the page if !pmd_write.
+ */
if (userfaultfd_huge_pmd_wp(vmf->vma, orig_pmd))
return handle_userfault(vmf, VM_UFFD_WP);
return do_huge_pmd_wp_page(vmf, orig_pmd);
@@ -4388,6 +4400,10 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
if (!pte_present(vmf->orig_pte))
return do_swap_page(vmf);
+ /*
+ * STALE_TLB_WARNING: if the pte is NUMA protnone the TLB can
+ * be stale.
+ */
if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma))
return do_numa_page(vmf);
@@ -4503,6 +4519,10 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
return 0;
}
if (pmd_trans_huge(orig_pmd) || pmd_devmap(orig_pmd)) {
+ /*
+ * STALE_TLB_WARNING: if the pmd is NUMA
+ * protnone the TLB can be stale.
+ */
if (pmd_protnone(orig_pmd) && vma_is_accessible(vma))
return do_huge_pmd_numa_page(&vmf, orig_pmd);
^ permalink raw reply related [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 19:31 ` Andrea Arcangeli
@ 2020-12-22 20:15 ` Matthew Wilcox
2020-12-22 20:26 ` Andrea Arcangeli
2020-12-22 21:14 ` Yu Zhao
2020-12-22 21:14 ` Nadav Amit
2 siblings, 1 reply; 141+ messages in thread
From: Matthew Wilcox @ 2020-12-22 20:15 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, Yu Zhao,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra,
Kent Overstreet
On Tue, Dec 22, 2020 at 02:31:52PM -0500, Andrea Arcangeli wrote:
> My previous suggestion to use a mutex to serialize
> userfaultfd_writeprotect with a mutex will still work, but we can run
> as many wrprotect and un-wrprotect as we want in parallel, as long as
> they're not simultaneous, we can do much better than a mutex.
>
> Ideally we would need a new two_group_semaphore, where each group can
> run as many parallel instances as it wants, but no instance of one
> group can run in parallel with any instance of the other group. AFIK
> such a kind of lock doesn't exist right now.
Kent and I worked on one for a bit, and we called it a red-black mutex.
If team red had the lock, more members of team red could join in.
If team black had the lock, more members of team black could join in.
I forget what our rule was around fairness (if team red has the lock,
and somebody from team black is waiting, can another member of team red
take the lock, or must they block?)
It was to solve the direct-IO vs buffered-IO problem (you can have as many
direct-IO readers/writers at once or you can have as many buffered-IO
readers/writers at once, but exclude a mix of direct and buffered I/O).
In the end, we decided it didn't work all that well.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 20:15 ` Matthew Wilcox
@ 2020-12-22 20:26 ` Andrea Arcangeli
0 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-22 20:26 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, Yu Zhao,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra,
Kent Overstreet
On Tue, Dec 22, 2020 at 08:15:53PM +0000, Matthew Wilcox wrote:
> On Tue, Dec 22, 2020 at 02:31:52PM -0500, Andrea Arcangeli wrote:
> > My previous suggestion to use a mutex to serialize
> > userfaultfd_writeprotect with a mutex will still work, but we can run
> > as many wrprotect and un-wrprotect as we want in parallel, as long as
> > they're not simultaneous, we can do much better than a mutex.
> >
> > Ideally we would need a new two_group_semaphore, where each group can
> > run as many parallel instances as it wants, but no instance of one
> > group can run in parallel with any instance of the other group. AFIK
> > such a kind of lock doesn't exist right now.
>
> Kent and I worked on one for a bit, and we called it a red-black mutex.
> If team red had the lock, more members of team red could join in.
> If team black had the lock, more members of team black could join in.
> I forget what our rule was around fairness (if team red has the lock,
> and somebody from team black is waiting, can another member of team red
> take the lock, or must they block?)
In this case they would need to block and provide full fairness.
Well maybe just a bit of unfariness (to let a few more through the
door before it shuts) wouldn't be a deal breaker but it would need to
be bound or it'd starve the other color/side indefinitely. Otherwise
an ioctl mode_wp = true would block forever, if more ioctl mode_wp =
false keep coming in other CPUs (or the other way around).
The approximation with rwsem and two atomics provides full fariness in
both read and write mode (originally the read would stave the write
IIRC which was an issue for all mprotect etc.. not anymore thankfully).
> It was to solve the direct-IO vs buffered-IO problem (you can have as many
> direct-IO readers/writers at once or you can have as many buffered-IO
> readers/writers at once, but exclude a mix of direct and buffered I/O).
> In the end, we decided it didn't work all that well.
Well mixing buffered and direct-IO is certainly not a good practice so
it's reasonable to leave it up to userland to serialize if such mix is
needed, the kernel behavior is undefined if the mix is concurrent out
of order.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 19:31 ` Andrea Arcangeli
2020-12-22 20:15 ` Matthew Wilcox
@ 2020-12-22 21:14 ` Yu Zhao
2020-12-22 22:02 ` Andrea Arcangeli
2020-12-22 21:14 ` Nadav Amit
2 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2020-12-22 21:14 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 02:31:52PM -0500, Andrea Arcangeli wrote:
> On Mon, Dec 21, 2020 at 07:19:35PM -0800, Andy Lutomirski wrote:
> > instance of this (with mmap_sem held for write) in x86:
> > mark_screen_rdonly(). Dare I ask how broken this is? We could likely
>
> That one is buggy despite it takes the mmap_write_lock... inverting
> the last two lines would fix it though.
>
> - mmap_write_unlock(mm);
> flush_tlb_mm_range(mm, 0xA0000, 0xA0000 + 32*PAGE_SIZE, PAGE_SHIFT, false);
> + mmap_write_unlock(mm);
>
> The issue here is that rightfully wp_page_copy copies outside the PT
> lock (good thing) and it correctly assumes during the copy nobody can
> modify the page if the fault happened in a write access and the pte
> had no _PAGE_WRITE bit set.
>
> The bug is clearly in userfaultfd_writeprotect that doesn't enforce
> the above invariant.
>
> If the userfaultfd_wrprotect(mode_wp = false) run after the deferred
> TLB flush this could never happen because the uffd_wp bit was still
> set and the page fault would hit the handle_userfault dead end and
> it'd have to be restarted from scratch.
>
> Leaving stale tlb entries after dropping the PT lock and with only the
> mmap_lock_read is only ok if the page fault has a "check" that catches
> that specific case, so that specific case is fully serialized by the
> PT lock alone and the "catch" path is fully aware about the stale TLB
> entries that were left around.
>
> So looking at the two cases that predates uffd-wp that already did a
> RW->RO transition with the mmap_lock_read, they both comply with the
> wp_page_copy invariant but with two different techniques:
>
> 1) change_protection is called by the scheduler with the
> mmap_read_lock and with a deferred TLB flush, and things don't fall
> apart completely there simply because we catch that in do_numa_page:
>
> if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma))
> + /* catch it: no risk to end up in wp_page_copy even if the change_protection
> + is running concurrently and the tlb flush is still pending */
> return do_numa_page(vmf);
>
> do_numa_page doesn't issue any further tlb flush, but basically
> restores the pte to the previous value, so then the pending flush
> becomes a noop, since there was no modification to the pte in the
> first place after do_numa_page runs.
>
> 2) ksm write_protect_page is also called with mmap_read_lock, and it
> will simply not defer the flush. If the tlb flush is done inside
> the PT lock it is ok to take mmap_read_lock since the page fault
> code will not make any assumption about the TLB before reading the
> pagetable content under PT lock.
>
> In fact if mm_tlb_flush_pending(mm) is true, write_protect_page in
> KSM has to issue an extra synchronous flush before dropping the PT
> lock, even if it finds the _PAGE_WRITE bit is already clear,
> precisely because there can be another deferred flush that cleared
> the pte but didn't flush the TLB yet.
>
> userfaultfd_writeprotect() is already using the technique in 1) to be
> safe, except the un-wrprotect can break it if run while the tlb flush
> is still pending...
>
> The good thing is this guarantee must be provided not more granular
> even than a vma, not per-mm and a vma cannot be registered on two
> different uffd ctx at the same time, so the locking can happen all
> internal to the uffd ctx.
>
> My previous suggestion to use a mutex to serialize
> userfaultfd_writeprotect with a mutex will still work, but we can run
> as many wrprotect and un-wrprotect as we want in parallel, as long as
> they're not simultaneous, we can do much better than a mutex.
>
> Ideally we would need a new two_group_semaphore, where each group can
> run as many parallel instances as it wants, but no instance of one
> group can run in parallel with any instance of the other group. AFIK
> such a kind of lock doesn't exist right now.
>
> wrprotect if left alone never had an issue since we had a working
> "catch" check for it well before wp_page_copy could run. unprotect
> also if left alone was ok since it's a permission upgrade.
>
> Alternatively, we can solve this with the same technique as in 2),
> because all it matters is that the 4k pte modification doesn't take
> the mmap_lock_write. A modification to a single 4k pte or a single 2m
> hugepmd is very likely indication that there's going to be a flood of
> those in parallel from all CPUs and likely there's also a flood of
> page faults from all CPUs in parallel. In addition for a 4k wrprotect
> or un-wrprotect there's not a significant benefit in deferring the TLB
> flush.
>
> I don't think we can take the mmap_write_lock unconditionally because
> one of the main reasons to deal with the extra complexity of resolving
> page faults in userland is to bypass mprotect entirely.
>
> Example, JITs can use unprivileged uffd to eliminate the software-set
> dirty bit every time it modifies memory, that would then require
> frequent wrprotect/un-wrprotect during page faults and the less likely
> we have to block for I/O during those CPU-only events, the
> better. This is one of the potential use cases, but there's plenty
> more.
>
> So the alternative would be to take mmap_read_lock for "len <=
> HPAGE_PMD_SIZE" and the mmap_write_lock otherwise, and to add a
> parameter in change_protection to tell it to flush before dropping the
> PT lock and not defer the flush. So this is going to be more intrusive
> in the VM and it's overall unnecessary.
>
> The below is mostly untested... but it'd be good to hear some feedback
> before doing more work in this direction.
This works but I don't prefer this option because 1) this is new
way of making pte_wrprotect safe and 2) it's specific to ufd and
can't be applied to clear_soft_dirty() which has no "catcher". No
matter how good the documentation about this new way is now, it
will be out of date, speaking from my personal experience.
I'd go with what Nadav has -- the memory corruption problem has been
there for a while and nobody has complained except Nadav. This makes
me think people is less likely to notice any performance issues from
holding mmap_lock for write in his patch either.
But I can't say I have zero concern with the potential performance
impact given that I have been expecting the fix to go to stable,
which I care most. So the next option on my list is to have a
common "catcher" in do_wp_page() which singles out pages that have
page_mapcount equal to one and reuse them instead of trying to
make copies when they have elevated page_count or are locked. Think
this as a common "catcher" for anybody who pte_wrprotect but doesn't
belong to the COW category (ksm, fork, etc.). I have to stress that
I don't know if this can be done confidently enough to be included
in stable, hence the second option on my list.
> From 4ace4d1b53f5cb3b22a5c2dc33facc4150b112d6 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Tue, 22 Dec 2020 14:30:16 -0500
> Subject: [PATCH 1/1] mm: userfaultfd: avoid leaving stale TLB after
> userfaultfd_writeprotect()
>
> change_protection() is called by userfaultfd_writeprotect() with the
> mmap_lock_read like in change_prot_numa().
>
> The page fault code in wp_copy_page() rightfully assumes if the CPU
> issued a write fault and the write bit in the pagetable is not set, no
> CPU can write to the page. That's wrong assumption after
> userfaultfd_writeprotect(). That's also wrong assumption after
> change_prot_numa() where the regular page fault code also would assume
> that if the present bit is not set and the page fault is running,
> there should be no stale TLB entry, but there is still.
>
> So to stay safe, the page fault code must be prevented to run as long
> as long as the TLB flush remains pending. That is already achieved by
> the do_numa_page() path for change_prot_numa() and by the
> userfaultfd_pte_wp() path for userfaultfd_writeprotect().
>
> The problem that needs fixing is that an un-wrprotect
> (i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP not
> set) could run in between the original wrprotect
> (i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP set)
> and wp_copy_page, while the TLB flush remains pending.
>
> In such case the userfaultfd_pte_wp() check will stop being effective
> to prevent the regular page fault code to run.
>
> CPU0 CPU 1 CPU 2
> ------ -------- -------
> userfaultfd_wrprotect(mode_wp = true)
> PT lock
> atomic set _PAGE_UFFD_WP and clear _PAGE_WRITE
> PT unlock
>
> do_page_fault FAULT_FLAG_WRITE
> userfaultfd_wrprotect(mode_wp = false)
> PT lock
> ATOMIC clear _PAGE_UFFD_WP <- problem
> /* _PAGE_WRITE not set */
> PT unlock
> XXXXXXXXXXXXXX BUG RACE window open here
>
> PT lock
> FAULT_FLAG_WRITE is set by CPU
> _PAGE_WRITE is still clear in pte
> PT unlock
>
> wp_page_copy
> copy_user_page runs with stale TLB
>
> deferred tlb flush <- too late
> XXXXXXXXXXXXXX BUG RACE window close here
> ================================================================================
>
> If the userfaultfd_wrprotect(mode_wp = false) can only run after the
> deferred TLB flush the above cannot happen either, because the uffd_wp
> bit will remain set until after the TLB flush and the page fault would
> reliably hit the handle_userfault() dead end as long as the TLB is
> stale.
>
> So to fix this we need to prevent any un-wrprotect to start until the
> last outstanding wrprotect completed and to prevent any further
> wrprotect until the last outstanding un-wrprotect completed. Then
> wp_page_copy can still run in parallel but only with the un-wrprotect,
> and that's fine since it's a permission promotion.
>
> We would need a new two_group_semaphore, where each group can run as
> many parallel instances as it wants, but no instance of one group can
> run in parallel with any instance of the other group. The below rwsem
> with two atomics approximates that kind lock.
>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
> fs/userfaultfd.c | 39 +++++++++++++++++++++++++++++++++++++++
> mm/memory.c | 20 ++++++++++++++++++++
> 2 files changed, 59 insertions(+)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 894cc28142e7..3729ca99dae5 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -76,6 +76,20 @@ struct userfaultfd_ctx {
> bool mmap_changing;
> /* mm with one ore more vmas attached to this userfaultfd_ctx */
> struct mm_struct *mm;
> + /*
> + * We cannot let userfaultd_writeprotect(mode_wp = false)
> + * clear _PAGE_UFFD_WP from the pgtable, until the last
> + * outstanding userfaultd_writeprotect(mode_wp = true)
> + * completes, because the TLB flush is deferred. The page
> + * fault must be forced to enter handle_userfault() while the
> + * TLB flush is deferred, and that's achieved with the
> + * _PAGE_UFFD_WP bit. The _PAGE_UFFD_WP can only be cleared
> + * after there are no stale TLB entries left, only then it'll
> + * be safe again for the page fault to continue and not hit
> + * the handle_userfault() dead end anymore.
> + */
> + struct rw_semaphore wrprotect_rwsem;
> + atomic64_t wrprotect_count[2];
> };
>
> struct userfaultfd_fork_ctx {
> @@ -1783,6 +1797,7 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
> struct uffdio_writeprotect __user *user_uffdio_wp;
> struct userfaultfd_wake_range range;
> bool mode_wp, mode_dontwake;
> + bool contention;
>
> if (READ_ONCE(ctx->mmap_changing))
> return -EAGAIN;
> @@ -1808,9 +1823,30 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
> if (mode_wp && mode_dontwake)
> return -EINVAL;
>
> + VM_WARN_ON(atomic64_read(&ctx->wrprotect_count[0]) < 0);
> + VM_WARN_ON(atomic64_read(&ctx->wrprotect_count[1]) < 0);
> + atomic64_inc(&ctx->wrprotect_count[mode_wp ? 0 : 1]);
> + smp_mb__after_atomic();
> + contention = atomic64_read(&ctx->wrprotect_count[mode_wp ? 1 : 0]) > 0;
> + if (!contention)
> + down_read(&ctx->wrprotect_rwsem);
> + else {
> + down_write(&ctx->wrprotect_rwsem);
> + if (!atomic64_read(&ctx->wrprotect_count[mode_wp ? 1 : 0])) {
> + contention = false;
> + downgrade_write(&ctx->wrprotect_rwsem);
> + }
> +
> + }
> ret = mwriteprotect_range(ctx->mm, uffdio_wp.range.start,
> uffdio_wp.range.len, mode_wp,
> &ctx->mmap_changing);
> + if (!contention)
> + up_read(&ctx->wrprotect_rwsem);
> + else
> + up_write(&ctx->wrprotect_rwsem);
> + smp_mb__before_atomic();
> + atomic64_dec(&ctx->wrprotect_count[mode_wp ? 0 : 1]);
> if (ret)
> return ret;
>
> @@ -1958,6 +1994,9 @@ static void init_once_userfaultfd_ctx(void *mem)
> init_waitqueue_head(&ctx->fault_wqh);
> init_waitqueue_head(&ctx->event_wqh);
> init_waitqueue_head(&ctx->fd_wqh);
> + init_rwsem(&ctx->wrprotect_rwsem);
> + atomic64_set(&ctx->wrprotect_count[0], 0);
> + atomic64_set(&ctx->wrprotect_count[1], 0);
> seqcount_spinlock_init(&ctx->refile_seq, &ctx->fault_pending_wqh.lock);
> }
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 7d608765932b..4ff9f21d5a7b 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3085,6 +3085,12 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
>
> + /*
> + * STALE_TLB_WARNING: while the uffd_wp bit is set, the TLB
> + * can be stale. We cannot allow do_wp_page to proceed or
> + * it'll wrongly assume that nobody can still be writing to
> + * the page if !pte_write.
> + */
> if (userfaultfd_pte_wp(vma, *vmf->pte)) {
> pte_unmap_unlock(vmf->pte, vmf->ptl);
> return handle_userfault(vmf, VM_UFFD_WP);
> @@ -4274,6 +4280,12 @@ static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
> static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf, pmd_t orig_pmd)
> {
> if (vma_is_anonymous(vmf->vma)) {
> + /*
> + * STALE_TLB_WARNING: while the uffd_wp bit is set,
> + * the TLB can be stale. We cannot allow wp_huge_pmd()
> + * to proceed or it'll wrongly assume that nobody can
> + * still be writing to the page if !pmd_write.
> + */
> if (userfaultfd_huge_pmd_wp(vmf->vma, orig_pmd))
> return handle_userfault(vmf, VM_UFFD_WP);
> return do_huge_pmd_wp_page(vmf, orig_pmd);
> @@ -4388,6 +4400,10 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> if (!pte_present(vmf->orig_pte))
> return do_swap_page(vmf);
>
> + /*
> + * STALE_TLB_WARNING: if the pte is NUMA protnone the TLB can
> + * be stale.
> + */
> if (pte_protnone(vmf->orig_pte) && vma_is_accessible(vmf->vma))
> return do_numa_page(vmf);
>
> @@ -4503,6 +4519,10 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
> return 0;
> }
> if (pmd_trans_huge(orig_pmd) || pmd_devmap(orig_pmd)) {
> + /*
> + * STALE_TLB_WARNING: if the pmd is NUMA
> + * protnone the TLB can be stale.
> + */
> if (pmd_protnone(orig_pmd) && vma_is_accessible(vma))
> return do_huge_pmd_numa_page(&vmf, orig_pmd);
>
>
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 21:14 ` Yu Zhao
@ 2020-12-22 22:02 ` Andrea Arcangeli
2020-12-22 23:39 ` Yu Zhao
0 siblings, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-22 22:02 UTC (permalink / raw)
To: Yu Zhao
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 02:14:41PM -0700, Yu Zhao wrote:
> This works but I don't prefer this option because 1) this is new
> way of making pte_wrprotect safe and 2) it's specific to ufd and
> can't be applied to clear_soft_dirty() which has no "catcher". No
I didn't look into clear_soft_dirty issue, I can look into that.
To avoid having to deal with a 3rd solution it will have to use one of
the two:
1) avoid deferring tlb flush and enforce a sync flush before dropping
the PT lock even if mm_mm_tlb_flush_pending is true ->
write_protect_page in KSM
2) add its own new catcher for its own specific marker (for UFFD-WP
the marker is _PAGE_UFFD_WP, for change_prot_numa is PROT_NONE on a
vma->vm_pgprot not PROT_NONE, for soft dirty it could be
_PAGE_SOFT_DIRTY) to send the page fault to a dead end before the
pte value is interpreted.
> matter how good the documentation about this new way is now, it
> will be out of date, speaking from my personal experience.
A common catcher for all 3 is not possible because each catcher
depends on whatever marker and whatever pte value they set that may
lead to a different deterministic path where to put the catcher or
multiple paths even. do_numa_page requires a catcher in a different
place already.
Or well, a common catcher for all 3 is technically possible but it'd
perform slower requiring to check things twice.
But perhaps the soft_dirty can use the same catcher of uffd-wp given
the similarity?
> I'd go with what Nadav has -- the memory corruption problem has been
> there for a while and nobody has complained except Nadav. This makes
> me think people is less likely to notice any performance issues from
> holding mmap_lock for write in his patch either.
uffd-wp is a fairly new feature, the current users are irrelevant,
keeping it optimal is just for the future potential.
> But I can't say I have zero concern with the potential performance
> impact given that I have been expecting the fix to go to stable,
> which I care most. So the next option on my list is to have a
Actually stable would be very fine to go with Nadav patch and use the
mmap_lock_write unconditionally. The optimal fix is only relevant for
the future potential, so it's only relevant for Linus's tree.
However the feature is recent enough that it won't require a deep
backport so the optimal fix is likely fine for stable as well,
generally stable prefers the same fix as in the upstream when there's
no major backport rejection issue.
The alternative solution for uffd is to do the deferred flush under
mmap_lock_write if len is > HPAGE_PMD_SIZE, or to tell
change_protection not to defer the flush and to take the
mmap_lock_read for <= HPAGE_PMD_SIZE. That will avoid depending on the
catcher and then userfaultfd_writeprotect(mode_wp=true)
userfaultfd_writeprotect(mode_wp=false) can even run in parallel at
all times. The cons is large userfaultfd_writeprotect will block for
I/O and those would happen at least in the postcopy live snapshotting
use case.
The main cons is that it'd require modification to change_protection
so it actually looks more intrusive, not less.
Overall anything that allows to wrprotect 1 pte with only the
mmap_lock_read exactly like KSM write_protect_page, would be enough for
uffd-wp.
What isn't ok in terms of future potential is unconditional
mmap_lock_write as in the original suggested patch in my view. It
doesn't mean we can take mmap_lock_write when the operation is large
and there is actually more benefit from deferring the flush.
> common "catcher" in do_wp_page() which singles out pages that have
> page_mapcount equal to one and reuse them instead of trying to
I don't think the page_mapcount matters here. If the wp page reuse was
more accurate (as it was before) we wouldn't notice this issue, but it
still would be a bug that there were stale TLB entries. It worked by
luck.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 22:02 ` Andrea Arcangeli
@ 2020-12-22 23:39 ` Yu Zhao
2020-12-22 23:50 ` Linus Torvalds
2020-12-23 2:56 ` Andrea Arcangeli
0 siblings, 2 replies; 141+ messages in thread
From: Yu Zhao @ 2020-12-22 23:39 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 05:02:37PM -0500, Andrea Arcangeli wrote:
> On Tue, Dec 22, 2020 at 02:14:41PM -0700, Yu Zhao wrote:
> > This works but I don't prefer this option because 1) this is new
> > way of making pte_wrprotect safe and 2) it's specific to ufd and
> > can't be applied to clear_soft_dirty() which has no "catcher". No
>
> I didn't look into clear_soft_dirty issue, I can look into that.
>
> To avoid having to deal with a 3rd solution it will have to use one of
> the two:
>
> 1) avoid deferring tlb flush and enforce a sync flush before dropping
> the PT lock even if mm_mm_tlb_flush_pending is true ->
> write_protect_page in KSM
>
> 2) add its own new catcher for its own specific marker (for UFFD-WP
> the marker is _PAGE_UFFD_WP, for change_prot_numa is PROT_NONE on a
> vma->vm_pgprot not PROT_NONE, for soft dirty it could be
> _PAGE_SOFT_DIRTY) to send the page fault to a dead end before the
> pte value is interpreted.
>
> > matter how good the documentation about this new way is now, it
> > will be out of date, speaking from my personal experience.
>
> A common catcher for all 3 is not possible because each catcher
> depends on whatever marker and whatever pte value they set that may
> lead to a different deterministic path where to put the catcher or
> multiple paths even. do_numa_page requires a catcher in a different
> place already.
>
> Or well, a common catcher for all 3 is technically possible but it'd
> perform slower requiring to check things twice.
>
> But perhaps the soft_dirty can use the same catcher of uffd-wp given
> the similarity?
>
> > I'd go with what Nadav has -- the memory corruption problem has been
> > there for a while and nobody has complained except Nadav. This makes
> > me think people is less likely to notice any performance issues from
> > holding mmap_lock for write in his patch either.
>
> uffd-wp is a fairly new feature, the current users are irrelevant,
> keeping it optimal is just for the future potential.
>
> > But I can't say I have zero concern with the potential performance
> > impact given that I have been expecting the fix to go to stable,
> > which I care most. So the next option on my list is to have a
>
> Actually stable would be very fine to go with Nadav patch and use the
> mmap_lock_write unconditionally. The optimal fix is only relevant for
> the future potential, so it's only relevant for Linus's tree.
>
> However the feature is recent enough that it won't require a deep
> backport so the optimal fix is likely fine for stable as well,
> generally stable prefers the same fix as in the upstream when there's
> no major backport rejection issue.
>
> The alternative solution for uffd is to do the deferred flush under
> mmap_lock_write if len is > HPAGE_PMD_SIZE, or to tell
> change_protection not to defer the flush and to take the
> mmap_lock_read for <= HPAGE_PMD_SIZE. That will avoid depending on the
> catcher and then userfaultfd_writeprotect(mode_wp=true)
> userfaultfd_writeprotect(mode_wp=false) can even run in parallel at
> all times. The cons is large userfaultfd_writeprotect will block for
> I/O and those would happen at least in the postcopy live snapshotting
> use case.
>
> The main cons is that it'd require modification to change_protection
> so it actually looks more intrusive, not less.
>
> Overall anything that allows to wrprotect 1 pte with only the
> mmap_lock_read exactly like KSM write_protect_page, would be enough for
> uffd-wp.
>
> What isn't ok in terms of future potential is unconditional
> mmap_lock_write as in the original suggested patch in my view. It
> doesn't mean we can take mmap_lock_write when the operation is large
> and there is actually more benefit from deferring the flush.
>
> > common "catcher" in do_wp_page() which singles out pages that have
> > page_mapcount equal to one and reuse them instead of trying to
>
> I don't think the page_mapcount matters here. If the wp page reuse was
> more accurate (as it was before) we wouldn't notice this issue, but it
> still would be a bug that there were stale TLB entries. It worked by
> luck.
Can you please correct my understanding below? Thank you.
Generally speaking, we have four possible combinations relevant to
this discussion:
1) anon, COW
2) anon, non-COW
3) file, COW
4) file, non-COW
Assume we pte_wrprotect while holding mmap_lock for read, all four
combinations can be routed to do_wp_page(). The difference is that
1) and 3) are guaranteed to be RO when they become COW, and what we
do on top of their existing state doesn't make any difference.
2) is the false positive because of what we do, and it's causing the
memory corruption because do_wp_page() tries to make copies of pages
that seem to be RO but may have stale RW tlb entries pending flush.
If we grab mmap_lock for write, we block the fault that tries to copy
before we flush. Once it's done, we'll have two faulting CPUs, one
that had the stale entry and would otherwise do the damage and the
other that tries to copy. Then there is the silly part: we make a copy
and swap it with the only user who already has it...
We are talking about non-COW anon pages here -- they can't be mapped
more than once. So why not just identify them by checking
page_mapcount == 1 and then unconditionally reuse them? (This is
probably where I've missed things.)
4) may also be false positive (there are other legit cases relying on
it), and it's correctly identified by !PageAnon() +
VM_WRITE|VM_SHARED. For this category, we always reuse and we hitch
a free ride.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 23:39 ` Yu Zhao
@ 2020-12-22 23:50 ` Linus Torvalds
2020-12-23 2:56 ` Andrea Arcangeli
1 sibling, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-22 23:50 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:39 PM Yu Zhao <yuzhao@google.com> wrote:
>
> 2) is the false positive because of what we do, and it's causing the
> memory corruption because do_wp_page() tries to make copies of pages
> that seem to be RO but may have stale RW tlb entries pending flush.
Yeah, that's definitely a different bug.
The rule is that the TLB flush has to be done before the page table
lock is released.
See zap_pte_range() for an example of doing it right, even in the
presence of complexities (ie that has an example of both flushing the
TLB, and doing the actual "free the pages after flush", and it does
the two cases separately).
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-22 23:50 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-22 23:50 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:39 PM Yu Zhao <yuzhao@google.com> wrote:
>
> 2) is the false positive because of what we do, and it's causing the
> memory corruption because do_wp_page() tries to make copies of pages
> that seem to be RO but may have stale RW tlb entries pending flush.
Yeah, that's definitely a different bug.
The rule is that the TLB flush has to be done before the page table
lock is released.
See zap_pte_range() for an example of doing it right, even in the
presence of complexities (ie that has an example of both flushing the
TLB, and doing the actual "free the pages after flush", and it does
the two cases separately).
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 23:50 ` Linus Torvalds
@ 2020-12-23 0:01 ` Linus Torvalds
-1 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 0:01 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:50 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> See zap_pte_range() for an example of doing it right, even in the
> presence of complexities (ie that has an example of both flushing the
> TLB, and doing the actual "free the pages after flush", and it does
> the two cases separately).
The more I look at the mprotect code, the less I like it. We seem to
be much better about the TLB flushes in other places (looking at
mremap, for example). The mprotect code seems to be very laissez-faire
about the TLB flushing.
Does adding a TLB flush to before that
pte_unmap_unlock(pte - 1, ptl);
fix things for you?
That's not the right fix - leaving a stale TLB entry around is fine if
the TLB entry is more strict wrt protections - but it might be worth
testing as a "does it at least close the problem" patch.
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-23 0:01 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 0:01 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:50 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> See zap_pte_range() for an example of doing it right, even in the
> presence of complexities (ie that has an example of both flushing the
> TLB, and doing the actual "free the pages after flush", and it does
> the two cases separately).
The more I look at the mprotect code, the less I like it. We seem to
be much better about the TLB flushes in other places (looking at
mremap, for example). The mprotect code seems to be very laissez-faire
about the TLB flushing.
Does adding a TLB flush to before that
pte_unmap_unlock(pte - 1, ptl);
fix things for you?
That's not the right fix - leaving a stale TLB entry around is fine if
the TLB entry is more strict wrt protections - but it might be worth
testing as a "does it at least close the problem" patch.
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 0:01 ` Linus Torvalds
(?)
@ 2020-12-23 0:23 ` Yu Zhao
2020-12-23 2:17 ` Andrea Arcangeli
-1 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2020-12-23 0:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 04:01:45PM -0800, Linus Torvalds wrote:
> On Tue, Dec 22, 2020 at 3:50 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > See zap_pte_range() for an example of doing it right, even in the
> > presence of complexities (ie that has an example of both flushing the
> > TLB, and doing the actual "free the pages after flush", and it does
> > the two cases separately).
>
> The more I look at the mprotect code, the less I like it. We seem to
> be much better about the TLB flushes in other places (looking at
> mremap, for example). The mprotect code seems to be very laissez-faire
> about the TLB flushing.
>
> Does adding a TLB flush to before that
>
> pte_unmap_unlock(pte - 1, ptl);
>
> fix things for you?
It definitely does. But if I had to choose, I'd go with holding
mmap_lock for write because 1) it's less likely to storm other CPUs by
IPI and would only have performance impact on processes that use ufd,
which I guess already have high tolerance for not-so-good performance,
and 2) people are spearheading multiple efforts to reduce the mmap_lock
contention, which hopefully would make ufd users suffer less soon.
> That's not the right fix - leaving a stale TLB entry around is fine if
> the TLB entry is more strict wrt protections - but it might be worth
> testing as a "does it at least close the problem" patch.
Well, things get trick if we do this. I'm not sure if I could vouch
such a fix for stable as confident as I do holding mmap_lock for
write.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 0:23 ` Yu Zhao
@ 2020-12-23 2:17 ` Andrea Arcangeli
0 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 2:17 UTC (permalink / raw)
To: Yu Zhao
Cc: Linus Torvalds, Andy Lutomirski, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 05:23:39PM -0700, Yu Zhao wrote:
> and 2) people are spearheading multiple efforts to reduce the mmap_lock
> contention, which hopefully would make ufd users suffer less soon.
In my view UFFD is an already deployed working solution that
eliminates the mmap_lock_write contention to allocate and free memory.
We need to add a UFFDIO_POPULATE to use in combination with
UFFD_FEATURE_SIGBUS (UFFDIO_POPULATE just needs to zero out a page or
THP and map it, it'll be indistinguishable to UFFDIO_ZEROPAGE, but it
will solve the last performance bottleneck by avoiding a suprious
wrprotect fault after the allocation).
After that malloc based on uffd should become competitive single
threaded and it won't ever require the mmap_lock_write so allocations
and freeing of memory can continue indefinitely from all threaded in
parallel. There will never be another mmap or munmap stalling all
threads.
This is not why uffd was created, it's just a secondary performance
benefit of uffd, but it's still a relevant benefit in my view.
Every time I hear people with major mmap_lock_write issues I recommend
uffd, but you know, until we add the UFFDIO_POPULATE, it will still
have higher fixed allocation overhead because of the wprotect fault
after UFFDIO_ZEROCOPY. UFFDIO_COPY also would be not as optimal as a
clear_page and currently it's not even THP capable.
In addition you'll get a SIGBUS after an user after free. It's not
like when you have a malloc lib doing MADV_DONTNEED at PAGE_SIZE
granularity to rate limit the costly munmap, and then the app does an
use after free and it reads zero or writes to a newly faulted in page.
The above will not require any special privilege and all allocated
virtual memory remains fully swappable, because SIGBUS mode will never
have to block any kernel initiated faults.
uffd-wp also is totally usable unprivileged by default to replace
various software dirty bits with the info provided in O(1) instead of
O(N), as long as the writes are done in userland also unprivileged by
default without tweaking any sysctl and with zero risk of increasing
reproduciblity of any exploit against unrelated random kernel bugs.
So if we're forced to take the mmap_lock_write it'd be cool if at
least we can avoid it for 1 single pte or hugepmd wrprotection, as it
happens in write_protect_page() KSM.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 0:01 ` Linus Torvalds
@ 2020-12-23 9:44 ` Linus Torvalds
-1 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 9:44 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The more I look at the mprotect code, the less I like it. We seem to
> be much better about the TLB flushes in other places (looking at
> mremap, for example). The mprotect code seems to be very laissez-faire
> about the TLB flushing.
No, this doesn't help.
> Does adding a TLB flush to before that
>
> pte_unmap_unlock(pte - 1, ptl);
>
> fix things for you?
It really doesn't fix it. Exactly because - as pointed out earlier -
the actual page *copy* happens outside the pte lock.
So what can happen is:
- CPU 1 holds the page table lock, while doing the write protect. It
has cleared the writable bit, but hasn't flushed the TLB's yet
- CPU 2 did *not* have the TLB entry, sees the new read-only state,
takes a COW page fault, and reads the PTE from memory (into
vmf->orig_pte)
- CPU 2 correctly decides it needs to be a COW, and copies the page contents
- CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
happened yet), and writes to that page in users apce
- CPU 1 now does the TLB invalidate, and releases the page table lock
- CPU 2 gets the page table lock, sees that its PTE matches
vmf->orig_pte, and switches it to be that writable copy of the page.
where the copy happened before CPU 3 had stopped writing to the page.
So the pte lock doesn't actually matter, unless we actually do the
page copy inside of it (on CPU2), in addition to doing the TLB flush
inside of it (on CPU1).
mprotect() is actually safe for two independent reasons: (a) it does
the mmap_sem for writing (so mprotect can't race with the COW logic at
all), and (b) it changes the vma permissions so turning something
read-only actually disables COW anyway, since it won't be a COW, it
will be a SIGSEGV.
So mprotect() is irrelevant, other than the fact that it shares some
code with that "turn it read-only in the page tables".
fork() is a much closer operation, in that it actually triggers that
COW behavior, but fork() takes the mmap_sem for writing, so it avoids
this too.
So it's really just userfaultfd and that kind of ilk that is relevant
here, I think. But that "you need to flush the TLB before releasing
the page table lock" was not true (well, it's true in other
circumstances - just not *here*), and is not part of the solution.
Or rather, if it's part of the solution here, it would have to be
matched with that "page copy needs to be done under the page table
lock too".
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-23 9:44 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 9:44 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The more I look at the mprotect code, the less I like it. We seem to
> be much better about the TLB flushes in other places (looking at
> mremap, for example). The mprotect code seems to be very laissez-faire
> about the TLB flushing.
No, this doesn't help.
> Does adding a TLB flush to before that
>
> pte_unmap_unlock(pte - 1, ptl);
>
> fix things for you?
It really doesn't fix it. Exactly because - as pointed out earlier -
the actual page *copy* happens outside the pte lock.
So what can happen is:
- CPU 1 holds the page table lock, while doing the write protect. It
has cleared the writable bit, but hasn't flushed the TLB's yet
- CPU 2 did *not* have the TLB entry, sees the new read-only state,
takes a COW page fault, and reads the PTE from memory (into
vmf->orig_pte)
- CPU 2 correctly decides it needs to be a COW, and copies the page contents
- CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
happened yet), and writes to that page in users apce
- CPU 1 now does the TLB invalidate, and releases the page table lock
- CPU 2 gets the page table lock, sees that its PTE matches
vmf->orig_pte, and switches it to be that writable copy of the page.
where the copy happened before CPU 3 had stopped writing to the page.
So the pte lock doesn't actually matter, unless we actually do the
page copy inside of it (on CPU2), in addition to doing the TLB flush
inside of it (on CPU1).
mprotect() is actually safe for two independent reasons: (a) it does
the mmap_sem for writing (so mprotect can't race with the COW logic at
all), and (b) it changes the vma permissions so turning something
read-only actually disables COW anyway, since it won't be a COW, it
will be a SIGSEGV.
So mprotect() is irrelevant, other than the fact that it shares some
code with that "turn it read-only in the page tables".
fork() is a much closer operation, in that it actually triggers that
COW behavior, but fork() takes the mmap_sem for writing, so it avoids
this too.
So it's really just userfaultfd and that kind of ilk that is relevant
here, I think. But that "you need to flush the TLB before releasing
the page table lock" was not true (well, it's true in other
circumstances - just not *here*), and is not part of the solution.
Or rather, if it's part of the solution here, it would have to be
matched with that "page copy needs to be done under the page table
lock too".
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 9:44 ` Linus Torvalds
(?)
@ 2020-12-23 10:06 ` Yu Zhao
2020-12-23 16:24 ` Peter Xu
-1 siblings, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2020-12-23 10:06 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 01:44:42AM -0800, Linus Torvalds wrote:
> On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > The more I look at the mprotect code, the less I like it. We seem to
> > be much better about the TLB flushes in other places (looking at
> > mremap, for example). The mprotect code seems to be very laissez-faire
> > about the TLB flushing.
>
> No, this doesn't help.
>
> > Does adding a TLB flush to before that
> >
> > pte_unmap_unlock(pte - 1, ptl);
> >
> > fix things for you?
>
> It really doesn't fix it. Exactly because - as pointed out earlier -
> the actual page *copy* happens outside the pte lock.
I appreciate all the pointers. It seems to me it does.
> So what can happen is:
>
> - CPU 1 holds the page table lock, while doing the write protect. It
> has cleared the writable bit, but hasn't flushed the TLB's yet
>
> - CPU 2 did *not* have the TLB entry, sees the new read-only state,
> takes a COW page fault, and reads the PTE from memory (into
> vmf->orig_pte)
In handle_pte_fault(), we lock page table and check pte_write(), so
we either see a RW pte before CPU 1 runs or a RO one with no stale tlb
entries after CPU 1 runs, assume CPU 1 flushes tlb while holding the
same page table lock (not mmap_lock).
> - CPU 2 correctly decides it needs to be a COW, and copies the page contents
>
> - CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
> happened yet), and writes to that page in users apce
>
> - CPU 1 now does the TLB invalidate, and releases the page table lock
>
> - CPU 2 gets the page table lock, sees that its PTE matches
> vmf->orig_pte, and switches it to be that writable copy of the page.
>
> where the copy happened before CPU 3 had stopped writing to the page.
>
> So the pte lock doesn't actually matter, unless we actually do the
> page copy inside of it (on CPU2), in addition to doing the TLB flush
> inside of it (on CPU1).
>
> mprotect() is actually safe for two independent reasons: (a) it does
> the mmap_sem for writing (so mprotect can't race with the COW logic at
> all), and (b) it changes the vma permissions so turning something
> read-only actually disables COW anyway, since it won't be a COW, it
> will be a SIGSEGV.
>
> So mprotect() is irrelevant, other than the fact that it shares some
> code with that "turn it read-only in the page tables".
>
> fork() is a much closer operation, in that it actually triggers that
> COW behavior, but fork() takes the mmap_sem for writing, so it avoids
> this too.
>
> So it's really just userfaultfd and that kind of ilk that is relevant
> here, I think. But that "you need to flush the TLB before releasing
> the page table lock" was not true (well, it's true in other
> circumstances - just not *here*), and is not part of the solution.
>
> Or rather, if it's part of the solution here, it would have to be
> matched with that "page copy needs to be done under the page table
> lock too".
>
> Linus
>
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 10:06 ` Yu Zhao
@ 2020-12-23 16:24 ` Peter Xu
2020-12-23 18:51 ` Andrea Arcangeli
2020-12-23 19:12 ` Yu Zhao
0 siblings, 2 replies; 141+ messages in thread
From: Peter Xu @ 2020-12-23 16:24 UTC (permalink / raw)
To: Yu Zhao
Cc: Linus Torvalds, Andrea Arcangeli, Andy Lutomirski, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 03:06:30AM -0700, Yu Zhao wrote:
> On Wed, Dec 23, 2020 at 01:44:42AM -0800, Linus Torvalds wrote:
> > On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > >
> > > The more I look at the mprotect code, the less I like it. We seem to
> > > be much better about the TLB flushes in other places (looking at
> > > mremap, for example). The mprotect code seems to be very laissez-faire
> > > about the TLB flushing.
> >
> > No, this doesn't help.
> >
> > > Does adding a TLB flush to before that
> > >
> > > pte_unmap_unlock(pte - 1, ptl);
> > >
> > > fix things for you?
> >
> > It really doesn't fix it. Exactly because - as pointed out earlier -
> > the actual page *copy* happens outside the pte lock.
>
> I appreciate all the pointers. It seems to me it does.
>
> > So what can happen is:
> >
> > - CPU 1 holds the page table lock, while doing the write protect. It
> > has cleared the writable bit, but hasn't flushed the TLB's yet
> >
> > - CPU 2 did *not* have the TLB entry, sees the new read-only state,
> > takes a COW page fault, and reads the PTE from memory (into
> > vmf->orig_pte)
>
> In handle_pte_fault(), we lock page table and check pte_write(), so
> we either see a RW pte before CPU 1 runs or a RO one with no stale tlb
> entries after CPU 1 runs, assume CPU 1 flushes tlb while holding the
> same page table lock (not mmap_lock).
I think this is not against Linus's example - where cpu2 does not have tlb
cached so it sees RO while cpu3 does have tlb cached so cpu3 can still modify
it. So IMHO there's no problem here.
But I do think in step 2 here we overlooked _PAGE_UFFD_WP bit. Note that if
it's uffd-wp wr-protection it's always applied along with removing of the write
bit in change_pte_range():
if (uffd_wp) {
ptent = pte_wrprotect(ptent);
ptent = pte_mkuffd_wp(ptent);
}
So instead of being handled as COW page do_wp_page() will always trap
userfaultfd-wp first, hence no chance to race with COW.
COW could only trigger after another uffd-wp-resolve ioctl which could remove
the _PAGE_UFFD_WP bit, but with Andrea's theory unprotect will only happen
after all wr-protect completes, which guarantees that when reaching the COW
path the tlb must has been flushed anyways. Then no one should be modifying
the page anymore even without pgtable lock in COW path.
So IIUC what Linus proposed on "flushing tlb within pgtable lock" seems to
work, but it just may cause more tlb flush than Andrea's proposal especially
when the protection range is large (it's common to have a huge protection range
for e.g. VM live snapshotting, where we'll protect all guest rw ram).
My understanding of current issue is that either we can take Andrea's proposal
(although I think the group rwsem may not be extremely better than a per-mm
rwsem, which I don't know... at least not worst than that?), or we can also go
the other way (also as Andrea mentioned) so that when wr-protect:
- for <=2M range (pmd or less), we take read rwsem, but flush tlb within
pgtable lock
- for >2M range, we take write rwsem directly but flush tlb once
Thanks,
>
> > - CPU 2 correctly decides it needs to be a COW, and copies the page contents
> >
> > - CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
> > happened yet), and writes to that page in users apce
> >
> > - CPU 1 now does the TLB invalidate, and releases the page table lock
> >
> > - CPU 2 gets the page table lock, sees that its PTE matches
> > vmf->orig_pte, and switches it to be that writable copy of the page.
> >
> > where the copy happened before CPU 3 had stopped writing to the page.
> >
> > So the pte lock doesn't actually matter, unless we actually do the
> > page copy inside of it (on CPU2), in addition to doing the TLB flush
> > inside of it (on CPU1).
> >
> > mprotect() is actually safe for two independent reasons: (a) it does
> > the mmap_sem for writing (so mprotect can't race with the COW logic at
> > all), and (b) it changes the vma permissions so turning something
> > read-only actually disables COW anyway, since it won't be a COW, it
> > will be a SIGSEGV.
> >
> > So mprotect() is irrelevant, other than the fact that it shares some
> > code with that "turn it read-only in the page tables".
> >
> > fork() is a much closer operation, in that it actually triggers that
> > COW behavior, but fork() takes the mmap_sem for writing, so it avoids
> > this too.
> >
> > So it's really just userfaultfd and that kind of ilk that is relevant
> > here, I think. But that "you need to flush the TLB before releasing
> > the page table lock" was not true (well, it's true in other
> > circumstances - just not *here*), and is not part of the solution.
> >
> > Or rather, if it's part of the solution here, it would have to be
> > matched with that "page copy needs to be done under the page table
> > lock too".
> >
> > Linus
> >
>
--
Peter Xu
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 16:24 ` Peter Xu
@ 2020-12-23 18:51 ` Andrea Arcangeli
2020-12-23 18:55 ` Andrea Arcangeli
2020-12-23 19:12 ` Yu Zhao
1 sibling, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 18:51 UTC (permalink / raw)
To: Peter Xu
Cc: Yu Zhao, Linus Torvalds, Andy Lutomirski, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 11:24:16AM -0500, Peter Xu wrote:
> I think this is not against Linus's example - where cpu2 does not have tlb
> cached so it sees RO while cpu3 does have tlb cached so cpu3 can still modify
> it. So IMHO there's no problem here.
>
> But I do think in step 2 here we overlooked _PAGE_UFFD_WP bit. Note that if
> it's uffd-wp wr-protection it's always applied along with removing of the write
> bit in change_pte_range():
>
> if (uffd_wp) {
> ptent = pte_wrprotect(ptent);
> ptent = pte_mkuffd_wp(ptent);
> }
>
> So instead of being handled as COW page do_wp_page() will always trap
> userfaultfd-wp first, hence no chance to race with COW.
>
> COW could only trigger after another uffd-wp-resolve ioctl which could remove
> the _PAGE_UFFD_WP bit, but with Andrea's theory unprotect will only happen
> after all wr-protect completes, which guarantees that when reaching the COW
> path the tlb must has been flushed anyways. Then no one should be modifying
> the page anymore even without pgtable lock in COW path.
>
> So IIUC what Linus proposed on "flushing tlb within pgtable lock" seems to
> work, but it just may cause more tlb flush than Andrea's proposal especially
> when the protection range is large (it's common to have a huge protection range
> for e.g. VM live snapshotting, where we'll protect all guest rw ram).
>
> My understanding of current issue is that either we can take Andrea's proposal
> (although I think the group rwsem may not be extremely better than a per-mm
> rwsem, which I don't know... at least not worst than that?), or we can also go
> the other way (also as Andrea mentioned) so that when wr-protect:
>
> - for <=2M range (pmd or less), we take read rwsem, but flush tlb within
> pgtable lock
>
> - for >2M range, we take write rwsem directly but flush tlb once
I fully agree indeed.
I still have to fully understand how the clear_refs_write was fixed.
clear_refs_write has not even a "catcher" in the page fault but it
clears the pte_write under mmap_read_lock and it doesn't flush before
releasing the PT lock. It appears way more broken than
userfaultfd_writeprotect ever was.
static inline void clear_soft_dirty(struct vm_area_struct *vma,
unsigned long addr, pte_t *pte)
{
/*
* The soft-dirty tracker uses #PF-s to catch writes
* to pages, so write-protect the pte as well. See the
* Documentation/admin-guide/mm/soft-dirty.rst for full description
* of how soft-dirty works.
*/
https://patchwork.kernel.org/project/linux-mm/patch/20201210121110.10094-2-will@kernel.org/
"Although this is fine when simply aging the ptes, in the case of
clearing the "soft-dirty" state we can end up with entries where
pte_write() is false, yet a writable mapping remains in the TLB.
Fix this by avoiding the mmu_gather API altogether: managing both the
'tlb_flush_pending' flag on the 'mm_struct' and explicit TLB
invalidation for the sort-dirty path, much like mprotect() does
already"
NOTE: about the above comment, that mprotect takes
mmap_read_lock. Your above code change in the commit above, still has
mmap_read_lock, there's still no similarity with mprotect whatsoever.
Did you test what happens when clear_refs_write leaves writable stale
TLB and you run do_wp_page copies the page while the other CPU still
is writing to the page? Has clear_refs_write a selftest as aggressive
and racy as the uffd selftest to reproduce that workload?
The race fixed with the group lock internally to uffd, is possible
thanks to marker+catcher in NUMA balancing style? But that's not there
for clear_refs_write even after the above commit.
It doesn't appear either that this patch is adding a synchronous tlb
flush inside the PT lock either.
Overall, it would be unfair if clear_refs_write is allowed to do the
same thing that userfaultfd_writeprotect has to do, but with the
mmap_read_lock, if userfaultfd_writeprotect will be forced to take
mmap_write_lock.
In my view there are 3 official ways to deal with this:
1) set a marker in the pte/hugepmd inside the PT lock, and add a
catcher for the marker in the page fault to send the page fault to
a dead end while there are stale TLB entries
cases: userfaultfd_writeprotect() and NUMA balancing
2) take mmap_write_lock
case: mprotect
3) flush the TLB before the PT lock release
case: KSM write_protect_page
Where does the below patch land in the 3 cases? It doesn't fit any of
them and what it does looks still not sufficient to fix the userfault
memory corruption.
https://patchwork.kernel.org/project/linux-mm/patch/20201210121110.10094-2-will@kernel.org/
It'd be backwards if the complexity of the kernel was increased for
clear_refs_write, but forbidden for the O(1) API that delivers the
exact same information (clear_refs_write API delivers that info in
O(N)).
We went the last mile to guarantee uffd can be always used fully
securely unprivileged and by default in current Linus's tree and in
future Android out of the box. It's simply impossible with the current
defaults, to use uffd to enlarge the any kernel user-after-free race
window either, so even that concern is gone. From on those research
testcases will stick to fuse instead I guess.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 18:51 ` Andrea Arcangeli
@ 2020-12-23 18:55 ` Andrea Arcangeli
0 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 18:55 UTC (permalink / raw)
To: Peter Xu
Cc: Yu Zhao, Linus Torvalds, Andy Lutomirski, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 01:51:59PM -0500, Andrea Arcangeli wrote:
> NOTE: about the above comment, that mprotect takes
> mmap_read_lock. Your above code change in the commit above, still has
^^^^ write
Correction to avoid any confusion.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 16:24 ` Peter Xu
2020-12-23 18:51 ` Andrea Arcangeli
@ 2020-12-23 19:12 ` Yu Zhao
2020-12-23 19:32 ` Peter Xu
1 sibling, 1 reply; 141+ messages in thread
From: Yu Zhao @ 2020-12-23 19:12 UTC (permalink / raw)
To: Peter Xu
Cc: Linus Torvalds, Andrea Arcangeli, Andy Lutomirski, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 11:24:16AM -0500, Peter Xu wrote:
> On Wed, Dec 23, 2020 at 03:06:30AM -0700, Yu Zhao wrote:
> > On Wed, Dec 23, 2020 at 01:44:42AM -0800, Linus Torvalds wrote:
> > > On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > >
> > > > The more I look at the mprotect code, the less I like it. We seem to
> > > > be much better about the TLB flushes in other places (looking at
> > > > mremap, for example). The mprotect code seems to be very laissez-faire
> > > > about the TLB flushing.
> > >
> > > No, this doesn't help.
> > >
> > > > Does adding a TLB flush to before that
> > > >
> > > > pte_unmap_unlock(pte - 1, ptl);
> > > >
> > > > fix things for you?
> > >
> > > It really doesn't fix it. Exactly because - as pointed out earlier -
> > > the actual page *copy* happens outside the pte lock.
> >
> > I appreciate all the pointers. It seems to me it does.
> >
> > > So what can happen is:
> > >
> > > - CPU 1 holds the page table lock, while doing the write protect. It
> > > has cleared the writable bit, but hasn't flushed the TLB's yet
> > >
> > > - CPU 2 did *not* have the TLB entry, sees the new read-only state,
> > > takes a COW page fault, and reads the PTE from memory (into
> > > vmf->orig_pte)
> >
> > In handle_pte_fault(), we lock page table and check pte_write(), so
> > we either see a RW pte before CPU 1 runs or a RO one with no stale tlb
> > entries after CPU 1 runs, assume CPU 1 flushes tlb while holding the
> > same page table lock (not mmap_lock).
>
> I think this is not against Linus's example - where cpu2 does not have tlb
> cached so it sees RO while cpu3 does have tlb cached so cpu3 can still modify
> it. So IMHO there's no problem here.
None of the CPUs has stale entries when CPU 2 sees a RO PTE. We are
assuming that TLB flush will be done on CPU 1 while it's still holding
page table lock.
CPU 2 (re)locks page table and (re)checks the PTE under question when
it decides if copy is necessary. If it sees a RO PTE, it means the
flush has been done on all CPUs, therefore it fixes the problem.
> But I do think in step 2 here we overlooked _PAGE_UFFD_WP bit. Note that if
> it's uffd-wp wr-protection it's always applied along with removing of the write
> bit in change_pte_range():
>
> if (uffd_wp) {
> ptent = pte_wrprotect(ptent);
> ptent = pte_mkuffd_wp(ptent);
> }
>
> So instead of being handled as COW page do_wp_page() will always trap
> userfaultfd-wp first, hence no chance to race with COW.
>
> COW could only trigger after another uffd-wp-resolve ioctl which could remove
> the _PAGE_UFFD_WP bit, but with Andrea's theory unprotect will only happen
> after all wr-protect completes, which guarantees that when reaching the COW
> path the tlb must has been flushed anyways. Then no one should be modifying
> the page anymore even without pgtable lock in COW path.
>
> So IIUC what Linus proposed on "flushing tlb within pgtable lock" seems to
> work, but it just may cause more tlb flush than Andrea's proposal especially
> when the protection range is large (it's common to have a huge protection range
> for e.g. VM live snapshotting, where we'll protect all guest rw ram).
>
> My understanding of current issue is that either we can take Andrea's proposal
> (although I think the group rwsem may not be extremely better than a per-mm
> rwsem, which I don't know... at least not worst than that?), or we can also go
> the other way (also as Andrea mentioned) so that when wr-protect:
>
> - for <=2M range (pmd or less), we take read rwsem, but flush tlb within
> pgtable lock
>
> - for >2M range, we take write rwsem directly but flush tlb once
>
> Thanks,
>
> >
> > > - CPU 2 correctly decides it needs to be a COW, and copies the page contents
> > >
> > > - CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
> > > happened yet), and writes to that page in users apce
> > >
> > > - CPU 1 now does the TLB invalidate, and releases the page table lock
> > >
> > > - CPU 2 gets the page table lock, sees that its PTE matches
> > > vmf->orig_pte, and switches it to be that writable copy of the page.
> > >
> > > where the copy happened before CPU 3 had stopped writing to the page.
> > >
> > > So the pte lock doesn't actually matter, unless we actually do the
> > > page copy inside of it (on CPU2), in addition to doing the TLB flush
> > > inside of it (on CPU1).
> > >
> > > mprotect() is actually safe for two independent reasons: (a) it does
> > > the mmap_sem for writing (so mprotect can't race with the COW logic at
> > > all), and (b) it changes the vma permissions so turning something
> > > read-only actually disables COW anyway, since it won't be a COW, it
> > > will be a SIGSEGV.
> > >
> > > So mprotect() is irrelevant, other than the fact that it shares some
> > > code with that "turn it read-only in the page tables".
> > >
> > > fork() is a much closer operation, in that it actually triggers that
> > > COW behavior, but fork() takes the mmap_sem for writing, so it avoids
> > > this too.
> > >
> > > So it's really just userfaultfd and that kind of ilk that is relevant
> > > here, I think. But that "you need to flush the TLB before releasing
> > > the page table lock" was not true (well, it's true in other
> > > circumstances - just not *here*), and is not part of the solution.
> > >
> > > Or rather, if it's part of the solution here, it would have to be
> > > matched with that "page copy needs to be done under the page table
> > > lock too".
> > >
> > > Linus
> > >
> >
>
> --
> Peter Xu
>
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 19:12 ` Yu Zhao
@ 2020-12-23 19:32 ` Peter Xu
0 siblings, 0 replies; 141+ messages in thread
From: Peter Xu @ 2020-12-23 19:32 UTC (permalink / raw)
To: Yu Zhao
Cc: Linus Torvalds, Andrea Arcangeli, Andy Lutomirski, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 12:12:23PM -0700, Yu Zhao wrote:
> On Wed, Dec 23, 2020 at 11:24:16AM -0500, Peter Xu wrote:
> > On Wed, Dec 23, 2020 at 03:06:30AM -0700, Yu Zhao wrote:
> > > On Wed, Dec 23, 2020 at 01:44:42AM -0800, Linus Torvalds wrote:
> > > > On Tue, Dec 22, 2020 at 4:01 PM Linus Torvalds
> > > > <torvalds@linux-foundation.org> wrote:
> > > > >
> > > > > The more I look at the mprotect code, the less I like it. We seem to
> > > > > be much better about the TLB flushes in other places (looking at
> > > > > mremap, for example). The mprotect code seems to be very laissez-faire
> > > > > about the TLB flushing.
> > > >
> > > > No, this doesn't help.
> > > >
> > > > > Does adding a TLB flush to before that
> > > > >
> > > > > pte_unmap_unlock(pte - 1, ptl);
> > > > >
> > > > > fix things for you?
> > > >
> > > > It really doesn't fix it. Exactly because - as pointed out earlier -
> > > > the actual page *copy* happens outside the pte lock.
> > >
> > > I appreciate all the pointers. It seems to me it does.
> > >
> > > > So what can happen is:
> > > >
> > > > - CPU 1 holds the page table lock, while doing the write protect. It
> > > > has cleared the writable bit, but hasn't flushed the TLB's yet
> > > >
> > > > - CPU 2 did *not* have the TLB entry, sees the new read-only state,
> > > > takes a COW page fault, and reads the PTE from memory (into
> > > > vmf->orig_pte)
> > >
> > > In handle_pte_fault(), we lock page table and check pte_write(), so
> > > we either see a RW pte before CPU 1 runs or a RO one with no stale tlb
> > > entries after CPU 1 runs, assume CPU 1 flushes tlb while holding the
> > > same page table lock (not mmap_lock).
> >
> > I think this is not against Linus's example - where cpu2 does not have tlb
> > cached so it sees RO while cpu3 does have tlb cached so cpu3 can still modify
> > it. So IMHO there's no problem here.
>
> None of the CPUs has stale entries when CPU 2 sees a RO PTE.
In this example we have - Please see [1] below.
> We are
> assuming that TLB flush will be done on CPU 1 while it's still holding
> page table lock.
>
> CPU 2 (re)locks page table and (re)checks the PTE under question when
> it decides if copy is necessary. If it sees a RO PTE, it means the
> flush has been done on all CPUs, therefore it fixes the problem.
I guess you had the assumption that pgtable lock released in step 1 already.
But it's not happening in this specific example, not until step5 [2] below.
Indeed if pgtable lock is not released from cpu1, then COW path won't even
triger, afaiu... do_wp_page() needs the pgtable lock. It seems just even safer.
Irrelevant of the small confusions here and there... I believe we're always on
the same page, at least the conclusion.
Thanks,
>
> > But I do think in step 2 here we overlooked _PAGE_UFFD_WP bit. Note that if
> > it's uffd-wp wr-protection it's always applied along with removing of the write
> > bit in change_pte_range():
> >
> > if (uffd_wp) {
> > ptent = pte_wrprotect(ptent);
> > ptent = pte_mkuffd_wp(ptent);
> > }
> >
> > So instead of being handled as COW page do_wp_page() will always trap
> > userfaultfd-wp first, hence no chance to race with COW.
> >
> > COW could only trigger after another uffd-wp-resolve ioctl which could remove
> > the _PAGE_UFFD_WP bit, but with Andrea's theory unprotect will only happen
> > after all wr-protect completes, which guarantees that when reaching the COW
> > path the tlb must has been flushed anyways. Then no one should be modifying
> > the page anymore even without pgtable lock in COW path.
> >
> > So IIUC what Linus proposed on "flushing tlb within pgtable lock" seems to
> > work, but it just may cause more tlb flush than Andrea's proposal especially
> > when the protection range is large (it's common to have a huge protection range
> > for e.g. VM live snapshotting, where we'll protect all guest rw ram).
> >
> > My understanding of current issue is that either we can take Andrea's proposal
> > (although I think the group rwsem may not be extremely better than a per-mm
> > rwsem, which I don't know... at least not worst than that?), or we can also go
> > the other way (also as Andrea mentioned) so that when wr-protect:
> >
> > - for <=2M range (pmd or less), we take read rwsem, but flush tlb within
> > pgtable lock
> >
> > - for >2M range, we take write rwsem directly but flush tlb once
> >
> > Thanks,
> >
> > >
> > > > - CPU 2 correctly decides it needs to be a COW, and copies the page contents
> > > >
> > > > - CPU 3 *does* have a stale TLB (because TLB invalidation hasn't
> > > > happened yet), and writes to that page in users apce
[1]
> > > >
> > > > - CPU 1 now does the TLB invalidate, and releases the page table lock
[2]
> > > >
> > > > - CPU 2 gets the page table lock, sees that its PTE matches
> > > > vmf->orig_pte, and switches it to be that writable copy of the page.
> > > >
> > > > where the copy happened before CPU 3 had stopped writing to the page.
> > > >
> > > > So the pte lock doesn't actually matter, unless we actually do the
> > > > page copy inside of it (on CPU2), in addition to doing the TLB flush
> > > > inside of it (on CPU1).
> > > >
> > > > mprotect() is actually safe for two independent reasons: (a) it does
> > > > the mmap_sem for writing (so mprotect can't race with the COW logic at
> > > > all), and (b) it changes the vma permissions so turning something
> > > > read-only actually disables COW anyway, since it won't be a COW, it
> > > > will be a SIGSEGV.
> > > >
> > > > So mprotect() is irrelevant, other than the fact that it shares some
> > > > code with that "turn it read-only in the page tables".
> > > >
> > > > fork() is a much closer operation, in that it actually triggers that
> > > > COW behavior, but fork() takes the mmap_sem for writing, so it avoids
> > > > this too.
> > > >
> > > > So it's really just userfaultfd and that kind of ilk that is relevant
> > > > here, I think. But that "you need to flush the TLB before releasing
> > > > the page table lock" was not true (well, it's true in other
> > > > circumstances - just not *here*), and is not part of the solution.
> > > >
> > > > Or rather, if it's part of the solution here, it would have to be
> > > > matched with that "page copy needs to be done under the page table
> > > > lock too".
> > > >
> > > > Linus
> > > >
> > >
> >
> > --
> > Peter Xu
> >
>
--
Peter Xu
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 23:50 ` Linus Torvalds
@ 2020-12-23 0:20 ` Linus Torvalds
-1 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 0:20 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:50 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The rule is that the TLB flush has to be done before the page table
> lock is released.
I take that back. I guess it's ok as long as the mmap_sem is held for
writing. Then the TLB flush can be delayed until just before releasing
the mmap_sem. I think.
The stale TLB entries still mean that somebody else can write through
them in another thread, but as long as anybody who actually unmaps the
page (and frees it - think rmap etc) is being careful, mprotect()
itself can probably afford to be a bit laissez-faire.
So mprotect() itself should be ok, I think, because it takes things for writing.
Even with the mmap_sem held for writing, truncate and friends can see
the read-only page table entries (because they can look things up
using the file i_mmap thing instead), but then they rely on the page
table lock and they'll also be careful if they then change that PTE
and will force their own TLB flushes.
So I think a pending TLB flush outside the page table lock is fine -
but once again only if you hold the mmap_sem for writing. Not for
reading, because then the page tables need to be synchronized with the
TLB so that other readers don't see the not-yet-synchronized state.
It once again looks like it's just userfaultfd that would trigger this
due to the read-lock on the mmap_sem. And mprotect() itself is fine.
Am I missing something?
But apparently Nadav sees problems even with that lock changed to a
write lock. Navad?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-23 0:20 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 0:20 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 3:50 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The rule is that the TLB flush has to be done before the page table
> lock is released.
I take that back. I guess it's ok as long as the mmap_sem is held for
writing. Then the TLB flush can be delayed until just before releasing
the mmap_sem. I think.
The stale TLB entries still mean that somebody else can write through
them in another thread, but as long as anybody who actually unmaps the
page (and frees it - think rmap etc) is being careful, mprotect()
itself can probably afford to be a bit laissez-faire.
So mprotect() itself should be ok, I think, because it takes things for writing.
Even with the mmap_sem held for writing, truncate and friends can see
the read-only page table entries (because they can look things up
using the file i_mmap thing instead), but then they rely on the page
table lock and they'll also be careful if they then change that PTE
and will force their own TLB flushes.
So I think a pending TLB flush outside the page table lock is fine -
but once again only if you hold the mmap_sem for writing. Not for
reading, because then the page tables need to be synchronized with the
TLB so that other readers don't see the not-yet-synchronized state.
It once again looks like it's just userfaultfd that would trigger this
due to the read-lock on the mmap_sem. And mprotect() itself is fine.
Am I missing something?
But apparently Nadav sees problems even with that lock changed to a
write lock. Navad?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 23:39 ` Yu Zhao
2020-12-22 23:50 ` Linus Torvalds
@ 2020-12-23 2:56 ` Andrea Arcangeli
2020-12-23 3:36 ` Yu Zhao
1 sibling, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 2:56 UTC (permalink / raw)
To: Yu Zhao
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 04:39:46PM -0700, Yu Zhao wrote:
> We are talking about non-COW anon pages here -- they can't be mapped
> more than once. So why not just identify them by checking
> page_mapcount == 1 and then unconditionally reuse them? (This is
> probably where I've missed things.)
The problem in depending on page_mapcount to decide if it's COW or
non-COW (respectively wp_page_copy or wp_page_reuse) is that is GUP
may elevate the count of a COW anon page that become a non-COW anon
page.
This is Jann's idea not mine.
The problem is we have an unprivileged long term GUP like vmsplice
that facilitates elevating the page count indefinitely, until the
parent finally writes a secret to it. Theoretically a short term pin
would do it too so it's not just vmpslice, but the short term pin
would be incredibly more challenging to become a concern since it'd
kill a phone battery and flash before it can read any data.
So what happens with your page_mapcount == 1 check is that it doesn't
mean non-COW (we thought it did until it didn't for the long term gup
pin in vmsplice).
Jann's testcases does fork() and set page_mapcount 2 and page_count to
2, vmsplice, take unprivileged infinitely long GUP pin to set
page_count to 3, queue the page in the pipe with page_count elevated,
munmap to drop page_count to 2 and page_mapcount to 1.
page_mapcount is 1, so you'd think the page is non-COW and owned by
the parent, but the child can still read it so it's very much still
wp_page_copy material if the parent tries to modify it. Otherwise the
child can read the content.
This was supposed to be solvable by just doing the COW in gup(write=0)
case if page_mapcount > 1 with commit 17839856fd58. I'm not exactly
sure why that didn't fly and it had to be reverted by Peter in
a308c71bf1e6e19cc2e4ced31853ee0fc7cb439a but at the time this was
happening I was side tracked by urgent issues and I didn't manage to
look back of how we ended up with the big hammer page_count == 1 check
instead to decide if to call wp_page_reuse or wp_page_shared.
So anyway, the only thing that is clear to me is that keeping the
child from reading the page_mapcount == 1 pages of the parent, is the
only reason why wp_page_reuse(vmf) will only be called on
page_count(page) == 1 and not on page_mapcount(page) == 1.
It's also the reason why your page_mapcount assumption will risk to
reintroduce the issue, and I only wish we could put back page_mapcount
== 1 back there.
Still even if we put back page_mapcount there, it is not ok to leave
the page fault with stale TLB entries and to rely on the fact
wp_page_shared won't run. It'd also avoid the problem but I think if
you leave stale TLB entries in change_protection just like NUMA
balancing does, it also requires a catcher just like NUMA balancing
has, or it'd truly work by luck.
So until we can put a page_mapcount == 1 check back there, the
page_count will be by definition unreliable because of the speculative
lookups randomly elevating all non zero page_counts at any time in the
background on all pages, so you will never be able to tell if a page
is true COW or if it's just a spurious COW because of a speculative
lookup. It is impossible to differentiate a speculative lookup from a
vmsplice ref in a child.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 2:56 ` Andrea Arcangeli
@ 2020-12-23 3:36 ` Yu Zhao
2020-12-23 15:52 ` Peter Xu
2020-12-23 21:39 ` Andrea Arcangeli
0 siblings, 2 replies; 141+ messages in thread
From: Yu Zhao @ 2020-12-23 3:36 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 09:56:11PM -0500, Andrea Arcangeli wrote:
> On Tue, Dec 22, 2020 at 04:39:46PM -0700, Yu Zhao wrote:
> > We are talking about non-COW anon pages here -- they can't be mapped
> > more than once. So why not just identify them by checking
> > page_mapcount == 1 and then unconditionally reuse them? (This is
> > probably where I've missed things.)
>
> The problem in depending on page_mapcount to decide if it's COW or
> non-COW (respectively wp_page_copy or wp_page_reuse) is that is GUP
> may elevate the count of a COW anon page that become a non-COW anon
> page.
>
> This is Jann's idea not mine.
>
> The problem is we have an unprivileged long term GUP like vmsplice
> that facilitates elevating the page count indefinitely, until the
> parent finally writes a secret to it. Theoretically a short term pin
> would do it too so it's not just vmpslice, but the short term pin
> would be incredibly more challenging to become a concern since it'd
> kill a phone battery and flash before it can read any data.
>
> So what happens with your page_mapcount == 1 check is that it doesn't
> mean non-COW (we thought it did until it didn't for the long term gup
> pin in vmsplice).
>
> Jann's testcases does fork() and set page_mapcount 2 and page_count to
> 2, vmsplice, take unprivileged infinitely long GUP pin to set
> page_count to 3, queue the page in the pipe with page_count elevated,
> munmap to drop page_count to 2 and page_mapcount to 1.
>
> page_mapcount is 1, so you'd think the page is non-COW and owned by
> the parent, but the child can still read it so it's very much still
> wp_page_copy material if the parent tries to modify it. Otherwise the
> child can read the content.
>
> This was supposed to be solvable by just doing the COW in gup(write=0)
> case if page_mapcount > 1 with commit 17839856fd58. I'm not exactly
> sure why that didn't fly and it had to be reverted by Peter in
> a308c71bf1e6e19cc2e4ced31853ee0fc7cb439a but at the time this was
> happening I was side tracked by urgent issues and I didn't manage to
> look back of how we ended up with the big hammer page_count == 1 check
> instead to decide if to call wp_page_reuse or wp_page_shared.
>
> So anyway, the only thing that is clear to me is that keeping the
> child from reading the page_mapcount == 1 pages of the parent, is the
> only reason why wp_page_reuse(vmf) will only be called on
> page_count(page) == 1 and not on page_mapcount(page) == 1.
>
> It's also the reason why your page_mapcount assumption will risk to
> reintroduce the issue, and I only wish we could put back page_mapcount
> == 1 back there.
>
> Still even if we put back page_mapcount there, it is not ok to leave
> the page fault with stale TLB entries and to rely on the fact
> wp_page_shared won't run. It'd also avoid the problem but I think if
> you leave stale TLB entries in change_protection just like NUMA
> balancing does, it also requires a catcher just like NUMA balancing
> has, or it'd truly work by luck.
>
> So until we can put a page_mapcount == 1 check back there, the
> page_count will be by definition unreliable because of the speculative
> lookups randomly elevating all non zero page_counts at any time in the
> background on all pages, so you will never be able to tell if a page
> is true COW or if it's just a spurious COW because of a speculative
> lookup. It is impossible to differentiate a speculative lookup from a
> vmsplice ref in a child.
Thanks for the details.
In your patch, do we need to take wrprotect_rwsem in
handle_userfault() as well? Otherwise, it seems userspace would have
to synchronize between its wrprotect ioctl and fault handler? i.e.,
the fault hander needs to be aware that the content of write-
protected pages can actually change before the iotcl returns.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 3:36 ` Yu Zhao
@ 2020-12-23 15:52 ` Peter Xu
2020-12-23 21:07 ` Andrea Arcangeli
2020-12-23 21:39 ` Andrea Arcangeli
1 sibling, 1 reply; 141+ messages in thread
From: Peter Xu @ 2020-12-23 15:52 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Linus Torvalds, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> In your patch, do we need to take wrprotect_rwsem in
> handle_userfault() as well? Otherwise, it seems userspace would have
> to synchronize between its wrprotect ioctl and fault handler? i.e.,
> the fault hander needs to be aware that the content of write-
> protected pages can actually change before the iotcl returns.
The handle_userfault() thread should be sleeping until another uffd_wp_resolve
fixes the page fault for it. However when the uffd_wp_resolve ioctl comes,
then rwsem (either the group rwsem lock as Andrea proposed, or the mmap_sem, or
any new rwsem lock we'd like to introduce, maybe per-uffd rather than per-mm)
should have guaranteed the previous wr-protect ioctls are finished and tlb must
have been flushed until this thread continues.
And I don't know why it matters even if the data changed - IMHO what uffd-wp
wants to do is simply to make sure after wr-protect ioctl returns to userspace,
no change on the page should ever happen anymore. So "whether data changed"
seems matter more on the ioctl thread rather than the handle_userfault()
thread. IOW, I think data changes before tlb flush but after pte wr-protect is
always fine - but that's not fine anymore if the syscall returns.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 15:52 ` Peter Xu
@ 2020-12-23 21:07 ` Andrea Arcangeli
0 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 21:07 UTC (permalink / raw)
To: Peter Xu
Cc: Yu Zhao, Andy Lutomirski, Linus Torvalds, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 10:52:35AM -0500, Peter Xu wrote:
> On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> > In your patch, do we need to take wrprotect_rwsem in
> > handle_userfault() as well? Otherwise, it seems userspace would have
> > to synchronize between its wrprotect ioctl and fault handler? i.e.,
> > the fault hander needs to be aware that the content of write-
> > protected pages can actually change before the iotcl returns.
>
> The handle_userfault() thread should be sleeping until another uffd_wp_resolve
> fixes the page fault for it. However when the uffd_wp_resolve ioctl comes,
> then rwsem (either the group rwsem lock as Andrea proposed, or the mmap_sem, or
> any new rwsem lock we'd like to introduce, maybe per-uffd rather than per-mm)
> should have guaranteed the previous wr-protect ioctls are finished and tlb must
> have been flushed until this thread continues.
>
> And I don't know why it matters even if the data changed - IMHO what uffd-wp
The data will change indeed and it's fine.
> wants to do is simply to make sure after wr-protect ioctl returns to userspace,
> no change on the page should ever happen anymore. So "whether data changed"
> seems matter more on the ioctl thread rather than the handle_userfault()
> thread. IOW, I think data changes before tlb flush but after pte wr-protect is
> always fine - but that's not fine anymore if the syscall returns.
Agreed.
From the userland point of view all it matters is that the writes
through the stale TLB entries will stop in both the two cases:
1) before returning from the UFFDIO_WRITEPROTECT(mode_wp = true) ioctl syscall
2) before a parallel UFFDIO_WRITEPROTECT(mode_wp = false) can clear
the _PAGE_UFFD_WP marker in the pte/hugepmd under the PT lock,
assuming the syscall at point 1) is still in flight
Both points are guaranteed at all times by the group lock now, so
userland cannot even measure or perceive the existence of any stale
TLB at any given time in the whole uffd-wp workload.
So it's perfectly safe and identical as NUMA balancing and requires
zero extra locking in handle_userfault().
handle_userfault() is a dead end that simply waits and when it's the
right time it restarts the page fault. It can have occasional false positives
after f9bf352224d7d4612b55b8d0cd0eaa981a3246cf, false positive as in
restarting too soon, but even then it's perfectly safe since it's
equivalent of one more CPU hitting the page fault path. As long as the
marker is there, any spurious userfault will re-enter
handle_userfault().
handle_userfault() doesn't care about the data and in turn it cannot
care less about any stale TLB either. Userland cares but userland
cannot make any assumption about writes being fully stopped, until the
ioctl returned anyway and by that time the pending flush will be done
and in fact by the time userland can make any assumption also the
mmap_write_lock would have been released with the first proposed patch.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 3:36 ` Yu Zhao
2020-12-23 15:52 ` Peter Xu
@ 2020-12-23 21:39 ` Andrea Arcangeli
2020-12-23 22:29 ` Yu Zhao
2020-12-23 23:39 ` Linus Torvalds
1 sibling, 2 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 21:39 UTC (permalink / raw)
To: Yu Zhao
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> Thanks for the details.
I hope we can find a way put the page_mapcount back where there's a
page_count right now.
If you're so worried about having to maintain a all defined well
documented (or to be documented even better if you ACK it)
marker/catcher for userfaultfd_writeprotect, I can't see how you could
consider to maintain the page fault safe against any random code
leaving too permissive TLB entries out of sync of the more restrictive
pte permissions as it was happening with clear_refs_write, which
worked by luck until page_mapcount was changed to page_count.
page_count is far from optimal, but it is a feature it finally allowed
us to notice that various code (clear_refs_write included apparently
even after the fix) leaves stale too permissive TLB entries when it
shouldn't.
The question is only which way you prefer to fix clear_refs_write and
I don't think we can deviate from those 3 methods that already exist
today. So clear_refs_write will have to pick one of those and
currently it's not falling in the same category with mprotect even
after the fix.
I think if clear_refs_write starts to take the mmap_write_lock and
really start to operate like mprotect, only then we can consider to
make userfaultfd_writeprotect also operate like mprotect.
Even then I'd hope we can at least be allowed to make it operate like
KSM write_protect_page for len <= HPAGE_PMD_SIZE, something that
clear_refs_write cannot do since it works in O(N) and tends to scan
everything at once, so there would be no point to optimize not to
defer the flush, for a process with a tiny amount of virtual memory
mapped.
vm86 also should be fixed to fall in the same category with mprotect,
since performance there is irrelevant.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 21:39 ` Andrea Arcangeli
@ 2020-12-23 22:29 ` Yu Zhao
2020-12-23 23:04 ` Andrea Arcangeli
2020-12-24 1:21 ` Andy Lutomirski
2020-12-23 23:39 ` Linus Torvalds
1 sibling, 2 replies; 141+ messages in thread
From: Yu Zhao @ 2020-12-23 22:29 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 04:39:00PM -0500, Andrea Arcangeli wrote:
> On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> > Thanks for the details.
>
> I hope we can find a way put the page_mapcount back where there's a
> page_count right now.
>
> If you're so worried about having to maintain a all defined well
> documented (or to be documented even better if you ACK it)
> marker/catcher for userfaultfd_writeprotect, I can't see how you could
> consider to maintain the page fault safe against any random code
> leaving too permissive TLB entries out of sync of the more restrictive
> pte permissions as it was happening with clear_refs_write, which
> worked by luck until page_mapcount was changed to page_count.
>
> page_count is far from optimal, but it is a feature it finally allowed
> us to notice that various code (clear_refs_write included apparently
> even after the fix) leaves stale too permissive TLB entries when it
> shouldn't.
>
> The question is only which way you prefer to fix clear_refs_write and
> I don't think we can deviate from those 3 methods that already exist
> today. So clear_refs_write will have to pick one of those and
> currently it's not falling in the same category with mprotect even
> after the fix.
>
> I think if clear_refs_write starts to take the mmap_write_lock and
> really start to operate like mprotect, only then we can consider to
> make userfaultfd_writeprotect also operate like mprotect.
>
> Even then I'd hope we can at least be allowed to make it operate like
> KSM write_protect_page for len <= HPAGE_PMD_SIZE, something that
> clear_refs_write cannot do since it works in O(N) and tends to scan
> everything at once, so there would be no point to optimize not to
> defer the flush, for a process with a tiny amount of virtual memory
> mapped.
>
> vm86 also should be fixed to fall in the same category with mprotect,
> since performance there is irrelevant.
I was hesitant to suggest the following because it isn't that straight
forward. But since you seem to be less concerned with the complexity,
I'll just bring it on the table -- it would take care of both ufd and
clear_refs_write, wouldn't it?
diff --git a/mm/memory.c b/mm/memory.c
index 5e9ca612d7d7..af38c5ee327e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
goto unlock;
}
if (vmf->flags & FAULT_FLAG_WRITE) {
- if (!pte_write(entry))
+ if (!pte_write(entry)) {
+ if (mm_tlb_flush_pending(vmf->vma->vm_mm))
+ flush_tlb_page(vmf->vma, vmf->address);
return do_wp_page(vmf);
+ }
entry = pte_mkdirty(entry);
}
entry = pte_mkyoung(entry);
^ permalink raw reply related [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 22:29 ` Yu Zhao
@ 2020-12-23 23:04 ` Andrea Arcangeli
2020-12-24 1:21 ` Andy Lutomirski
1 sibling, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-23 23:04 UTC (permalink / raw)
To: Yu Zhao
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 03:29:51PM -0700, Yu Zhao wrote:
> I was hesitant to suggest the following because it isn't that straight
> forward. But since you seem to be less concerned with the complexity,
> I'll just bring it on the table -- it would take care of both ufd and
> clear_refs_write, wouldn't it?
It certainly would since this is basically declaring "leaving stale
TLB entries past mmap_read_lock" is now permitted as long as the
pending flush counter is elevated under mmap_sem for reading.
Anything that prevents uffd-wp to take mmap_write_lock looks great to
me, anything, the below included, as long as it looks like easy to
enforce and understand. And the below certainly is.
My view is that the below is at the very least an improvement in terms
of total complexity, compared to v5.10. At least it'll be documented.
So what would be not ok to me is to depend on undocumented not
guaranteed behavior in do_wp_page like the page_mapcount, which is
what we had until now in clear_refs_write and in uffd-wp (but only if
wrprotect raced against un-wrprotect, a tiny window if compared to
clear_refs_write).
Documenting that clearing pte_write and deferring the flush is allowed
if mm_tlb_flush_pending was elevated before taking the PT lock is less
complex and very well defined rule, if compared to what we had before
in the page_mapcount dependency of clear_refs_write which was prone to
break, and in fact it just did.
We'll need a commentary in both userfaultfd_writeprotect and
clear_refs_write that links to the below snippet.
If we abstract it in a header we can hide there also a #ifdef
CONFIG_HAVE_ARCH_SOFT_DIRTY=y && CONFIG_HAVE_ARCH_USERFAULTFD_WP=y &&
CONFIG_USERFAULTFD=y to make it even more explicit.
However it may be simpler to keep it unconditional, I don't mind
either ways. If it was up to me I'd write it to those 3 config options
to be sure I remember where it comes from and to force any other user
to register to be explicit they depend on that.
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 5e9ca612d7d7..af38c5ee327e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> goto unlock;
> }
> if (vmf->flags & FAULT_FLAG_WRITE) {
> - if (!pte_write(entry))
> + if (!pte_write(entry)) {
> + if (mm_tlb_flush_pending(vmf->vma->vm_mm))
> + flush_tlb_page(vmf->vma, vmf->address);
> return do_wp_page(vmf);
> + }
> entry = pte_mkdirty(entry);
> }
> entry = pte_mkyoung(entry);
>
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 22:29 ` Yu Zhao
2020-12-23 23:04 ` Andrea Arcangeli
@ 2020-12-24 1:21 ` Andy Lutomirski
2020-12-24 2:00 ` Andrea Arcangeli
1 sibling, 1 reply; 141+ messages in thread
From: Andy Lutomirski @ 2020-12-24 1:21 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Linus Torvalds, Peter Xu,
Nadav Amit, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 23, 2020, at 2:29 PM, Yu Zhao <yuzhao@google.com> wrote:
>
>
> I was hesitant to suggest the following because it isn't that straight
> forward. But since you seem to be less concerned with the complexity,
> I'll just bring it on the table -- it would take care of both ufd and
> clear_refs_write, wouldn't it?
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 5e9ca612d7d7..af38c5ee327e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4403,8 +4403,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
> goto unlock;
> }
> if (vmf->flags & FAULT_FLAG_WRITE) {
> - if (!pte_write(entry))
> + if (!pte_write(entry)) {
> + if (mm_tlb_flush_pending(vmf->vma->vm_mm))
> + flush_tlb_page(vmf->vma, vmf->address);
> return do_wp_page(vmf);
> + }
I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
I’m not immediately sure how to do all that much better, though. We could potentially keep a record of pending ranges that need flushing per mm or per PTL, protected by the PTL, and arrange to do the flush if we notice that flushes are pending when we want to do_wp_page(). At least this would limit us to one point extra flush, at least until the concurrent mprotect (or whatever) makes further progress. The bookkeeping might be nasty, though.
But x86 already sort of does some of this bookkeeping, and arguably x86’s code could be improved by tracking TLB ranges to flush per mm instead of per flush request — Nadav already got us half way there by making a little cache of flush_tlb_info structs. IMO it wouldn’t be totally crazy to integrate this better with tlb_gather_mmu to make the pending flush data visible to other CPUs even before actually kicking off the flush. In the limit, this starts to look a bit like a fully async flush mechanism. We would have a function to request a flush, and that function would return a generation count but not actually flush anything. The case of flushing a range adjacent to a still-pending range would be explicitly optimized. Then another function would actually initiate and wait for the flush to complete. And we could, while holding PTL, scan the list of pending flushes, if any, to see if the PTE we’re looking at has a flush pending. This is sort of easy in the one-PTL-per-mm case but potentially rather complicated in the split PTL case. And I’m genuinely unsure where the “batch” TLB flush interface fits in, because it can create a batch that spans more than one mm. x86 can deal with this moderately efficiently since we limit the number of live mms per CPU and our flushes are (for now?) per cpu, not per mm.
u64 gen = 0;
for(...)
gen = queue_flush(mm, start, end, freed_tables);
flush_to_gen(mm, gen);
and the wp fault path does:
wait_for_pending_flushes(mm, address);
Other than the possibility of devolving to one flush per operation if one thread is page faulting at the same speed that another thread is modifying PTEs, this should be decently performant.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 1:21 ` Andy Lutomirski
@ 2020-12-24 2:00 ` Andrea Arcangeli
2020-12-24 3:09 ` Nadav Amit
2020-12-24 3:31 ` Andrea Arcangeli
0 siblings, 2 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 2:00 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu Zhao, Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 05:21:43PM -0800, Andy Lutomirski wrote:
> I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
mprotect can't run concurrently with a page fault in the first place.
One other near zero cost improvement easy to add if this would be "if
(vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
conditional to the two config options too.
Still I don't mind doing it in some other way, uffd-wp has much easier
time doing it in another way in fact.
Whatever performs better is fine, but queuing up pending invalidate
ranges don't look very attractive since it'd be a fixed cost that we'd
always have to pay even when there's no fault (and there can't be any
fault at least for mprotect).
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 2:00 ` Andrea Arcangeli
@ 2020-12-24 3:09 ` Nadav Amit
2020-12-24 3:30 ` Nadav Amit
2020-12-24 3:34 ` Yu Zhao
2020-12-24 3:31 ` Andrea Arcangeli
1 sibling, 2 replies; 141+ messages in thread
From: Nadav Amit @ 2020-12-24 3:09 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Yu Zhao, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 23, 2020, at 6:00 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> On Wed, Dec 23, 2020 at 05:21:43PM -0800, Andy Lutomirski wrote:
>> I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
>
> mprotect can't run concurrently with a page fault in the first place.
>
> One other near zero cost improvement easy to add if this would be "if
> (vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
> conditional to the two config options too.
>
> Still I don't mind doing it in some other way, uffd-wp has much easier
> time doing it in another way in fact.
>
> Whatever performs better is fine, but queuing up pending invalidate
> ranges don't look very attractive since it'd be a fixed cost that we'd
> always have to pay even when there's no fault (and there can't be any
> fault at least for mprotect).
I think there are other cases in which Andy’s concern is relevant
(MADV_PAGEOUT).
Perhaps holding some small bitmap based on part of the deferred flushed
pages (e.g., bits 12-17 of the address or some other kind of a single
hash-function bloom-filter) would be more performant to avoid (most)
unnecessary TLB flushes. It will be cleared before a TLB flush and set while
holding the PTL.
Checking if a flush is needed, under the PTL, would require a single memory
access (although potentially cache miss). It will however require one atomic
operation for each page-table whose PTEs’ flushes are deferred - in contrast
to the current scheme which requires two atomic operations for the *entire*
operation.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 3:09 ` Nadav Amit
@ 2020-12-24 3:30 ` Nadav Amit
2020-12-24 3:34 ` Yu Zhao
1 sibling, 0 replies; 141+ messages in thread
From: Nadav Amit @ 2020-12-24 3:30 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Yu Zhao, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 23, 2020, at 7:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
>
>> On Dec 23, 2020, at 6:00 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>>
>> On Wed, Dec 23, 2020 at 05:21:43PM -0800, Andy Lutomirski wrote:
>>> I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
>>
>> mprotect can't run concurrently with a page fault in the first place.
>>
>> One other near zero cost improvement easy to add if this would be "if
>> (vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
>> conditional to the two config options too.
>>
>> Still I don't mind doing it in some other way, uffd-wp has much easier
>> time doing it in another way in fact.
>>
>> Whatever performs better is fine, but queuing up pending invalidate
>> ranges don't look very attractive since it'd be a fixed cost that we'd
>> always have to pay even when there's no fault (and there can't be any
>> fault at least for mprotect).
>
> I think there are other cases in which Andy’s concern is relevant
> (MADV_PAGEOUT).
>
> Perhaps holding some small bitmap based on part of the deferred flushed
> pages (e.g., bits 12-17 of the address or some other kind of a single
> hash-function bloom-filter) would be more performant to avoid (most)
> unnecessary TLB flushes. It will be cleared before a TLB flush and set while
> holding the PTL.
>
> Checking if a flush is needed, under the PTL, would require a single memory
> access (although potentially cache miss). It will however require one atomic
> operation for each page-table whose PTEs’ flushes are deferred - in contrast
> to the current scheme which requires two atomic operations for the *entire*
> operation.
Just to finish my thought - clearing the bitmap is the tricky part,
which I still did not figure out.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 3:09 ` Nadav Amit
2020-12-24 3:30 ` Nadav Amit
@ 2020-12-24 3:34 ` Yu Zhao
2020-12-24 4:01 ` Andrea Arcangeli
2020-12-24 4:37 ` Nadav Amit
1 sibling, 2 replies; 141+ messages in thread
From: Yu Zhao @ 2020-12-24 3:34 UTC (permalink / raw)
To: Nadav Amit
Cc: Andrea Arcangeli, Andy Lutomirski, Andy Lutomirski,
Linus Torvalds, Peter Xu, linux-mm, lkml, Pavel Emelyanov,
Mike Kravetz, Mike Rapoport, stable, Minchan Kim, Will Deacon,
Peter Zijlstra
On Wed, Dec 23, 2020 at 07:09:10PM -0800, Nadav Amit wrote:
> > On Dec 23, 2020, at 6:00 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> >
> > On Wed, Dec 23, 2020 at 05:21:43PM -0800, Andy Lutomirski wrote:
> >> I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
> >
> > mprotect can't run concurrently with a page fault in the first place.
> >
> > One other near zero cost improvement easy to add if this would be "if
> > (vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
> > conditional to the two config options too.
> >
> > Still I don't mind doing it in some other way, uffd-wp has much easier
> > time doing it in another way in fact.
> >
> > Whatever performs better is fine, but queuing up pending invalidate
> > ranges don't look very attractive since it'd be a fixed cost that we'd
> > always have to pay even when there's no fault (and there can't be any
> > fault at least for mprotect).
>
> I think there are other cases in which Andy’s concern is relevant
> (MADV_PAGEOUT).
That patch only demonstrate a rough idea and I should have been
elaborate: if we ever decide to go that direction, we only need to
worry about "jumping through hoops", because the final patch (set)
I have in mind would not only have the build time optimization Andrea
suggested but also include runtime optimizations like skipping
do_swap_page() path and (!PageAnon() || page_mapcount > 1). Rest
assured, the performance impact on do_wp_page() from occasionally an
additional TLB flush on top of a page copy is negligible.
> Perhaps holding some small bitmap based on part of the deferred flushed
> pages (e.g., bits 12-17 of the address or some other kind of a single
> hash-function bloom-filter) would be more performant to avoid (most)
> unnecessary TLB flushes. It will be cleared before a TLB flush and set while
> holding the PTL.
>
> Checking if a flush is needed, under the PTL, would require a single memory
> access (although potentially cache miss). It will however require one atomic
> operation for each page-table whose PTEs’ flushes are deferred - in contrast
> to the current scheme which requires two atomic operations for the *entire*
> operation.
>
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 3:34 ` Yu Zhao
@ 2020-12-24 4:01 ` Andrea Arcangeli
2020-12-24 5:18 ` Nadav Amit
2020-12-24 4:37 ` Nadav Amit
1 sibling, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 4:01 UTC (permalink / raw)
To: Yu Zhao
Cc: Nadav Amit, Andy Lutomirski, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
> On Wed, Dec 23, 2020 at 07:09:10PM -0800, Nadav Amit wrote:
> > I think there are other cases in which Andy’s concern is relevant
> > (MADV_PAGEOUT).
I didn't try to figure how it would help MADV_PAGEOUT. MADV_PAGEOUT
sounds cool feature, but maybe it'd need a way to flush the
invalidates out and a take a static key to enable the buildup of those
ranges?
I wouldn't like to slow down the fast paths even for MADV_PAGEOUT, and
the same applies to uffd-wp and softdirty in fact.
On Wed, Dec 23, 2020 at 08:34:00PM -0700, Yu Zhao wrote:
> That patch only demonstrate a rough idea and I should have been
> elaborate: if we ever decide to go that direction, we only need to
> worry about "jumping through hoops", because the final patch (set)
> I have in mind would not only have the build time optimization Andrea
> suggested but also include runtime optimizations like skipping
> do_swap_page() path and (!PageAnon() || page_mapcount > 1). Rest
> assured, the performance impact on do_wp_page() from occasionally an
> additional TLB flush on top of a page copy is negligible.
Agreed, I just posted something in that direction. Feel free to
refactor it, it's just a tentative implementation to show something
that may deliver all the performance we need in all cases involved
without slowing down the fast paths of non-uffd and non-softdirty
(well 1 branch).
> On Wed, Dec 23, 2020 at 07:09:10PM -0800, Nadav Amit wrote:
> > Perhaps holding some small bitmap based on part of the deferred flushed
> > pages (e.g., bits 12-17 of the address or some other kind of a single
> > hash-function bloom-filter) would be more performant to avoid (most)
The concern here aren't only the page faults having to run the bloom
filter, but how to manage the RAM storage pointed by the bloomfilter
or whatever index into the storage, which would slowdown mprotect.
Granted that mprotect is slow to begin with, but the idea we can't make
it any slower to make MADV_PAGEOUT or uffd-wp or clear_refs run
faster since it's too important and too frequent in comparison.
Just to restrict the potential false positive IPI caused by page_count
inevitable inaccuracies to uffd-wp and softdirty runtimes, a simple
check on vm_flags should be enough.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 4:01 ` Andrea Arcangeli
@ 2020-12-24 5:18 ` Nadav Amit
2020-12-24 18:49 ` Andrea Arcangeli
0 siblings, 1 reply; 141+ messages in thread
From: Nadav Amit @ 2020-12-24 5:18 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Yu Zhao, Andy Lutomirski, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 23, 2020, at 8:01 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>
>> On Wed, Dec 23, 2020 at 07:09:10PM -0800, Nadav Amit wrote:
>>> Perhaps holding some small bitmap based on part of the deferred flushed
>>> pages (e.g., bits 12-17 of the address or some other kind of a single
>>> hash-function bloom-filter) would be more performant to avoid (most)
>
> The concern here aren't only the page faults having to run the bloom
> filter, but how to manage the RAM storage pointed by the bloomfilter
> or whatever index into the storage, which would slowdown mprotect.
>
> Granted that mprotect is slow to begin with, but the idea we can't make
> it any slower to make MADV_PAGEOUT or uffd-wp or clear_refs run
> faster since it's too important and too frequent in comparison.
>
> Just to restrict the potential false positive IPI caused by page_count
> inevitable inaccuracies to uffd-wp and softdirty runtimes, a simple
> check on vm_flags should be enough.
Andrea,
I am not trying to be argumentative, and I did not think through about an
alternative solution. It sounds to me that your proposed solution is correct
and would probably be eventually (slightly) more efficient than anything
that I can propose.
Yet, I do want to explain my position. Reasoning on TLB flushes is hard, as
this long thread shows. The question is whether it has to be so hard. In
theory, we can only think about architectural considerations - whether a PTE
permissions are promoted/demoted and whether the PTE was changed/cleared.
Obviously, it is more complex than that. Yet, once you add into the equation
various parameters such as the VMA flags or whether a page is locked (which
Mel told me was once a consideration), things become much more complicated.
If all the logic of TLB flushes had been concentrated in a single point and
maintenance of this code did not require thought about users and use-cases,
I think things would have been much simpler, at least for me.
Regards,
Nadav
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 5:18 ` Nadav Amit
@ 2020-12-24 18:49 ` Andrea Arcangeli
2020-12-24 19:16 ` Andrea Arcangeli
0 siblings, 1 reply; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 18:49 UTC (permalink / raw)
To: Nadav Amit
Cc: Yu Zhao, Andy Lutomirski, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 09:18:09PM -0800, Nadav Amit wrote:
> I am not trying to be argumentative, and I did not think through about an
> alternative solution. It sounds to me that your proposed solution is correct
> and would probably be eventually (slightly) more efficient than anything
> that I can propose.
On a side note, on the last proposed solution, I've been wondering
about the memory ordering mm_tlb_flush_pending.
There's plenty of locking before the point where the actual data is
read, but it's not a release/acquire full barrier (or more accurately
it is only on x86), so smp_rmb() seems needed before cow_user_page to
be sure no data can be read before we read the value of
mm_tlb_flush_pending.
To avoid an explicit smp_rmb() and to inherit the implicit smp_mb()
from the release/acquire of PT unlock/PT lock, we'd need to put the
flush before the previous PT unlock. (note, not after the next PT lock
or it'd be even worse).
So this made me look also the inc/dec:
+ smp_mb__before_atomic();
atomic_dec(&mm->tlb_flush_pending);
Without the above, can't the CPU decrement the tlb_flush_pending while
the IPI to ack didn't arrive yet in csd_lock_wait?
The smp_rmb() is still needed in the page fault (implicit or explicit
doesn't matter), but we also need a smp_mb__before_atomic() above to
really make this work.
> Yet, I do want to explain my position. Reasoning on TLB flushes is hard, as
> this long thread shows. The question is whether it has to be so hard. In
> theory, we can only think about architectural considerations - whether a PTE
> permissions are promoted/demoted and whether the PTE was changed/cleared.
>
> Obviously, it is more complex than that. Yet, once you add into the equation
> various parameters such as the VMA flags or whether a page is locked (which
> Mel told me was once a consideration), things become much more complicated.
> If all the logic of TLB flushes had been concentrated in a single point and
> maintenance of this code did not require thought about users and use-cases,
> I think things would have been much simpler, at least for me.
In your previous email you also suggested to add range invalidates and
bloom filter to index them by the address in the page fault since it'd
help MADV_PAGEOUT. That would increase complexity and it won't go
exactly in the above direction.
I assume that here nobody wants to add gratuitous complexity, and I
never suggested the code shouldn't become simpler and easier to
understand and maintain. But we can't solve everything in a single
thread in terms of cleaning up and auditing the entirety of the TLB
code.
To refocus: the only short term objective in this thread, is to fix
the data corruption in uffd-wp and clear_refs_write without
introducing new performance regressions compared to the previous
clear_refs_write runtime.
Once that is fixed and we didn't introduce a performance regression
while fixing an userland memory corruption regression (i.e. not really
a regression in theory, but in practice it is because it worked by
luck), then we can look at all the rest without hurry.
So if already want to start the cleanups like I mentioned in a
previous email, and I'll say it more explicitly, the tlb gather is for
freeing memory, it's just pure overhead and gives a false sense of
security, like it can make any difference, when just changing
protection with mmap_read_lock. It wouldn't be needed with the write
lock and it can't help solve the races that trigger with
mmap_read_lock either.
It is needed when you have to store the page pointer outside the pte,
clear a pte, flush the tlb and only then put_page. So it is needed to
keep tracks of which pages got cleared in the ptes so you don't have
to issue a tlb flush for each single pte that gets cleared.
So the only case when to use the tlb_gather is when you must make the
pte stop pointing to the page and you need an external storage that
will keep track of those pages that we cannot yet lose track of since
we didn't do put_page yet. That kind of external storage to keep track
of the pages that have pending tlb flushes, is never needed in
change_protection and clear_refs_write.
change_protection is already correct in that respect, it doesn't use
the tlb_gather. clear_refs_write is not ok and it need to stop using
tlb_gather_mmu/tlb_finish_mmu.
* tlb_gather_mmu - initialize an mmu_gather structure for page-table tear-down
See also the tear-down mention which doesn't apply to clear_refs_write.
clear_refs_write needs to become identical to
change_protection_range. Just increasing inc_tlb_flush_pending and
then doing a flush of the range right before the
dec_tlb_flush_pending.
That change is actually orthogonal to the regression fix to teach the
COW to deal with stale TLB entries and it will clean up the code.
So to pursue your simplification objective you could already go ahead
doing this improvement since it's very confusing to see the
clear_refs_write use something that it shouldn't use like if it
actually could make any difference and then seeing an incremental
patch trying to perfect the tlb_gather logic in clear_refs instead of
removing it. In fact it's so strange that it's hard to suggest
dropping tlb_gather entirely, I feel like I must be missing
something, so if I miss something this would be a good time to explain
me why tlb_gather is used in clear_refs.
Once that is also done, we can look at the flush_tlb_batched_pending
that I see you mentioned to Yu. I didn't go check it yet, but we can
certainly look at it deeper later, maybe starting a new thread for
it is simpler?
In the short term, for this thread, we can't solve everything at once
and reduce the complexity at the same time.
So refocusing on the memory ordering of dec_tlb_flush_pending and the
mm_tlb_flush_pending mentioned above, to find a proper abstraction and
write proper documentation for the flush in wp_copy_page would be
ideal. Then we can do the rest.
On my to-list before worrying about the rest in fact I also need to
re-send (I already proposed it for merging a few times on lkml) of the
ARM64 tlb flushing optimization to skip the hardware SMP un-scalable
ITLB broadcast for single threaded processes or multithreaded
processes that are temporarily running single threaded or bind to a
single CPU, which increases SMP scalability of the arm64 TLB flushing
by an order of magnitude for some workloads and we had to ship in
production already).
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 18:49 ` Andrea Arcangeli
@ 2020-12-24 19:16 ` Andrea Arcangeli
0 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 19:16 UTC (permalink / raw)
To: Nadav Amit
Cc: Yu Zhao, Andy Lutomirski, Andy Lutomirski, Linus Torvalds,
Peter Xu, linux-mm, lkml, Pavel Emelyanov, Mike Kravetz,
Mike Rapoport, stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Thu, Dec 24, 2020 at 01:49:45PM -0500, Andrea Arcangeli wrote:
> Without the above, can't the CPU decrement the tlb_flush_pending while
> the IPI to ack didn't arrive yet in csd_lock_wait?
Ehm: csd_lock_wait has smp_acquire__after_ctrl_dep() so the write side
looks ok after all sorry.
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 3:34 ` Yu Zhao
2020-12-24 4:01 ` Andrea Arcangeli
@ 2020-12-24 4:37 ` Nadav Amit
1 sibling, 0 replies; 141+ messages in thread
From: Nadav Amit @ 2020-12-24 4:37 UTC (permalink / raw)
To: Yu Zhao
Cc: Andrea Arcangeli, Andy Lutomirski, Andy Lutomirski,
Linus Torvalds, Peter Xu, linux-mm, lkml, Pavel Emelyanov,
Mike Kravetz, Mike Rapoport, stable, Minchan Kim, Will Deacon,
Peter Zijlstra
> On Dec 23, 2020, at 7:34 PM, Yu Zhao <yuzhao@google.com> wrote:
>
> On Wed, Dec 23, 2020 at 07:09:10PM -0800, Nadav Amit wrote:
>>> On Dec 23, 2020, at 6:00 PM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>>>
>>> On Wed, Dec 23, 2020 at 05:21:43PM -0800, Andy Lutomirski wrote:
>>>> I don’t love this as a long term fix. AFAICT we can have mm_tlb_flush_pending set for quite a while — mprotect seems like it can wait in IO while splitting a huge page, for example. That gives us a window in which every write fault turns into a TLB flush.
>>>
>>> mprotect can't run concurrently with a page fault in the first place.
>>>
>>> One other near zero cost improvement easy to add if this would be "if
>>> (vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
>>> conditional to the two config options too.
>>>
>>> Still I don't mind doing it in some other way, uffd-wp has much easier
>>> time doing it in another way in fact.
>>>
>>> Whatever performs better is fine, but queuing up pending invalidate
>>> ranges don't look very attractive since it'd be a fixed cost that we'd
>>> always have to pay even when there's no fault (and there can't be any
>>> fault at least for mprotect).
>>
>> I think there are other cases in which Andy’s concern is relevant
>> (MADV_PAGEOUT).
>
> That patch only demonstrate a rough idea and I should have been
> elaborate: if we ever decide to go that direction, we only need to
> worry about "jumping through hoops", because the final patch (set)
> I have in mind would not only have the build time optimization Andrea
> suggested but also include runtime optimizations like skipping
> do_swap_page() path and (!PageAnon() || page_mapcount > 1). Rest
> assured, the performance impact on do_wp_page() from occasionally an
> additional TLB flush on top of a page copy is negligible.
I agree with you to a certain extent, since there is anyhow another TLB
flush in this path when the PTE is set after copying.
Yet, I think that having a combined and efficient central mechanism for
pending TLB flushes is important even for robustness: to prevent the
development of new independent deferred flushing schemes. I specifically do
not like tlb_flush_batched which every time that I look at gets me confused.
For example the following code completely confuses me:
void flush_tlb_batched_pending(struct mm_struct *mm)
{
if (data_race(mm->tlb_flush_batched)) {
flush_tlb_mm(mm);
/*
* Do not allow the compiler to re-order the clearing of
* tlb_flush_batched before the tlb is flushed.
*/
barrier();
mm->tlb_flush_batched = false;
}
}
… and then I ask myself (no answer):
1. What prevents concurrent flush_tlb_batched_pending() which is called by
madvise_free_pte_range(), for instance from madvise_free_pte_range(), from
clearing new deferred flush indication that was just set by
set_tlb_ubc_flush_pending()? Can it cause a missed TLB flush later?
2. Why the write to tlb_flush_batched is not done with WRITE_ONCE()?
3. Should we have smp_wmb() instead of barrier()? (probably the barrier() is
not needed at all since flush_tlb_mm() serializes if a flush is needed).
4. Why do we need 2 deferred TLB flushing mechanisms?
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-24 2:00 ` Andrea Arcangeli
2020-12-24 3:09 ` Nadav Amit
@ 2020-12-24 3:31 ` Andrea Arcangeli
1 sibling, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 3:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu Zhao, Andy Lutomirski, Linus Torvalds, Peter Xu, Nadav Amit,
linux-mm, lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport,
stable, Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 09:00:26PM -0500, Andrea Arcangeli wrote:
> One other near zero cost improvement easy to add if this would be "if
> (vma->vm_flags & (VM_SOFTDIRTY|VM_UFFD_WP))" and it could be made
The next worry then is if UFFDIO_WRITEPROTECT is very large then there
would be a flood of wrprotect faults, and they'd end up all issuing a
tlb flush during the UFFDIO_WRITEPROTECT itself which again is a
performance concern for certain uffd-wp use cases.
Those use cases would be for example redis and postcopy live
snapshotting, to use it for async snapshots, unprivileged too in the
case of redis if it temporarily uses bounce buffers for the syscall
I/O for the duration of the snapshot. hypervisors tuned profiles need
to manually lift the unprivileged_userfaultfd to 1 unless their jailer
leaves one capability in the snapshot thread.
Moving the check after userfaultfd_pte_wp would solve
userfaultfd_writeprotect(mode_wp=true), but that still wouldn't avoid
a flood of tlb flushes during userfaultfd_writeprotect(mode_wp=false)
because change_protection doesn't restore the pte_write:
} else if (uffd_wp_resolve) {
/*
* Leave the write bit to be handled
* by PF interrupt handler, then
* things like COW could be properly
* handled.
*/
ptent = pte_clear_uffd_wp(ptent);
}
When the snapshot is complete userfaultfd_writeprotect(mode_wp=false)
would need to run again on the whole range which can be very big
again.
Orthogonally I think we should also look to restore the pte_write
above orthogonally in uffd-wp, so it'll get yet an extra boost if
compared to current redis snapshotting fork(), that cannot restore all
pte_write after the snapshot child quit and forces a flood of spurious
wrprotect faults (uffd-wp can solve that too).
However, even if uffd-wp restored the pte_write, things would remain
suboptimal for a terabyte process under clear_refs, since softdirty
wrprotect faults that start happening while softdirty is still running
on the mm, won't be caught in userfaultfd_pte_wp.
Something like below, if cleaned up, abstracted properly and
documented well in the two places involved, will have a better chance
to perform optimally for softdirty too.
And on a side note the CONFIG_MEM_SOFT_DIRTY compile time check is
compulsory because VM_SOFTDIRTY is defined to zero if softdirty is not
built in. (for VM_UFFD_WP the CONFIG_HAVE_ARCH_USERFAULTFD_WP can be
removed and it won't make any measurable difference even when
USERFAULTFD=n)
RFC untested below, it's supposed to fix the softdirty testcase too,
even without the incremental fix, since it already does tlb_gather_mmu
before walk_page_range and tlb_finish_mmu after it and that appears
enough to define the inc/dec_tlb_flush_pending.
diff --git a/mm/memory.c b/mm/memory.c
index 7d608765932b..66fd6d070c47 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2844,11 +2844,26 @@ static vm_fault_t wp_page_copy(struct vm_fault *vmf)
if (!new_page)
goto oom;
} else {
+ bool in_uffd_wp, in_softdirty;
new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma,
vmf->address);
if (!new_page)
goto oom;
+#ifdef CONFIG_HAVE_ARCH_USERFAULTFD_WP
+ in_uffd_wp = !!(vma->vm_flags & VM_UFFD_WP);
+#else
+ in_uffd_wp = false;
+#endif
+#ifdef CONFIG_MEM_SOFT_DIRTY
+ in_softdirty = !(vma->vm_flags & VM_SOFTDIRTY);
+#else
+ in_softdirty = false;
+#endif
+ if ((in_uffd_wp || in_softdirty) &&
+ mm_tlb_flush_pending(mm))
+ flush_tlb_page(vma, vmf->address);
+
if (!cow_user_page(new_page, old_page, vmf)) {
/*
* COW failed, if the fault was solved by other,
^ permalink raw reply related [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 21:39 ` Andrea Arcangeli
@ 2020-12-23 23:39 ` Linus Torvalds
2020-12-23 23:39 ` Linus Torvalds
1 sibling, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 23:39 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Yu Zhao, Andy Lutomirski, Peter Xu, Nadav Amit, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 1:39 PM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> > Thanks for the details.
>
> I hope we can find a way put the page_mapcount back where there's a
> page_count right now.
I really don't think that's ever going to happen - at least if you're
talking about do_wp_page().
I refuse to make the *simple* core operations VM have to jump through
hoops, and the old COW mapcount logic was that. I *much* prefer the
newer COW code, because the rules are more straightforward.
> page_count is far from optimal, but it is a feature it finally allowed
> us to notice that various code (clear_refs_write included apparently
> even after the fix) leaves stale too permissive TLB entries when it
> shouldn't.
I absolutely agree that page_count isn't exactly optimal, but
"mapcount" is just so much worse.
page_count() is at least _logical_, and has a very clear meaning:
"this page has other users". mapcount() means something else, and is
simply not sufficient or relevant wrt COW.
That doesn't mean that page_mapcount() is wrong - it's just that it's
wrong for COW. page_mapcount() is great for rmap, so that we can see
when we need to shoot down a memory mapping of a page that gets
released (truncate being the classic example).
I think that the mapcount games we used to have were horrible. I
absolutely much prefer where we are now wrt COW.
The modern rules for COW handling are:
- if we got a COW fault there might be another user, we copy (and
this is where the page count makes so much logical sense).
- if somebody needs to pin the page in the VM, we either make sure
that it is pre-COWed and we
(a) either never turn it back into a COW page (ie the fork()-time
stuff we have for pinned pages)
(b) or there is some explicit marker on the page or in the page
table (ie the userfaultfd_pte_wp thing).
those are _so_ much more straightforward than the very complex rules
we used to have that were actively buggy, in addition to requiring the
page lock. So they were buggy and slow.
And yes, I had forgotten about that userfaultfd_pte_wp() because I was
being myopic and only looking at wp_page_copy(). So using that as a
way to make sure that a page doesn't go through COW is a good way to
avoid the COW race, but I think that thing requires a bit in the page
table which might be a problem on some architectures?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
@ 2020-12-23 23:39 ` Linus Torvalds
0 siblings, 0 replies; 141+ messages in thread
From: Linus Torvalds @ 2020-12-23 23:39 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Yu Zhao, Andy Lutomirski, Peter Xu, Nadav Amit, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
On Wed, Dec 23, 2020 at 1:39 PM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> > Thanks for the details.
>
> I hope we can find a way put the page_mapcount back where there's a
> page_count right now.
I really don't think that's ever going to happen - at least if you're
talking about do_wp_page().
I refuse to make the *simple* core operations VM have to jump through
hoops, and the old COW mapcount logic was that. I *much* prefer the
newer COW code, because the rules are more straightforward.
> page_count is far from optimal, but it is a feature it finally allowed
> us to notice that various code (clear_refs_write included apparently
> even after the fix) leaves stale too permissive TLB entries when it
> shouldn't.
I absolutely agree that page_count isn't exactly optimal, but
"mapcount" is just so much worse.
page_count() is at least _logical_, and has a very clear meaning:
"this page has other users". mapcount() means something else, and is
simply not sufficient or relevant wrt COW.
That doesn't mean that page_mapcount() is wrong - it's just that it's
wrong for COW. page_mapcount() is great for rmap, so that we can see
when we need to shoot down a memory mapping of a page that gets
released (truncate being the classic example).
I think that the mapcount games we used to have were horrible. I
absolutely much prefer where we are now wrt COW.
The modern rules for COW handling are:
- if we got a COW fault there might be another user, we copy (and
this is where the page count makes so much logical sense).
- if somebody needs to pin the page in the VM, we either make sure
that it is pre-COWed and we
(a) either never turn it back into a COW page (ie the fork()-time
stuff we have for pinned pages)
(b) or there is some explicit marker on the page or in the page
table (ie the userfaultfd_pte_wp thing).
those are _so_ much more straightforward than the very complex rules
we used to have that were actively buggy, in addition to requiring the
page lock. So they were buggy and slow.
And yes, I had forgotten about that userfaultfd_pte_wp() because I was
being myopic and only looking at wp_page_copy(). So using that as a
way to make sure that a page doesn't go through COW is a good way to
avoid the COW race, but I think that thing requires a bit in the page
table which might be a problem on some architectures?
Linus
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-23 23:39 ` Linus Torvalds
(?)
@ 2020-12-24 1:01 ` Andrea Arcangeli
-1 siblings, 0 replies; 141+ messages in thread
From: Andrea Arcangeli @ 2020-12-24 1:01 UTC (permalink / raw)
To: Linus Torvalds
Cc: Yu Zhao, Andy Lutomirski, Peter Xu, Nadav Amit, linux-mm, lkml,
Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
Hello Linus,
On Wed, Dec 23, 2020 at 03:39:53PM -0800, Linus Torvalds wrote:
> On Wed, Dec 23, 2020 at 1:39 PM Andrea Arcangeli <aarcange@redhat.com> wrote:
> >
> > On Tue, Dec 22, 2020 at 08:36:04PM -0700, Yu Zhao wrote:
> > > Thanks for the details.
> >
> > I hope we can find a way put the page_mapcount back where there's a
> > page_count right now.
>
> I really don't think that's ever going to happen - at least if you're
> talking about do_wp_page().
Yes, I was referring to do_wp_page().
> I refuse to make the *simple* core operations VM have to jump through
> hoops, and the old COW mapcount logic was that. I *much* prefer the
> newer COW code, because the rules are more straightforward.
Agreed. I don't have any practical alternative proposal in fact, it
was only an hypothetical wish/hope, echoing Hugh's view on this
matter.
> > page_count is far from optimal, but it is a feature it finally allowed
> > us to notice that various code (clear_refs_write included apparently
> > even after the fix) leaves stale too permissive TLB entries when it
> > shouldn't.
>
> I absolutely agree that page_count isn't exactly optimal, but
> "mapcount" is just so much worse.
Agreed. The "isn't exactly optimal" part is the only explanation for
wish/hope.
> page_count() is at least _logical_, and has a very clear meaning:
> "this page has other users". mapcount() means something else, and is
> simply not sufficient or relevant wrt COW.
>
> That doesn't mean that page_mapcount() is wrong - it's just that it's
> wrong for COW. page_mapcount() is great for rmap, so that we can see
> when we need to shoot down a memory mapping of a page that gets
> released (truncate being the classic example).
>
> I think that the mapcount games we used to have were horrible. I
> absolutely much prefer where we are now wrt COW.
>
> The modern rules for COW handling are:
>
> - if we got a COW fault there might be another user, we copy (and
> this is where the page count makes so much logical sense).
>
> - if somebody needs to pin the page in the VM, we either make sure
> that it is pre-COWed and we
>
> (a) either never turn it back into a COW page (ie the fork()-time
> stuff we have for pinned pages)
>
> (b) or there is some explicit marker on the page or in the page
> table (ie the userfaultfd_pte_wp thing).
>
> those are _so_ much more straightforward than the very complex rules
> we used to have that were actively buggy, in addition to requiring the
> page lock. So they were buggy and slow.
Agreed, this is a very solid solution that solves the problem the
mapcount had with GUP pins.
The only alternative but very vapourware thought I had on this matter,
is that all troubles start when we let a GUP-pinned page be unmapped
from the pgtables.
I already told Jens, is io_uring could use mmu notifier already (that
would make any io_uring GUP pin not count anymore with regard to
page_mapcount vs page_count difference) and vmsplice also needs some
handling or maybe become privileged.
The above two improvements are orthogonal with the COW issue since
long term GUP pins do more than just mlock and they break most
advanced VM features. It's not ok just to account GUP pins in rlimit
mlock.
The above two improvements however would go into the direction to make
mapcount more suitable for COW, but it'd still not be enough.
The transient GUP pins also would need to be taken care of, but we
could wait for those to be released, since those are guaranteed to be
released or something else, not just munmap()/MADV_DONTNEED, will
remain in D state.
Anyway.. until io_uring and vmsplice are solved first, it's pointless
to even wonder about transient GUP pins.
> And yes, I had forgotten about that userfaultfd_pte_wp() because I was
> being myopic and only looking at wp_page_copy(). So using that as a
> way to make sure that a page doesn't go through COW is a good way to
> avoid the COW race, but I think that thing requires a bit in the page
> table which might be a problem on some architectures?
Correct, it requires a bit in the pgtable.
The bit is required in order to disambiguate which regions have been
marked for wrprotect faults and which didn't.
A practical example would be: fork() called by an app with a very
large vma with VM_UFFD_WP set.
There would be a storm of WP userfaults if we didn't have the software
bit to detect exactly which pte were wrprotected by uffd-wp and which
ones were wrprotected by fork.
That bit then sends the COW fault to a safe place if wrprotect is
running (the problem is we didn't see the un-wrprotect would clear the
bit while the TLB flush of the wrprotect could be still pending).
The page_mapcount I guess hidden that race to begin with, just like it
did in clear_refs_write.
uffd-wp is similar to soft dirty: it won't work well without a pgtable
software bit. The software bit, to avoid storms of false positives
during memory pressure, also survives swapin/swapout, again like soft
dirty.
The bit requirement is handled through a per-arch option
arch/x86/Kconfig similarly to soft dirty.
select HAVE_ARCH_USERFAULTFD_WP if X86_64 && USERFAULTFD
From an high level, the extra bit in the pte/hugepmd could be seen as
holding the information that would be generated by split_vma()
followed by clearing VM_WRITE in the middle vma. Setting that bit
results in a cheaper runtime than allocating 2 more vmas with the
associated locking and rbtree size increase, but same accuracy.
Thanks,
Andrea
^ permalink raw reply [flat|nested] 141+ messages in thread
* Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect
2020-12-22 19:31 ` Andrea Arcangeli
2020-12-22 20:15 ` Matthew Wilcox
2020-12-22 21:14 ` Yu Zhao
@ 2020-12-22 21:14 ` Nadav Amit
2 siblings, 0 replies; 141+ messages in thread
From: Nadav Amit @ 2020-12-22 21:14 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Andy Lutomirski, Linus Torvalds, Peter Xu, Yu Zhao, linux-mm,
lkml, Pavel Emelyanov, Mike Kravetz, Mike Rapoport, stable,
Minchan Kim, Will Deacon, Peter Zijlstra
> On Dec 22, 2020, at 11:31 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> From 4ace4d1b53f5cb3b22a5c2dc33facc4150b112d6 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Tue, 22 Dec 2020 14:30:16 -0500
> Subject: [PATCH 1/1] mm: userfaultfd: avoid leaving stale TLB after
> userfaultfd_writeprotect()
>
> change_protection() is called by userfaultfd_writeprotect() with the
> mmap_lock_read like in change_prot_numa().
>
> The page fault code in wp_copy_page() rightfully assumes if the CPU
> issued a write fault and the write bit in the pagetable is not set, no
> CPU can write to the page. That's wrong assumption after
> userfaultfd_writeprotect(). That's also wrong assumption after
> change_prot_numa() where the regular page fault code also would assume
> that if the present bit is not set and the page fault is running,
> there should be no stale TLB entry, but there is still.
>
> So to stay safe, the page fault code must be prevented to run as long
> as long as the TLB flush remains pending. That is already achieved by
> the do_numa_page() path for change_prot_numa() and by the
> userfaultfd_pte_wp() path for userfaultfd_writeprotect().
>
> The problem that needs fixing is that an un-wrprotect
> (i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP not
> set) could run in between the original wrprotect
> (i.e. userfaultfd_writeprotect() with UFFDIO_WRITEPROTECT_MODE_WP set)
> and wp_copy_page, while the TLB flush remains pending.
I may need to read your patch more carefully, but in general I do not like
the approach. You are much more experienced than I am, but IMHO the TLB
flushing logic needs to be further simplified and generalized and not the
other way around.
The complexity is already too high. We have tlb_flush_batched and
tlb_flush_pending, which I think should be (somehow) combined.
tlb_gather_mmu() is designed for zapping, but can’t it be modified to suit
protection changes to avoid buggy use-cases (as the wrong use in
clear_refs_write() ) ?
So adding new userfaultfd specific code, which potentially does not address
all the interactions (now or the future), is concerning.
In this regard, a similar problem to the one in userfaultfd
(mmap_read_lock() while write-protecting) already exists with soft-dirty
clearing, so any solution should also address the soft-dirty issue.
^ permalink raw reply [flat|nested] 141+ messages in thread