[PATCH v3 0/3] Fixes for vma_merge() error path

[PATCH v3 0/3] Fixes for vma_merge() error path

[Liam R. Howlett]

Jann Horn reported a potential vma iterator issue in the failure path of
the vma_merge() code.  After examining the interface, it seemed the best
course of action is to simply add an undo path in the unlikely case of
an error.

On examining the vma iterator issue, another issue was discovered that
would increase the memory usage during failure scenarios, so this is
addressed in patch 2.

Since it is unclear in the code, another patch adds comments to the
vma_merge() function on why dup_anon_vma() is safe in 'case 6'.

Changes since v2:
 - Updated dup_anon_vma() comment to include new argument.
 - Change vma iterator undo to a reset and rewalk to ensure
   vma_iter_config() changes are dropped. - Thanks Vlastimil Babka
 - Update user visible changes in patch 1 again. - Thanks Vlastimil
   Babka
 - Only set the anon_dup vma when the duplication is successful. -
   Thanks Vlastimil Babka

v2: https://lore.kernel.org/linux-mm/20230927160746.1928098-1-Liam.Howlett@oracle.com/
v1: https://lore.kernel.org/linux-mm/20230927160746.1928098-1-Liam.Howlett@oracle.com/

Liam R. Howlett (3):
  mmap: Fix vma_iterator in error path of vma_merge()
  mmap: Fix error paths with dup_anon_vma()
  mmap: Add clarifying comment to vma_merge() code

 mm/mmap.c | 45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)

-- 
2.40.1

[PATCH v3 1/3] mmap: Fix vma_iterator in error path of vma_merge()

[Liam R. Howlett]

During the error path, the vma iterator may not be correctly positioned
or set to the correct range.  Undo the vma_prev() call by resetting to
the passed in address.  Re-walking to the same range will fix the range
to the area previously passed in.

Users would notice increased cycles as vma_merge() would be called an
extra time with vma == prev, and thus would fail to merge and return.

Link: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
Closes: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
Fixes: 18b098af2890 ("vma_merge: set vma iterator to correct position.")
Cc: stable@vger.kernel.org
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index b56a7f0c9f85..acb7dea49e23 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -975,7 +975,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 
 	/* Error in anon_vma clone. */
 	if (err)
-		return NULL;
+		goto anon_vma_fail;
 
 	if (vma_start < vma->vm_start || vma_end > vma->vm_end)
 		vma_expanded = true;
@@ -988,7 +988,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 	}
 
 	if (vma_iter_prealloc(vmi, vma))
-		return NULL;
+		goto prealloc_fail;
 
 	init_multi_vma_prep(&vp, vma, adjust, remove, remove2);
 	VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma &&
@@ -1016,6 +1016,12 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 	vma_complete(&vp, vmi, mm);
 	khugepaged_enter_vma(res, vm_flags);
 	return res;
+
+prealloc_fail:
+anon_vma_fail:
+	vma_iter_set(vmi, addr);
+	vma_iter_load(vmi);
+	return NULL;
 }
 
 /*
-- 
2.40.1

[PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Liam R. Howlett]

When the calling function fails after the dup_anon_vma(), the
duplication of the anon_vma is not being undone.  Add the necessary
unlink_anon_vma() call to the error paths that are missing them.

This issue showed up during inspection of the error path in vma_merge()
for an unrelated vma iterator issue.

Users may experience increased memory usage, which may be problematic as
the failure would likely be caused by a low memory situation.

Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
Cc: stable@vger.kernel.org
Cc: Jann Horn <jannh@google.com>
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index acb7dea49e23..f9f0a5fe4db4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
  * dup_anon_vma() - Helper function to duplicate anon_vma
  * @dst: The destination VMA
  * @src: The source VMA
+ * @dup: Pointer to the destination VMA when successful.
  *
  * Returns: 0 on success.
  */
 static inline int dup_anon_vma(struct vm_area_struct *dst,
-			       struct vm_area_struct *src)
+		struct vm_area_struct *src, struct vm_area_struct **dup)
 {
 	/*
 	 * Easily overlooked: when mprotect shifts the boundary, make sure the
@@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_struct *dst,
 	 * anon pages imported.
 	 */
 	if (src->anon_vma && !dst->anon_vma) {
+		int ret;
+
 		vma_assert_write_locked(dst);
 		dst->anon_vma = src->anon_vma;
-		return anon_vma_clone(dst, src);
+		ret = anon_vma_clone(dst, src);
+		if (ret)
+			return ret;
+
+		*dup = dst;
 	}
 
 	return 0;
@@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	       unsigned long start, unsigned long end, pgoff_t pgoff,
 	       struct vm_area_struct *next)
 {
+	struct vm_area_struct *anon_dup = NULL;
 	bool remove_next = false;
 	struct vma_prepare vp;
 
@@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
 
 		remove_next = true;
 		vma_start_write(next);
-		ret = dup_anon_vma(vma, next);
+		ret = dup_anon_vma(vma, next, &anon_dup);
 		if (ret)
 			return ret;
 	}
@@ -661,6 +669,8 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	return 0;
 
 nomem:
+	if (anon_dup)
+		unlink_anon_vmas(anon_dup);
 	return -ENOMEM;
 }
 
