Re: [PATCH v2 2/6] mm/munmap: Replace can_modify_mm with can_modify_vma

[Pedro Falcato <pedro.falcato@gmail.com>]

On Fri, Aug 9, 2024 at 5:48 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Liam R. Howlett <Liam.Howlett@oracle.com> [240809 12:15]:
> > * Pedro Falcato <pedro.falcato@gmail.com> [240807 17:13]:
> > > We were doing an extra mmap tree traversal just to check if the entire
> > > range is modifiable. This can be done when we iterate through the VMAs
> > > instead.
> > >
> > > Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>
> > > ---
> > >  mm/mmap.c | 13 +------------
> > >  mm/vma.c  | 23 ++++++++++++-----------
> > >  2 files changed, 13 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 4a9c2329b09..c1c7a7d00f5 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -1740,18 +1740,7 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >             unsigned long start, unsigned long end, struct list_head *uf,
> > >             bool unlock)
> > >  {
> > > -   struct mm_struct *mm = vma->vm_mm;
> > > -
> > > -   /*
> > > -    * Check if memory is sealed before arch_unmap.
> > > -    * Prevent unmapping a sealed VMA.
> > > -    * can_modify_mm assumes we have acquired the lock on MM.
> > > -    */
> > > -   if (unlikely(!can_modify_mm(mm, start, end)))
> > > -           return -EPERM;
> > > -
> > > -   arch_unmap(mm, start, end);
> > > -   return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlock);
> > > +   return do_vmi_align_munmap(vmi, vma, vma->vm_mm, start, end, uf, unlock);
> > >  }
> > >
> > >  /*
> > > diff --git a/mm/vma.c b/mm/vma.c
> > > index bf0546fe6ea..7a121bcc907 100644
> > > --- a/mm/vma.c
> > > +++ b/mm/vma.c
> > > @@ -712,6 +712,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >             if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
> > >                     goto map_count_exceeded;
> > >
> > > +           /* Don't bother splitting the VMA if we can't unmap it anyway */
> > > +           if (!can_modify_vma(vma)) {
> > > +                   error = -EPERM;
> > > +                   goto start_split_failed;
> > > +           }
> > > +
> >
> > Would this check be better placed in __split_vma()?  It could replace
> > both this and the next chunk of code.
>
> not quite.

Yeah, I was going to say that splitting a sealed VMA is okay (and we
allow it on mlock and madvise).

>
> >
> > >             error = __split_vma(vmi, vma, start, 1);
> > >             if (error)
> > >                     goto start_split_failed;
> > > @@ -723,6 +729,11 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >      */
> > >     next = vma;
> > >     do {
> > > +           if (!can_modify_vma(vma)) {
> > > +                   error = -EPERM;
> > > +                   goto modify_vma_failed;
> > > +           }
> > > +
>
> This chunk would need to be moved below the end check so that we catch
> full vma unmaps.

Why below the end check? I believe we can avoid the split? Is there
something I'm missing?
But I did find a bug, what I really seem to want is:

 +           if (!can_modify_vma(next)) {
instead of (vma). It's somewhat concerning how the mseal selftests
didn't trip on this?

-- 
Pedro