[PATCH 0/2] mm/mremap: Remove extra vma tree walk

[PATCH 0/2] mm/mremap: Remove extra vma tree walk

[Liam R. Howlett]

An extra vma tree walk was discovered in some mremap call paths during
the discussion on mseal() changes.  This patch set removes the extra vma
tree walk and further cleans up mremap_to().

Liam R. Howlett (2):
  mm/mremap: Clean up vma_to_resize()
  mm/mremap: Remove goto from mremap_to()

 mm/mremap.c | 67 +++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 38 deletions(-)

-- 
2.43.0

[PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Liam R. Howlett]

From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>

vma_to_resize() is used in two locations to find and validate the vma
for the mremap location.  One of the two locations already has the vma,
which is then re-found to validate the same vma.

This code can be simplified by moving the vma_lookup() from
vma_to_resize() to mremap_to() and changing the return type to an int
error.

Since the function now just validates the vma, the function is renamed
to mremap_vma_check() to better reflect what it is doing.

Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 mm/mremap.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 5917feafe8cc..648c29f568af 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -826,17 +826,12 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	return new_addr;
 }
 
-static struct vm_area_struct *vma_to_resize(unsigned long addr,
+static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
 	unsigned long old_len, unsigned long new_len, unsigned long flags)
 {
 	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma;
 	unsigned long pgoff;
 
-	vma = vma_lookup(mm, addr);
-	if (!vma)
-		return ERR_PTR(-EFAULT);
-
 	/*
 	 * !old_len is a special case where an attempt is made to 'duplicate'
 	 * a mapping.  This makes no sense for private mappings as it will
@@ -847,37 +842,37 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
 	 */
 	if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
 		pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 	}
 
 	if ((flags & MREMAP_DONTUNMAP) &&
 			(vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)))
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	/* We can't remap across vm area boundaries */
 	if (old_len > vma->vm_end - addr)
-		return ERR_PTR(-EFAULT);
+		return -EFAULT;
 
 	if (new_len == old_len)
-		return vma;
+		return 0;
 
 	/* Need to be careful about a growing mapping */
 	pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
 	pgoff += vma->vm_pgoff;
 	if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
-		return ERR_PTR(-EINVAL);
+		return -EINVAL;
 
 	if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
-		return ERR_PTR(-EFAULT);
+		return -EFAULT;
 
 	if (!mlock_future_ok(mm, vma->vm_flags, new_len - old_len))
-		return ERR_PTR(-EAGAIN);
+		return -EAGAIN;
 
 	if (!may_expand_vm(mm, vma->vm_flags,
 				(new_len - old_len) >> PAGE_SHIFT))
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
-	return vma;
+	return 0;
 }
 
 static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
@@ -936,7 +931,13 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 		old_len = new_len;
 	}
 
-	vma = vma_to_resize(addr, old_len, new_len, flags);
+	vma = vma_lookup(mm, addr);
+	if (!vma) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	mremap_vma_check(vma, addr, old_len, new_len, flags);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		goto out;
@@ -1114,11 +1115,9 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 	/*
 	 * Ok, we need to grow..
 	 */
-	vma = vma_to_resize(addr, old_len, new_len, flags);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
+	ret = mremap_vma_check(vma, addr, old_len, new_len, flags);
+	if (ret)
 		goto out;
-	}
 
 	/* old_len exactly to the end of the area..
 	 */
-- 
2.43.0

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Jeff Xu]

On Wed, Oct 16, 2024 at 1:17 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
>
> vma_to_resize() is used in two locations to find and validate the vma
> for the mremap location.  One of the two locations already has the vma,
> which is then re-found to validate the same vma.
>
> This code can be simplified by moving the vma_lookup() from
> vma_to_resize() to mremap_to() and changing the return type to an int
> error.
>
> Since the function now just validates the vma, the function is renamed
> to mremap_vma_check() to better reflect what it is doing.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> ---
>  mm/mremap.c | 39 +++++++++++++++++++--------------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 5917feafe8cc..648c29f568af 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -826,17 +826,12 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>         return new_addr;
>  }
>
> -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> +static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
The original function is vma_to_resize, and mremap_vma_check is
missing this context.
Maybe mremap_vma_check_resize is a better name ?