@@ -860,6 +870,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 {
 	struct vm_area_struct *curr, *next, *res;
 	struct vm_area_struct *vma, *adjust, *remove, *remove2;
+	struct vm_area_struct *anon_dup = NULL;
 	struct vma_prepare vp;
 	pgoff_t vma_pgoff;
 	int err = 0;
@@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 		vma_start_write(next);
 		remove = next;				/* case 1 */
 		vma_end = next->vm_end;
-		err = dup_anon_vma(prev, next);
+		err = dup_anon_vma(prev, next, &anon_dup);
 		if (curr) {				/* case 6 */
 			vma_start_write(curr);
 			remove = curr;
 			remove2 = next;
 			if (!next->anon_vma)
-				err = dup_anon_vma(prev, curr);
+				err = dup_anon_vma(prev, curr, &anon_dup);
 		}
 	} else if (merge_prev) {			/* case 2 */
 		if (curr) {
 			vma_start_write(curr);
-			err = dup_anon_vma(prev, curr);
+			err = dup_anon_vma(prev, curr, &anon_dup);
 			if (end == curr->vm_end) {	/* case 7 */
 				remove = curr;
 			} else {			/* case 5 */
@@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 			vma_end = addr;
 			adjust = next;
 			adj_start = -(prev->vm_end - addr);
-			err = dup_anon_vma(next, prev);
+			err = dup_anon_vma(next, prev, &anon_dup);
 		} else {
 			/*
 			 * Note that cases 3 and 8 are the ONLY ones where prev
@@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 				vma_pgoff = curr->vm_pgoff;
 				vma_start_write(curr);
 				remove = curr;
-				err = dup_anon_vma(next, curr);
+				err = dup_anon_vma(next, curr, &anon_dup);
 			}
 		}
 	}
@@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 	return res;
 
 prealloc_fail:
+	if (anon_dup)
+		unlink_anon_vmas(anon_dup);
+
 anon_vma_fail:
 	vma_iter_set(vmi, addr);
 	vma_iter_load(vmi);
-- 
2.40.1

[PATCH v3 3/3] mmap: Add clarifying comment to vma_merge() code

[Liam R. Howlett]

When tracing through the code in vma_merge(), it was not completely
clear why the error return to a dup_anon_vma() call would not overwrite
a previous attempt to the same function.  This commit adds a comment
specifying why it is safe.

Suggested-by: Jann Horn <jannh@google.com>
Link: https://lore.kernel.org/linux-mm/CAG48ez3iDwFPR=Ed1BfrNuyUJPMK_=StjxhUsCkL6po1s7bONg@mail.gmail.com/
Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/mmap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/mmap.c b/mm/mmap.c
index f9f0a5fe4db4..9967acbd070f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -943,6 +943,11 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
 			vma_start_write(curr);
 			remove = curr;
 			remove2 = next;
+			/*
+			 * Note that the dup_anon_vma below cannot overwrite err
+			 * since the first caller would do nothing unless next
+			 * has an anon_vma.
+			 */
 			if (!next->anon_vma)
 				err = dup_anon_vma(prev, curr, &anon_dup);
 		}
-- 
2.40.1

Re: [PATCH v3 1/3] mmap: Fix vma_iterator in error path of vma_merge()

[Lorenzo Stoakes]

On Fri, Sep 29, 2023 at 02:30:39PM -0400, Liam R. Howlett wrote:
> During the error path, the vma iterator may not be correctly positioned
> or set to the correct range.  Undo the vma_prev() call by resetting to
> the passed in address.  Re-walking to the same range will fix the range
> to the area previously passed in.
>
> Users would notice increased cycles as vma_merge() would be called an
> extra time with vma == prev, and thus would fail to merge and return.
>
> Link: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
> Closes: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
> Fixes: 18b098af2890 ("vma_merge: set vma iterator to correct position.")
> Cc: stable@vger.kernel.org
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>  mm/mmap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index b56a7f0c9f85..acb7dea49e23 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -975,7 +975,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>
>  	/* Error in anon_vma clone. */
>  	if (err)
> -		return NULL;
> +		goto anon_vma_fail;
>
>  	if (vma_start < vma->vm_start || vma_end > vma->vm_end)
>  		vma_expanded = true;
> @@ -988,7 +988,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	}
>
>  	if (vma_iter_prealloc(vmi, vma))
> -		return NULL;
> +		goto prealloc_fail;
>
>  	init_multi_vma_prep(&vp, vma, adjust, remove, remove2);
>  	VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma &&
> @@ -1016,6 +1016,12 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	vma_complete(&vp, vmi, mm);
>  	khugepaged_enter_vma(res, vm_flags);
>  	return res;
> +
> +prealloc_fail:
> +anon_vma_fail:
> +	vma_iter_set(vmi, addr);
> +	vma_iter_load(vmi);
> +	return NULL;
>  }
>
>  /*
> --
> 2.40.1
>
Looks good to me,

Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Lorenzo Stoakes]

On Fri, Sep 29, 2023 at 02:30:40PM -0400, Liam R. Howlett wrote:
> When the calling function fails after the dup_anon_vma(), the
> duplication of the anon_vma is not being undone.  Add the necessary
> unlink_anon_vma() call to the error paths that are missing them.
>
> This issue showed up during inspection of the error path in vma_merge()
> for an unrelated vma iterator issue.
>
> Users may experience increased memory usage, which may be problematic as
> the failure would likely be caused by a low memory situation.
>
> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> Cc: stable@vger.kernel.org
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>  mm/mmap.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index acb7dea49e23..f9f0a5fe4db4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
>   * dup_anon_vma() - Helper function to duplicate anon_vma
>   * @dst: The destination VMA
>   * @src: The source VMA
> + * @dup: Pointer to the destination VMA when successful.
>   *
>   * Returns: 0 on success.

Being a bit nitpicky/refactory here, but anon_vma_clone() appears to have
two possible return values - 0 for success, and -ENOMEM.

As a result, it's not really gaining us much passing through this value.

It'd be nice if dup_anon_vma() and anon_vma_clone() were therefore updated
to instead return NULL on ENOMEM and the dst otherwise.

Then we could de-clunk this whole code path, and the quite natural fact of
'thing didn't return a pointer therefore had no memory to allocate it' fals
out.

But this isn't exactly an earth-shattering concern :)

>   */
>  static inline int dup_anon_vma(struct vm_area_struct *dst,
> -			       struct vm_area_struct *src)
> +		struct vm_area_struct *src, struct vm_area_struct **dup)
>  {
>  	/*
>  	 * Easily overlooked: when mprotect shifts the boundary, make sure the
> @@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_struct *dst,
>  	 * anon pages imported.
>  	 */
>  	if (src->anon_vma && !dst->anon_vma) {
> +		int ret;
> +
>  		vma_assert_write_locked(dst);
>  		dst->anon_vma = src->anon_vma;
> -		return anon_vma_clone(dst, src);
> +		ret = anon_vma_clone(dst, src);
> +		if (ret)
> +			return ret;
> +
> +		*dup = dst;
>  	}
>
>  	return 0;
> @@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	       unsigned long start, unsigned long end, pgoff_t pgoff,
>  	       struct vm_area_struct *next)
>  {
> +	struct vm_area_struct *anon_dup = NULL;
>  	bool remove_next = false;
>  	struct vma_prepare vp;
>
> @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>
>  		remove_next = true;
>  		vma_start_write(next);
> -		ret = dup_anon_vma(vma, next);
> +		ret = dup_anon_vma(vma, next, &anon_dup);
>  		if (ret)
>  			return ret;
>  	}
> @@ -661,6 +669,8 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	return 0;
>
>  nomem:
> +	if (anon_dup)
> +		unlink_anon_vmas(anon_dup);
>  	return -ENOMEM;
>  }
>
> @@ -860,6 +870,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  {
>  	struct vm_area_struct *curr, *next, *res;
>  	struct vm_area_struct *vma, *adjust, *remove, *remove2;
> +	struct vm_area_struct *anon_dup = NULL;
>  	struct vma_prepare vp;
>  	pgoff_t vma_pgoff;
>  	int err = 0;
> @@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  		vma_start_write(next);
>  		remove = next;				/* case 1 */
>  		vma_end = next->vm_end;
> -		err = dup_anon_vma(prev, next);
> +		err = dup_anon_vma(prev, next, &anon_dup);
>  		if (curr) {				/* case 6 */
>  			vma_start_write(curr);
>  			remove = curr;
>  			remove2 = next;
>  			if (!next->anon_vma)
> -				err = dup_anon_vma(prev, curr);
> +				err = dup_anon_vma(prev, curr, &anon_dup);
>  		}
>  	} else if (merge_prev) {			/* case 2 */
>  		if (curr) {
>  			vma_start_write(curr);
> -			err = dup_anon_vma(prev, curr);
> +			err = dup_anon_vma(prev, curr, &anon_dup);
>  			if (end == curr->vm_end) {	/* case 7 */
>  				remove = curr;
>  			} else {			/* case 5 */
> @@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  			vma_end = addr;
>  			adjust = next;
>  			adj_start = -(prev->vm_end - addr);
> -			err = dup_anon_vma(next, prev);
> +			err = dup_anon_vma(next, prev, &anon_dup);
>  		} else {
>  			/*
>  			 * Note that cases 3 and 8 are the ONLY ones where prev
> @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  				vma_pgoff = curr->vm_pgoff;
>  				vma_start_write(curr);
>  				remove = curr;
> -				err = dup_anon_vma(next, curr);
> +				err = dup_anon_vma(next, curr, &anon_dup);
>  			}
>  		}
>  	}
> @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	return res;
>
>  prealloc_fail:
> +	if (anon_dup)
> +		unlink_anon_vmas(anon_dup);
> +
>  anon_vma_fail:
>  	vma_iter_set(vmi, addr);
>  	vma_iter_load(vmi);
> --
> 2.40.1
>

Other than the nice-to-have, this looks good to me:

Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>

Re: [PATCH v3 3/3] mmap: Add clarifying comment to vma_merge() code

[Lorenzo Stoakes]

On Fri, Sep 29, 2023 at 02:30:41PM -0400, Liam R. Howlett wrote:
> When tracing through the code in vma_merge(), it was not completely
> clear why the error return to a dup_anon_vma() call would not overwrite
> a previous attempt to the same function.  This commit adds a comment
> specifying why it is safe.
>
> Suggested-by: Jann Horn <jannh@google.com>
> Link: https://lore.kernel.org/linux-mm/CAG48ez3iDwFPR=Ed1BfrNuyUJPMK_=StjxhUsCkL6po1s7bONg@mail.gmail.com/
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>  mm/mmap.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f9f0a5fe4db4..9967acbd070f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -943,6 +943,11 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  			vma_start_write(curr);
>  			remove = curr;
>  			remove2 = next;
> +			/*
> +			 * Note that the dup_anon_vma below cannot overwrite err
> +			 * since the first caller would do nothing unless next
> +			 * has an anon_vma.
> +			 */
>  			if (!next->anon_vma)
>  				err = dup_anon_vma(prev, curr, &anon_dup);
>  		}
> --
> 2.40.1
>

Nice comment! It causes me to sick up a bit in my mouth that this is a thing,
but it's good to have it documented.

Reviewed-by: Lorenzo Stoakes <lstoakes@gmail.com>

Re: [PATCH v3 1/3] mmap: Fix vma_iterator in error path of vma_merge()

[Vlastimil Babka]

On 9/29/23 20:30, Liam R. Howlett wrote:
> During the error path, the vma iterator may not be correctly positioned
> or set to the correct range.  Undo the vma_prev() call by resetting to
> the passed in address.  Re-walking to the same range will fix the range
> to the area previously passed in.
> 
> Users would notice increased cycles as vma_merge() would be called an
> extra time with vma == prev, and thus would fail to merge and return.
> 
> Link: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
> Closes: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejtiHpgJAbdQ@mail.gmail.com/
> Fixes: 18b098af2890 ("vma_merge: set vma iterator to correct position.")
> Cc: stable@vger.kernel.org
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/mmap.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index b56a7f0c9f85..acb7dea49e23 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -975,7 +975,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  
>  	/* Error in anon_vma clone. */
>  	if (err)
> -		return NULL;
> +		goto anon_vma_fail;
>  
>  	if (vma_start < vma->vm_start || vma_end > vma->vm_end)
>  		vma_expanded = true;
> @@ -988,7 +988,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	}
>  
>  	if (vma_iter_prealloc(vmi, vma))
> -		return NULL;
> +		goto prealloc_fail;
>  
>  	init_multi_vma_prep(&vp, vma, adjust, remove, remove2);
>  	VM_WARN_ON(vp.anon_vma && adjust && adjust->anon_vma &&
> @@ -1016,6 +1016,12 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  	vma_complete(&vp, vmi, mm);
>  	khugepaged_enter_vma(res, vm_flags);
>  	return res;
> +
> +prealloc_fail:
> +anon_vma_fail:
> +	vma_iter_set(vmi, addr);
> +	vma_iter_load(vmi);
> +	return NULL;
>  }
>  
>  /*

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Vlastimil Babka]

On 9/29/23 20:30, Liam R. Howlett wrote:
> When the calling function fails after the dup_anon_vma(), the
> duplication of the anon_vma is not being undone.  Add the necessary
> unlink_anon_vma() call to the error paths that are missing them.
> 
> This issue showed up during inspection of the error path in vma_merge()
> for an unrelated vma iterator issue.
> 
> Users may experience increased memory usage, which may be problematic as
> the failure would likely be caused by a low memory situation.
> 
> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> Cc: stable@vger.kernel.org
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Re: [PATCH v3 3/3] mmap: Add clarifying comment to vma_merge() code

[Vlastimil Babka]

On 9/29/23 20:30, Liam R. Howlett wrote:
> When tracing through the code in vma_merge(), it was not completely
> clear why the error return to a dup_anon_vma() call would not overwrite
> a previous attempt to the same function.  This commit adds a comment
> specifying why it is safe.
> 
> Suggested-by: Jann Horn <jannh@google.com>
> Link: https://lore.kernel.org/linux-mm/CAG48ez3iDwFPR=Ed1BfrNuyUJPMK_=StjxhUsCkL6po1s7bONg@mail.gmail.com/
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/mmap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f9f0a5fe4db4..9967acbd070f 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -943,6 +943,11 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  			vma_start_write(curr);
>  			remove = curr;
>  			remove2 = next;
> +			/*
> +			 * Note that the dup_anon_vma below cannot overwrite err
> +			 * since the first caller would do nothing unless next
> +			 * has an anon_vma.
> +			 */
>  			if (!next->anon_vma)
>  				err = dup_anon_vma(prev, curr, &anon_dup);
>  		}

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Vlastimil Babka]

