On Mon, 7 Feb 2022, Hillf Danton wrote:
> On Sun, 6 Feb 2022 13:42:09 -0800 (PST) Hugh Dickins wrote:
> > +static void mlock_vma_pages_range(struct vm_area_struct *vma,
> > + unsigned long start, unsigned long end, vm_flags_t newflags)
> > {
> > - /* Reimplementation to follow in later commit */
> > + static const struct mm_walk_ops mlock_walk_ops = {
> > + .pmd_entry = mlock_pte_range,
> > + };
> > +
> > + /*
> > + * There is a slight chance that concurrent page migration,
> > + * or page reclaim finding a page of this now-VM_LOCKED vma,
> > + * will call mlock_vma_page() and raise page's mlock_count:
> > + * double counting, leaving the page unevictable indefinitely.
> > + * Communicate this danger to mlock_vma_page() with VM_IO,
> > + * which is a VM_SPECIAL flag not allowed on VM_LOCKED vmas.
> > + * mmap_lock is held in write mode here, so this weird
> > + * combination should not be visible to others.
> > + */
> > + if (newflags & VM_LOCKED)
> > + newflags |= VM_IO;
> > + WRITE_ONCE(vma->vm_flags, newflags);
>
> Nit
>
> The WRITE_ONCE is not needed, given the certainty of invisibility to
> others - it will quiesce syzbot reporting the case of visibility.
Ah, maybe I can rewrite that comment better: when I said "visible to
others", I meant visible to "the outside world", those participating in
the usual mmap_lock'ed access, syscalls and /proc/pid/maps and smaps etc.
The point here is that some kernel low-level internals (page migration
and page reclaim) peek at vma->vm_flags without mmap_lock (but with
anon_vma lock or i_mmap_rwsem).
Originally I had VM_LOCKED set in vma->vm_flags before calling
mlock_vma_pages_range(), no need for a newflags parameter. Then
realized that left a tiny window in which VM_LOCKED was visible to
migration and reclaim without the safening VM_IO, so changed it to pass
in newflags, then "newflags |= VM_IO", then "vma->vm_flags = newflags"
there. Then realized that perhaps an uncooperative compiler might be
inspired to mutate that into "vma->vm_flags = newflags" followed by
"vma->vm_flags |= VM_IO". I hope it would not, but can I be sure
that it would not? That's why I ended up with WRITE_ONCE() there.
Maybe all rather overkill: but trying to ensure that we undercount
mmap_locked rather than risk overcounting it.
Hugh