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 3183DC52D7D for ; Fri, 16 Aug 2024 01:58:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A09648D002E; Thu, 15 Aug 2024 21:58:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9B8818D002B; Thu, 15 Aug 2024 21:58:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 880878D002E; Thu, 15 Aug 2024 21:58:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 69C638D002B for ; Thu, 15 Aug 2024 21:58:29 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id E90BD1218D5 for ; Fri, 16 Aug 2024 01:58:28 +0000 (UTC) X-FDA: 82456449096.01.CD612D4 Received: from mail-vs1-f46.google.com (mail-vs1-f46.google.com [209.85.217.46]) by imf18.hostedemail.com (Postfix) with ESMTP id 2DC331C0008 for ; Fri, 16 Aug 2024 01:58:27 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XwlExRvq; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of pedro.falcato@gmail.com designates 209.85.217.46 as permitted sender) smtp.mailfrom=pedro.falcato@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723773494; a=rsa-sha256; cv=none; b=nxWJmsVB7OOT8gX9QoZ9QLw2Kb6OJOooqLhyUyn+jKtq9IpqBwemvBqAphqj9RI3hqOEAa 8ZQjvztqskKv1d+zycDofFfd2KFLi1wL1HRUoPGkIhwi74zrJ4WHaidVEYtFetY8BgI1Mq g9V0W4T8Zmtqf1UAJQ48YGADRBINlzQ= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=XwlExRvq; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf18.hostedemail.com: domain of pedro.falcato@gmail.com designates 209.85.217.46 as permitted sender) smtp.mailfrom=pedro.falcato@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723773494; 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=PAgZgdZ6tdR/kq3Id/eEPEbSkIxjDL9T60LA8o7yEqo=; b=jJhNm3A90+rOFxLWzhT1OSIah6fI8m2SNMS+I1hxvHUS5g7wYWl8GIOG3RXZ7WW8SX58AK JvMROAuYMitir691ShMcwPaQDf0s7aeWZXdw25CH5PL5tAbpNgrELAHJ0uMq+6axc0684c m/BPzJvks902Fhn3H1HDKMGZdEwvWTU= Received: by mail-vs1-f46.google.com with SMTP id ada2fe7eead31-49291b02e20so579118137.2 for ; Thu, 15 Aug 2024 18:58:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723773506; x=1724378306; 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=PAgZgdZ6tdR/kq3Id/eEPEbSkIxjDL9T60LA8o7yEqo=; b=XwlExRvq474+4/PmqCKTfyd15vG8G6tbeTy2grQLGhMBXwh40Pz9tXHitiAvX2kY9y 9dW0NlvF4+hW/lXjIxC9td1jVYEFC47oEGyzGTakvwml2CagDDwxroppSQiVNk6kBsR2 hxXOyKJr2D2EptaA2WqSWJcMeFSaQwwN4De5fm2MJXsTnq81sVXWmK3Xrh3vbwg7t7Lm dXhVgdqMfLS8bmGUby3zAnD4pZg1sYjRTENHOvHvPkhmyfkU5MVkR2JUcBICqXOSRob9 llRZmjP2gzrrl4+PGR/LN0mPphWy18rS0tD/+SSU4TLA5XAPQVc41tToFFvdUQFwGN1T 87yQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723773506; x=1724378306; 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=PAgZgdZ6tdR/kq3Id/eEPEbSkIxjDL9T60LA8o7yEqo=; b=jbFnZYBBw5dO517fCwUPi2L+5bbo1lVvGe72KrTIUPkW2cWJtfUjngiQLIaggxjZlT bZAwKjG3G1Lu5YBZHR3vvCmsC477i7CjfEMsDNXaLsrBA3MLb2VWDPzkD55ZX7ezZ16w pJOg5yNDkziTCwB3Kt9p3+bc8kjaNcZi03/y+Qd0d3CMvWAzDZe3mdbG5IgDvFERo3CT etJRSM9akUEqSU743wbVgTQ0iy6j7YgE07hr5tnIlSFnnW12eveCB+E7ciIdegL0k4pX qksCgTHGMXob5XmZ0xdxHwINAl7kHRMDx+rxNBkKA6CaIuf3te2zHzgkOfRdvO/MZyE8 jygw== X-Forwarded-Encrypted: i=1; AJvYcCVREYtx06XMfT39wArT+U0vGE0zDQ79FqCJwxWRYTa4EhTNfnmfATNHk4u5mBQC9qEJawrw1FTEcfPMQvjUV4M9L8A= X-Gm-Message-State: AOJu0YwTfviTJXe+I5FAEbJ2Hr5yePRUPEL+BzAamunPqEBGgVS3Jl6k H7CCVQ3PIB8tBXblQNb5gVBCFCzrvzimLj4syYQj2uqrg621951ZRI8hpLPDwviV0TpC3ygEoEV P8m34kJ1qMGODES1bVUHyUqnb++A= X-Google-Smtp-Source: AGHT+IHDASiYuhpMCbXxbDWnm1ksdVAopUItKTYEdpJTS1ry85XYBxIHA/3mE5dqFyDzBZwEUq+VzJdETWu207GyQvk= X-Received: by 2002:a05:6102:f0a:b0:497:5faa:af74 with SMTP id ada2fe7eead31-497799455b2mr2225033137.11.1723773505876; Thu, 15 Aug 2024 18:58:25 -0700 (PDT) MIME-Version: 1.0 References: <20240807211309.2729719-1-pedro.falcato@gmail.com> <20240808161226.b853642c0ecf530b5cef2ecc@linux-foundation.org> In-Reply-To: From: Pedro Falcato Date: Fri, 16 Aug 2024 02:58:14 +0100 Message-ID: Subject: Re: [PATCH v2 0/6] mm: Optimize mseal checks To: Jeff Xu Cc: Jeff Xu , Andrew Morton , "Liam R. Howlett" , Vlastimil Babka , Lorenzo Stoakes , linux-mm@kvack.org, linux-kernel@vger.kernel.org, oliver.sang@intel.com, torvalds@linux-foundation.org, Michael Ellerman Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 2DC331C0008 X-Rspamd-Server: rspam01 X-Stat-Signature: a19thpbri4zcyzhi63ywpg9j8zrzdhnm X-HE-Tag: 1723773507-354552 X-HE-Meta: U2FsdGVkX1+Goq9eIKrQuMn7PZggw5QtLJcRKyj9V3S6BcVDOqlGLjC6GbmS55tMFJIT8DPu08+G2uffHrFa8g6bpemvhIJvPCqFjFSMgNooZS83x2TEm4i2qL1Hsa6b2d7Anqc64sCEtEj5puZX8Aea6BU+1g1Ns3uP0rmfbOiDQQ9gOiFTiTBdXk/8MfPXi0ukkZhGHal/J9aUYv1J8BAdazDfSrh9xnA/QkT7+N4dI2D1Ww4dp1OfOmyEuwG/daRG9R4qyFfBr0RGxB2We07PZt9MT9XWO754uY6SC8q4vEjAwUozlkXptV28pEa3DKog02Yylzvy98POHcoS+kW9umMgLmXEyhUqBuXF1pNhDo961f4c9NlUPh+7RPJHouyWMMCh/rupPMOD5uWVcUYPOAJRGJj+qkPgwtVaQ9q0PsvZl+SNWN17WBtUOVtrHnhTXPMox2PMGyOOWuKQG31DNDcOO8nezWVGvfsGoW940lVdN0KKbxUWk8CEZyfIelufk7EdJ8xHkBO+ty8Ga5A8Kl5PJdI/wrPjKZ1v9DDaqVr0pbWGKL3OGWFvLLRmOzWEu/ArQ+ZBXhSQkWGlChbKAxshBciCeKBI0uiKGUFXGlrT+UfAmDjG/PhGk8AMduoAGmA3IjLSpQN4N2XuZeKV5DWYN+qFD3aUU/Qzb2Xa0dU1Xq0F3jGQHOTT970oIDNQ6EIkHyoPc/8W48sww1Dnrl4bAAqxvPESq5bPHIuqILG/Sa78dAk+hN2YxdKtMW4ru//JNgIhionzLJTehk+HIzoTC3zu14f+NeBEeW2gDHNUsJo7eC6bXEKX1NKUA0LNvinaM0wIb4nBhFKnvAepLuKsSUCb5Uf/aanZOB+tcLGxDlv1HlPf2PL92t9WTpgIH4E9AxDcMsrTjje8vfUlvT7LNhx/ovzWHX4ZZ1Kez/b80IbcOa3quPRFmvVH7/rGPZ383IS1Lz+Teug n9srstMi ktnjMNrDP97p4+U96/D0apaylspbbjDO0bEjpKvc+GtTjYzWTFX0vycUSrPuFLLY1uXk+ZY/L/YItavRxCW1MyWgOwpaagSLiw+Qxj9U7ZmgN34Ekk+7JjMGCZPenA+p5bVrFt7+tPwZhfC+sis+A1qxo5Pn+nfhyJqzGY3ohtbNU8h6jAUY6qdwTKWizP1OebTT99dOuSdle7lTCY+Ch1nq0C9Dq/nb+c7IuwYDfGnu3suUJ1e/j8uBGWCTOgiJBy+/ZsJ1h58aVdrj+6sp+vER5h+O6y4tgx0rrVVvK2dIMvbsnMd8EdgaENXWAtCp07z0E3C/xd5ddns0slnwRalRIy45uLLgylDKpRpygAIUv9cL+bfZMpYHA+5fmBN+IST7leEHvcGYlfq+w8zESS0TOZdT41q6MLDwlD2jqEzKSMnyEnTph6jRzXzbdDNaBHOrmpcXVsP2sYlRPC6VTntDM/AaJveirzRZJK9fHG3Pz51umWEdkbZKDzg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000514, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Thu, Aug 15, 2024 at 11:10=E2=80=AFPM Jeff Xu wrot= e: > > Hi Andrew, Pedro > > On Thu, Aug 8, 2024 at 6:03=E2=80=AFPM Jeff Xu wrote: > > > > On Thu, Aug 8, 2024 at 5:34=E2=80=AFPM Pedro Falcato wrote: > > > > > > On Fri, Aug 9, 2024 at 12:12=E2=80=AFAM Andrew Morton wrote: > > > > > > > > On Wed, 7 Aug 2024 22:13:03 +0100 Pedro Falcato wrote: > > > > > > > > > This series also depends on (and will eventually very slightly co= nflict with) > > > > > the powerpc series that removes arch_unmap[2]. > > > > > > > > That's awkward. Please describe the dependency? > > > > > > One of the transformations done in this patch series (patch 2) assume= s > > > that arch_unmap either doesn't exist or does nothing. > > > PPC is the only architecture with an arch_unmap implementation, and > > > through the series I linked they're going to make it work via > > > ->close(). > > > > > > What's the easiest way to deal with this? Can the PPC series go > > > through the mm tree? > > > > > This patch can't be merged until arch_unmap() is all removed (ppc chang= e) > > > > Also I'm still doing a test/reviewing for this patch, perhaps it is > > better to wait till my test is done. > > > Sorry that I'm late for updating this thread. > > With removing arch_unmap() change landed , there is no dependency for > the patch. However: I have other comments: > > 1. Testing > Testing is 90% of work when I modify kernel code and send a patch. So > I'm a little disappointed that this patch doesn't have any test > updates or add new tests. Which makes me less confident about the > regression risk on mseal itself, i.e. a sealed mapping being > overwritten by mprotect/mmap/mremap/munmap. I have posted the comment > in [1], and I would like to repeat it to stress my point. > > 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. Sorry, cut the crap. Your backhanded concerns are very funny. mseal was _your_ feature. You wrote the tests. You either didn't write or didn't understand what kinds of tests need/should be done, otherwise you would've done them. I wrote some code to fix the awful performance hit. It is a _fix_, not a feature. Your previous mseal tests should've covered this. If they didn't, that's okay (we all make mistakes), but pushing your problems onto me is seriously laughable. I want to stress this bit: There's no mseal feature, there's no business logic. There's mmap, munmap, mprotect, madvise, mremap (among others). These are the things people actually care about, and need to stay fast. Memory management is a core part of the kernel. All of the very pretty abstractions you're talking about aren't applicable to kernel code (for any kernel, really) in general. No one wants to pay the cost of walking through a data structure 2 or 3 times just to "separate the business logic" for a random, minimally simple feature. > > Yes. I promised to run some tests, which I did, with the existing self > test (that passed), also I added more tests in the mremap selftest. > However I'm bound by the time that I can spend on this (my other > duties and deliverables), I can't test it as much as I like to for > in-loop change (in a time frame demanded by a dev in this ml). Because > this patch is not getting tested as it should be, my confidence for > the V2 patch is low . Ok so you: tried to explain to me how to run selftests in v1 (when I actively did _before sending_, and found a bug in your tests, and wrote about it in-depth), pledge to "run some tests", never get back to us, push the "the testsuite I wrote is lacking" concern onto me, send a whole separate parallel patch set that tries to address _one_ of the regressions with complete disregard for the work done here, complain about a lack of time, and now say a backhanded "your confidence for the V2 patch is low". I seriously have no words. I want to stress I have no way to test real software that uses mseal because APPARENTLY THERE IS NONE. THE FEATURE ADDED EXPLICITLY FOR CHROMIUM IS NOT USED BY UPSTREAM CHROMIUM. > > 2 perf testing > stress-ng is not stable in my test with Chromebook, and I'm requesting > Oliver to take more samples [2] . This due diligence assures that > this patch accurately fulfills its purpose. The in-loop approach adds > complexity to the code, i.e. future dev is harder to understand the > sealing logic. Additionally, it sacrifices a security feature that > makes it harder for an attacker to modify mapping (currently if an > attacker uses munmap with a large address range, if one of the > addresses is sealed, the entire range is not modified. In the in-loop > approach, memory will be unmapped till it hits the sealed memory). Wrong. munmap is atomic and always has been. It's required by POSIX. I would also ask you to back these partial mprotect (assuming that's what you meant) concerns with a real attack that takes this into account, instead of hand waving "security". While noting that all of these operations can already partially fail, this is not new (and is explicitly permitted by POSIX for syscalls-not-named-munmap). > Therefore, I would like to ascertain the gain. The gain is real. I've proven it with will-it-scale, Oliver ran his tests and found the regression to be gone (and they do seem to do proper statistical analysis). I would wager you to find a workload or hardware where *doing substantially less work* makes for slower code. > > 3 mremap refactor work. mremap refactoring is not related to these mmap regressions. In the v3 I'm cleaning up and sending out tomorrow, I minimally change mremap (as Liam wanted). --=20 Pedro