>         unsigned long old_len, unsigned long new_len, unsigned long flags)
>  {
>         struct mm_struct *mm = current->mm;
> -       struct vm_area_struct *vma;
>         unsigned long pgoff;
>
> -       vma = vma_lookup(mm, addr);
> -       if (!vma)
> -               return ERR_PTR(-EFAULT);
> -
>         /*
>          * !old_len is a special case where an attempt is made to 'duplicate'
>          * a mapping.  This makes no sense for private mappings as it will
> @@ -847,37 +842,37 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,
>          */
>         if (!old_len && !(vma->vm_flags & (VM_SHARED | VM_MAYSHARE))) {
>                 pr_warn_once("%s (%d): attempted to duplicate a private mapping with mremap.  This is not supported.\n", current->comm, current->pid);
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>         }
>
>         if ((flags & MREMAP_DONTUNMAP) &&
>                         (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP)))
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>
>         /* We can't remap across vm area boundaries */
>         if (old_len > vma->vm_end - addr)
> -               return ERR_PTR(-EFAULT);
> +               return -EFAULT;
>
>         if (new_len == old_len)
> -               return vma;
> +               return 0;
>
>         /* Need to be careful about a growing mapping */
>         pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
>         pgoff += vma->vm_pgoff;
>         if (pgoff + (new_len >> PAGE_SHIFT) < pgoff)
> -               return ERR_PTR(-EINVAL);
> +               return -EINVAL;
>
>         if (vma->vm_flags & (VM_DONTEXPAND | VM_PFNMAP))
> -               return ERR_PTR(-EFAULT);
> +               return -EFAULT;
>
>         if (!mlock_future_ok(mm, vma->vm_flags, new_len - old_len))
> -               return ERR_PTR(-EAGAIN);
> +               return -EAGAIN;
>
>         if (!may_expand_vm(mm, vma->vm_flags,
>                                 (new_len - old_len) >> PAGE_SHIFT))
> -               return ERR_PTR(-ENOMEM);
> +               return -ENOMEM;
>
> -       return vma;
> +       return 0;
>  }
>
>  static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> @@ -936,7 +931,13 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>                 old_len = new_len;
>         }
>
> -       vma = vma_to_resize(addr, old_len, new_len, flags);
> +       vma = vma_lookup(mm, addr);
> +       if (!vma) {
> +               ret = -EFAULT;
> +               goto out;
> +       }
> +
> +       mremap_vma_check(vma, addr, old_len, new_len, flags);
>         if (IS_ERR(vma)) {
>                 ret = PTR_ERR(vma);
>                 goto out;
> @@ -1114,11 +1115,9 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>         /*
>          * Ok, we need to grow..
>          */
> -       vma = vma_to_resize(addr, old_len, new_len, flags);
> -       if (IS_ERR(vma)) {
> -               ret = PTR_ERR(vma);
> +       ret = mremap_vma_check(vma, addr, old_len, new_len, flags);
> +       if (ret)
>                 goto out;
> -       }
>
>         /* old_len exactly to the end of the area..
>          */
> --
> 2.43.0
>

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Liam R. Howlett]

* Jeff Xu <jeffxu@chromium.org> [241017 13:55]:
> On Wed, Oct 16, 2024 at 1:17 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> >
> > vma_to_resize() is used in two locations to find and validate the vma
> > for the mremap location.  One of the two locations already has the vma,
> > which is then re-found to validate the same vma.
> >
> > This code can be simplified by moving the vma_lookup() from
> > vma_to_resize() to mremap_to() and changing the return type to an int
> > error.
> >
> > Since the function now just validates the vma, the function is renamed
> > to mremap_vma_check() to better reflect what it is doing.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > ---
> >  mm/mremap.c | 39 +++++++++++++++++++--------------------
> >  1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index 5917feafe8cc..648c29f568af 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -826,17 +826,12 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> >         return new_addr;
> >  }
> >
> > -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> > +static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
> The original function is vma_to_resize, and mremap_vma_check is
> missing this context.
> Maybe mremap_vma_check_resize is a better name ?

