* Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [240809 12:15]:
> * Pedro Falcato <pedro.falcato@xxxxxxxxx> [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@xxxxxxxxx>
> > ---
> > 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.
>
> > 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.
> > /* Does it split the end? */
> > if (next->vm_end > end) {
> > error = __split_vma(vmi, next, end, 0);
> > @@ -815,6 +826,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > __mt_destroy(&mt_detach);
> > return 0;
> >
> > +modify_vma_failed:
> > clear_tree_failed:
> > userfaultfd_error:
> > munmap_gather_failed:
> > @@ -860,17 +872,6 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
> > if (end == start)
> > return -EINVAL;
> >
> > - /*
> > - * 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() might do unmaps itself. */
> > - arch_unmap(mm, start, end);
> > -
> > /* Find the first overlapping VMA */
> > vma = vma_find(vmi, end);
> > if (!vma) {
> > --
> > 2.46.0
> >
