Messages in this thread | | | Subject | Re: [PATCH] mm/userfaultfd: fix memory corruption due to writeprotect | From | Laurent Dufour <> | Date | Tue, 12 Jan 2021 20:02:24 +0100 |
| |
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.
| |