On 9/30/23 00:28, Lorenzo Stoakes wrote:
> On Fri, Sep 29, 2023 at 02:30:40PM -0400, Liam R. Howlett wrote:
>> When the calling function fails after the dup_anon_vma(), the
>> duplication of the anon_vma is not being undone.  Add the necessary
>> unlink_anon_vma() call to the error paths that are missing them.
>>
>> This issue showed up during inspection of the error path in vma_merge()
>> for an unrelated vma iterator issue.
>>
>> Users may experience increased memory usage, which may be problematic as
>> the failure would likely be caused by a low memory situation.
>>
>> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
>> Cc: stable@vger.kernel.org
>> Cc: Jann Horn <jannh@google.com>
>> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
>> ---
>>  mm/mmap.c | 30 ++++++++++++++++++++++--------
>>  1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index acb7dea49e23..f9f0a5fe4db4 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
>>   * dup_anon_vma() - Helper function to duplicate anon_vma
>>   * @dst: The destination VMA
>>   * @src: The source VMA
>> + * @dup: Pointer to the destination VMA when successful.
>>   *
>>   * Returns: 0 on success.
> 
> Being a bit nitpicky/refactory here, but anon_vma_clone() appears to have
> two possible return values - 0 for success, and -ENOMEM.
> 
> As a result, it's not really gaining us much passing through this value.
> 
> It'd be nice if dup_anon_vma() and anon_vma_clone() were therefore updated
> to instead return NULL on ENOMEM and the dst otherwise.

