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 CC068C197BF for ; Thu, 27 Feb 2025 21:46:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 59DD56B0085; Thu, 27 Feb 2025 16:46:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 54E6F280003; Thu, 27 Feb 2025 16:46:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 415F6280002; Thu, 27 Feb 2025 16:46:46 -0500 (EST) 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 211E56B0085 for ; Thu, 27 Feb 2025 16:46:46 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id CA9DF161D67 for ; Thu, 27 Feb 2025 21:46:45 +0000 (UTC) X-FDA: 83167059570.04.AE9DCAD Received: from mail-qv1-f52.google.com (mail-qv1-f52.google.com [209.85.219.52]) by imf24.hostedemail.com (Postfix) with ESMTP id 656B5180017 for ; Thu, 27 Feb 2025 21:46:43 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=OoHzBMY2; spf=pass (imf24.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740692803; 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=ZL8zzCb45AFuRkDUPLgML0Rmw+lZQRdAQIOYM3KKBdg=; b=4JtLZ4gjqZZ4Pu/LQnvhqYVAnz4ZIty9fhXcxXvIQQtCY072dn+vtKmKDw/uGRwbVFm6Ah UT8iujGP1GGC580eX/vrsc62tSKPoYJ9i6jt05AZZ4XNs55E636Tv4MX3FFKWDdil0KYoe a8gi0xU50ewLSr2WEOjf6ufCG8QaLXE= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740692803; a=rsa-sha256; cv=none; b=ftgDdbKZpGjuc8xdcDulgXATUFYsiCAnAadov1SUSJcHI7CCdDv9TqOkiL8XMzN9C6Hs8U nWt693zMoHC3TX3irY9tIMjaSVgtRRIEhkv9stx9k9nsKMVtA0RmA/j807NWlMiNAbsLrt TiqB6fK1/83n+jG3aolUAgwDEeDD3fE= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=OoHzBMY2; spf=pass (imf24.hostedemail.com: domain of nphamcs@gmail.com designates 209.85.219.52 as permitted sender) smtp.mailfrom=nphamcs@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-qv1-f52.google.com with SMTP id 6a1803df08f44-6e88f3159e3so12592146d6.2 for ; Thu, 27 Feb 2025 13:46:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740692802; x=1741297602; 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=ZL8zzCb45AFuRkDUPLgML0Rmw+lZQRdAQIOYM3KKBdg=; b=OoHzBMY2C8E0/qh8n7VULbB1ajhjPMCrFhfi7wF5+3TAKRqvKoSaci+mbgtvnjbuEA 6T0B9SVXBfzuAJxMBxNVq0H0UB3b4McFMtubPJXO83gkyd+sU4DQkgmtls+zsuiRvv98 nppxrZ83u3fjlrss0iW52eDc2t+gdoI7g+0BtZm5r5evAjFdDha6Zr0HLGA6LN7M8dKA /uHzK/5BpHh8QSwMjDaJLMxVnaeDc/+QbWh3iMByxOvXOXahlRaO3Zcwj9FYclRKKnmd TjLOy2s9gNr9g6FQYwbkqean3F9aSHBvXcqYnrQeIIoIpAOJ1UoueDOSebvZSY6HXaBv 80GA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740692802; x=1741297602; 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=ZL8zzCb45AFuRkDUPLgML0Rmw+lZQRdAQIOYM3KKBdg=; b=ZIFYAvDjusJ6+532ww/liKiKXgwWZwxjspWL/+McZOC+2PleaVC+aftsQGWmnOx8yw WI9ipd4VxX8HbBC7ZQMpTyLMj3l4fMJnExRlmp0+eSOmLXULaLwEOUQPMDTkh+nqMYxf KYg+hBq8mC3zPLsvU5+6hNQKwtAyxog6qtGORDGdROv6nTLX5CadW/DopoRrQlZ4BQXK TY4is6ISvuvHMMJUqmaKcwFrlXziupXpCM4P8y+6vYF0uNYYPvvjlCTZXhCb13LydZps GYZu5w4ILwtSkcHTHdfdUI5LLOqfza6Xc1Ygg5yvf7qIrPezH+FKbqqJS8/ZsJ+o7RUh FNXQ== X-Forwarded-Encrypted: i=1; AJvYcCWzgVZf9UAN3eZjJYvyP3rgsDLhDfvtSPZVSUCB+7kzdYyk/DSWDwNnrKjMS7WsLM0OyJanVVFOBA==@kvack.org X-Gm-Message-State: AOJu0Ywy3QrN/l8iryG2Jl868wbZVOGQQp+ePjHwCfjf5EoC3tsl7h4S F69CkcvyxFWc/EiFRpuU6HZc6AMIdhsTCk+BxEbLDwR3q3G9D6Zl4uDLUGVOY47wh12BhJbYoN9 p048rDFbNi6MsY1xHQ7vLBSvD1ug= X-Gm-Gg: ASbGncuwQdIJiP5HM0EYTj5mCitXNgVDpqGgvPGVUtxS+KXozLAirycuLoI61QDuKRq hJNXWHB8Yq3M0NKQrBjbYI31bvDTZy55ViHz8doxwMceOUuqilOUujh4419Y6K71pn78CE1/b9D Su2CNeG28PK5I7IbW75XX+Xc0= X-Google-Smtp-Source: AGHT+IHV1VCVzr58KTYPEJnYKfegFCFU4Fv7QG0TO7EOPnHQdPziAGdaYvachGBiZX1FWmX0w67aKJdxiVhWHOqN0Ys= X-Received: by 2002:a05:6214:e88:b0:6e4:f090:3634 with SMTP id 6a1803df08f44-6e8a0d8faf6mr14568916d6.33.1740692802283; Thu, 27 Feb 2025 13:46:42 -0800 (PST) MIME-Version: 1.0 References: <20250227001445.1099203-1-nphamcs@gmail.com> In-Reply-To: From: Nhat Pham Date: Thu, 27 Feb 2025 13:46:29 -0800 X-Gm-Features: AQ5f1JqIMDgGka-Xqsa3dIsLbDldvDQ8kuWf5o1kbPKDvT6Jex6OrJTRWbzwSI8 Message-ID: Subject: Re: [PATCH v2] zswap: do not crash the kernel on decompression failure To: Yosry Ahmed Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, chengming.zhou@linux.dev, linux-mm@kvack.org, kernel-team@meta.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam02 X-Stat-Signature: aicqgnoj9q68hzack93rd4nkmt3hczz9 X-Rspamd-Queue-Id: 656B5180017 X-Rspam-User: X-HE-Tag: 1740692803-54573 X-HE-Meta: U2FsdGVkX19Y+2nGIN2o6nc8+6O9RslB59mUtaS0dK3apWspPQS+FRwMh7nIjYL88VCMJMv99ZFuEchgyAQvg+1JOY+oCS0Rw2nbUNfFjx3Y/d+M8nyfNEYmKjJso+2o75Qyl5cZY797vYIkUTPUIU9TE6F/SaUKpwF+2sAqiXRIlIaR+wm5micK671WAwCEhApLobKRHR1gGFafRv+JfUWIbH9Whor+7MFzMka+SEsxHCeg7tQ/qv43psDlI5Tc4R9GAexulbqvshhW+e3JC1EP5qGbk3PPLD1WvDxrSC6FAnxkViFwYt+OTNGftfP7NpJ9Jvwo/WrjBAooNFGIMn+tiHckftHDAVGyfdUMx60f4yx5GCDSQs/tTXTTPt53R5rpktt49MjimDQLIQV1itn6y7m51UN9bFmbDrLrkZXy6bJYCwH9zZNAD0RymLsk38LBp1XNZ+qt5z2r+SOhHL7BlgjJFcU5z5lCyeYp/ZCpFwKwyTEQf9PhxxkR+bqOuUxJYE2DRkzrf1tGG9xCwXPbO3vT1gDokk19vh3EbCEfgsbqNa3cqd1Ur4iIcwMJzoQn5bLCZpQTJgWtTaLI8SJEDtwKDAgCUToRKK91+7RqO2c2O3eKoYRAxysejHPL8el3i4d1vnspBl8InoL/qVhRToZwD8XfhQQ60tf3Zg8H1zHEP+NYTtFD76HWSeDVIQe1eHG9Gl4zc15OKCGbdC3EHNUvDnLBoNL7gq8jKo39DO8ozB26vr9gdVgIF7RXO+HyRyXptqb986kEzNkUlc8p2Vp26mv8CijKKgFBONb/huQzRphAS2fjcZJLhnOU4IemwFLgoeDTyqpd1JM7ClDhfovTK2zz98T5NeHeLpJwOfDsYkqJelQaMWAf6Gj2d5igmQbTWcqhvfSne1eu+KRnylbOa+/IqwPG3/GpNi7xxhJoevVIZOOCbvVSY8QpwhF++g2zY/nqqPgGAqF v7LgN4iR DHZIwJfD/Z4KKUIElNR1moQZnSepsTcr2fCmPIms48LreRoVGF8olxdq+AhjD7Be47cd2mWLUMHmhV0CocEWu+BubJMrqjf7lxa9adM31hPk4lw3NYnXF9/yh0owDalELLlTrktfTrFEbEeA9UBEvy5pzec3ZMaSXJSLGrrKdvDvbN6vSkEJsn52gpUNCi7PY15ABX9R6Ye7QqOGEVEk0bETGPWJBzgiydFiuYCi3NN1TT998MG3H7DAqFafc7RyvwIe3VlGBukDI/B7EvhQ9cr7/K0j/aw6M8xD6gmsN0opKW8j3HzMRI43lvns2SWeJXZlEkjxOxDU0fTL7GM/LWzduyChAVCrXuK88Zc1q3O6AoO+lYLHYI6bdBblJmxk929LVMillORqrZLka3K0Kh9UMCgN0yJlt5Phh 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 5:19=E2=80=AFPM Yosry Ahmed = wrote: > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > Currently, we crash the kernel when a decompression failure occurs in > > zswap (either because of memory corruption, or a bug in the compression > > algorithm). This is overkill. We should only SIGBUS the unfortunate > > process asking for the zswap entry on zswap load, and skip the corrupte= d > > entry in zswap writeback. The former is accomplished by returning true > > from zswap_load(), indicating that zswap owns the swapped out content, > > but without flagging the folio as up-to-date. The process trying to swa= p > > in the page will check for the uptodate folio flag and SIGBUS (see > > do_swap_page() in mm/memory.c for more details). > > We should call out the extra xarray walks and their perf impact (if > any). Lemme throw this in a quick and dirty test. I doubt there's any impact, but since I'm reworking this patch for a third version anyway might as well. > > > > > See [1] for a recent upstream discussion about this. > > > > [1]: https://lore.kernel.org/all/ZsiLElTykamcYZ6J@casper.infradead.org/ > > > > Suggested-by: Matthew Wilcox > > Suggested-by: Yosry Ahmed > > Signed-off-by: Nhat Pham > > --- > > mm/zswap.c | 94 ++++++++++++++++++++++++++++++++++++++---------------- > > 1 file changed, 67 insertions(+), 27 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 6dbf31bd2218..e4a2157bbc64 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -62,6 +62,8 @@ static u64 zswap_reject_reclaim_fail; > > static u64 zswap_reject_compress_fail; > > /* Compressed page was too big for the allocator to (optimally) store = */ > > static u64 zswap_reject_compress_poor; > > +/* Load or writeback failed due to decompression failure */ > > +static u64 zswap_decompress_fail; > > /* Store failed because underlying allocator could not get memory */ > > static u64 zswap_reject_alloc_fail; > > /* Store failed because the entry metadata could not be allocated (rar= e) */ > > @@ -996,11 +998,13 @@ static bool zswap_compress(struct page *page, str= uct zswap_entry *entry, > > return comp_ret =3D=3D 0 && alloc_ret =3D=3D 0; > > } > > > > -static void zswap_decompress(struct zswap_entry *entry, struct folio *= folio) > > +static bool zswap_decompress(struct zswap_entry *entry, struct folio *= folio) > > { > > struct zpool *zpool =3D entry->pool->zpool; > > struct scatterlist input, output; > > struct crypto_acomp_ctx *acomp_ctx; > > + int decomp_ret; > > + bool ret =3D true; > > u8 *src; > > > > acomp_ctx =3D acomp_ctx_get_cpu_lock(entry->pool); > > @@ -1025,12 +1029,25 @@ static void zswap_decompress(struct zswap_entry= *entry, struct folio *folio) > > sg_init_table(&output, 1); > > sg_set_folio(&output, folio, PAGE_SIZE, 0); > > acomp_request_set_params(acomp_ctx->req, &input, &output, entry->= length, PAGE_SIZE); > > - BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &= acomp_ctx->wait)); > > - BUG_ON(acomp_ctx->req->dlen !=3D PAGE_SIZE); > > + decomp_ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx-= >req), &acomp_ctx->wait); > > + if (decomp_ret || acomp_ctx->req->dlen !=3D PAGE_SIZE) { > > + ret =3D false; > > + zswap_decompress_fail++; > > + pr_alert_ratelimited( > > + "decompression failed with returned value %d on z= swap entry with swap entry value %08lx, swap type %d, and swap offset %lu. = compression algorithm is %s. compressed size is %u bytes, and decompressed = size is %u bytes.\n", > > This is a very long line. I think we should break it into multiple > lines. I know multiline strings are frowned upon by checkpatch, by this > exist (see the warning in mem_cgroup_oom_control_write() for example), > and they are definitely better than a very long line imo. My personal take is I prefer multi line strings, but was not sure what is the "preferred" or "official" style. Oh well. > > > + decomp_ret, > > + entry->swpentry.val, > > + swp_type(entry->swpentry), > > + swp_offset(entry->swpentry), > > + entry->pool->tfm_name, > > + entry->length, > > + acomp_ctx->req->dlen); > > + } > > > > if (src !=3D acomp_ctx->buffer) > > zpool_unmap_handle(zpool, entry->handle); > > acomp_ctx_put_unlock(acomp_ctx); > > + return ret; > > Not a big deal but we could probably store the length in a local > variable and move the check here, and avoid needing 'ret'. Ah the suggestion you made in an older version right? But sounds like we're just trading some one local variable for another? That said, it *technically* move some work outside of the lock section. I'll just give it a try :) > > > } > > > > /********************************* > > @@ -1060,6 +1077,7 @@ static int zswap_writeback_entry(struct zswap_ent= ry *entry, > > struct writeback_control wbc =3D { > > .sync_mode =3D WB_SYNC_NONE, > > }; > > + int ret =3D 0; > > > > /* try to allocate swap cache folio */ > > si =3D get_swap_device(swpentry); > > @@ -1081,8 +1099,8 @@ static int zswap_writeback_entry(struct zswap_ent= ry *entry, > > * and freed when invalidated by the concurrent shrinker anyway. > > */ > > if (!folio_was_allocated) { > > - folio_put(folio); > > - return -EEXIST; > > + ret =3D -EEXIST; > > + goto put_folio; > > } > > > > /* > > @@ -1095,14 +1113,17 @@ static int zswap_writeback_entry(struct zswap_e= ntry *entry, > > * be dereferenced. > > */ > > tree =3D swap_zswap_tree(swpentry); > > - if (entry !=3D xa_cmpxchg(tree, offset, entry, NULL, GFP_KERNEL))= { > > - delete_from_swap_cache(folio); > > - folio_unlock(folio); > > - folio_put(folio); > > - return -ENOMEM; > > + if (entry !=3D xa_load(tree, offset)) { > > + ret =3D -ENOMEM; > > + goto delete_unlock; > > + } > > + > > + if (!zswap_decompress(entry, folio)) { > > + ret =3D -EIO; > > + goto delete_unlock; > > } > > > > - zswap_decompress(entry, folio); > > + xa_erase(tree, offset); > > > > count_vm_event(ZSWPWB); > > if (entry->objcg) > > @@ -1118,9 +1139,14 @@ static int zswap_writeback_entry(struct zswap_en= try *entry, > > > > /* start writeback */ > > __swap_writepage(folio, &wbc); > > - folio_put(folio); > > > > - return 0; > > +put_folio: > > + folio_put(folio); > > + return ret; > > +delete_unlock: > > + delete_from_swap_cache(folio); > > + folio_unlock(folio); > > + goto put_folio; > > I think I suggested a way to avoid this goto in v1: > https://lore.kernel.org/lkml/Z782SPcJI8DFISRa@google.com/. > > Did this not work out? Oh I thought your suggestion was the same as Johannes. Let me take a closer look... > > > } > > > > /********************************* > > @@ -1620,6 +1646,20 @@ bool zswap_store(struct folio *folio) > > return ret; > > } > > > > +/** > > + * zswap_load() - load a page from zswap > > + * @folio: folio to load > > + * > > + * Returns: true if zswap owns the swapped out contents, false otherwi= se. > > + * > > + * Note that the zswap_load() return value doesn't indicate success or= failure, > > + * but whether zswap owns the swapped out contents. This MUST return t= rue if > > + * zswap does own the swapped out contents, even if it fails to write = the > > + * contents to the folio. Otherwise, the caller will try to read garba= ge from > > + * the backend. > > + * > > + * Success is signaled by marking the folio uptodate. > > + */ > > bool zswap_load(struct folio *folio) > > { > > swp_entry_t swp =3D folio->swap; > > @@ -1644,6 +1684,17 @@ bool zswap_load(struct folio *folio) > > The comment that exists here (not visible in the diff) should be > abbreviated now that we already explained the whole uptodate thing > above, right? Lemme take a stab at it :) > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > return true; > > > > + entry =3D xa_load(tree, offset); > > + if (!entry) > > + return false; > > + > > A small comment here pointing out that we are deliberatly not setting > uptodate because of the failure may make things more obvious, or do you > think that's not needed? I was thinking the comment above zswap_load() should be necessary, but a small comment here won't hurt. > > > + if (!zswap_decompress(entry, folio)) > > + return true; > > + > > + count_vm_event(ZSWPIN); > > + if (entry->objcg) > > + count_objcg_events(entry->objcg, ZSWPIN, 1); > > + > > /* > > * When reading into the swapcache, invalidate our entry. The > > * swapcache can be the authoritative owner of the page and > > @@ -1656,21 +1707,8 @@ bool zswap_load(struct folio *folio) > > * files, which reads into a private page and may free it if > > * the fault fails. We remain the primary owner of the entry.) > > */ > > - if (swapcache) > > - entry =3D xa_erase(tree, offset); > > - else > > - entry =3D xa_load(tree, offset); > > - > > - if (!entry) > > - return false; > > - > > - zswap_decompress(entry, folio); > > - > > - count_vm_event(ZSWPIN); > > - if (entry->objcg) > > - count_objcg_events(entry->objcg, ZSWPIN, 1); > > - > > if (swapcache) { > > + xa_erase(tree, offset); > > zswap_entry_free(entry); > > folio_mark_dirty(folio); > > } > > @@ -1771,6 +1809,8 @@ static int zswap_debugfs_init(void) > > zswap_debugfs_root, &zswap_reject_compress_fai= l); > > debugfs_create_u64("reject_compress_poor", 0444, > > zswap_debugfs_root, &zswap_reject_compress_poo= r); > > + debugfs_create_u64("decompress_fail", 0444, > > + zswap_debugfs_root, &zswap_decompress_fail); > > debugfs_create_u64("written_back_pages", 0444, > > zswap_debugfs_root, &zswap_written_back_pages)= ; > > debugfs_create_file("pool_total_size", 0444, > > > > base-commit: 598d34afeca6bb10554846cf157a3ded8729516c > > -- > > 2.43.5