Good point.  That suggestion is long though.  Perhaps
vma_check_resize(), since this is a static function in the mremap.c
file, it is fine to drop mremap from the name.

...

Thanks,
Liam

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Jeff Xu]

On Thu, Oct 17, 2024 at 11:07 AM Liam R. Howlett
<Liam.Howlett@oracle.com> wrote:
>
> * Jeff Xu <jeffxu@chromium.org> [241017 13:55]:
> > On Wed, Oct 16, 2024 at 1:17 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> > >
> > > From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>
> > >
> > > vma_to_resize() is used in two locations to find and validate the vma
> > > for the mremap location.  One of the two locations already has the vma,
> > > which is then re-found to validate the same vma.
> > >
> > > This code can be simplified by moving the vma_lookup() from
> > > vma_to_resize() to mremap_to() and changing the return type to an int
> > > error.
> > >
> > > Since the function now just validates the vma, the function is renamed
> > > to mremap_vma_check() to better reflect what it is doing.
> > >
> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > > ---
> > >  mm/mremap.c | 39 +++++++++++++++++++--------------------
> > >  1 file changed, 19 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index 5917feafe8cc..648c29f568af 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -826,17 +826,12 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> > >         return new_addr;
> > >  }
> > >
> > > -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> > > +static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
> > The original function is vma_to_resize, and mremap_vma_check is
> > missing this context.
> > Maybe mremap_vma_check_resize is a better name ?
>
> Good point.  That suggestion is long though.  Perhaps
> vma_check_resize(), since this is a static function in the mremap.c
> file, it is fine to drop mremap from the name.
>
I'm ok with that.

> ...
>
> Thanks,
> Liam

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Andrew Morton]

On Thu, 17 Oct 2024 11:15:20 -0700 Jeff Xu <jeffxu@chromium.org> wrote:

> > > > -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> > > > +static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
> > > The original function is vma_to_resize, and mremap_vma_check is
> > > missing this context.
> > > Maybe mremap_vma_check_resize is a better name ?
> >
> > Good point.  That suggestion is long though.  Perhaps
> > vma_check_resize(), since this is a static function in the mremap.c
> > file, it is fine to drop mremap from the name.
> >
> I'm ok with that.

Any function with "check" in the name makes my teeth ache.  "check"
what?  And the name doesn't carry any connotation of the return value's
meaning.

eg, "check_the_cheese()" versus "cheese_is_fresh()".  The latter name
tells you what is being checked and it tells you what a "true" return
value means.

Also, the dang function isn't documented.

Also, why is mremap_to() calling mremap_vma_check() without testing its
return value?  mremap_vma_check() doesn't actually alter anything, does
it?  If it does then it's misnamed.

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Liam R. Howlett]

