| Message ID | 20230929183041.2835469-3-Liam.Howlett@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for vma_merge() error path | expand |
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>
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>
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 :) >
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 :) > > >
* 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 :) > > > > >
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 >
* 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 > >
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 > > >
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.
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.
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);
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(-)