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 1ABF7C47DD9 for ; Mon, 22 Jan 2024 20:39:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8B1DD8D0001; Mon, 22 Jan 2024 15:39:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 861626B00A9; Mon, 22 Jan 2024 15:39:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7290B8D0001; Mon, 22 Jan 2024 15:39:56 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 635466B00A8 for ; Mon, 22 Jan 2024 15:39:56 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 2CF79A033F for ; Mon, 22 Jan 2024 20:39:56 +0000 (UTC) X-FDA: 81708113592.28.2791786 Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) by imf05.hostedemail.com (Postfix) with ESMTP id 5AD85100014 for ; Mon, 22 Jan 2024 20:39:54 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Rnn4EbaC; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.49 as permitted sender) smtp.mailfrom=yosryahmed@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=1705955994; 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=Qp43HjWs5gh7SYsr5bjGImR/GWKtUKGFeU/FebP+27s=; b=EAgKPiesVQct+EGMIYH8ztLFTC0ljSFMaXy9XyLqJV0WXV14fvA7WDKnOAFtvXiienIGVV Bu/IV8Brno211goKHYEZUtseiIgs7QjqDXIV13xxI1X4g9+c3CIoMbMTqGG3346t7fEHQj WobSAEEXU4Q4Js65oDXuo8ccft1l7Ic= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1705955994; a=rsa-sha256; cv=none; b=aiNkQYEiqbqAIy4W7uymXxJ0u2mDuOBnbgvXfkn4nE3a8lsn1vaKrja2vZh7O5dbvRPOAw S3iAb956tdabssWmzTRFcRQ7tR5SlZwZhDcl4oUvu2T1r+/VYfSWjlCJhIgXdS3tvxpp7z EmCbnJJBHMfPkF1jrHSBbRNHq0uP2Qc= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=Rnn4EbaC; spf=pass (imf05.hostedemail.com: domain of yosryahmed@google.com designates 209.85.167.49 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-50eaabc36bcso3787776e87.2 for ; Mon, 22 Jan 2024 12:39:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1705955992; x=1706560792; 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=Qp43HjWs5gh7SYsr5bjGImR/GWKtUKGFeU/FebP+27s=; b=Rnn4EbaCU/jDQJ7sLSqu+Wj+crxtCy1Jb3pmS7L0aRfWULhIOFIfvjKWenQPnoQNSS JT0qoiQniMRmgIlL0eEL+WqEHMh0P7iKkBWfRMW/AmiyKf+iuqE7+qiqZiXdUoKG+qMd OGle5chy2AG337EQF7fHT9n+6IkNpFRiyuYQt9KCCL8+8047aeujARqa8YsfqwSWTDQI /JhE/DPlB1rFJE8lTkTGQESBiaijnebho8mARFGOfcj08W30lUBwHsjQdFXsj+zeko6U +/TpKz9AJs1dqCOeyeG4YAr1/rcjN5DmtRJ72Zv+qu0+mPhRle10i3Pwixkz32gsHSXE etlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1705955992; x=1706560792; 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=Qp43HjWs5gh7SYsr5bjGImR/GWKtUKGFeU/FebP+27s=; b=UqwJ17GF08Ch7VI6yh7PlRaqfFcjOb18a7NxSnB3mOkMBnO9JYYGVgtMbftE0k57VL BHtbLC82B1wdSgNFNzdmRZBg3MaeomeNI1eHx3hohxAAMIYpOtIkvGKllyE25cXqJvST JMm1OSGjvBAqopJu/HFGenXKwsO2kUfe+w6zCG2kNvgLpFUKWrpGR1Q/lMRZUPMnoyMI CiT6CBHi/1FMmcCFkxvqaypOmhjLdqbt2pCmWJySjjDmlS/YMy9BEr2A+gOmwzTY/AOZ sZ1e14U7lE7xfBTWZjyhEjRlW+LEQS9PyZcaZ2jZMCq67o7gFh3UhqaCtn4aaAAbJkkn WYLg== X-Gm-Message-State: AOJu0YxCwvLZbNe2DE1cUjTnduihXNBDX/fmVwtW/F896yCxgzqE/VDj 86W8rh4fbF5a7qoOay3bgK6zDPq0gxgS/eM0v+dr+8MqaasFhgWVUJ09EY2N8FKPAadzm7HG+CR 36mCQ+nTq8Fq2lmTuRivXQlKtYd8S3u5dcPhA X-Google-Smtp-Source: AGHT+IGSsDHWdLNHJCP6f1D5LB5UE9M7XSsi34GZdGViNkgp8UDOP3/4akTdkpy0x2oX5uGwnP6Xo4jrukX7I3VaM6E= X-Received: by 2002:a05:6512:1c7:b0:50e:e888:2c3d with SMTP id f7-20020a05651201c700b0050ee8882c3dmr2184929lfp.25.1705955992458; Mon, 22 Jan 2024 12:39:52 -0800 (PST) MIME-Version: 1.0 References: <20240120024007.2850671-1-yosryahmed@google.com> <20240120024007.2850671-3-yosryahmed@google.com> <20240122201906.GA1567330@cmpxchg.org> In-Reply-To: <20240122201906.GA1567330@cmpxchg.org> From: Yosry Ahmed Date: Mon, 22 Jan 2024 12:39:16 -0800 Message-ID: Subject: Re: [PATCH 2/2] mm: zswap: remove unnecessary tree cleanups in zswap_swapoff() To: Johannes Weiner Cc: Andrew Morton , Nhat Pham , Chris Li , Chengming Zhou , Huang Ying , linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 5AD85100014 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: z334tygmous3abq9qpdwxxe4968dx95w X-HE-Tag: 1705955994-786003 X-HE-Meta: U2FsdGVkX18pmH/xLBKYfsbOU1b11CZKQ56lbofAzxHz4zbD8ux9bFnfivZ+WSg7pbHWvVZXPhznGdu2Cb0WDXZAC3bHGhsECA8Twsfsz6Mv3d+xw0Z0ORuejwDwShDJIJsVO5pjus8svK2I6UiC3uYkQg9305CulRd/ta4js1H8AEZd0vJfB3EjCkxMirUhAnBrLasB9E9I6D54M+4fKXKXdAji0hYQtpgGjZgszayFNMhC6koaTt/0hXHaV36ID1PKGVfHsITRWz/1rRog96Pk9Gg7FpdUyDKVuGB/OGGH0vQO+64rS/Qyu/8W+s7rY9yRTbNt7KaFpj4e0U5ViTEUs343WBUpqoP06GYk1FcKLWcf8dMVZGFjC+VAPtFCoEE17ZzeRGngsAdv0Vm+umY7eEYpGxod6LipxizVWWVxzh0OB2tXX8lLYVrnHJedwVvzjksJbA/bkv28eLnFw6zFeVz65KpRZpGDTovEk7y0mguiZ742230ZdjkOMDFFtgmIyPGQm+emzxlF0sk4qtpz/qEDBHcgArQhp6LeWBpRXwjBWpuVE+nLIq9DHl8zDQ8K/Lq2vjMJCw9Ts3DjkcOnXyqORC9iHlvn9f4yQharsIp2yKFRuj5tzOxPRNm8JhBGpBR7RsWc14UyLbkneddwP5oXcOzjbX7dTY5Q+6VoD8DRD7XP54/PyA9vzOl69tKzYfVdF7ajqTniKemhgNQY1reQSQDJOzs/IseodaJGKfIweqqvbC+MgNcq6WD+Krpf35wibxMpiXTZVXg6+rnWU/11z76AOg2nPoC62uRmQLnxfKqhadnJmcWO3fJNuVUBKhBzACzFwsYw64WI4CMVMYJCQLf21DPdvLy20LOsvuQZ4yL1U3Ckjr1j2OQQdV5QqJjRlUMsKFlPri3uD0eGTVzcWzqME78GJwAvIzffwfhXMjZcBJ40EYp+6giGpRTbp1JUxzqAWOVjDjd M8sWoVTp HC26RFJOnR0w4qLn3Hh3lZwPR5rHtuLkgYdLpobSro2prD6PF1dxnRG9C+Cbv1+1kXQ4ZdeVaMf8poDG2H5UUnJLfZLxcjVvstqwpJfPLGfzeU4UJcaZbteef3PeiVK0kEx/Qt4cVeAWOaCVo7k51XGw5wrSe2c5RD9GQ0mv3tQ0jCZMRQjcrTo1XiZr2TN9GILAkuLb+K6hfKfqzQx20ZvMX66trjM7yQXHdjg/lRDEmnAWEDSI0nPi5WdUw//S1wS5K3/u7wFKQgSCdvAIpjhMYr0iZ55MWQB7LjfIuMZ4u0e/EePPQinr/7HqUFGmy3pkrB7oHkANET6gg4gycFl4nfexNSWTwI6iT X-Bogosity: Ham, tests=bogofilter, spamicity=0.000002, 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 Mon, Jan 22, 2024 at 12:19=E2=80=AFPM Johannes Weiner wrote: > > On Sat, Jan 20, 2024 at 02:40:07AM +0000, Yosry Ahmed wrote: > > During swapoff, try_to_unuse() makes sure that zswap_invalidate() is > > called for all swap entries before zswap_swapoff() is called. This mean= s > > that all zswap entries should already be removed from the tree. Simplif= y > > zswap_swapoff() by removing the tree cleanup loop, and leaving an > > assertion in its place. > > > > Signed-off-by: Yosry Ahmed > > Acked-by: Johannes Weiner > > That's a great simplification. > > Removing the tree->lock made me double take, but at this point the > swapfile and its cache should be fully dead and I don't see how any of > the zswap operations that take tree->lock could race at this point. It took me a while staring at the code to realize this loop is pointless. However, while I have your attention on the swapoff path, there's a slightly irrelevant problem that I think might be there, but I am not sure. It looks to me like swapoff can race with writeback, and there may be a chance of UAF for the zswap tree. For example, if zswap_swapoff() races with shrink_memcg_cb(), I feel like we may free the tree as it is being used. For example if zswap_swapoff()->kfree(tree) happen right before shrink_memcg_cb()->list_lru_isolate(l, item). Please tell me that I am being paranoid and that there is some protection against zswap writeback racing with swapoff. It feels like we are very careful with zswap entries refcounting, but not with the zswap tree itself. > > > --- > > Chengming, Chris, I think this should make the tree split and the xarra= y > > conversion patches simpler (especially the former). If others agree, > > both changes can be rebased on top of this. > > The resulting code is definitely simpler, but this patch is not a > completely trivial cleanup, either. If you put it before Chengming's > patch and it breaks something, it would be difficult to pull out > without affecting the tree split. Are you suggesting I rebase this on top of Chengming's patches? I can definitely do this, but the patch will be slightly less straightforward, and if the tree split patches break something it would be difficult to pull out as well. If you feel like this patch is more likely to break things, I can rebase.