* Andrew Morton <akpm@linux-foundation.org> [241017 17:08]:
> On Thu, 17 Oct 2024 11:15:20 -0700 Jeff Xu <jeffxu@chromium.org> wrote:
> 
> > > > > -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> > > > > +static int mremap_vma_check(struct vm_area_struct *vma, unsigned long addr,
> > > > The original function is vma_to_resize, and mremap_vma_check is
> > > > missing this context.
> > > > Maybe mremap_vma_check_resize is a better name ?
> > >
> > > Good point.  That suggestion is long though.  Perhaps
> > > vma_check_resize(), since this is a static function in the mremap.c
> > > file, it is fine to drop mremap from the name.
> > >
> > I'm ok with that.
> 
> Any function with "check" in the name makes my teeth ache.  "check"
> what?  And the name doesn't carry any connotation of the return value's
> meaning.
> 
> eg, "check_the_cheese()" versus "cheese_is_fresh()".  The latter name
> tells you what is being checked and it tells you what a "true" return
> value means.

Fair enough, how about resize_is_valid()?

> 
> Also, the dang function isn't documented.

I'll add that, thanks.

> 
> Also, why is mremap_to() calling mremap_vma_check() without testing its
> return value?  mremap_vma_check() doesn't actually alter anything, does
> it?  If it does then it's misnamed.

That's an issue, I'll fix it.

Thanks,
Liam

Re: [PATCH 1/2] mm/mremap: Clean up vma_to_resize()

[Andrew Morton]

On Thu, 17 Oct 2024 19:53:10 -0400 "Liam R. Howlett" <Liam.Howlett@oracle.com> wrote:

> > eg, "check_the_cheese()" versus "cheese_is_fresh()".  The latter name
> > tells you what is being checked and it tells you what a "true" return
> > value means.
> 
> Fair enough, how about resize_is_valid()?

Sounds great.  Although I do prefer cheese_is_fresh().

[PATCH 2/2] mm/mremap: Remove goto from mremap_to()

[Liam R. Howlett]

From: "Liam R. Howlett" <Liam.Howlett@Oracle.com>

mremap_to() has a goto label at the end that doesn't unwind anything.
Removing the label makes the code cleaner.

Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 mm/mremap.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index 648c29f568af..5a1eb3da730d 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -883,18 +883,18 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 {
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
-	unsigned long ret = -EINVAL;
+	unsigned long ret;
 	unsigned long map_flags = 0;
 
 	if (offset_in_page(new_addr))
-		goto out;
+		return -EINVAL;
 
 	if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
-		goto out;
+		return -EINVAL;
 
 	/* Ensure the old/new locations do not overlap */
 	if (addr + old_len > new_addr && new_addr + new_len > addr)
-		goto out;
+		return -EINVAL;
 
 	/*
 	 * move_vma() need us to stay 4 maps below the threshold, otherwise
@@ -921,33 +921,28 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 		 */
 		ret = do_munmap(mm, new_addr, new_len, uf_unmap_early);
 		if (ret)
-			goto out;
+			return ret;
 	}
 
 	if (old_len > new_len) {
 		ret = do_munmap(mm, addr+new_len, old_len - new_len, uf_unmap);
 		if (ret)
-			goto out;
+			return ret;
 		old_len = new_len;
 	}
 
 	vma = vma_lookup(mm, addr);
-	if (!vma) {
-		ret = -EFAULT;
-		goto out;
-	}
+	if (!vma)
+		return -EFAULT;
 
-	mremap_vma_check(vma, addr, old_len, new_len, flags);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
-		goto out;
-	}
+	ret = mremap_vma_check(vma, addr, old_len, new_len, flags);
+	if (ret)
+		return ret;
 
 	/* MREMAP_DONTUNMAP expands by old_len since old_len == new_len */
 	if (flags & MREMAP_DONTUNMAP &&
 		!may_expand_vm(mm, vma->vm_flags, old_len >> PAGE_SHIFT)) {
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	if (flags & MREMAP_FIXED)
@@ -960,17 +955,14 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 				((addr - vma->vm_start) >> PAGE_SHIFT),
 				map_flags);
 	if (IS_ERR_VALUE(ret))
-		goto out;
+		return ret;
 
 	/* We got a new mapping */
 	if (!(flags & MREMAP_FIXED))
 		new_addr = ret;
 
-	ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
-		       uf_unmap);
-
-out:
-	return ret;
+	return move_vma(vma, addr, old_len, new_len, new_addr, locked, flags,
+			uf, uf_unmap);
 }
 
 static int vma_expandable(struct vm_area_struct *vma, unsigned long delta)
-- 
2.43.0