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 DE143C021A4 for ; Wed, 26 Feb 2025 03:57:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3EB7F280007; Tue, 25 Feb 2025 22:57:39 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 39A63280001; Tue, 25 Feb 2025 22:57:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 261FB280007; Tue, 25 Feb 2025 22:57:39 -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 07011280001 for ; Tue, 25 Feb 2025 22:57:39 -0500 (EST) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 7FA1F1A0915 for ; Wed, 26 Feb 2025 03:57:38 +0000 (UTC) X-FDA: 83160736596.14.7702079 Received: from mail-qk1-f179.google.com (mail-qk1-f179.google.com [209.85.222.179]) by imf12.hostedemail.com (Postfix) with ESMTP id 73C8E4000C for ; Wed, 26 Feb 2025 03:57:36 +0000 (UTC) Authentication-Results: imf12.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=ojNKmwZ2; spf=pass (imf12.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.179 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740542256; 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=GezmTNX8+C8WiOApS4hZZNlcHuSlvsEOcsQtDlYSxss=; b=CWJshZawsvN7UzbfMTgkdiFfGu4GCBbxk/LvHvjKkvMqZ3ZwZQncbDCqnuqRCgzTZO9B3W quQFodTM/Fzh3/QZtIDuSToBHB9RWJbkFbaO8stm5Dv+vlui8lJKWZ9ngHqN90nWFeS5yi Z13T2KeOQ0T0i5PazIROYzCIWB8aVVs= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740542256; a=rsa-sha256; cv=none; b=W36k2dp9ao/cxj7aX5F5d5J3H1a0fCdSYG6ayK7BqKUEviqNGdAGq1J0sEBymDkJpO2TvS sQ7SNIqTXrUF3u68EAgsQOBYJE3s6SpUvVCrCWvUh4m9KTpD3ZdnieFQ98gyb0SzCKRKc8 Iz5ZF4vU5Z7gvmaQS6WhMwQHGjSMk6Q= ARC-Authentication-Results: i=1; imf12.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b=ojNKmwZ2; spf=pass (imf12.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.179 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org Received: by mail-qk1-f179.google.com with SMTP id af79cd13be357-7c089b2e239so52557685a.0 for ; Tue, 25 Feb 2025 19:57:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1740542255; x=1741147055; 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=GezmTNX8+C8WiOApS4hZZNlcHuSlvsEOcsQtDlYSxss=; b=ojNKmwZ2Jw7caMTDmPOcDiuDyjMOX5t6tPt0qPLEumWruukgbQfn55A3InqZzJTFXM DZqF7aGeaS93C1pyg4LE7ErbpsiFWpYlTUih6++pcU2RjdWJlrrf1CGVMDyG5YP+Ascc KwyMTK3iDEefkafJk9TxaqUW1g+wlXLhpsjtynzuRkn3lXUBIIuJRGocUPyJZoioKzP2 FOMt98PQuV479NA1lE//C6mzHZGkE+iReNxiOzKsIDEPL7jsWGITQxrAbA0A58vTtTB0 N4ENtV9fhVC6NlNtVHqSh+UByKTEjGRmm9wP+hni+tN/3l3MCH5usqCHNUfcV4Zu9wfm RRkw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740542255; x=1741147055; 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=GezmTNX8+C8WiOApS4hZZNlcHuSlvsEOcsQtDlYSxss=; b=T7hQOkBYb9/UlIJ3sNb8LE5VQCiKFoMq20o+DkVduPFSsl2MsD4Wgw2NLCB1q56uTQ 9dEM30k+XcTZdI70ni3WvsGSKKBoC68So5VSJwVNnKKMWfeC+QEdxEcUr7/w4B9oMloN MeZLBz3zOawmoVBgQxfWcz5t+PRX87D7hQg5zAK/rfoHBtRWFPsXFsyJpNc4Q/h4k/Um E4ozafbMsn30tuTFefxVGBQf5B8AY4WnmLJIK6CC6n1YA00yHParg7AmCQoErJVR+t2h RIZeBz/qlWiCCPHslHhd0rhl/5LhhwwHc8amFd58FXoRQSUXyaQMuNTpSTSwriKhtug6 4eBg== X-Forwarded-Encrypted: i=1; AJvYcCUF2M2LT3pfSDbZhxG0KP2246wbrCHM/goWZ2cScw3DtAHsxNChht60x9rUq+xnKzNVymrV9B+XKA==@kvack.org X-Gm-Message-State: AOJu0YzJWrjx7Gb9RTXtg5x+bXxgdxt8jZhjvJk0sYZjrzuN4TtSfLmi 8HQ/ubVx+IQOTLixa+8NcaRD2SDwVscxOIqMzrqQvcKFBGNjtCVlxZC/tacoLWs= X-Gm-Gg: ASbGncsir0skmeNhRYuaqGj/Fb0AZLKPwrlmac5xYEHg9lyoxhqu0ky+PSMdNEYDQdg DypyQXjOzhdNwEMXNKSlhg2itiqeOkXQNCHa2aDV7AJ7Cv4mqxiAABuO2kPKI1KlxOIz4+8YvCl Nd80W+7b1HVyN15/CJeRnGpzPBHYSdUEYps1zPOnJrUSWxOh8tEo+trIViX34Cb/aMG96o4aQmf I+2FblxtihaabRz9IK0qr7v5ngmza+G/b/ZAsFDotDMmLiTG1FBaUqRoB8zBt5OJhJprzovo0fY OFiB8HO++hfxO3JasBuz7App X-Google-Smtp-Source: AGHT+IFxVs7WaOj9SrpMU2Qpcc6TtLkLZ68EjS+bv1tuwu7c6k3k8VtnWZ6bWvllrdQzwoexERXttw== X-Received: by 2002:a05:620a:1a8a:b0:7c0:a9ee:e6c1 with SMTP id af79cd13be357-7c0c2190e26mr3207066685a.7.1740542255247; Tue, 25 Feb 2025 19:57:35 -0800 (PST) Received: from localhost ([2603:7000:c01:2716:da5e:d3ff:fee7:26e7]) by smtp.gmail.com with UTF8SMTPSA id af79cd13be357-7c23c2a5bedsm193006685a.26.2025.02.25.19.57.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 25 Feb 2025 19:57:33 -0800 (PST) Date: Tue, 25 Feb 2025 22:57:30 -0500 From: Johannes Weiner To: Yosry Ahmed Cc: Nhat Pham , akpm@linux-foundation.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] zswap: do not crash the kernel on decompression failure Message-ID: <20250226035730.GA1775487@cmpxchg.org> References: <20250225213200.729056-1-nphamcs@gmail.com> <20250226005149.GA1500140@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspam-User: X-Rspamd-Queue-Id: 73C8E4000C X-Rspamd-Server: rspam07 X-Stat-Signature: csffif3i5omodonbgz5wzoxrwe8o8ofq X-HE-Tag: 1740542256-882921 X-HE-Meta: U2FsdGVkX18OmqdtsFRE6DZTAH7kTjB5iJFEu4P46/Iy4F1yiW7021OiB6HjFQg90lZEMTZvrS1mWVnJ02BsEB1QGQ5uPwSdvN0mKG/AnYRSXMskY8xg6rH2OYv/Kcoyl6+YF2j50dytdl9REB5h0okh3CThLRwrBQZOkmQqd6CBr6jWMXh3ic6wqbiKNrGVDskk4N1OFbfMk6/uUq7MmT5yyuTWHEiiTSb27eLUAeDRh1LmQpM7QO2tsiO01Rn5HyliZgJp48HI49GiVlXK+ic4+XZdEakZ+9+bT7VnnmBwAVW/ZZj9xGx6LCdro9UMniOb4mlRwL5stsBT/jtcTKwzV9zMcuRVd4HP3rLNABhZl07QT5fzyhedv6o9+gmhMikPsLpFCOMmD4gvrTvPnTiushGTVlz5cIkdy9xTI15arzjXqlyAlz72Ky5My5v77BQgQHC9zm79Obs+8KGTnkoJAoFLuLIQFRSLh2FuRV8ADPrRBSubaq3JlT4DQepamrweR1F58tQbNm6Dzfxk5zRZhlFs5h3z2V6TZK6kKlinsf7b7tXhdDeX+i4jFc+BGHFZtLOnH5Wb46uSMP7AD/dKy80TbqFLLR4AHZv1r0HAV68FUghYD2E64R+UBeIPuXHCqsq2bSVicPKbvx5Bf+OgyswFWmqUHyLyuPXuMjlM82glq7QeNmiEwKY/aNfmdAby+mM5fK4lgULWpsb2hvWArkqPDZ+hmqI/KTOFzhEyJOXC4ohDguME80eiI3UXJ4lt6Rbt9QsFmyz7rxsWpE6FA2xZW6e6MatKXXUnO712JKlujbQ2x/miRSL8RUKEj7vG5U+3wnpzLCwEOiLWUjFN3WboSX+ChG06WukzDF1XbocPwoffdTbjRimep3GhsHrmlLumClwaxVM5IrM6VksAFdecwVPjY0bxe4gk6WHvGXBs0M9oBLvtSodhLVyAvak6PSDGspmhKA1gwYZ 5xO+yx8q HhwLDYaPdaKgI5u5PlT5ZdRDkcwsifg3nMmKM2WsL9KplSomWHMk+K2Pjai3xQz/V+gV437webdpY1seG+HHuyKaf3Q189Bnu8Lg4s/Us86wApl9gxeJ6Ve+cxQa3eFUrQYNvb/By9CteTaPxYMvDHdzlKPS5b0iVODWMcFOqzDvwVm6fvOtzAwoOLDlc++niar99dkQpLaKFv8AJ+01QyWyA0dkYB/NoLnGuoUU/uBO68eAJaZ+WTqBo3EYhOlFf/3CaKv2Jv++naDkVO+peqDl31hDInjdxfMZ5kyiSaDPnKD2VWwa44NttA1SCywqmT95Uo2a+yAFsd+Z8HbHy7V7rCfRAVJ7xChOOA8/28kTwTP0VTPQGUt+XYGtDIHnOvEy2m7P9maMCGt51qUhA0bSsGEtgn28JhYSLccVCMfVBOE47CUpLt1AlE5yVH41DCm6aOOkn5VqPQ2hb69seUXH8YQ== 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: On Wed, Feb 26, 2025 at 02:45:41AM +0000, Yosry Ahmed wrote: > On Tue, Feb 25, 2025 at 07:51:49PM -0500, Johannes Weiner wrote: > > On Tue, Feb 25, 2025 at 01:32:00PM -0800, Nhat Pham wrote: > > > + } > > > mutex_unlock(&acomp_ctx->mutex); > > > > > > if (src != acomp_ctx->buffer) > > > zpool_unmap_handle(zpool, entry->handle); > > > + return ret; > > > } > > > > > > /********************************* > > > @@ -1018,6 +1028,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > struct writeback_control wbc = { > > > .sync_mode = WB_SYNC_NONE, > > > }; > > > + int ret = 0; > > > > > > /* try to allocate swap cache folio */ > > > mpol = get_task_policy(current); > > > @@ -1034,8 +1045,8 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > * and freed when invalidated by the concurrent shrinker anyway. > > > */ > > > if (!folio_was_allocated) { > > > - folio_put(folio); > > > - return -EEXIST; > > > + ret = -EEXIST; > > > + goto put_folio; > > > } > > > > > > /* > > > @@ -1048,14 +1059,17 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > * be dereferenced. > > > */ > > > tree = swap_zswap_tree(swpentry); > > > - if (entry != xa_cmpxchg(tree, offset, entry, NULL, GFP_KERNEL)) { > > > - delete_from_swap_cache(folio); > > > - folio_unlock(folio); > > > - folio_put(folio); > > > - return -ENOMEM; > > > + if (entry != xa_load(tree, offset)) { > > > + ret = -ENOMEM; > > > + goto fail; > > > } > > > > > > - zswap_decompress(entry, folio); > > > + if (!zswap_decompress(entry, folio)) { > > > + ret = -EIO; > > > + goto fail; > > > + } > > > + > > > + xa_erase(tree, offset); > > > > > > count_vm_event(ZSWPWB); > > > if (entry->objcg) > > > @@ -1071,9 +1085,14 @@ static int zswap_writeback_entry(struct zswap_entry *entry, > > > > > > /* start writeback */ > > > __swap_writepage(folio, &wbc); > > > - folio_put(folio); > > > + goto put_folio; > > > > > > - return 0; > > > +fail: > > > + delete_from_swap_cache(folio); > > > + folio_unlock(folio); > > > +put_folio: > > > + folio_put(folio); > > > + return ret; > > > } > > > > Nice, yeah it's time for factoring out the error unwinding. If you > > write it like this, you can save a jump in the main sequence: > > > > __swap_writepage(folio, &wbc); > > ret = 0; > > put: > > folio_put(folio); > > return ret; > > delete_unlock: > > (I like how you sneaked the label rename in here, I didn't like 'fail' > either :P) > > > delete_from_swap_cache(folio); > > folio_unlock(folio); > > goto put; > > I would go even further and avoid gotos completely (and make it super > clear what gets executed in the normal path vs the failure path): > > __swap_writepage(folio, &wbc); > folio_put(folio); > if (ret) { > delete_from_swap_cache(folio); > folio_unlock(folio); > } > return ret; The !folio_was_allocated case only needs the put. I guess that could stay open-coded. And I think you still need one goto for the other two error legs to jump past the __swap_writepage. > > Something like this? > > > > if (!zswap_decompress(entry, folio)) { > > /* > > * The zswap_load() return value doesn't indicate success or > > * failure, but whether zswap owns the swapped out contents. > > * This MUST return true here, otherwise swap_readpage() will > > * read garbage from the backend. > > * > > * Success is signaled by marking the folio uptodate. > > */ > > We use the same trick in the folio_test_large() branch, so maybe this > should be moved to above the function definition. Then we can perhaps > refer to it in places where we return true wihout setting uptodate for > added clarity if needed. That makes sense to me. Nhat, what do you think?