Re: [PATCH v6 10/16] mm: replace vm_lock and detached flag with a reference count

[Suren Baghdasaryan]

On Wed, Dec 18, 2024 at 7:37 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Peter Zijlstra <peterz@xxxxxxxxxxxxx> [241218 05:06]:
> > On Wed, Dec 18, 2024 at 10:41:04AM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 17, 2024 at 08:27:46AM -0800, Suren Baghdasaryan wrote:
> > >
> > > > > So I just replied there, and no, I don't think it makes sense. Just put
> > > > > the kmem_cache_free() in vma_refcount_put(), to be done on 0.
> > > >
> > > > That's very appealing indeed and makes things much simpler. The
> > > > problem I see with that is the case when we detach a vma from the tree
> > > > to isolate it, then do some cleanup and only then free it. That's done
> > > > in vms_gather_munmap_vmas() here:
> > > > https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1240 and we
> > > > even might reattach detached vmas back:
> > > > https://elixir.bootlin.com/linux/v6.12.5/source/mm/vma.c#L1312. IOW,
> > > > detached state is not final and we can't destroy the object that
> > > > reached this state.
> > >
> > > Urgh, so that's the munmap() path, but arguably when that fails, the
> > > map stays in place.
> > >
> > > I think this means you're marking detached too soon; you should only
> > > mark detached once you reach the point of no return.
> > >
> > > That said, once you've reached the point of no return; and are about to
> > > go remove the page-tables, you very much want to ensure a lack of
> > > concurrency.
> > >
> > > So perhaps waiting for out-standing readers at this point isn't crazy.
> > >
> > > Also, I'm having a very hard time reading this maple tree stuff :/
> > > Afaict vms_gather_munmap_vmas() only adds the VMAs to be removed to a
> > > second tree, it does not in fact unlink them from the mm yet.
>
> Yes, that's correct.  I tried to make this clear with a gather/complete
> naming like other areas of the mm.  I hope that helped.
>
> Also, the comments for the function state that's what's going on:
>
>  * vms_gather_munmap_vmas() - Put all VMAs within a range into a maple tree
>  * for removal at a later date.  Handles splitting first and last if necessary
>  * and marking the vmas as isolated.
>
> ... might be worth updating with new information.
>
> > >
> > > AFAICT it's vma_iter_clear_gfp() that actually wipes the vmas from the
> > > mm -- and that being able to fail is mind boggling and I suppose is what
> > > gives rise to much of this insanity :/
>
> This is also correct.  The maple tree is a b-tree variant that has
> internal nodes.  When you write to it, including nulls, they are tracked
> and may need to allocate.  This is a cost for rcu lookups; we will use
> the same or less memory in the end but must maintain a consistent view
> of the ranges.
>
> But to put this into perspective, we get 16 nodes per 4k page, most
> writes will use 1 or 3 of these from a kmem_cache, so we are talking
> about a very unlikely possibility.  Except when syzbot decides to fail
> random allocations.
>
> We could preallocate for the write, but this section of the code is
> GFP_KERNEL, so we don't.  Preallocation is an option to simplify the
> failure path though... which is what you did below.
>
> > >
> > > Anyway, I would expect remove_vma() to be the one that marks it detached
> > > (it's already unreachable through vma_lookup() at this point) and there
> > > you should wait for concurrent readers to bugger off.
> >
> > Also, I think vma_start_write() in that gather look is too early, you're
> > not actually going to change the VMA yet -- with obvious exception of
> > the split cases.
>
> The split needs to start the write on the vma to avoid anyone reading it
> while it's being altered.

I think vma_start_write() should be done inside
vms_gather_munmap_vmas() for __mmap_prepare() to work correctly:

__mmap_prepare
    vms_gather_munmap_vmas
    vms_clean_up_area // clears PTEs
...
__mmap_complete
    vms_complete_munmap_vmas

If we do not write-lock the vmas inside vms_gather_munmap_vmas(), we
will be clearing PTEs from under a discoverable vma.
There might be other places like this too but I think we can move
vma_mark_detach() like you suggested without moving vma_start_write()
and that should be enough.

>
> >
> > That too should probably come after you've passes all the fail/unwind
> > spots.
>
> Do you mean the split?  I'd like to move the split later as well..
> tracking that is a pain and may need an extra vma for when one vma is
> split twice before removing the middle part.
>
> Actually, I think we need to allocate two (or at least one) vmas in this
> case and just pass one through to unmap (written only to the mas_detach
> tree?).  It would be nice to find a way to NOT need to do that even.. I
> had tried to use a vma on the stack years ago, which didn't work out.
>
> >
> > Something like so perhaps? (yeah, I know, I wrecked a bunch)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 8e31b7e25aeb..45d43adcbb36 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -1173,6 +1173,11 @@ static void vms_complete_munmap_vmas(struct vma_munmap_struct *vms,
> >       struct vm_area_struct *vma;
> >       struct mm_struct *mm;
> >
>
> mas_set(mas_detach, 0);
>
> > +     mas_for_each(mas_detach, vma, ULONG_MAX) {
> > +             vma_start_write(next);
> > +             vma_mark_detached(next, true);
> > +     }
> > +
> >       mm = current->mm;
> >       mm->map_count -= vms->vma_count;
> >       mm->locked_vm -= vms->locked_vm;
> > @@ -1219,9 +1224,6 @@ static void reattach_vmas(struct ma_state *mas_detach)
> >       struct vm_area_struct *vma;
> >
>
> >       mas_set(mas_detach, 0);
> Drop the mas_set here.
>
> > -     mas_for_each(mas_detach, vma, ULONG_MAX)
> > -             vma_mark_detached(vma, false);
> > -
> >       __mt_destroy(mas_detach->tree);
> >  }
> >
> > @@ -1289,13 +1291,11 @@ static int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
> >                       if (error)
> >                               goto end_split_failed;
> >               }
> > -             vma_start_write(next);
> >               mas_set(mas_detach, vms->vma_count++);
> >               error = mas_store_gfp(mas_detach, next, GFP_KERNEL);
> >               if (error)
> >                       goto munmap_gather_failed;
> >
> > -             vma_mark_detached(next, true);
> >               nrpages = vma_pages(next);
> >
> >               vms->nr_pages += nrpages;
> > @@ -1431,14 +1431,17 @@ int do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >       struct vma_munmap_struct vms;
> >       int error;
> >
>
> The preallocation needs to know the range being stored to know what's
> going to happen.
>
> vma_iter_config(vmi, start, end);
>
> > +     error = mas_preallocate(vmi->mas);
>
> We haven't had a need to have a vma iterator preallocate for storing a
> null, but we can add one for this.
>
> > +     if (error)
> > +             goto gather_failed;
> > +
> >       init_vma_munmap(&vms, vmi, vma, start, end, uf, unlock);
> >       error = vms_gather_munmap_vmas(&vms, &mas_detach);
> >       if (error)
> >               goto gather_failed;
> >
>
> Drop this stuff.
> >       error = vma_iter_clear_gfp(vmi, start, end, GFP_KERNEL);
> > -     if (error)
> > -             goto clear_tree_failed;
> > +     VM_WARN_ON(error);
>
> Do this instead
> vma_iter_config(vmi, start, end);
> vma_iter_clear(vmi);
>
> >
> >       /* Point of no return */
> >       vms_complete_munmap_vmas(&vms, &mas_detach);