linux-kernel - Re: [PATCH RFC 1/2] mm, madvise: simplify 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 for Android: free password hash cracker in your pocket

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

Message-ID: <rhjfvvndg23mhvuddtrn35fmbcteafhjusqcj6uug3blvrykld@voxejacky3cb>
Date: Mon, 23 Jun 2025 12:22:26 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        David Hildenbrand <david@...hat.com>, Jann Horn <jannh@...gle.com>,
        Mike Rapoport <rppt@...nel.org>, Michal Hocko <mhocko@...e.com>,
        Colin Cross <ccross@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] mm, madvise: simplify anon_name handling

* Suren Baghdasaryan <surenb@...gle.com> [250623 11:39]:
> On Mon, Jun 23, 2025 at 8:00 AM Vlastimil Babka <vbabka@...e.cz> wrote:
> >
> > Since the introduction in 9a10064f5625 ("mm: add a field to store names
> > for private anonymous memory") the code to set anon_name on a vma has
> > been using madvise_update_vma() to call replace_vma_anon_name(). Since
> 
> s/replace_vma_anon_name()/replace_anon_vma_name()
> 
> > the former is called also by a number of other madvise behaviours that
> > do not set a new anon_name, they have been passing the existing
> > anon_name of the vma to make replace_vma_anon_name() a no-op.
> >
> > This is rather wasteful as it needs anon_vma_name_eq() to determine the
> > no-op situations, and checks for when replace_vma_anon_name() is allowed
> > (the vma is anon/shmem) duplicate the checks already done earlier in
> > madvise_vma_behavior(). It has also lead to commit 942341dcc574 ("mm:
> > fix use-after-free when anon vma name is used after vma is freed")
> > adding anon_name refcount get/put operations exactly to the cases that
> > actually do not change anon_name - just so the replace_vma_anon_name()
> > can keep safely determining it has nothing to do.
> >
> > The recent madvise cleanups made this suboptimal handling very obvious,
> > but happily also allow for an easy fix. madvise_update_vma() now has the
> > complete information whether it's been called to set a new anon_name, so
> > stop passing it the existing vma's name and doing the refcount get/put
> > in its only caller madvise_vma_behavior().
> >
> > In madvise_update_vma() itself, limit calling of replace_anon_vma_name()
> > only to cases where we are setting a new name, otherwise we know it's a
> > no-op. We can rely solely on the __MADV_SET_ANON_VMA_NAME behaviour and
> > can remove the duplicate checks for vma being anon/shmem that were done
> > already in madvise_vma_behavior().
> >
> > The remaining reason to obtain the vma's existing anon_name is to pass
> > it to vma_modify_flags_name() for the splitting and merging to work
> > properly. In case of merging, the vma might be freed along with the
> > anon_name, but madvise_update_vma() will not access it afterwards
> 
> This is quite subtle. Can we add a comment in the code that anon_name
> might be freed as a result of vma merge after vma_modify_flags_name()
> gets called and anon_name should not be accessed afterwards?

Surely that's not the common pattern since the anon vma name is ref
counted?

And it's probably the case for more than just the anon name?

...

Thanks,
Liam