[cc’ing IOMMU people, which for some reason are not cc’d] Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote: > On Wed, Aug 30, 2017 at 11:00:32AM -0700, Nadav Amit wrote: >> It is not trivial to flush TLBs (primary or secondary) without holding the >> page-table lock, and as we recently encountered this resulted in several >> bugs [1]. The main problem is that even if you perform the TLB flush >> immediately after the PT-lock is released, you cause a situation in which >> other threads may make decisions based on the updated PTE value, without >> being aware that a TLB flush is needed. >> >> For example, we recently encountered a Linux bug when two threads run >> MADV_DONTNEED concurrently on the same address range [2]. One of the threads >> may update a PTE, setting it as non-present, and then deferring the TLB >> flush (for batching). As a result, however, it would cause the second >> thread, which also changes the PTEs to assume that the PTE is already >> non-present and TLB flush is not necessary. As a result the second core may >> still hold stale PTEs in its TLB following MADV_DONTNEED. > > The source of those complex races that requires taking into account > nested tlb gather to solve it, originates from skipping primary MMU > tlb flushes depending on the value of the pagetables (as an > optimization). > > For mmu_notifier_invalidate_range_end we always ignore the value of > the pagetables and mmu_notifier_invalidate_range_end always runs > unconditionally invalidating the secondary MMU for the whole range > under consideration. There are no optimizations that attempts to skip > mmu_notifier_invalidate_range_end depending on the pagetable value and > there's no TLB gather for secondary MMUs either. That is to keep it > simple of course. > > As long as those mmu_notifier_invalidate_range_end stay unconditional, > I don't see how those races you linked, can be possibly relevant in > evaluating if ->invalidate_range (again only for iommuv2 and > intel-svm) has to run inside the PT lock or not. Thanks for the clarifications. It now makes much more sense. > >> There is a swarm of such problems, and some are not too trivial. Deferring >> TLB flushes needs to be done in a very careful manner. > > I agree it's not trivial, but I don't think any complexity comes from > above. > > The only complexity is about, what if the page is copied to some other > page and replaced, because the copy is the only case where coherency > could be retained by the primary MMU. What if the primary MMU starts > working on the new page in between PT lock release and > mmu_notifier_invalidate_range_end, while the secondary MMU is stuck on > the old page? That is the only problem we deal with here, the copy to > other page and replace. Any other case that doesn't involve the copy > seems non coherent by definition, and you couldn't measure it. > > I can't think of a scenario that requires the explicit > mmu_notifier_invalidate_range call before releasing the PT lock, at > least for try_to_unmap_one. > > Could you think of a scenario where calling ->invalidate_range inside > mmu_notifier_invalidate_range_end post PT lock breaks iommuv2 or > intel-svm? Those two are the only ones requiring > ->invalidate_range calls, all other mmu notifier users are safe > without running mmu_notifier_invalidate_range_end under PT lock thanks > to mmu_notifier_invalidate_range_start blocking the secondary MMU. > > Could you post a tangible scenario that invalidates my theory that > those mmu_notifier_invalidate_range calls inside PT lock would be > superfluous? > > Some of the scenarios under consideration: > > 1) migration entry -> migration_entry_wait -> page lock, plus > migrate_pages taking the lock so it can't race with try_to_unmap > from other places > 2) swap entry -> lookup_swap_cache -> page lock (page not really replaced) > 3) CoW -> do_wp_page -> page lock on old page > 4) KSM -> replace_page -> page lock on old page > 5) if the pte is zapped as in MADV_DONTNEED, no coherency possible so > it's not measurable that we let the guest run a bit longer on the > old page past PT lock release For both CoW and KSM, the correctness is maintained by calling ptep_clear_flush_notify(). If you defer the secondary MMU invalidation (i.e., replacing ptep_clear_flush_notify() with ptep_clear_flush() ), you will cause memory corruption, and page-lock would not be enough. BTW: I see some calls to ptep_clear_flush_notify() which are followed immediately after by set_pte_at_notify(). I do not understand why it makes sense, as both notifications end up doing the same thing - invalidating the secondary MMU. The set_pte_at_notify() in these cases can be changed to set_pte(). No? > If you could post a multi CPU trace that shows how iommuv2 or > intel-svm are broken if ->invalidate_range is run inside > mmu_notifier_invalidate_range_end post PT lock in try_to_unmap_one it > would help. > > Of course if we run mmu_notifier_invalidate_range inside PT lock and > we remove ->invalidate_range from mmu_notifier_invalidate_range_stop > all will be obviously safe, so we could just do it to avoid thinking > about the above, but the resulting code will be uglier and less > optimal (even when disarmed there will be dummy branches I wouldn't > love) and I currently can't see why it wouldn't be safe. > > Replacing a page completely without any relation to the old page > content allows no coherency anyway, so even if it breaks you cannot > measure it because it's undefined. > > If the page has a relation with the old contents and coherency has to > be provided for both primary MMU and secondary MMUs, this relation > between old and new page during the replacement, is enforced by some > other mean besides the PT lock, migration entry on locked old page > with migration_entry_wait and page lock in migrate_pages etc.. I think that basically you are correct, and assuming that you always notify/invalidate unconditionally any PTE range you read/write, you are safe. Yet, I want to have another look at the code. Anyhow, just deferring all the TLB flushes, including those of set_pte_at_notify(), is likely to result in errors. Regards, Nadav -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href
