Re: [PATCH v4 5/9] mm: Add write-protect and clean utilities for address space ranges

[Linus Torvalds]

On Tue, Oct 8, 2019 at 2:15 AM Thomas HellstrÃm (VMware)
<thomas_os@xxxxxxxxxxxx> wrote:
>
> Add two utilities to 1) write-protect and 2) clean all ptes pointing into
> a range of an address space.

The code looks much simpler and easier to understand now, I think, but
that also makes some thing more obvious..

> +static int clean_record_pte(pte_t *pte, unsigned long addr,
> +                           unsigned long end, struct mm_walk *walk)
> +{
> +       struct wp_walk *wpwalk = walk->private;
> +       struct clean_walk *cwalk = to_clean_walk(wpwalk);
> +       pte_t ptent = *pte;
> +
> +       if (pte_dirty(ptent)) {
> +               pgoff_t pgoff = ((addr - walk->vma->vm_start) >> PAGE_SHIFT) +
> +                       walk->vma->vm_pgoff - cwalk->bitmap_pgoff;
> +               pte_t old_pte = ptep_modify_prot_start(walk->vma, addr, pte);
> +
> +               ptent = pte_mkclean(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);
> +
> +               __set_bit(pgoff, cwalk->bitmap);

The above looks fundamentally racy.

You clean the pte in memory, but it remains dirty and writable in the
TLB, and you only flush it much later.

So now writes can continue to happen to that page, without it
necessarily being marked dirty again in the page tables, because all
the CPU TLB caches say "it's already dirty".

This may be ok - you've moved the diry bit into that bitmap, and you
will flush the TLB before then taking action on the bitmap. So you
haven't really _lost_ any dirty bits, but it still may be worth a
comment.

You do have comments about the _other_ issues (ie the whole "If a
caller needs to make sure all dirty ptes are picked up and none
additional are added..") but you don't actually have comments about
the TLB race basically potentially now causing "missing" dirty stuff.

And this may actually be a big problem on some architectures. Not x86,
and maybe nothing else, but I have this dim memory of some
architectures being unhappy due to virtual caches, and writeback when
the page table entry says it's read-only and clean.

Our normal "clean pages" is very anal about this, and does

                        flush_cache_page(vma, address, pte_pfn(*pte));
                        entry = ptep_clear_flush(vma, address, pte);
                        entry = pte_wrprotect(entry);
                        entry = pte_mkclean(entry);
                        set_pte_at(vma->vm_mm, address, pte, entry);

when it cleans a page, and I note both the cache flush and the
"clear_flush()" (which invalidates the pte and does a synchronous TLB
flush) and we have magic semantics for that at least on s390 because
there are some low-level architecture details wrt flushing TLB entries
and modifying them.

End result: I think the code is probably ok in practice because you
don't mind the slightly fuzzy dirty bit state, and it's _probably_ ok
on all architectures that use drm for the TLB invalidation side. But I
think it bears a bit of thinking about.

              Linus