Re: [PATCH v9 05/17] mm: mark vmas detached upon exit

[Suren Baghdasaryan <surenb@google.com>]

On Mon, Jan 13, 2025 at 12:32 PM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> On 1/13/25 20:11, Suren Baghdasaryan wrote:
> > On Mon, Jan 13, 2025 at 9:13 AM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> >>
> >> On Mon, Jan 13, 2025 at 09:02:50AM -0800, Suren Baghdasaryan wrote:
> >> > On Mon, Jan 13, 2025 at 4:05 AM Lorenzo Stoakes
> >> > <lorenzo.stoakes@oracle.com> wrote:
> >> > >
> >> > > On Fri, Jan 10, 2025 at 08:25:52PM -0800, Suren Baghdasaryan wrote:
> >> > > > When exit_mmap() removes vmas belonging to an exiting task, it does not
> >> > > > mark them as detached since they can't be reached by other tasks and they
> >> > > > will be freed shortly. Once we introduce vma reuse, all vmas will have to
> >> > > > be in detached state before they are freed to ensure vma when reused is
> >> > > > in a consistent state. Add missing vma_mark_detached() before freeing the
> >> > > > vma.
> >> > >
> >> > > Hmm this really makes me worry that we'll see bugs from this detached
> >> > > stuff, do we make this assumption anywhere else I wonder?
> >> >
> >> > This is the only place which does not currently detach the vma before
> >> > freeing it. If someone tries adding a case like that in the future,
> >> > they will be met with vma_assert_detached() inside vm_area_free().
> >>
> >> OK good to know!
> >>
> >> Again, I wonder if we should make these assertions stronger as commented
> >> elsewhere, because if we see them in production isn't that worth an actual
> >> non-debug WARN_ON_ONCE()?
> >
> > Sure. I'll change vma_assert_attached()/vma_assert_detached() to use
> > WARN_ON_ONCE() and to return a bool (see also my reply in the patch
> > [0/17]).
>
> So is this a case of "someone might introduce code later that will violate
> them" as alluded to above? Unconditional WARN_ON_ONCE seems too much then.

Yes, I wanted to make sure refcounting will not be broken by someone
doing re-attach/re-detach.

>
> In general it's not easy to determine how paranoid we should be in non-debug
> code, but I'm not sure what's the need here specifically.

I'm not sure how strict we should be but we definitely should try to
catch refcounting mistakes and that's my goal here.

>