On 20.04.22 19:10, Vlastimil Babka wrote: > On 3/29/22 18:43, David Hildenbrand wrote: >> Currently, we clear PG_anon_exclusive in try_to_unmap() and forget about >> it. We do this, to keep fork() logic on swap entries easy and efficient: >> for example, if we wouldn't clear it when unmapping, we'd have to lookup >> the page in the swapcache for each and every swap entry during fork() and >> clear PG_anon_exclusive if set. >> >> Instead, we want to store that information directly in the swap pte, >> protected by the page table lock, similarly to how we handle >> SWP_MIGRATION_READ_EXCLUSIVE for migration entries. However, for actual >> swap entries, we don't want to mess with the swap type (e.g., still one >> bit) because it overcomplicates swap code. >> >> In try_to_unmap(), we already reject to unmap in case the page might be >> pinned, because we must not lose PG_anon_exclusive on pinned pages ever. >> Checking if there are other unexpected references reliably *before* >> completely unmapping a page is unfortunately not really possible: THP >> heavily overcomplicate the situation. Once fully unmapped it's easier -- >> we, for example, make sure that there are no unexpected references >> *after* unmapping a page before starting writeback on that page. >> >> So, we currently might end up unmapping a page and clearing >> PG_anon_exclusive if that page has additional references, for example, >> due to a FOLL_GET. >> >> do_swap_page() has to re-determine if a page is exclusive, which will >> easily fail if there are other references on a page, most prominently >> GUP references via FOLL_GET. This can currently result in memory >> corruptions when taking a FOLL_GET | FOLL_WRITE reference on a page even >> when fork() is never involved: try_to_unmap() will succeed, and when >> refaulting the page, it cannot be marked exclusive and will get replaced >> by a copy in the page tables on the next write access, resulting in writes >> via the GUP reference to the page being lost. >> >> In an ideal world, everybody that uses GUP and wants to modify page >> content, such as O_DIRECT, would properly use FOLL_PIN. However, that >> conversion will take a while. It's easier to fix what used to work in the >> past (FOLL_GET | FOLL_WRITE) remembering PG_anon_exclusive. In addition, >> by remembering PG_anon_exclusive we can further reduce unnecessary COW >> in some cases, so it's the natural thing to do. >> >> So let's transfer the PG_anon_exclusive information to the swap pte and >> store it via an architecture-dependant pte bit; use that information when >> restoring the swap pte in do_swap_page() and unuse_pte(). During fork(), we >> simply have to clear the pte bit and are done. >> >> Of course, there is one corner case to handle: swap backends that don't >> support concurrent page modifications while the page is under writeback. >> Special case these, and drop the exclusive marker. Add a comment why that >> is just fine (also, reuse_swap_page() would have done the same in the >> past). >> >> In the future, we'll hopefully have all architectures support >> __HAVE_ARCH_PTE_SWP_EXCLUSIVE, such that we can get rid of the empty >> stubs and the define completely. Then, we can also convert >> SWP_MIGRATION_READ_EXCLUSIVE. For architectures it's fairly easy to >> support: either simply use a yet unused pte bit that can be used for swap >> entries, steal one from the arch type bits if they exceed 5, or steal one >> from the offset bits. >> >> Note: R/O FOLL_GET references were never really reliable, especially >> when taking one on a shared page and then writing to the page (e.g., GUP >> after fork()). FOLL_GET, including R/W references, were never really >> reliable once fork was involved (e.g., GUP before fork(), >> GUP during fork()). KSM steps back in case it stumbles over unexpected >> references and is, therefore, fine. >> >> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx> > > With the fixup as reportedy by Miaohe Lin > > Acked-by: Vlastimil Babka <vbabka@xxxxxxx> > > (sent a separate mm-commits mail to inquire about the fix going missing from > mmotm) > > https://lore.kernel.org/mm-commits/c3195d8a-2931-0749-973a-1d04e4baec94@xxxxxxx/T/#m4e98ccae6f747e11f45e4d0726427ba2fef740eb Yes I saw that, thanks for catching that! -- Thanks, David / dhildenb
