Message-ID: <8aa656f1-72bf-41d6-a79a-a37d56bea08b@lucifer.local>
Date: Mon, 23 Jun 2025 17:47:35 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        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

On Mon, Jun 23, 2025 at 12:22:26PM -0400, Liam R. Howlett wrote:
> * 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?

This is all quite tricky.

I think the key thing is that madvise_set_anon_name() is invoked by
prctl_set_vma() (yuck) which allocates a brand new anon_vma_name.

When we merge, we don't actually set that anon_vma_name to anything, but we
might put (and thereby maybe free) an anon_vma_name that is identical in
terms of the string as part of the merge.

But that'll be the vma->anon_vma_name that gets killed, not anon_vma_name
in madvise_update_vma(), as that hasn't been set yet :)

The problem was when doing so and referencing vma->anon_vma_name, and then
afterwards invoking replace_anon_vma_name().

The VMA being changed might have got deleted as part of a merge, and so
this could then be a dangling pointer.

But the irony is, in that case, there's really no need to call
replace_anon_vma_name() the one thing that actually uses anon_vma_name
after the merge... because trivially anon_vma_name_eq() effectively
comparing vma->anon_vma_name to itself will always be true and thus the
operation will always be a no-op.

Except it'll be a no-op referencing a dangling pointer...

The previous cleanups made this whole thing clearer (see, this all sounds
very claer right? :P) meaning the get/put are just totally unnecessary.

TL;DR - vma->anon_vma_name is _not being used after the merge_ here in any
case.

>
> ...
>
> Thanks,
> Liam

Powered by blists - more mailing lists