Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20230929183041.2835469-1-Liam.Howlett@oracle.com> <20230929183041.2835469-3-Liam.Howlett@oracle.com>
In-Reply-To: <20230929183041.2835469-3-Liam.Howlett@oracle.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Tue, 3 Oct 2023 09:21:22 -0700
Message-ID: <CAJuCfpFx3zxv1ZgkLh4dkafOCHvL_674cysJiuQPQhKXX9BuzQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma()
To:     "Liam R. Howlett" <Liam.Howlett@oracle.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>,
        Lorenzo Stoakes <lstoakes@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Matthew Wilcox <willy@infradead.org>, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Sep 29, 2023 at 11:30=E2=80=AFAM 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 sur=
e 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 =3D src->anon_vma;
> -               return anon_vma_clone(dst, src);
> +               ret =3D anon_vma_clone(dst, src);
> +               if (ret)
> +                       return ret;
> +
> +               *dup =3D dst;
>         }
>
>         return 0;
> @@ -624,6 +631,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_ar=
ea_struct *vma,
>                unsigned long start, unsigned long end, pgoff_t pgoff,
>                struct vm_area_struct *next)
>  {
> +       struct vm_area_struct *anon_dup =3D NULL;
>         bool remove_next =3D false;
>         struct vma_prepare vp;
>
> @@ -633,7 +641,7 @@ int vma_expand(struct vma_iterator *vmi, struct vm_ar=
ea_struct *vma,
>
>                 remove_next =3D true;
>                 vma_start_write(next);
> -               ret =3D dup_anon_vma(vma, next);
> +               ret =3D 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_ar=
ea_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 =3D NULL;
>         struct vma_prepare vp;
>         pgoff_t vma_pgoff;
>         int err =3D 0;
> @@ -927,18 +938,18 @@ struct vm_area_struct *vma_merge(struct vma_iterato=
r *vmi, struct mm_struct *mm,
>                 vma_start_write(next);
>                 remove =3D next;                          /* case 1 */
>                 vma_end =3D next->vm_end;
> -               err =3D dup_anon_vma(prev, next);
> +               err =3D dup_anon_vma(prev, next, &anon_dup);
>                 if (curr) {                             /* case 6 */
>                         vma_start_write(curr);
>                         remove =3D curr;
>                         remove2 =3D next;
>                         if (!next->anon_vma)
> -                               err =3D dup_anon_vma(prev, curr);
> +                               err =3D dup_anon_vma(prev, curr, &anon_du=
p);
>                 }
>         } else if (merge_prev) {                        /* case 2 */
>                 if (curr) {
>                         vma_start_write(curr);
> -                       err =3D dup_anon_vma(prev, curr);
> +                       err =3D dup_anon_vma(prev, curr, &anon_dup);
>                         if (end =3D=3D curr->vm_end) {      /* case 7 */
>                                 remove =3D curr;
>                         } else {                        /* case 5 */
> @@ -954,7 +965,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator =
*vmi, struct mm_struct *mm,
>                         vma_end =3D addr;
>                         adjust =3D next;
>                         adj_start =3D -(prev->vm_end - addr);
> -                       err =3D dup_anon_vma(next, prev);
> +                       err =3D dup_anon_vma(next, prev, &anon_dup);
>                 } else {
>                         /*
>                          * Note that cases 3 and 8 are the ONLY ones wher=
e prev
> @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_iterator =
*vmi, struct mm_struct *mm,
>                                 vma_pgoff =3D curr->vm_pgoff;
>                                 vma_start_write(curr);
>                                 remove =3D curr;
> -                               err =3D dup_anon_vma(next, curr);
> +                               err =3D dup_anon_vma(next, curr, &anon_du=
p);
>                         }
>                 }
>         }
> @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_iterato=
r *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
>