* Pedro Falcato <pedro.falcato@xxxxxxxxx> [240809 14:53]:
> On Fri, Aug 9, 2024 at 5:48 PM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> >
> > * 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.
>
> 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?
>
> >
> > >
> > > > 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.
