From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 80B63E7AD76 for ; Tue, 3 Oct 2023 16:21:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DBCB26B0108; Tue, 3 Oct 2023 12:21:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D6AB36B0109; Tue, 3 Oct 2023 12:21:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BE5126B010D; Tue, 3 Oct 2023 12:21:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id AA1E16B0108 for ; Tue, 3 Oct 2023 12:21:39 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 72DDBC03FB for ; Tue, 3 Oct 2023 16:21:39 +0000 (UTC) X-FDA: 81304665918.23.69587D1 Received: from mail-yw1-f173.google.com (mail-yw1-f173.google.com [209.85.128.173]) by imf21.hostedemail.com (Postfix) with ESMTP id AF3341C0017 for ; Tue, 3 Oct 2023 16:21:36 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=YsX42Axz; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.128.173 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1696350096; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=dpyVNkJRXjRkFR/rn/ddNmNe1O9yMqnXww/ltlxxeCg=; b=m18ln1cQY0wQPyfwXfiuq9EJhkcxx0xUrcfx1X5rBgVzQFry2uBiRzhnGnlIbOZOWbX4oy yOEEg10p7/BqYnvpDyLtd1uwz72XdhJmCs7bO1IUrz17zPmwzriGKgn4pqLJze4YkIFw6y Puz7gO8XTtBNGEMXr1rij/m6ptpGjQA= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1696350096; a=rsa-sha256; cv=none; b=X+KkaVe0QINk3ywCwYO/Shp8+k8E04pgIT72Dm1FZ+iMzUZ5Ux7I6BZrja2fkHDHq8Atgl 71g6rIxSeFdcKK9ZGowyABrfLl/JvEeyg/3TQPCwFmbwBS2cnPilYGdrJhZuS2jxk841iG kqxjshpttjZsC+geN0Zequd/Ggg0XFo= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=YsX42Axz; spf=pass (imf21.hostedemail.com: domain of surenb@google.com designates 209.85.128.173 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-yw1-f173.google.com with SMTP id 00721157ae682-59c215f2f4aso13986027b3.1 for ; Tue, 03 Oct 2023 09:21:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1696350096; x=1696954896; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=dpyVNkJRXjRkFR/rn/ddNmNe1O9yMqnXww/ltlxxeCg=; b=YsX42AxzxkE0ZsRlbzTbMQAo/kGJCqhfpA59yDfLY9hS6pGGT9ABwfB0PYb5sTt7tz Pz0r6urn2iCg/GJEqUW+t7LUL24DxrO+SmjR0xdFWY7/WfaOCvw9eofUiCKNSp01aCIQ C3NBFBpCk0aujVtvqpeVJTmJNqhMgcFunq8s13Wg215eR98UHt4J0J6G+KDrO6PyTEY/ 8LgbQzeZb+zQM1Pj/DbejWbrjgVNJ7xvyKx3s9hoXaEj2oepPc1Jd1glmnTmUBEjQ9Vr U25S2cRj/Csp+jIthtBnmVfXW1IQvLiMKCVlZJ6qFoTTdXH4+T6CRxiP1VuoFdETWzBC FGFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696350096; x=1696954896; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=dpyVNkJRXjRkFR/rn/ddNmNe1O9yMqnXww/ltlxxeCg=; b=NDdEADOXltEJudzm8sBpONHbT+EhtAsmgG5Dlm0nyGDMUHCq0x+KdhQwGaC09M78Qu ROamO13xBc5hbaueqVQubKq6WcePWNHZUVtFFYyffxnt5Ni9I9i7VfPEe/Zj/JbFUy4O GxAV9pGoXVq/RPhIQP2WgYDbLpcTuAwyuypS2jJdDNKyiN4/7sNEAS944EIbHoJ27mJy WYvN50ffWnFAPK6Wme33yXWYyILexxsho1T9vWY2oQHliO59WWgDzXI1VN7uCOKJt7ks Z0Zg78QA55L1pLDzhzOAYet2aAOb0EcBnpmqOHaBRLA64sVuXnse1SJmZvFhKT3L6J3J MTnQ== X-Gm-Message-State: AOJu0Yy68AZxNsfAnWCAWDvjUcIa8hbbk7sz5KI6d4gg5GIElZsjzXNa XQnTsf3eG4VzOu4xx09aD2HPB5DX8LBu2DcJVk7+HA== X-Google-Smtp-Source: AGHT+IHLgDM/yoxhM8u9nOe2pHp+nHs4+ebDacp3i5wmm0zfXZH6bnV+yIRQ2mERuQ97Bs+LZ6TAu1lbdrpuwLOysl8= X-Received: by 2002:a81:5251:0:b0:586:a170:7dbe with SMTP id g78-20020a815251000000b00586a1707dbemr111089ywb.13.1696350095583; Tue, 03 Oct 2023 09:21:35 -0700 (PDT) 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 Date: Tue, 3 Oct 2023 09:21:22 -0700 Message-ID: Subject: Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma() To: "Liam R. Howlett" Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Jann Horn , Lorenzo Stoakes , Vlastimil Babka , Matthew Wilcox , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: t3qacobykqhgzrsasf1t6b8ogkb9aszw X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: AF3341C0017 X-Rspam-User: X-HE-Tag: 1696350096-732638 X-HE-Meta: U2FsdGVkX1/E1+EOSrOIvjf3JMqe38mQYSxvZxoHs/mfAfAdAbwW3Ef+QmBk+VKTjkKQoej9s6j9I83W66M9qYWoNaIB4rfSRMZ1Hd1B+gmPtAhtIg381LdStjNDNjuf/AtCMWqpE+Grr2Ca7JwLOw0F0YOCewtyJtV5qXr4lx3a3JFkwm3jQrhfK4mPY/8L/6X751Zw8MFq1wrNi9+I4iLptNR71ZrLwuK1GuGrgU5AhpSz+ZseKlaCZHgov5oG7zHXB+b5aKiz/o5boKoH9mMPPA0/sSoAKjPXSDMZVzStqf7WpYelzhQNtCKwag9h9qKvgOJWLZN/pmIz3mOz6JJRv4fAglnpesjNTg8SFoxYrgtlS9Q4TmwPacH8dQjnrvv9n1hYqw6lLkYgokI8MQ9bljg/xiuRT4G2zz6CO82V5KpqTKnQUCbyqjGfEF4pQOMNKn9e3lLTn9mEdIo5JHJ4y0Qr5DONknIECbccaVtORUEIxOKy+fm1i4kD77HsiGCgyGksU5/4OkW1JXckK8O8xzMl+Ju6yZBGUlrt2zdw0u4GHbN+U03PKERnrRM6dhFYWq5/Y7mFzukT5rFmQNZ8I2ANqDH/n0TgtIhWgaAIZS6SXsRyncFueJw23YEh8O3F0hcgIK1sjDOWO1ulg5eFP7YcIqB4f1UcKYZiwKba/aZHm/oVYfYmNA6gHVsRzV+49i3DIb/7EmaypLhR7P5aUrVpF1ySxEY7j4x2l7szsnO4JJCksf/rl2POCe7sQzCSCHH38DF0bG4e4cUDU+2H5JuHbYUxKZWdDkW81lPlRhdgMVipbhgko1HzMy38R5r34IL1VKezGDi4QLbbW0J97sd6lezmoLEfADg7e/0pGd2Z45EBhs4Zl70Tqc8+WKunxIcZSktw9efv9jVlS7FSjQ8vAfoIrGbOkbge9oh+Tv7wp9ZbCFRefxElmboKU7xwjPQ65LsMzfhp00Q We0zpwCd tI1446fzk6i0ftEQJpAjsdnyL+EYs1jLnfe/MbNaO4v6g8sydQO7+3scNTdv+TqMNDa6n8TL8yMftviIR2vtPWjQzxo+N6Te7qx4j9cLac2ky29dO2yZTA6/YgMeLHfT9EGcvWOjCVmps+6SUAt6zo98T5AJPCT4Bfb1yECYAwDVxYs9RR3pCYB9cQhEozMhM0q6gHmXZEpiq4SWozKdMNXhhsxyfb6KTmCL8NUjSvfO6B1H5CbIPAcdtZn1kqVktK7XiSCL355xRz0YRiOVHohj8PxtoJThvYZ6tw4aXeC6cRf/q/3rCa+S2E36uuWWrKOvlgzNalIUoPBrpTgF7RhbJow4MWfuOd6K1GCY4a/6iuP5ZrjcGEqc//XaYEWHpS+Vg X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Sep 29, 2023 at 11:30=E2=80=AFAM 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 > Signed-off-by: Liam R. Howlett > --- > 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 >