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