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

[Jeff Xu <jeffxu@google.com>]

+ Kees who commented on mseal() series before. Please keep Kees in the
cc for future response to this series.

On Fri, Aug 9, 2024 at 12:25 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Pedro Falcato <pedro.falcato@gmail.com> [240809 14:53]:
> > 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).
>
> splitting a sealed vma wasn't supposed to be okay.. but it is Jeff's
> feature.  Jeff?
>
Splitting a sealed VMA is wrong.
Whoever wants to split a sealed VMA should  answer this question
first: what is the use case for splitting a sealed VMA?

The V2 series doesn't have selftest change which indicates lack of
testing. The out-of-loop check is positioned nearer to the API entry
point and separated from internal business logic, thereby minimizing
the testing requirements. However, as we move the sealing check
further inward and intertwine it with business logic, greater test
coverage becomes necessary to ensure  the correctness of  sealing
is preserved.




-Jeff
















> >
> > >
> > > >
> > > > >             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)) {
>
> Good catch.
>
> > instead of (vma). It's somewhat concerning how the mseal selftests
> > didn't trip on this?
>
> the end check will call split and will fail in there, if you move the
> code as I suggested.
>
> That means, if we aren't splitting, we still have to check the vma, so
> the check is necessary.
>
>