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]

Date: Thu, 11 Apr 2024 08:42:23 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Andrew Morton <akpm@...ux-foundation.org>, Lokesh Gidra <lokeshgidra@...gle.com>, 
	"Liam R . Howlett" <Liam.Howlett@...cle.com>, Alistair Popple <apopple@...dia.com>
Subject: Re: [PATCH] mm: Always sanity check anon_vma first for per-vma locks

On Wed, Apr 10, 2024 at 1:26 PM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Wed, Apr 10, 2024 at 01:06:21PM -0400, Peter Xu wrote:
> > anon_vma is a tricky object in the context of per-vma lock, because it's
> > racy to modify it in that context and mmap lock is needed if it's not
> > stable yet.
>
> I object to this commit message.  First, it's not a "sanity check".  It's
> a check to see if we already have an anon VMA.  Second, it's not "racy
> to modify it" at all.  The problem is that we need to look at other
> VMAs, for which we do not hold the lock.
>
> > So the trivial side effect of such patch is:
> >
> >   - We may do slightly better on the first WRITE of a private file mapping,
> >   because we can retry earlier (in lock_vma_under_rcu(), rather than
> >   vmf_anon_prepare() later).
> >
> >   - We may always use mmap lock for the initial READs on a private file
> >   mappings, while before this patch it _can_ (only when no WRITE ever
> >   happened... but it doesn't make much sense for a MAP_PRIVATE..) do the
> >   read fault with per-vma lock.
>
> But that's a super common path!  Look at 'cat /proc/self/maps'.  All
> your program text (including libraries) is mapped PRIVATE, and never
> written to (except by ptrace, I guess).

Uh, indeed I didn't realize this would be the side-effect from this
early check. And that's exactly why I wanted Matthew's input on this
in [1].

>
> NAK this patch.
>