But we also need to represent that dup_anon_vma() had nothing to do, because
"(src->anon_vma && !dst->anon_vma)" was false, and in that case we should
not be returning dst from there?

So maybe we could return NULL for that case and ERR_PTR(ret) for the -ENOMEM
from anon_vma_clone() ?

> Then we could de-clunk this whole code path, and the quite natural fact of
> 'thing didn't return a pointer therefore had no memory to allocate it' fals
> out.
> 
> But this isn't exactly an earth-shattering concern :)
> 

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Lorenzo Stoakes]

On Mon, Oct 02, 2023 at 09:26:03AM +0200, Vlastimil Babka wrote:
> On 9/30/23 00:28, Lorenzo Stoakes wrote:
> > On Fri, Sep 29, 2023 at 02:30:40PM -0400, Liam R. Howlett wrote:
> >> When the calling function fails after the dup_anon_vma(), the
> >> duplication of the anon_vma is not being undone.  Add the necessary
> >> unlink_anon_vma() call to the error paths that are missing them.
> >>
> >> This issue showed up during inspection of the error path in vma_merge()
> >> for an unrelated vma iterator issue.
> >>
> >> Users may experience increased memory usage, which may be problematic as
> >> the failure would likely be caused by a low memory situation.
> >>
> >> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> >> Cc: stable@vger.kernel.org
> >> Cc: Jann Horn <jannh@google.com>
> >> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> >> ---
> >>  mm/mmap.c | 30 ++++++++++++++++++++++--------
> >>  1 file changed, 22 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/mm/mmap.c b/mm/mmap.c
> >> index acb7dea49e23..f9f0a5fe4db4 100644
> >> --- a/mm/mmap.c
> >> +++ b/mm/mmap.c
> >> @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
> >>   * dup_anon_vma() - Helper function to duplicate anon_vma
> >>   * @dst: The destination VMA
> >>   * @src: The source VMA
> >> + * @dup: Pointer to the destination VMA when successful.
> >>   *
> >>   * Returns: 0 on success.
> >
> > Being a bit nitpicky/refactory here, but anon_vma_clone() appears to have
> > two possible return values - 0 for success, and -ENOMEM.
> >
> > As a result, it's not really gaining us much passing through this value.
> >
> > It'd be nice if dup_anon_vma() and anon_vma_clone() were therefore updated
> > to instead return NULL on ENOMEM and the dst otherwise.
>
> But we also need to represent that dup_anon_vma() had nothing to do, because
> "(src->anon_vma && !dst->anon_vma)" was false, and in that case we should
> not be returning dst from there?
>
> So maybe we could return NULL for that case and ERR_PTR(ret) for the -ENOMEM
> from anon_vma_clone() ?

