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 35BA5E77347 for ; Fri, 29 Sep 2023 22:28:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B67508D00ED; Fri, 29 Sep 2023 18:28:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B17158D006D; Fri, 29 Sep 2023 18:28:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9B7A38D00ED; Fri, 29 Sep 2023 18:28:47 -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 8A3F68D006D for ; Fri, 29 Sep 2023 18:28:47 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 4B6D4A0617 for ; Fri, 29 Sep 2023 22:28:47 +0000 (UTC) X-FDA: 81291075894.22.8E92EFE Received: from mail-wr1-f49.google.com (mail-wr1-f49.google.com [209.85.221.49]) by imf20.hostedemail.com (Postfix) with ESMTP id 58FCC1C0005 for ; Fri, 29 Sep 2023 22:28:45 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PjMVMV+c; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf20.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.49 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1696026525; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ojq66DYQP0PdZxJFspae4896e0n9cWPdtlOpAHGIcv4=; b=KL7pVBeDWSx6lFs4NVR3Hk7UuLyJOoMPRBzgKXwUUhy5wgrvlGAkDCf/Pu6+k613KWK2fn 3w26Bw07Fr92sqvAlnmR0BSlB7Sjd7bFDK/a1EhBfutWMBGrCa/RgrUCQsPYnnd4uBIEXs Mw8BYuC1c7OZbdUzKq/4NPTmnritpNQ= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=PjMVMV+c; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf20.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.221.49 as permitted sender) smtp.mailfrom=lstoakes@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1696026525; a=rsa-sha256; cv=none; b=rY1wWgB9jnIjWT/WlQ+zq9r4FQg6HAwOCTFFylYI1hpzdCJoSMKC1Oeb+bYkElPN627GTX /pBt/P4LdY4/Fgsv2QSKaf1TotmXiQxEig7fJ6QMx3GT60wjDRgCc6+DnoRw+FXGLaDJq3 F5XaFrieAA2WUTWLoOFVjB19yRPZ4PA= Received: by mail-wr1-f49.google.com with SMTP id ffacd0b85a97d-31f7400cb74so13250111f8f.2 for ; Fri, 29 Sep 2023 15:28:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1696026524; x=1696631324; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ojq66DYQP0PdZxJFspae4896e0n9cWPdtlOpAHGIcv4=; b=PjMVMV+crFxcfs/mjVMyJbU5vr8M9atc53zdtSoAFBmOsThdg4s9//3wKoBpOwX3nQ o9SAtZBNjAaw97chPcbNWytkDrcYskS+CpZ/db2UbUhD5g4eEelUXo9t+CrUESDwqBjh 3PQz6IIFBn3q6afxkqM2HL98J3HW1kGDdexO9K44CIMaLe1/yrn6gSx4gWn4M+DDyBNK KK+hAnH/8TR5z2AnGafclWogzH8eiid6J0RAqqPzAp9oPpL2Nbof2yoeOIyOoCTzoIUJ +innoDY/DmXxx3Qp/jXoHtvfVGTDm2G2eKUAwBq60p1eXErE72zkds6Z0cjYDvCOzTlK G1zw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696026524; x=1696631324; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ojq66DYQP0PdZxJFspae4896e0n9cWPdtlOpAHGIcv4=; b=dZPHfZVasW4mYOwuT8TblJA4VzRZfCfoxBO7x3fUxTK6Q0Yy+H9Dqtb5x9go4nZ7xN vT6g+ifZ2wQ/ESafp8DTyJSRdMIhyzXCJQiMsyHRlKko6xjlsuYuRVmkbXNTnRWgt+dA byt5x5drOlHuapVFBjup+ZcxRiFPWXMQ2ikR+ZrXCvFs3kAxtJXX3GVsDfvf9owzQsBV ebSPMXhugY5pmIu8vuIXiPMbnOzSIxJyA8JRGuIRhqZE1COZFPBht93jqTUOGXC8OF7K F5io6yCONUWs0QOMP1h0hUyRIiRE4DC353Kink4M013C31IoDzi1RwmcZSocJzbDMy1S SuHg== X-Gm-Message-State: AOJu0Yy77UipQiVdt89lpERyRXn4MQ+mFiDTLOWdz0UR8gAvHWj7lkqR cNEqdyNzCZ5SmpiDxLJZc8I= X-Google-Smtp-Source: AGHT+IE/nRAGW3TuLc3zOGLZH8EsqMxQyvoagnVPerL5NY7ZyUeIJkuWzsTdAHMsxGMYMFkK1OqeBA== X-Received: by 2002:a5d:4f8a:0:b0:317:5f13:5c2f with SMTP id d10-20020a5d4f8a000000b003175f135c2fmr5059956wru.0.1696026523393; Fri, 29 Sep 2023 15:28:43 -0700 (PDT) Received: from localhost ([2a00:23c5:dc8c:8701:1663:9a35:5a7b:1d76]) by smtp.gmail.com with ESMTPSA id x17-20020a5d6511000000b0031fd849e797sm22328639wru.105.2023.09.29.15.28.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Sep 2023 15:28:42 -0700 (PDT) Date: Fri, 29 Sep 2023 23:28:41 +0100 From: Lorenzo Stoakes To: "Liam R. Howlett" Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Jann Horn , Vlastimil Babka , Suren Baghdasaryan , Matthew Wilcox , stable@vger.kernel.org Subject: Re: [PATCH v3 2/3] mmap: Fix error paths with dup_anon_vma() Message-ID: <843f059f-dd54-4481-b46a-e87e56274db3@lucifer.local> References: <20230929183041.2835469-1-Liam.Howlett@oracle.com> <20230929183041.2835469-3-Liam.Howlett@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230929183041.2835469-3-Liam.Howlett@oracle.com> X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 58FCC1C0005 X-Stat-Signature: 3hmfpy4soat8u59a7b1yeyarpj8j557h X-HE-Tag: 1696026525-402738 X-HE-Meta: U2FsdGVkX18bRtEVpyx30wo0IvlKVBYyLqFstD4bHRnzJ77c5agiRHdB4WicfHCW7eVM+EO04mrJsDtCS+02GQjGNu9LsbJY+eyZlllMalg1y1ujYpfBhddA9WZ1/e5eg+E0QNmLXi0vTLYLxoZHix5l4osgKoUtceAyuEPg7SUgFHSESUx/Gi/Mu0p9LyepI7ZRuURCgrdFyGLN8xGkd2I8KNP/BlRSXf4xIjNoht7a4lQ/D3I/0hx5W/fPVPKvDQNSbdN6qFReGzb/vY6CHi3Wv2ASPtDfVTXTLhG3+D+aF9VePYMugUu/YLBDPDYWYrHxWqvvNetqXKliPVdl8tafwbvYimktajiB9xH0gURI31UVM2gSEa/4/ujJBUQs4xaUoeEga7uZ5upUECY8THHgR/k4npu6FKSheBQxbFh+uDx/u7QQ9rYpdT1UVmjI86oAKwxNu6ym/arlPIWO+W07Zmc6dFX2yFZtDwfTBQ3BfUj+QExoc5KbQTXwM048ZgewgCef7LZsdudH2Og5iXAqa1gmVT8q8n1Dltq/o4dS5pO5/Tg1S+IFV138cs/T/oQ75svn+wbbYDpDK0Gvxy+FBlFq6TWYWMdQZ64Ga7LCCC+0K04fy6hr2SawJeGEi+amcKNX8g8z//NJ68kLLGxwJ4/iPpzD2qLvEN9NO6eCTO29YuLTEhJiFKnOTbH8gqYuB5PYtXK6Es8TorEdv85nYKR3nqPHii2YKX73n/vgJraQIzxG9vHzufOLvhIRylaxzaN3DFSw0q0Jn+5oaA/pWktOMMKnSlt4+L0miCpAKLxkY9yim5T0fW27RGnGLM7i/we8p5RAPgOLHdI6QkUDex/jkJn8+j7pgVVjmyVQ63U+MWgyHOKD4obePJPwJSpkzYX/GzJpYZf92Uf4HDZA4oCXhv20fYtmM/1UKJzEehZGNQxDAl2Jg9QkMM9Ubu4D3B/netcuFIYoeA2 QT0JfhKc r/5wKnWk+0MShWjJTFdL57rcX354zgj04zN77QOKz+53fGyub6RkYhy4ireiiVBeDYAW+X9t9+f1mLQ5LqhWCUllFRNOdluNAG/grUz4+xraZSgWf7WegnBi9Aw4K4kabwcLXNLFfVfplet4QGbHdoqA8Sh5reYPKsdeRQbFTqTQSoMLwohdy2CjH53UsWoM4JoBmvUOsI6FIBdEzJe9LhRxyfXGglo/UmSlenY3X2CbZ5jWSdrymGHVeH4GImyc+Gcp/0YTJBcwSwQs2dKO7Nexss/g6D0rTx/VDiSw7WXEniuMgUOzoWgTiP51f39tcFK+IXuQ3En7a3aPweEVkG5uMSwyMPoFxxhGtJ4r2/JZftjsmtyqBN2lGu1IxIKjTXuO9l0EpnlAfIdzg1U0ysQY9JZypcoo0m8RJfPDP/C3+mSt5H1IyvwDXJIgQv7AA6zupT3hZ/cj64fl353BeOYyExkzPmU/k314W+kqqpu8xawPY87sb/NfZAL5aylYUMDSEHW9QamJD7Pl53WwlxRpL6WO0VzTpTebwwT6DmhZwMOiGjlA77DrQR8N+rqKlOCxa 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 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 > 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. 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