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

[PATCH v2 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().

v1: https://lore.kernel.org/all/20241016201719.2449143-1-Liam.Howlett@oracle.com/

Changes since v1:
 Change mremap_vma_check() to resize_is_valid() - Thanks Jeff & Andrew
 Fixed missing return check - Thanks Andrew
 Added comments to both functions that are touched - Thanks Andrew

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

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

-- 
2.43.0

[PATCH v2 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 resize_is_valid() to better reflect what it is doing.

This commit also adds documentation about the function.

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

diff --git a/mm/mremap.c b/mm/mremap.c
index 5917feafe8cc..e781ec4573ca 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -826,17 +826,24 @@ static unsigned long move_vma(struct vm_area_struct *vma,
 	return new_addr;
 }
 
-static struct vm_area_struct *vma_to_resize(unsigned long addr,
+/*
+ * resize_is_valid() - Ensure the vma can be resized to the new length at the give
+ * address.
+ *
+ * @vma: The vma to resize
+ * @addr: The old address
+ * @old_len: The current size
+ * @new_len: The desired size
+ * @flags: The vma flags
+ *
+ * Return 0 on success, error otherwise.
+ */
+static int resize_is_valid(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 +854,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,12 +943,16 @@ 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);
-	if (IS_ERR(vma)) {
-		ret = PTR_ERR(vma);
+	vma = vma_lookup(mm, addr);
+	if (!vma) {
+		ret = -EFAULT;
 		goto out;
 	}
 
+	ret = resize_is_valid(vma, addr, old_len, new_len, flags);
+	if (ret)
+		goto out;
+
 	/* 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)) {
@@ -1114,11 +1125,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 = resize_is_valid(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 v2 1/2] mm/mremap: Clean up vma_to_resize()

[Lorenzo Stoakes]

On Fri, Oct 18, 2024 at 01:41:13PM -0400, Liam R. Howlett 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 resize_is_valid() to better reflect what it is doing.
>
> This commit also adds documentation about the function.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>

git-icdiff helped here a lot :)

This LGTM,

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Generally a really nice clean up in any case, thanks for doing this!

> ---
>  mm/mremap.c | 53 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 5917feafe8cc..e781ec4573ca 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -826,17 +826,24 @@ static unsigned long move_vma(struct vm_area_struct *vma,
>  	return new_addr;
>  }
>
> -static struct vm_area_struct *vma_to_resize(unsigned long addr,
> +/*
> + * resize_is_valid() - Ensure the vma can be resized to the new length at the give
> + * address.
> + *
> + * @vma: The vma to resize
> + * @addr: The old address
> + * @old_len: The current size
> + * @new_len: The desired size
> + * @flags: The vma flags
> + *
> + * Return 0 on success, error otherwise.
> + */
> +static int resize_is_valid(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 +854,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,12 +943,16 @@ 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);
> -	if (IS_ERR(vma)) {
> -		ret = PTR_ERR(vma);
> +	vma = vma_lookup(mm, addr);
> +	if (!vma) {
> +		ret = -EFAULT;
>  		goto out;
>  	}
>
> +	ret = resize_is_valid(vma, addr, old_len, new_len, flags);
> +	if (ret)
> +		goto out;
> +
>  	/* 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)) {
> @@ -1114,11 +1125,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 = resize_is_valid(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 v2 1/2] mm/mremap: Clean up vma_to_resize()

[Pedro Falcato]

On Fri, Oct 18, 2024 at 6:41 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 resize_is_valid() to better reflect what it is doing.

Small nit: Could we pick a stable naming scheme?
I understand the kernel has historically had plenty of ways to name
functions, including
do_stuff
is_stuff
stuff_do
stuff_is

I thought we were starting to converge into vma_/vmi_/vms_ :(
I would personally prefer vma_resize_is_valid/vma_resize_valid (even
if it's a static function, so it doesn't matter _too_ much).
Anyway, enough bikeshedding...

>
> This commit also adds documentation about the function.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

This patch made me realize there's a couple of small improvements we
can still do (maybe with a vmi) to clean up and speed up mremap (at
least!). I'll look into those if I find some time.

-- 
Pedro

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

[Liam R. Howlett]

* Pedro Falcato <pedro.falcato@gmail.com> [241022 18:08]:
> On Fri, Oct 18, 2024 at 6:41 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 resize_is_valid() to better reflect what it is doing.
> 
> Small nit: Could we pick a stable naming scheme?
> I understand the kernel has historically had plenty of ways to name
> functions, including
> do_stuff
> is_stuff
> stuff_do
> stuff_is
> 
> I thought we were starting to converge into vma_/vmi_/vms_ :(
> I would personally prefer vma_resize_is_valid/vma_resize_valid (even
> if it's a static function, so it doesn't matter _too_ much).
> Anyway, enough bikeshedding...

Right.  Usually the vma_/vmi_/vms_ stuff indicates the first argument to
the functions exposed to other files.  Yes, vma_ would work here, and I
don't really mind that it's static and has the vma_ name.

I'd suggest relocating it to vma.c or vma.h, but then we'd have to put
mremap in the name and that would be rather long - I don't think we do
anything like this elsewhere.

> 
> >
> > This commit also adds documentation about the function.
> >
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> 
> Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

Thanks!

> 
> This patch made me realize there's a couple of small improvements we
> can still do (maybe with a vmi) to clean up and speed up mremap (at
> least!). I'll look into those if I find some time.

Agreed.  If you do get to doing more clean up, I'd be fine with a rename
of this function.

Regards,
Liam

[PATCH v2 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.

This commit also adds documentation on the function.

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

diff --git a/mm/mremap.c b/mm/mremap.c
index e781ec4573ca..4c79ab92eb8f 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -887,6 +887,20 @@ static int resize_is_valid(struct vm_area_struct *vma, unsigned long addr,
 	return 0;
 }
 
+/*
+ * mremap_to() - remap a vma to a new location
+ * @addr: The old address
+ * @old_len: The old size
+ * @new_addr: The target address
+ * @new_len: The new size
+ * @locked: If the returned vma is locked (VM_LOCKED)
+ * @flags: the mremap flags
+ * @uf: The mremap userfaultfd context
+ * @uf_unmap_early: The userfaultfd unmap early context
+ * @uf_unmap: The userfaultfd unmap context
+ *
+ * Returns: The new address of the vma or an error.
+ */
 static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
 		unsigned long new_addr, unsigned long new_len, bool *locked,
 		unsigned long flags, struct vm_userfaultfd_ctx *uf,
@@ -895,18 +909,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
@@ -933,31 +947,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;
 
 	ret = resize_is_valid(vma, addr, old_len, new_len, flags);
 	if (ret)
-		goto out;
+		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)
@@ -970,17 +981,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

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

[Lorenzo Stoakes]

On Fri, Oct 18, 2024 at 01:41:14PM -0400, Liam R. Howlett wrote:
> 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.
>
> This commit also adds documentation on the function.
>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>

This is a nice cleanup, thanks!

Other than nits below, LGTM, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  mm/mremap.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index e781ec4573ca..4c79ab92eb8f 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -887,6 +887,20 @@ static int resize_is_valid(struct vm_area_struct *vma, unsigned long addr,
>  	return 0;
>  }
>
> +/*
> + * mremap_to() - remap a vma to a new location
> + * @addr: The old address
> + * @old_len: The old size
> + * @new_addr: The target address
> + * @new_len: The new size
> + * @locked: If the returned vma is locked (VM_LOCKED)
> + * @flags: the mremap flags
> + * @uf: The mremap userfaultfd context
> + * @uf_unmap_early: The userfaultfd unmap early context
> + * @uf_unmap: The userfaultfd unmap context

Nit - these are lists of userfaultfd_unmap_ctx rather than contexts. But I
mean, the naming around this is generally not great and not your fault so
not a big deal.

> + *
> + * Returns: The new address of the vma or an error.
> + */
>  static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>  		unsigned long new_addr, unsigned long new_len, bool *locked,
>  		unsigned long flags, struct vm_userfaultfd_ctx *uf,
> @@ -895,18 +909,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
> @@ -933,31 +947,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;
>
>  	ret = resize_is_valid(vma, addr, old_len, new_len, flags);
>  	if (ret)
> -		goto out;
> +		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)
> @@ -970,17 +981,14 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
>  				((addr - vma->vm_start) >> PAGE_SHIFT),
>  				map_flags);

Unrelated to your change, but I'm really confused as to why we're doing
this even if MREMAP_FIXED might be set, then ignore its return value if it
is set?

Would be way nicer to do something like:

	if (!(flags & MREMAP_FIXED)) {
		new_addr = get_unmapped_area(vma->vm_file, new_addr, new_len,
					vma->vm_pgoff +
					((addr - vma->vm_start) >> PAGE_SHIFT),
					map_flags);
		if (IS_ERR_VALUE(new_addr))
			return new_addr;
	}

Anyway one for another series.

I'd say the 'ret' variable is badly named now, but it was badly named
before so there's zero delta on that.

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

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

[Pedro Falcato]

On Fri, Oct 18, 2024 at 6:41 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> 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.
>
> This commit also adds documentation on the function.

Thanks for the nice cleanup across this series :)

>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>

Reviewed-by: Pedro Falcato <pedro.falcato@gmail.com>

-- 
Pedro