Yeah, you're right, actually I think that would probably be the best
approach as you'd both eliminate the awkward out parameter but retain the
fact that there's 3 possible return states (dup'd, no need to dup, error).

>
> > Then we could de-clunk this whole code path, and the quite natural fact of
> > 'thing didn't return a pointer therefore had no memory to allocate it' fals
> > out.
> >
> > But this isn't exactly an earth-shattering concern :)
> >
>

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Liam R. Howlett]

* Lorenzo Stoakes <lstoakes@gmail.com> [231002 03:43]:
> On Mon, Oct 02, 2023 at 09:26:03AM +0200, Vlastimil Babka wrote:
> > On 9/30/23 00:28, Lorenzo Stoakes wrote:
> > > On Fri, Sep 29, 2023 at 02:30:40PM -0400, Liam R. Howlett wrote:
> > >> When the calling function fails after the dup_anon_vma(), the
> > >> duplication of the anon_vma is not being undone.  Add the necessary
> > >> unlink_anon_vma() call to the error paths that are missing them.
> > >>
> > >> This issue showed up during inspection of the error path in vma_merge()
> > >> for an unrelated vma iterator issue.
> > >>
> > >> Users may experience increased memory usage, which may be problematic as
> > >> the failure would likely be caused by a low memory situation.
> > >>
> > >> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> > >> Cc: stable@vger.kernel.org
> > >> Cc: Jann Horn <jannh@google.com>
> > >> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > >> ---
> > >>  mm/mmap.c | 30 ++++++++++++++++++++++--------
> > >>  1 file changed, 22 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/mm/mmap.c b/mm/mmap.c
> > >> index acb7dea49e23..f9f0a5fe4db4 100644
> > >> --- a/mm/mmap.c
> > >> +++ b/mm/mmap.c
> > >> @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
> > >>   * dup_anon_vma() - Helper function to duplicate anon_vma
> > >>   * @dst: The destination VMA
> > >>   * @src: The source VMA
> > >> + * @dup: Pointer to the destination VMA when successful.
> > >>   *
> > >>   * Returns: 0 on success.
> > >
> > > Being a bit nitpicky/refactory here, but anon_vma_clone() appears to have
> > > two possible return values - 0 for success, and -ENOMEM.
> > >
> > > As a result, it's not really gaining us much passing through this value.

Passing through the pointer simplifies the success code path, in most
cases.. for these callers though, it doesn't appear to matter.

> > >
> > > It'd be nice if dup_anon_vma() and anon_vma_clone() were therefore updated
> > > to instead return NULL on ENOMEM and the dst otherwise.
> >
> > But we also need to represent that dup_anon_vma() had nothing to do, because
> > "(src->anon_vma && !dst->anon_vma)" was false, and in that case we should
> > not be returning dst from there?
> >
> > So maybe we could return NULL for that case and ERR_PTR(ret) for the -ENOMEM
> > from anon_vma_clone() ?
> 
> Yeah, you're right, actually I think that would probably be the best
> approach as you'd both eliminate the awkward out parameter but retain the
> fact that there's 3 possible return states (dup'd, no need to dup, error).

I don't like 3 possible returns as it makes things less readable, IMO.
But, since the two callers to the function don't use the assigned
variable again it won't make things too bad here.  We can document it by
using the variable name.  eg: anon_duped = dup_anon_vma() instead of
err = dup_anon_vma().

> 
> >
> > > Then we could de-clunk this whole code path, and the quite natural fact of
> > > 'thing didn't return a pointer therefore had no memory to allocate it' fals
> > > out.
> > >
> > > But this isn't exactly an earth-shattering concern :)
> > >
> >

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Suren Baghdasaryan]

