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 v1: - Moved dup_anon_vma() call update from patch 1 to patch 2 - Thanks Matthew Wilcox - Changed comment on patch 1 - Thanks Andrew Morton
v1: https://lore.kernel.org/linux-mm/20230927160746.1928098-1-Liam.Howlett@oracl...
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 | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-)
When merging of the previous VMA fails after the vma iterator has been moved to the previous entry, the vma iterator must be advanced to ensure the caller takes the correct action on the next vma iterator event. Fix this by adding a vma_next() call to the error path.
Users would not notice the effects as it would result in an extra vma_merge() call if the first call ended due to an out-of-memory event.
Link: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejti... Closes: https://lore.kernel.org/linux-mm/CAG48ez12VN1JAOtTNMY+Y2YnsU45yL5giS-Qn=ejti... 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..a4eb5a5626bb 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: + if (merge_prev) + vma_next(vmi); + return NULL; }
/*
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 | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c index a4eb5a5626bb..2f0ee489db8a 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -587,7 +587,7 @@ static inline void vma_complete(struct vma_prepare *vp, * 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 @@ -597,6 +597,7 @@ static inline int dup_anon_vma(struct vm_area_struct *dst, if (src->anon_vma && !dst->anon_vma) { vma_assert_write_locked(dst); dst->anon_vma = src->anon_vma; + *dup = dst; return anon_vma_clone(dst, src); }
@@ -624,6 +625,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 +635,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 +663,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 +864,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 +932,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 +959,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 +973,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 +1023,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: if (merge_prev) vma_next(vmi);
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_=StjxhUsCkL6p... 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 2f0ee489db8a..3c78afb707cf 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -937,6 +937,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); }
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html/#opt...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v2 3/3] mmap: Add clarifying comment to vma_merge() code Link: https://lore.kernel.org/stable/20230928171634.2245042-4-Liam.Howlett%40oracl...
* kernel test robot lkp@intel.com [230928 13:18]:
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/s...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v2 3/3] mmap: Add clarifying comment to vma_merge() code Link: https://urldefense.com/v3/__https://lore.kernel.org/stable/20230928171634.22...
Right, thanks. This is part of the series but stable does not need to backport this one.
Sorry for Cc'ing stable on the last patch of the series.
On Thu, Sep 28, 2023 at 01:26:39PM -0400, Liam R. Howlett wrote:
- kernel test robot lkp@intel.com [230928 13:18]:
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
The check is based on https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/process/s...
Rule: add the tag "Cc: stable@vger.kernel.org" in the sign-off area to have the patch automatically included in the stable tree. Subject: [PATCH v2 3/3] mmap: Add clarifying comment to vma_merge() code Link: https://urldefense.com/v3/__https://lore.kernel.org/stable/20230928171634.22...
Right, thanks. This is part of the series but stable does not need to backport this one.
Sorry for Cc'ing stable on the last patch of the series.
It's not an issue, I think the kernel test robot is a bit pedantic here, so no worries :)
On 9/28/23 19:16, 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_=StjxhUsCkL6p... 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 2f0ee489db8a..3c78afb707cf 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -937,6 +937,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);
linux-stable-mirror@lists.linaro.org
-
Greg KH -
kernel test robot -
Liam R. Howlett -
Liam R. Howlett
-
Vlastimil Babka