On Fri, Jul 5, 2024 at 11:12 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
>
> * Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [240705 13:02]:
> > On Thu, Jul 04, 2024 at 02:27:04PM GMT, Liam R. Howlett wrote:
> > > Extract clean up of failed munmap() operations from
> > > do_vmi_align_munmap(). This simplifies later patches in the series.
> > >
> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > ---
> > > mm/mmap.c | 25 ++++++++++++++++++++-----
> > > 1 file changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 28a46d9ddde0..d572e1ff8255 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -2586,6 +2586,25 @@ struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi,
> > > vma->vm_userfaultfd_ctx, anon_vma_name(vma));
> > > }
> > >
> > > +/*
> > > + * abort_munmap_vmas - Undo any munmap work and free resources
> > > + *
> > > + * Reattach detached vmas, free up maple tree used to track the vmas.
> > > + */
> > > +static inline void abort_munmap_vmas(struct ma_state *mas_detach)
> > > +{
> > > + struct vm_area_struct *vma;
> > > + int limit;
> > > +
> > > + limit = mas_detach->index;
> >
> > This feels like a change to existing behaviour actually, I mean a sensible
> > one - as you are not just walking the tree start-to-end but rather only
> > walking up to the point that it has been populated (assuming I'm not
> > missing anything, looks to me like mas_for_each is _inclusive_ on max).
>
> This is not the main tree, but the detached tree. It only contains the
> vmas that are going to be freed (or, rather aborted from being freed).
>
> I see what you mean that the end in the abort code below would be one
> beyond the tree walk. The new abort code uses the index (from the
> previous write) as the limit.
>
> All that really matters is that we go to a number high enough to cover
> all vmas that were detached. I used 'end' in the below code because I
> knew it would cover all of the vmas added (we actually start at index
> 0).
>
> The value of 'mas_detach->index' is used in the new code because I knew
> that's as far as I had to go, and I could limit the arguments passed
> to the function.
>
> I think that I'll actually change limit to ULONG_MAX in another revision
> because I like that better than expecting the index to have not been
> touched by others.
>
> >
> > Maybe worth mentioning in commit msg?
>
> Yes, good idea. Thanks for catching this.
>
> >
> > > + mas_set(mas_detach, 0);
> > > + /* Re-attach any detached VMAs */
> > > + mas_for_each(mas_detach, vma, limit)
> > > + vma_mark_detached(vma, false);
> > > +
> > > + __mt_destroy(mas_detach->tree);
> > > +}
> > > +
> > > /*
> > > * do_vmi_align_munmap() - munmap the aligned region from @start to @end.
> > > * @vmi: The vma iterator
> > > @@ -2740,11 +2759,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > > userfaultfd_error:
> > > munmap_gather_failed:
> > > end_split_failed:
> > > - mas_set(&mas_detach, 0);
> > > - mas_for_each(&mas_detach, next, end)
> > > - vma_mark_detached(next, false);
> > > -
> > > - __mt_destroy(&mt_detach);
> > > + abort_munmap_vmas(&mas_detach);
> > > start_split_failed:
> > > map_count_exceeded:
> > > validate_mm(mm);
> > > --
> > > 2.43.0
> > >
> >
> > This looks fine though, feel free to add:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>
> Thanks.
>