On Fri, Sep 29, 2023 at 11:30 AM Liam R. Howlett
<Liam.Howlett@oracle.com> wrote:
>
> When the calling function fails after the dup_anon_vma(), the
> duplication of the anon_vma is not being undone.  Add the necessary
> unlink_anon_vma() call to the error paths that are missing them.
>
> This issue showed up during inspection of the error path in vma_merge()
> for an unrelated vma iterator issue.
>
> Users may experience increased memory usage, which may be problematic as
> the failure would likely be caused by a low memory situation.
>
> Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> Cc: stable@vger.kernel.org
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
>  mm/mmap.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index acb7dea49e23..f9f0a5fe4db4 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
>   * dup_anon_vma() - Helper function to duplicate anon_vma
>   * @dst: The destination VMA
>   * @src: The source VMA
> + * @dup: Pointer to the destination VMA when successful.
>   *
>   * Returns: 0 on success.
>   */
>  static inline int dup_anon_vma(struct vm_area_struct *dst,
> -                              struct vm_area_struct *src)
> +               struct vm_area_struct *src, struct vm_area_struct **dup)
>  {
>         /*
>          * Easily overlooked: when mprotect shifts the boundary, make sure the
> @@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_struct *dst,
>          * anon pages imported.
>          */
>         if (src->anon_vma && !dst->anon_vma) {
> +               int ret;
> +
>                 vma_assert_write_locked(dst);
>                 dst->anon_vma = src->anon_vma;
> -               return anon_vma_clone(dst, src);
> +               ret = anon_vma_clone(dst, src);
> +               if (ret)
> +                       return ret;
> +
> +               *dup = dst;
>         }
>
>         return 0;
> @@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>                unsigned long start, unsigned long end, pgoff_t pgoff,
>                struct vm_area_struct *next)
>  {
> +       struct vm_area_struct *anon_dup = NULL;
>         bool remove_next = false;
>         struct vma_prepare vp;
>
> @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>
>                 remove_next = true;
>                 vma_start_write(next);
> -               ret = dup_anon_vma(vma, next);
> +               ret = dup_anon_vma(vma, next, &anon_dup);
>                 if (ret)
>                         return ret;

Shouldn't the above be changed to a "goto nomem" instead of "return ret" ?


>         }
> @@ -661,6 +669,8 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>         return 0;
>
>  nomem:
> +       if (anon_dup)
> +               unlink_anon_vmas(anon_dup);
>         return -ENOMEM;
>  }
>
> @@ -860,6 +870,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>  {
>         struct vm_area_struct *curr, *next, *res;
>         struct vm_area_struct *vma, *adjust, *remove, *remove2;
> +       struct vm_area_struct *anon_dup = NULL;
>         struct vma_prepare vp;
>         pgoff_t vma_pgoff;
>         int err = 0;
> @@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>                 vma_start_write(next);
>                 remove = next;                          /* case 1 */
>                 vma_end = next->vm_end;
> -               err = dup_anon_vma(prev, next);
> +               err = dup_anon_vma(prev, next, &anon_dup);
>                 if (curr) {                             /* case 6 */
>                         vma_start_write(curr);
>                         remove = curr;
>                         remove2 = next;
>                         if (!next->anon_vma)
> -                               err = dup_anon_vma(prev, curr);
> +                               err = dup_anon_vma(prev, curr, &anon_dup);
>                 }
>         } else if (merge_prev) {                        /* case 2 */
>                 if (curr) {
>                         vma_start_write(curr);
> -                       err = dup_anon_vma(prev, curr);
> +                       err = dup_anon_vma(prev, curr, &anon_dup);
>                         if (end == curr->vm_end) {      /* case 7 */
>                                 remove = curr;
>                         } else {                        /* case 5 */
> @@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>                         vma_end = addr;
>                         adjust = next;
>                         adj_start = -(prev->vm_end - addr);
> -                       err = dup_anon_vma(next, prev);
> +                       err = dup_anon_vma(next, prev, &anon_dup);
>                 } else {
>                         /*
>                          * Note that cases 3 and 8 are the ONLY ones where prev
> @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>                                 vma_pgoff = curr->vm_pgoff;
>                                 vma_start_write(curr);
>                                 remove = curr;
> -                               err = dup_anon_vma(next, curr);
> +                               err = dup_anon_vma(next, curr, &anon_dup);
>                         }
>                 }
>         }
> @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
>         return res;
>
>  prealloc_fail:
> +       if (anon_dup)
> +               unlink_anon_vmas(anon_dup);

Maybe a stupid question, but why can't we do this unlinking inside
dup_anon_vma() itself when anon_vma_clone() fails? That would
eliminate the need for the out parameter in that function. I suspect
that there is a reason for that which I'm missing.

> +
>  anon_vma_fail:
>         vma_iter_set(vmi, addr);
>         vma_iter_load(vmi);
> --
> 2.40.1
>

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Liam R. Howlett]

* Suren Baghdasaryan <surenb@google.com> [231003 12:21]:
> On Fri, Sep 29, 2023 at 11:30 AM Liam R. Howlett
> <Liam.Howlett@oracle.com> wrote:
> >
> > When the calling function fails after the dup_anon_vma(), the
> > duplication of the anon_vma is not being undone.  Add the necessary
> > unlink_anon_vma() call to the error paths that are missing them.
> >
> > This issue showed up during inspection of the error path in vma_merge()
> > for an unrelated vma iterator issue.
> >
> > Users may experience increased memory usage, which may be problematic as
> > the failure would likely be caused by a low memory situation.
> >
> > Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> > Cc: stable@vger.kernel.org
> > Cc: Jann Horn <jannh@google.com>
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > ---
> >  mm/mmap.c | 30 ++++++++++++++++++++++--------
> >  1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index acb7dea49e23..f9f0a5fe4db4 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
> >   * dup_anon_vma() - Helper function to duplicate anon_vma
> >   * @dst: The destination VMA
> >   * @src: The source VMA
> > + * @dup: Pointer to the destination VMA when successful.
> >   *
> >   * Returns: 0 on success.
> >   */
> >  static inline int dup_anon_vma(struct vm_area_struct *dst,
> > -                              struct vm_area_struct *src)
> > +               struct vm_area_struct *src, struct vm_area_struct **dup)
> >  {
> >         /*
> >          * Easily overlooked: when mprotect shifts the boundary, make sure the
> > @@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_struct *dst,
> >          * anon pages imported.
> >          */
> >         if (src->anon_vma && !dst->anon_vma) {
> > +               int ret;
> > +
> >                 vma_assert_write_locked(dst);
> >                 dst->anon_vma = src->anon_vma;
> > -               return anon_vma_clone(dst, src);
> > +               ret = anon_vma_clone(dst, src);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               *dup = dst;
> >         }
> >
> >         return 0;
> > @@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >                unsigned long start, unsigned long end, pgoff_t pgoff,
> >                struct vm_area_struct *next)
> >  {
> > +       struct vm_area_struct *anon_dup = NULL;
> >         bool remove_next = false;
> >         struct vma_prepare vp;
> >
> > @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >
> >                 remove_next = true;
> >                 vma_start_write(next);
> > -               ret = dup_anon_vma(vma, next);
> > +               ret = dup_anon_vma(vma, next, &anon_dup);
> >                 if (ret)
> >                         return ret;
> 
> Shouldn't the above be changed to a "goto nomem" instead of "return ret" ?
> 
> 
> >         }
> > @@ -661,6 +669,8 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >         return 0;
> >
> >  nomem:
> > +       if (anon_dup)
> > +               unlink_anon_vmas(anon_dup);
> >         return -ENOMEM;
> >  }
> >
> > @@ -860,6 +870,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >  {
> >         struct vm_area_struct *curr, *next, *res;
> >         struct vm_area_struct *vma, *adjust, *remove, *remove2;
> > +       struct vm_area_struct *anon_dup = NULL;
> >         struct vma_prepare vp;
> >         pgoff_t vma_pgoff;
> >         int err = 0;
> > @@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >                 vma_start_write(next);
> >                 remove = next;                          /* case 1 */
> >                 vma_end = next->vm_end;
> > -               err = dup_anon_vma(prev, next);
> > +               err = dup_anon_vma(prev, next, &anon_dup);
> >                 if (curr) {                             /* case 6 */
> >                         vma_start_write(curr);
> >                         remove = curr;
> >                         remove2 = next;
> >                         if (!next->anon_vma)
> > -                               err = dup_anon_vma(prev, curr);
> > +                               err = dup_anon_vma(prev, curr, &anon_dup);
> >                 }
> >         } else if (merge_prev) {                        /* case 2 */
> >                 if (curr) {
> >                         vma_start_write(curr);
> > -                       err = dup_anon_vma(prev, curr);
> > +                       err = dup_anon_vma(prev, curr, &anon_dup);
> >                         if (end == curr->vm_end) {      /* case 7 */
> >                                 remove = curr;
> >                         } else {                        /* case 5 */
> > @@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >                         vma_end = addr;
> >                         adjust = next;
> >                         adj_start = -(prev->vm_end - addr);
> > -                       err = dup_anon_vma(next, prev);
> > +                       err = dup_anon_vma(next, prev, &anon_dup);
> >                 } else {
> >                         /*
> >                          * Note that cases 3 and 8 are the ONLY ones where prev
> > @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >                                 vma_pgoff = curr->vm_pgoff;
> >                                 vma_start_write(curr);
> >                                 remove = curr;
> > -                               err = dup_anon_vma(next, curr);
> > +                               err = dup_anon_vma(next, curr, &anon_dup);
> >                         }
> >                 }
> >         }
> > @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> >         return res;
> >
> >  prealloc_fail:
> > +       if (anon_dup)
> > +               unlink_anon_vmas(anon_dup);
> 
> Maybe a stupid question, but why can't we do this unlinking inside
> dup_anon_vma() itself when anon_vma_clone() fails? That would
> eliminate the need for the out parameter in that function. I suspect
> that there is a reason for that which I'm missing.

It's too late.  This is to undo the link when the preallocation for the
maple tree fails.  So we had memory to dup the anon vma, but not to put
it in the tree.

> 
> > +
> >  anon_vma_fail:
> >         vma_iter_set(vmi, addr);
> >         vma_iter_load(vmi);
> > --
> > 2.40.1
> >

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Suren Baghdasaryan]

On Tue, Oct 3, 2023 at 11:51 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Suren Baghdasaryan <surenb@google.com> [231003 12:21]:
> > On Fri, Sep 29, 2023 at 11:30 AM Liam R. Howlett
> > <Liam.Howlett@oracle.com> wrote:
> > >
> > > When the calling function fails after the dup_anon_vma(), the
> > > duplication of the anon_vma is not being undone.  Add the necessary
> > > unlink_anon_vma() call to the error paths that are missing them.
> > >
> > > This issue showed up during inspection of the error path in vma_merge()
> > > for an unrelated vma iterator issue.
> > >
> > > Users may experience increased memory usage, which may be problematic as
> > > the failure would likely be caused by a low memory situation.
> > >
> > > Fixes: d4af56c5c7c6 ("mm: start tracking VMAs with maple tree")
> > > Cc: stable@vger.kernel.org
> > > Cc: Jann Horn <jannh@google.com>
> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > > ---
> > >  mm/mmap.c | 30 ++++++++++++++++++++++--------
> > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index acb7dea49e23..f9f0a5fe4db4 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -583,11 +583,12 @@ static inline void vma_complete(struct vma_prepare *vp,
> > >   * dup_anon_vma() - Helper function to duplicate anon_vma
> > >   * @dst: The destination VMA
> > >   * @src: The source VMA
> > > + * @dup: Pointer to the destination VMA when successful.
> > >   *
> > >   * Returns: 0 on success.
> > >   */
> > >  static inline int dup_anon_vma(struct vm_area_struct *dst,
> > > -                              struct vm_area_struct *src)
> > > +               struct vm_area_struct *src, struct vm_area_struct **dup)
> > >  {
> > >         /*
> > >          * Easily overlooked: when mprotect shifts the boundary, make sure the
> > > @@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_struct *dst,
> > >          * anon pages imported.
> > >          */
> > >         if (src->anon_vma && !dst->anon_vma) {
> > > +               int ret;
> > > +
> > >                 vma_assert_write_locked(dst);
> > >                 dst->anon_vma = src->anon_vma;
> > > -               return anon_vma_clone(dst, src);
> > > +               ret = anon_vma_clone(dst, src);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               *dup = dst;
> > >         }
> > >
> > >         return 0;
> > > @@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >                unsigned long start, unsigned long end, pgoff_t pgoff,
> > >                struct vm_area_struct *next)
> > >  {
> > > +       struct vm_area_struct *anon_dup = NULL;
> > >         bool remove_next = false;
> > >         struct vma_prepare vp;
> > >
> > > @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >
> > >                 remove_next = true;
> > >                 vma_start_write(next);
> > > -               ret = dup_anon_vma(vma, next);
> > > +               ret = dup_anon_vma(vma, next, &anon_dup);
> > >                 if (ret)
> > >                         return ret;
> >
> > Shouldn't the above be changed to a "goto nomem" instead of "return ret" ?
> >
> >
> > >         }
> > > @@ -661,6 +669,8 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >         return 0;
> > >
> > >  nomem:
> > > +       if (anon_dup)
> > > +               unlink_anon_vmas(anon_dup);
> > >         return -ENOMEM;
> > >  }
> > >
> > > @@ -860,6 +870,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > >  {
> > >         struct vm_area_struct *curr, *next, *res;
> > >         struct vm_area_struct *vma, *adjust, *remove, *remove2;
> > > +       struct vm_area_struct *anon_dup = NULL;
> > >         struct vma_prepare vp;
> > >         pgoff_t vma_pgoff;
> > >         int err = 0;
> > > @@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > >                 vma_start_write(next);
> > >                 remove = next;                          /* case 1 */
> > >                 vma_end = next->vm_end;
> > > -               err = dup_anon_vma(prev, next);
> > > +               err = dup_anon_vma(prev, next, &anon_dup);
> > >                 if (curr) {                             /* case 6 */
> > >                         vma_start_write(curr);
> > >                         remove = curr;
> > >                         remove2 = next;
> > >                         if (!next->anon_vma)
> > > -                               err = dup_anon_vma(prev, curr);
> > > +                               err = dup_anon_vma(prev, curr, &anon_dup);
> > >                 }
> > >         } else if (merge_prev) {                        /* case 2 */
> > >                 if (curr) {
> > >                         vma_start_write(curr);
> > > -                       err = dup_anon_vma(prev, curr);
> > > +                       err = dup_anon_vma(prev, curr, &anon_dup);
> > >                         if (end == curr->vm_end) {      /* case 7 */
> > >                                 remove = curr;
> > >                         } else {                        /* case 5 */
> > > @@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > >                         vma_end = addr;
> > >                         adjust = next;
> > >                         adj_start = -(prev->vm_end - addr);
> > > -                       err = dup_anon_vma(next, prev);
> > > +                       err = dup_anon_vma(next, prev, &anon_dup);
> > >                 } else {
> > >                         /*
> > >                          * Note that cases 3 and 8 are the ONLY ones where prev
> > > @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > >                                 vma_pgoff = curr->vm_pgoff;
> > >                                 vma_start_write(curr);
> > >                                 remove = curr;
> > > -                               err = dup_anon_vma(next, curr);
> > > +                               err = dup_anon_vma(next, curr, &anon_dup);
> > >                         }
> > >                 }
> > >         }
> > > @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterator *vmi, struct mm_struct *mm,
> > >         return res;
> > >
> > >  prealloc_fail:
> > > +       if (anon_dup)
> > > +               unlink_anon_vmas(anon_dup);
> >
> > Maybe a stupid question, but why can't we do this unlinking inside
> > dup_anon_vma() itself when anon_vma_clone() fails? That would
> > eliminate the need for the out parameter in that function. I suspect
> > that there is a reason for that which I'm missing.
>
> It's too late.  This is to undo the link when the preallocation for the
> maple tree fails.  So we had memory to dup the anon vma, but not to put
> it in the tree.

Ah, I see what I missed now. Sorry for the noise.

>
> >
> > > +
> > >  anon_vma_fail:
> > >         vma_iter_set(vmi, addr);
> > >         vma_iter_load(vmi);
> > > --
> > > 2.40.1
> > >

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Andrew Morton]

On Tue, 3 Oct 2023 09:21:22 -0700 Suren Baghdasaryan <surenb@google.com> wrote:

> > @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >
> >                 remove_next = true;
> >                 vma_start_write(next);
> > -               ret = dup_anon_vma(vma, next);
> > +               ret = dup_anon_vma(vma, next, &anon_dup);
> >                 if (ret)
> >                         return ret;
> 
> Shouldn't the above be changed to a "goto nomem" instead of "return ret" ?

It looks OK to me as-is - dup_anon_vma() leaves anon_dup==NULL
when it returns error.

Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()

[Suren Baghdasaryan]

On Fri, Oct 6, 2023 at 2:08 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 3 Oct 2023 09:21:22 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > > @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> > >
> > >                 remove_next = true;
> > >                 vma_start_write(next);
> > > -               ret = dup_anon_vma(vma, next);
> > > +               ret = dup_anon_vma(vma, next, &anon_dup);
> > >                 if (ret)
> > >                         return ret;
> >
> > Shouldn't the above be changed to a "goto nomem" instead of "return ret" ?
>
> It looks OK to me as-is - dup_anon_vma() leaves anon_dup==NULL
> when it returns error.

Yes, I realised that after sending the question and didn't want to
create more noise than I already did... All seems good.