linux-kernel - Re: [PATCH] mm: Always sanity check anon

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZhlCVOz7qaDtldfL@casper.infradead.org>
Date: Fri, 12 Apr 2024 15:16:52 +0100
From: Matthew Wilcox <willy@...radead.org>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Peter Xu <peterx@...hat.com>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Lokesh Gidra <lokeshgidra@...gle.com>,
	Alistair Popple <apopple@...dia.com>
Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks

On Fri, Apr 12, 2024 at 06:06:46AM -0700, Suren Baghdasaryan wrote:
> > On Fri, Apr 12, 2024 at 04:14:16AM +0100, Matthew Wilcox wrote:
> > > -       /*
> > > -        * find_mergeable_anon_vma uses adjacent vmas which are not locked.
> > > -        * This check must happen after vma_start_read(); otherwise, a
> > > -        * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA
> > > -        * from its anon_vma.
> > > -        */
> > > -       if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma))
> > > -               goto inval_end_read;
> > > -
> > >         /* Check since vm_start/vm_end might change before we lock the VMA */
> > >         if (unlikely(address < vma->vm_start || address >= vma->vm_end))
> > >                 goto inval_end_read;
> > >
> > > That takes a few insns out of the page fault path (good!) at the cost
> > > of one extra trip around the fault handler for the first fault on an
> > > anon vma.  It makes the file & anon paths more similar to each other
> > > (good!)
> > >
> > > We'd need some data to be sure it's really a win, but less code is
> > > always good.
> 
> I agree, if we make this change we should keep this comment and maybe
> move it into vmf_anon_prepare()

Most of the comment makes no sense if you move it out of
lock_vma_under_rcu().  It's justifying where it needs to be in that
function.  If it's no longer in that function, there's not much left of
the comment.  What part do you think is valuable and needs to be retained?