On Mon, Jan 04, 2021 at 02:24:38PM -0500, Andrea Arcangeli wrote:

On Mon, Jan 04, 2021 at 01:22:27PM +0100, Peter Zijlstra wrote:

quoted

On Fri, Dec 25, 2020 at 01:25:28AM -0800, Nadav Amit wrote:

quoted

The scenario that happens in selftests/vm/userfaultfd is as follows:

cpu0				cpu1			cpu2
----				----			----
							[ Writable PTE
							  cached in TLB ]
userfaultfd_writeprotect()
[ write-*unprotect* ]
mwriteprotect_range()
mmap_read_lock()
change_protection()

change_protection_range()
...
change_pte_range()
[ *clear* “write”-bit ]
[ defer TLB flushes ]
				[ page-fault ]
				...
				wp_page_copy()
				 cow_user_page()
				  [ copy page ]
							[ write to old
							  page ]
				...
				 set_pte_at_notify()

Yuck!

Note, the above was posted before we figured out the details so it
wasn't showing the real deferred tlb flush that caused problems (the
one showed on the left causes zero issues).

The problematic one not pictured is the one of the wrprotect that has
to be running in another CPU which is also isn't picture above. More
accurate traces are posted later in the thread.

Lets assume CPU0 does a read-lock, W -> RO with deferred flush.

quoted

Isn't this all rather similar to the problem that resulted in the
tlb_flush_pending mess?

I still think that's all fundamentally buggered, the much saner solution
(IMO) would've been to make things wait for the pending flush, instead

How do intend you wait in PT lock while the writer also has to take PT
lock repeatedly before it can do wake_up_var?

If you release the PT lock before calling wait_tlb_flush_pending it
all falls apart again.

I suppose you can check for pending, if found, release lock, wait for 0,
and re-take the fault?

This I guess explains why a local pte/hugepmd smp local invlpg is the
only working solution for this issue, similarly to how it's done in rmap.

In that case a local invalidate on CPU1 simply doesn't help anything.

CPU1 needs to do a global invalidate or wait for the in-progress one to
complete, such that CPU2 is sure to not have a W entry left before CPU1
goes and copies the page.