linux-kernel - Re: [PATCH 07/13] mm/munlock: mlock_pte

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <203c549d-ad8-948d-1a3a-13be026864e@google.com>
Date:   Mon, 7 Feb 2022 10:46:36 -0800 (PST)
From:   Hugh Dickins <hughd@...gle.com>
To:     Hillf Danton <hdanton@...a.com>
cc:     Hugh Dickins <hughd@...gle.com>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [PATCH 07/13] mm/munlock: mlock_pte_range() when mlocking or
 munlocking

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