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 AE95AC3DA7F for ; Mon, 12 Aug 2024 14:30:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C72BC6B008A; Mon, 12 Aug 2024 10:30:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BD4706B008C; Mon, 12 Aug 2024 10:30:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A751F6B0095; Mon, 12 Aug 2024 10:30:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 857406B008A for ; Mon, 12 Aug 2024 10:30:14 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id EDCFF1A020F for ; Mon, 12 Aug 2024 14:30:13 +0000 (UTC) X-FDA: 82443828306.04.903E114 Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by imf01.hostedemail.com (Postfix) with ESMTP id 1A4924000A for ; Mon, 12 Aug 2024 14:30:11 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="MkZa/MTx"; spf=pass (imf01.hostedemail.com: domain of jeffxu@google.com designates 209.85.208.53 as permitted sender) smtp.mailfrom=jeffxu@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=1723472943; 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=UgmtalDDpDr/MN1t4TqgaC8ETll2KaDq52qckxLfkss=; b=Ont23ID8JohhD4FIx3gphxYJqSbKvhY24RvgF6jX+jxjgDNFRGY1QAQ5Egd0ONgF9ZnVbN PbrF/yHRKzypYDoP0R2Laki2zG4f6kPEOVHHQ4JbPY9ICzahTt3nWl7PTr59t+oQjhk3P7 HsFMmPg4Il4nM/GBILVgGTLQYmOPm7s= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723472943; a=rsa-sha256; cv=none; b=7UcQQFUXB4Lr5AmlR8A5BNLTNVzAgops0F9SDWdMiA9cjjWxPEBh36NQvg3eeEzFU0Uj5e 2YjUybHaCTZOvCTtXvMqPj36/gy9We94e1QeiUJU6ypIS3AH3ucj4AoHs9jO7t8r4pU72F KrVwx2h3PLATOhuYHgy8SiYqjEyOTsI= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b="MkZa/MTx"; spf=pass (imf01.hostedemail.com: domain of jeffxu@google.com designates 209.85.208.53 as permitted sender) smtp.mailfrom=jeffxu@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-5a28b61b880so14566a12.1 for ; Mon, 12 Aug 2024 07:30:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1723473010; x=1724077810; 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=UgmtalDDpDr/MN1t4TqgaC8ETll2KaDq52qckxLfkss=; b=MkZa/MTxGOIHgijx6vJ18VxD63lWa/wWYypVmQw2fF92ZBmoxz5zSWN9Cy8uLd/6Ez Mni5HAvzmWsKz2hKDhsALGXr7yGm4EfI+7Vp5FnnyJd/wgJSze2c6o124XuPi89kWGCB SI8ctmWEwZQy52UDJaoXpYyceUkYkz0e9gbndD/R7KdGNWMoykh+KS6TyGDTNg+Tp04S Ln24au7oxb7tBnN3TDTD/LIiM34Y16KIs4Gjittgi2N6nh29ShxAl5acsx2txD6hwB9Y HUh8WILNieU88VdY8f3USBKNeEjrxSoMcHCZf1R8BGChWxxKXb/NAsjsJUqne74lhiVq EhMw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723473010; x=1724077810; 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=UgmtalDDpDr/MN1t4TqgaC8ETll2KaDq52qckxLfkss=; b=tYVSDEdL5B4LM2uNgYrTLh49J06JzaMv512+gDUtUlHwovVBhUMHDds3S4NMybyQfn VoJHgJz8mKwFMhRpbPAE1s7Fjcy01vHLMkrJoHPwLUZpxzZL5APPkn02qiA7i9Ms6IJQ ZB4QDbiSufKxs4h53/4crZKauJZUG+2G62XCCGu9BjY2x5FFyZyaLMk5yG75w7wgkwKd j4Tubk0V9lj8wXs6EY5kH/90CBpD78yUmE83hNCYcs26civD8Twl+SM8Xjmi/FhwVxKB ZduqvQLrYcdfv3HdR4WSsiS1dbnTS+buCg3WEGvf0B5ONCXSXmUKRGn3xQK8txqR9kim YuFA== X-Forwarded-Encrypted: i=1; AJvYcCU8p4eNO9esVXOM862DQbwnO/rVOTtMtSrKRaeVE6lncRaUQEAM+XZbmWnQcGv2F5pgJmHiwt/Eq/Lh1iByaWXFZZo= X-Gm-Message-State: AOJu0YxlvMckMhR6Upyq1ws7mE4fHKjxZVMWZH3Hk9bYWE+7mMvWXIw4 6gY1MfmTTWHbQnF/kayQ7UL1JxiM1MI57ShVH20/FJTFw0nnJHjuvyQReUHVqeiqF/JUYMVRnu0 ZkJZwdS54Awe/yf/s3PGaGizh5FBlP/w6OpOq X-Google-Smtp-Source: AGHT+IGRJXeBEjboJZFkfR+yCIyyvKdUM/p3cp8P7yhd6aGpDPBM0Xw0RiUE2SdGQ77/srGD5rh3AQKZhRUMoF/azP4= X-Received: by 2002:a05:6402:27c9:b0:5b8:ccae:a8b8 with SMTP id 4fb4d7f45d1cf-5bd1b3d0b41mr208907a12.3.1723473009940; Mon, 12 Aug 2024 07:30:09 -0700 (PDT) MIME-Version: 1.0 References: <20240807211309.2729719-1-pedro.falcato@gmail.com> <20240807211309.2729719-3-pedro.falcato@gmail.com> <3hzwtm7jw25ng5gemkp42k5ypkfky25fxeevccnk2d6gcpft32@qwkwofgauqna> In-Reply-To: From: Jeff Xu Date: Mon, 12 Aug 2024 07:30:00 -0700 Message-ID: Subject: Re: [PATCH v2 2/6] mm/munmap: Replace can_modify_mm with can_modify_vma To: "Liam R. Howlett" , Pedro Falcato , Andrew Morton , Vlastimil Babka , Lorenzo Stoakes , linux-mm@kvack.org, linux-kernel@vger.kernel.org, oliver.sang@intel.com, torvalds@linux-foundation.org, jeffxu@google.com, Michael Ellerman , Kees Cook Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 1A4924000A X-Stat-Signature: xdqh4jshbprpa9s144jmqrfpagjfwb4s X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1723473011-503142 X-HE-Meta: U2FsdGVkX1+2T3TGGmgUM8uezJ+zNe2TylzEB+yHxh2BaylhQKIlWbq0IycyUycuieQR+/WrAvOv12riLfNJpAIS/mK7vACJw2HmKe0ooEoN/ef1mvPb+BKHhupcyLr9nl9OyQSeL45t04rk+w0zMO6KO1R10pkj4D4biz9464Z+QuMTrs3N3oWYoHMRPr3VHFGhBYNkKkvpMVzgSuAb6TmhlbCzum/w+S3WUPbXxi3NKDictWnCLdP11oZPK0eZYV5w4KChqfkaVuxf7Gu8zdTbrAXpM3Q5H9gaDP3KWBCIAAVNkiSpO7g+ZlO1jME2IeA8FLknOy4KTSEAEpNEOf5awjSN4XsApGe/B4/lUjCEdY6r6VrubUe6U6w77SAGp6f5TXzQgeaDurd3Q2YgIiWIzxKb5UKOUTP2P5VzqrBP1ghoNXqhoNphdlkVyuKck4bHc3zksBcAwEzlyOEI1XF/n5CtRxExspWm+Stpf9ftGP729dMs7Y/MPt4NIXWNd8mpysYjOgRF3CHkBPxV7oFP90g7fekuu7/tOBOpFim8gIumuH0psacl0iooJfdD9iSA2fiNbNh83T/1AcmW63x/xzkMU3w6Q+aAF5FqAjXON+XyVlfnzHCBHs43vO5IbLF8YycGSx+oJ8IxvSkUgOxG5hZQZjcpv8O5mTTuF9QWUt28oQhqKcY0P+eQakJPbDByMb5ae5jQIr1biLRPYWIm8PBMZxXMkO66SB8NHDaLgVCRKU4Vt+xgT0tOtR+/56OCFfV4O1uExmFWXGzW3yb6KwWoAgYx/w5Cu68sYD+Cc7sOevYEMJu46iYzQ5pOalQYlxnZeRdM168sqGL63+wcG206BE/E0152vxSqae21BZlGeHv9vJkf+CvZiK9XdTB+vWnAzrRq+zmhas50AJhPdDKr+JDvWaDWpAc8zT9gAoEbTY85RdXVFReudqtbIHTaqrNxRo2Me2RRiaQ DUmPlXm+ MOEAWptGbnshXblwWLNsQSP9IHIZhIz9hbijV6qcdVWpZ+f+eLJKeFLdeXpQaKEkIJWI6mUzfzSkTKhbttLeFj0sEgVJ4fpdY6ixz7LeRwpQPd0cW+TGEJQ6g74hH4bx4u8QbiNDy3ap5w0E9hz0cWYBRnPJ2G6oEKSQ8PP2DtADfGIhugRJgSPxaT4nhVCYrGOxednMYHFhpVBXIXoI5pmx+N32YjUC0wfSJZNP0eTEMIsQFJcoUHIbg5JpLAU/xni7gMNTaZE4Zapmodq9BkE+Vzr3ao/I+yim8m2nqJxG8vm5xyXsQQMeg/oTXUli5vkXhsxJmEaQtdyE2vmk/eZkXvg== 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: List-Subscribe: List-Unsubscribe: + Kees who commented on mseal() series before. Please keep Kees in the cc for future response to this series. On Fri, Aug 9, 2024 at 12:25=E2=80=AFPM Liam R. Howlett wrote: > > * Pedro Falcato [240809 14:53]: > > On Fri, Aug 9, 2024 at 5:48=E2=80=AFPM Liam R. Howlett wrote: > > > > > > * Liam R. Howlett [240809 12:15]: > > > > * Pedro Falcato [240807 17:13]: > > > > > We were doing an extra mmap tree traversal just to check if the e= ntire > > > > > range is modifiable. This can be done when we iterate through the= VMAs > > > > > instead. > > > > > > > > > > Signed-off-by: Pedro Falcato > > > > > --- > > > > > mm/mmap.c | 13 +------------ > > > > > mm/vma.c | 23 ++++++++++++----------- > > > > > 2 files changed, 13 insertions(+), 23 deletions(-) > > > > > > > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > > > > index 4a9c2329b09..c1c7a7d00f5 100644 > > > > > --- a/mm/mmap.c > > > > > +++ b/mm/mmap.c > > > > > @@ -1740,18 +1740,7 @@ int do_vma_munmap(struct vma_iterator *vmi= , struct vm_area_struct *vma, > > > > > unsigned long start, unsigned long end, struct list_h= ead *uf, > > > > > bool unlock) > > > > > { > > > > > - struct mm_struct *mm =3D vma->vm_mm; > > > > > - > > > > > - /* > > > > > - * Check if memory is sealed before arch_unmap. > > > > > - * Prevent unmapping a sealed VMA. > > > > > - * can_modify_mm assumes we have acquired the lock on MM. > > > > > - */ > > > > > - if (unlikely(!can_modify_mm(mm, start, end))) > > > > > - return -EPERM; > > > > > - > > > > > - arch_unmap(mm, start, end); > > > > > - return do_vmi_align_munmap(vmi, vma, mm, start, end, uf, unlo= ck); > > > > > + return do_vmi_align_munmap(vmi, vma, vma->vm_mm, start, end, = uf, unlock); > > > > > } > > > > > > > > > > /* > > > > > diff --git a/mm/vma.c b/mm/vma.c > > > > > index bf0546fe6ea..7a121bcc907 100644 > > > > > --- a/mm/vma.c > > > > > +++ b/mm/vma.c > > > > > @@ -712,6 +712,12 @@ do_vmi_align_munmap(struct vma_iterator *vmi= , struct vm_area_struct *vma, > > > > > if (end < vma->vm_end && mm->map_count >=3D sysctl_ma= x_map_count) > > > > > goto map_count_exceeded; > > > > > > > > > > + /* Don't bother splitting the VMA if we can't unmap i= t anyway */ > > > > > + if (!can_modify_vma(vma)) { > > > > > + error =3D -EPERM; > > > > > + goto start_split_failed; > > > > > + } > > > > > + > > > > > > > > Would this check be better placed in __split_vma()? It could repla= ce > > > > both this and the next chunk of code. > > > > > > not quite. > > > > Yeah, I was going to say that splitting a sealed VMA is okay (and we > > allow it on mlock and madvise). > > splitting a sealed vma wasn't supposed to be okay.. but it is Jeff's > feature. Jeff? > Splitting a sealed VMA is wrong. Whoever wants to split a sealed VMA should answer this question first: what is the use case for splitting a sealed VMA? The V2 series doesn't have selftest change which indicates lack of testing. The out-of-loop check is positioned nearer to the API entry point and separated from internal business logic, thereby minimizing the testing requirements. However, as we move the sealing check further inward and intertwine it with business logic, greater test coverage becomes necessary to ensure the correctness of sealing is preserved. -Jeff > > > > > > > > > > > > > > error =3D __split_vma(vmi, vma, start, 1); > > > > > if (error) > > > > > goto start_split_failed; > > > > > @@ -723,6 +729,11 @@ do_vmi_align_munmap(struct vma_iterator *vmi= , struct vm_area_struct *vma, > > > > > */ > > > > > next =3D vma; > > > > > do { > > > > > + if (!can_modify_vma(vma)) { > > > > > + error =3D -EPERM; > > > > > + goto modify_vma_failed; > > > > > + } > > > > > + > > > > > > This chunk would need to be moved below the end check so that we catc= h > > > full vma unmaps. > > > > Why below the end check? I believe we can avoid the split? Is there > > something I'm missing? > > But I did find a bug, what I really seem to want is: > > > > + if (!can_modify_vma(next)) { > > Good catch. > > > instead of (vma). It's somewhat concerning how the mseal selftests > > didn't trip on this? > > the end check will call split and will fail in there, if you move the > code as I suggested. > > That means, if we aren't splitting, we still have to check the vma, so > the check is necessary. > >