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).
>
> >
> > > 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
