Re: [PATCH 07/13] mm/munlock: mlock_pte_range() when mlocking or munlocking

[Hugh Dickins]

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