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 74250E8FDAF for ; Tue, 3 Oct 2023 19:40:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9E7618D0085; Tue, 3 Oct 2023 15:40:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 997B68D0003; Tue, 3 Oct 2023 15:40:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 837588D0085; Tue, 3 Oct 2023 15:40:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 73B388D0003 for ; Tue, 3 Oct 2023 15:40:25 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 41DEB1A03EB for ; Tue, 3 Oct 2023 19:40:25 +0000 (UTC) X-FDA: 81305166810.26.5AA1E2A Received: from mail-yw1-f182.google.com (mail-yw1-f182.google.com [209.85.128.182]) by imf28.hostedemail.com (Postfix) with ESMTP id 8721AC000F for ; Tue, 3 Oct 2023 19:40:23 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=kpgz1nqZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf28.hostedemail.com: domain of surenb@google.com designates 209.85.128.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1696362023; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to: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=lfr2egyOR1+C8mShorjIOzO6y7VY3Fj1mtMvJxoOK94=; b=dytZPVSiR69TZJzOgOqOq0bnx5/qKGG0SKdVQg1kjOjj8W2Nxnw8hPplObq7aKobE61gQu s3bAIq+0fFcw3hv5SLbb29dT/WKevhzPHefNLdnRvfyRgCEIo6qFNcon4eZ2VQ/3MeF/xM PYDC+4yRoZPgiW9NzpJjs8qE3TSUZwo= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=kpgz1nqZ; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf28.hostedemail.com: domain of surenb@google.com designates 209.85.128.182 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1696362023; a=rsa-sha256; cv=none; b=LDCGWd4Z62dHPLWEAbWpShiurie6RDIa252GxiWj4FMYSWzhecPlvfS6SBGQOOGSLRFQrz wWzTy8agjcZRuJIKWKV5LAFOGoSUMZEVO07S2TSA0euZappZMFlex5CrVET/8KKLsv7Xh2 nF1ZflyLECrPen2xlwel74iAZXhNhfI= Received: by mail-yw1-f182.google.com with SMTP id 00721157ae682-59b5484fbe6so16148927b3.1 for ; Tue, 03 Oct 2023 12:40:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1696362022; x=1696966822; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=lfr2egyOR1+C8mShorjIOzO6y7VY3Fj1mtMvJxoOK94=; b=kpgz1nqZYGo3kbCGT7CxT/AJ2XCAKtAi5zdXavnbpWj+hKI0hDTTPVNvi5u1KDSOJG 6D6dZq2bgkv/uyEt+HRA7Ykrh8KdPsTfEy11qqAptHEeGcGJLLjCTQYjcKDo1zA0QYWo IJvMlURILlwBOUWDyKVwTcLhXnrpUT0GBTyZBVEyrRTrLfkozBpf/ilooakFMLnL9Rzg VgoPeQnH66JbEPywt1fCweAiqGK2UUTKFcImepvwOIGX38mfDRE3d8qhzemEuRvNRPlu v84+hkK4ZHRvbat7/+QW0NoCuyhxoo/TJZJS70HY8itCp0bqGQCw6KDxf43k1OdPhStm U0Bw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696362022; x=1696966822; h=content-transfer-encoding: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=lfr2egyOR1+C8mShorjIOzO6y7VY3Fj1mtMvJxoOK94=; b=sezK58p1KzhYsn2qBX39Rmly0XCixGn93BOM9VzYYoahJvJ2YkRWJ/InBZTEB+i1c7 q3p2UI+h7lCAQvoGiiZCWEMYwSfin9c1aqJuEL0SzqAE/Sp+Fx6oCnpvQi/GJJWqmCLM eP8mN9rQ2UrBJKzAwnOF8DvCLEryOf/f+3d+UNiYh3NOaWLNN95uWEg82QcM4nT0LHUd VsLFLldWf+LKAg+qChckTPbh0FfcZ03n4zcnML7xn/KikyRtDB1poiKWly9qTexDeniu 8C3+vW0oup0FLAPSVHxl2T0L+FB+LMihCzVqdhJPNxCe444+hgXSg/i6jIb/zvi1EUKt Sp6Q== X-Gm-Message-State: AOJu0YyRfLbKY+yFOc2rUQ0MucjP9MxuaWWLm7PnuoClvKDsxIsrlr0v mrXUXtEd5URJy/t6aYbTLSCbZsHRxS2+evnFFlpx/A== X-Google-Smtp-Source: AGHT+IHMRHSRLs+lH7EW8L10zxsOV/xPpBMFnE87yJyz8Bgq+zHVJ8yFa+IWyhMreU9et69QGkrqkHneSbi5sLXaf+4= X-Received: by 2002:a0d:e001:0:b0:59e:7fc1:dba0 with SMTP id j1-20020a0de001000000b0059e7fc1dba0mr523083ywe.44.1696362022441; Tue, 03 Oct 2023 12:40:22 -0700 (PDT) MIME-Version: 1.0 References: <20230929183041.2835469-1-Liam.Howlett@oracle.com> <20230929183041.2835469-3-Liam.Howlett@oracle.com> <20231003185149.brbzyu2ivn25tkeu@revolver> In-Reply-To: <20231003185149.brbzyu2ivn25tkeu@revolver> From: Suren Baghdasaryan Date: Tue, 3 Oct 2023 12:40:07 -0700 Message-ID: Subject: Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma() To: "Liam R. Howlett" , Suren Baghdasaryan , 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-Rspamd-Queue-Id: 8721AC000F X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: 554i9hcc9kqbsq1s9n8xtfrohyo495m5 X-HE-Tag: 1696362023-164985 X-HE-Meta: U2FsdGVkX18k4E8RzkCbLBH7tk7EdQSYk/v8G3+fgGYNUR9jBfDcJLD/Sd8jFNIIXn8WCURZ4sgcNR8yOoYxO61JbaY/4eSLE8fVEf8suNvdjVrNWI7YI6LAY1zi2nXoGyxT8Vq+j4Ykv2hHsL1W4jxDI6th2WdUjBrKBnfoM/tD4JM9LvRHJRHVwIQczuQ2pRt8AoO4oYYjEDbDsUCdyJFsvmpKENvCIvD9RjdCZAE3iIzgZeFzoYalboZWJIbVRoATgIeozmyFL7hnRBmZAhCKQ1wBzA8+d5zTi/yU1efhy53C7uPetbNrd3yAzvAA0r+Qn3XAHffpaGvgdMytiyJF0moAZ+DipJ6gmfnLFMUQqu+gIlfs2jfl1mIjBF+iVWVDXgI/6XpjI/Nh5EnHWO5OPlNHp4kqRkZ2TtgzhTnuqLRDtNM/vR5+fB0pkZXCp2BH1ybrXyyJFI465OzXYqGU6uUZOqLTCRUgYJsbeHN9syNgj2B7uWZF8kch+8NmkHruI2hWqvj1zFkM4DKr3bg0vqoNKRSI6/98mSvTi+dfJt4oiAIvfcT6+SgTVevXTmB4nXNecsEizlI6+/+LdOOZDswADrZE2rCLbqZb5mE4xGJ3cybGHYSCEKg3to7szWe6qCBwNl4Vb0QdHwBRLzilPELIYEyIR4QEguNyDNAfWAasBM6nHLS26pvIqDsDWH5ngHhnWUGjnXTj7PKtT7v4s/FTeqJ7absaMIuvnkpA1FYTpABs3x2G2kaJ9tludYr98CV5rOU3NBT+1dyrV3giH7FFAolGiwGcw4J2k2YDaJlWlkdOjLtzF1ARpm2W8jBJlUBf6pNJJN63DPkhokeN4V6wqL8qQwxYdyQ9FCu9smiUn7/tH1QWfdSk78RRJ1LzEE6Z0b8HYGDRy5/MxmAPB0H7xLSG3OGQWwIdIITklzEJnvQgmQq/1W3ECtGcQNsN2E6Ck67OWNXnst2 keRqm+00 otfQV+8nwRz5v/+u1L2vzrgCDcAltDf4OOS5hFmWVcXyEyWiff9dESMu7/fvLaL+EMfghp79sbfdelS9lbtkSjytlf8o5w8BJH8gLLNlR7KCY4gTFz4yVGevN6vTVSiPH18Q6+iaN9Xp0XAl3XXb2WN6tybr90mR2MZKKwQNJsJZrJUHZccBuaAcbcoTu33xnihjjuFlT+GeA1OhNlfeUCQVPiaMxWoXAaURrmKtRKufG5jFqmD28DWZhVvjq1xILbMp/DQX+2RRCs4R43s5qy/6sXGaOmm/BieZFF4cahBlqNno5xRwlYxK8QGfJQBxyCkjlMXgutrJKdyVNcRb5Pxvx/psREK8snMvIkkgBsnfR6eXnHyDJqfHsIbWt18UmOtY7 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 Tue, Oct 3, 2023 at 11:51=E2=80=AFAM Liam R. Howlett wrote: > > * Suren Baghdasaryan [231003 12:21]: > > 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_prep= are *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 **d= up) > > > { > > > /* > > > * Easily overlooked: when mprotect shifts the boundary, make= sure the > > > @@ -595,9 +596,15 @@ static inline int dup_anon_vma(struct vm_area_st= ruct *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 v= m_area_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 v= m_area_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 v= m_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_itera= tor *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_ite= rator *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, &ano= n_dup); > > > } > > > } 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_itera= tor *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 = where prev > > > @@ -968,7 +979,7 @@ struct vm_area_struct *vma_merge(struct vma_itera= tor *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, &ano= n_dup); > > > } > > > } > > > } > > > @@ -1018,6 +1029,9 @@ struct vm_area_struct *vma_merge(struct vma_ite= rator *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 > > >