> On Dec 19, 2020, at 10:05 PM, Yu Zhao <yuzhao@xxxxxxxxxx> wrote: > > On Sat, Dec 19, 2020 at 01:34:29PM -0800, Nadav Amit wrote: >> [ cc’ing some more people who have experience with similar problems ] >> >>> On Dec 19, 2020, at 11:15 AM, Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote: >>> >>> Hello, >>> >>> On Fri, Dec 18, 2020 at 08:30:06PM -0800, Nadav Amit wrote: >>>> Analyzing this problem indicates that there is a real bug since >>>> mmap_lock is only taken for read in mwriteprotect_range(). This might >>> >>> Never having to take the mmap_sem for writing, and in turn never >>> blocking, in order to modify the pagetables is quite an important >>> feature in uffd that justifies uffd instead of mprotect. It's not the >>> most important reason to use uffd, but it'd be nice if that guarantee >>> would remain also for the UFFDIO_WRITEPROTECT API, not only for the >>> other pgtable manipulations. >>> >>>> Consider the following scenario with 3 CPUs (cpu2 is not shown): >>>> >>>> cpu0 cpu1 >>>> ---- ---- >>>> userfaultfd_writeprotect() >>>> [ write-protecting ] >>>> mwriteprotect_range() >>>> mmap_read_lock() >>>> change_protection() >>>> change_protection_range() >>>> ... >>>> change_pte_range() >>>> [ defer TLB flushes] >>>> userfaultfd_writeprotect() >>>> mmap_read_lock() >>>> change_protection() >>>> [ write-unprotect ] >>>> ... >>>> [ unprotect PTE logically ] >>>> ... >>>> [ page-fault] >>>> ... >>>> wp_page_copy() >>>> [ set new writable page in PTE] > > I don't see any problem in this example -- wp_page_copy() calls > ptep_clear_flush_notify(), which should take care of the stale entry > left by cpu0. > > That being said, I suspect the memory corruption you observed is > related this example, with cpu1 running something else that flushes > conditionally depending on pte_write(). > > Do you know which type of pages were corrupted? file, anon, etc. First, Yu, you are correct. My analysis is incorrect, but let me have another try (below). To answer your (and Andrea’s) question - this happens with upstream without any changes, excluding a small fix to the selftest, since it failed (got stuck) due to missing wake events. [1] We are talking about anon memory. So to correct myself, I think that what I really encountered was actually during MM_CP_UFFD_WP_RESOLVE (i.e., when the protection is removed). The problem was that in this case the “write”-bit was removed during unprotect. Sorry for the strange formatting to fit within 80 columns: [ Start: PTE is writable ] 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() [ End: cpu2 write not copied form old to new page. ] So this was actually resolved by the second part of the patch - changing preserve_write in change_pte_range(). I removed the acquisition of mmap_lock for write, left the change in change_pte_range() and the test passes. Let me give some more thought on whether a mmap_lock is needed for write. I need to rehash this TLB flushing algorithm. Thanks, Nadav [1] https://lore.kernel.org/patchwork/patch/1346386
