On Thu, Oct 10, 2019 at 02:43:10PM +0200, Thomas Hellström (VMware) wrote:
> +/** > + * struct wp_walk - Private struct for pagetable walk callbacks > + * @range: Range for mmu notifiers > + * @tlbflush_start: Address of first modified pte > + * @tlbflush_end: Address of last modified pte + 1 > + * @total: Total number of modified ptes > + */ > +struct wp_walk { > + struct mmu_notifier_range range; > + unsigned long tlbflush_start; > + unsigned long tlbflush_end; > + unsigned long total; > +}; > + > +/** > + * wp_pte - Write-protect a pte > + * @pte: Pointer to the pte > + * @addr: The virtual page address > + * @walk: pagetable walk callback argument > + * > + * The function write-protects a pte and records the range in > + * virtual address space of touched ptes for efficient range TLB flushes. > + */ > +static int wp_pte(pte_t *pte, unsigned long addr, unsigned long end, > + struct mm_walk *walk) > +{ > + struct wp_walk *wpwalk = walk->private; > + pte_t ptent = *pte; > + > + if (pte_write(ptent)) { > + pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte); > + > + ptent = pte_wrprotect(old_pte); > + ptep_modify_prot_commit(walk->vma, addr, pte, old_pte, ptent); > + wpwalk->total++; > + wpwalk->tlbflush_start = min(wpwalk->tlbflush_start, addr); > + wpwalk->tlbflush_end = max(wpwalk->tlbflush_end, > + addr + PAGE_SIZE); > + } > + > + return 0; > +} > +/* > + * wp_clean_pre_vma - The pagewalk pre_vma callback. > + * > + * The pre_vma callback performs the cache flush, stages the tlb flush > + * and calls the necessary mmu notifiers. > + */ > +static int wp_clean_pre_vma(unsigned long start, unsigned long end, > + struct mm_walk *walk) > +{ > + struct wp_walk *wpwalk = walk->private; > + > + wpwalk->tlbflush_start = end; > + wpwalk->tlbflush_end = start; > + > + mmu_notifier_range_init(&wpwalk->range, MMU_NOTIFY_PROTECTION_PAGE, 0, > + walk->vma, walk->mm, start, end); > + mmu_notifier_invalidate_range_start(&wpwalk->range); > + flush_cache_range(walk->vma, start, end); > + > + /* > + * We're not using tlb_gather_mmu() since typically > + * only a small subrange of PTEs are affected, whereas > + * tlb_gather_mmu() records the full range. > + */ > + inc_tlb_flush_pending(walk->mm); > + > + return 0; > +} > + > +/* > + * wp_clean_post_vma - The pagewalk post_vma callback. > + * > + * The post_vma callback performs the tlb flush and calls necessary mmu > + * notifiers. > + */ > +static void wp_clean_post_vma(struct mm_walk *walk) > +{ > + struct wp_walk *wpwalk = walk->private; > + > + if (wpwalk->tlbflush_end > wpwalk->tlbflush_start) > + flush_tlb_range(walk->vma, wpwalk->tlbflush_start, > + wpwalk->tlbflush_end); > + > + mmu_notifier_invalidate_range_end(&wpwalk->range); > + dec_tlb_flush_pending(walk->mm); > +} > +/** > + * wp_shared_mapping_range - Write-protect all ptes in an address space range > + * @mapping: The address_space we want to write protect > + * @first_index: The first page offset in the range > + * @nr: Number of incremental page offsets to cover > + * > + * Note: This function currently skips transhuge page-table entries, since > + * it's intended for dirty-tracking on the PTE level. It will warn on > + * encountering transhuge write-enabled entries, though, and can easily be > + * extended to handle them as well. > + * > + * Return: The number of ptes actually write-protected. Note that > + * already write-protected ptes are not counted. > + */ > +unsigned long wp_shared_mapping_range(struct address_space *mapping, > + pgoff_t first_index, pgoff_t nr) > +{ > + struct wp_walk wpwalk = { .total = 0 }; > + > + i_mmap_lock_read(mapping); > + WARN_ON(walk_page_mapping(mapping, first_index, nr, &wp_walk_ops, > + &wpwalk)); > + i_mmap_unlock_read(mapping); > + > + return wpwalk.total; > +} That's a read lock, this means there's concurrency to self. What happens if someone does two concurrent wp_shared_mapping_range() on the same mapping? The thing is, because of pte_wrprotect() the iteration that starts last will see a smaller pte_write range, if it completes first and does flush_tlb_range(), it will only flush a partial range. This is exactly what {inc,dec}_tlb_flush_pending() is for, but you're not using mm_tlb_flush_nested() to detect the situation and do a bigger flush. Or if you're not needing that, then I'm missing why.