* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [240722 10:25]:
> On Wed, Jul 17, 2024 at 04:07:05PM GMT, Liam R. Howlett wrote:
> > From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> >
> > The arch_unmap call was previously moved above the rbtree modifications
> > in commit 5a28fc94c914 ("x86/mpx, mm/core: Fix recursive munmap()
> > corruption"). The move was motivated by an issue with calling
> > arch_unmap() after the rbtree was modified.
> >
> > Since the above commit, mpx was dropped from the kernel in 45fc24e89b7c
> > ("x86/mpx: remove MPX from arch/x86"), so the motivation for calling
> > arch_unmap() prior to modifying the vma tree no longer exists
> > (regardless of rbtree or maple tree implementations).
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > Cc: LEROY Christophe <christophe.leroy2@xxxxxxxxxxxxxxxxxx>
> > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> > ---
> > mm/mmap.c | 12 ++----------
> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 9f870e715a47..117e8240f697 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2680,6 +2680,7 @@ static void vms_complete_munmap_vmas(struct vma_munmap_struct *vms,
> > mm = vms->mm;
> > mm->map_count -= vms->vma_count;
> > mm->locked_vm -= vms->locked_vm;
> > + arch_unmap(mm, vms->start, vms->end); /* write lock needed */
>
> Worth having a mmap_assert_write_locked() here? Would make this
> self-documenting also.
No, this is just to point out it cannot be lowered further in this
function.
>
> > if (vms->unlock)
> > mmap_write_downgrade(mm);
> >
> > @@ -2907,7 +2908,7 @@ do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > *
> > * This function takes a @mas that is either pointing to the previous VMA or set
> > * to MA_START and sets it up to remove the mapping(s). The @len will be
> > - * aligned and any arch_unmap work will be preformed.
> > + * aligned prior to munmap.
> > *
> > * Return: 0 on success and drops the lock if so directed, error and leaves the
> > * lock held otherwise.
> > @@ -2927,16 +2928,12 @@ int do_vmi_munmap(struct vma_iterator *vmi, struct mm_struct *mm,
> > 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) {
> > @@ -2997,9 +2994,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > if (unlikely(!can_modify_mm(mm, addr, end)))
> > return -EPERM;
> >
> > - /* arch_unmap() might do unmaps itself. */
> > - arch_unmap(mm, addr, end);
> > -
>
> It seems to me that the intent of this particular invocation was to ensure
> we have done what we can to unmap before trying to unmap ourselves.
>
> However this seems stupid to me anyway - 'hey maybe the arch will do this
> for us' - yeah probably not.
>
> So this should definitely go regardless, given we will invoke it later now
> anyway.
This was covered in the commit message, it was because we needed to
remove the VMAs earlier for a dead feature (mpx).
>
> > /* Find the first overlapping VMA */
> > vma = vma_find(&vmi, end);
> > init_vma_munmap(&vms, &vmi, vma, addr, end, uf, /* unlock = */ false);
> > @@ -3377,14 +3371,12 @@ int do_vma_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > 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);
> > }
> >
> > --
> > 2.43.0
> >
>
> I hope we can find a way to eliminate these kind of hooks altogether as
> they reduce our control over how VMA operations are performed.
Agreed. I see a path forward on doing just that.
>
> LGTM,
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
