| Message ID | 20250227001445.1099203-1-nphamcs@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] zswap: do not crash the kernel on decompression failure | expand |
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 corrupted > 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 swap > 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). > > See [1] for a recent upstream discussion about this. > > [1]: https://lore.kernel.org/all/ZsiLElTykamcYZ6J@casper.infradead.org/ > > Suggested-by: Matthew Wilcox <willy@infradead.org> > Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev> > Signed-off-by: Nhat Pham <nphamcs@gmail.com> > --- > 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 (rare) */ > @@ -996,11 +998,13 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, > return comp_ret == 0 && alloc_ret == 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 = entry->pool->zpool; > struct scatterlist input, output; > struct crypto_acomp_ctx *acomp_ctx; > + int decomp_ret; > + bool ret = true; > u8 *src; > > acomp_ctx = 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 != PAGE_SIZE); > + decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); > + if (decomp_ret || acomp_ctx->req->dlen != PAGE_SIZE) { > + ret = false; > + zswap_decompress_fail++; > + pr_alert_ratelimited( > + "decompression failed with returned value %d on zswap 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. > + 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 != 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'. > } > > /********************************* > @@ -1060,6 +1077,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 */ > si = get_swap_device(swpentry); > @@ -1081,8 +1099,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; > } > > /* > @@ -1095,14 +1113,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 delete_unlock; > + } > + > + if (!zswap_decompress(entry, folio)) { > + ret = -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_entry *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? > } > > /********************************* > @@ -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 otherwise. > + * > + * Note that the zswap_load() return value doesn't indicate success or failure, > + * but whether zswap owns the swapped out contents. This MUST return true 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 garbage from > + * the backend. > + * > + * Success is signaled by marking the folio uptodate. > + */ > bool zswap_load(struct folio *folio) > { > swp_entry_t swp = 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? > if (WARN_ON_ONCE(folio_test_large(folio))) > return true; > > + entry = 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? > + 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 = xa_erase(tree, offset); > - else > - entry = 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_fail); > debugfs_create_u64("reject_compress_poor", 0444, > zswap_debugfs_root, &zswap_reject_compress_poor); > + 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
On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > if (WARN_ON_ONCE(folio_test_large(folio))) > > return true; > > > > + entry = 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? > > > + if (!zswap_decompress(entry, folio)) > > + return true; How about an actual -ev and have this in swap_read_folio(): ret = zswap_load(folio); if (ret != -ENOENT) { folio_unlock(folio); goto finish; } read from swapfile... Then in zswap_load(), move uptodate further up like this (I had previously suggested this): if (!zswap_decompress(entry, folio)) return -EIO; folio_mark_uptodate(folio); and I think it would be clear, even without or just minimal comments.
On Wed, Feb 26, 2025 at 11:31:41PM -0500, Johannes Weiner wrote: > On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > > return true; > > > > > > + entry = 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? > > > > > + if (!zswap_decompress(entry, folio)) > > > + return true; > > How about an actual -ev and have this in swap_read_folio(): Good idea, I was going to suggest an enum but this is simpler. > > ret = zswap_load(folio); > if (ret != -ENOENT) { > folio_unlock(folio); > goto finish; > } > > read from swapfile... > > Then in zswap_load(), move uptodate further up like this (I had > previously suggested this): > > if (!zswap_decompress(entry, folio)) > return -EIO; > > folio_mark_uptodate(folio); > > and I think it would be clear, even without or just minimal comments. Another possibility is moving folio_mark_uptodate() back to swap_read_folio(), which should make things even clearer imo as the success/failure logic is all in one place: ret = zswap_load(folio); if (ret != -ENOENT) { folio_unlock(folio); /* Comment about not marking uptodate */ if (!ret) folio_mark_uptodate(); goto finish; } or we can make it crystal clear we have 3 distinct cases: ret = zswap_load(folio); if (!ret) { folio_unlock(folio); folio_mark_uptodate(); goto finish; } else if (ret != -ENOENT) { /* Comment about not marking uptodate */ folio_unlock(folio); goto finish; } WDYT?
On Thu, Feb 27, 2025 at 05:44:29AM +0000, Yosry Ahmed wrote: > On Wed, Feb 26, 2025 at 11:31:41PM -0500, Johannes Weiner wrote: > > On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > > > return true; > > > > > > > > + entry = 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? > > > > > > > + if (!zswap_decompress(entry, folio)) > > > > + return true; > > > > How about an actual -ev and have this in swap_read_folio(): > > Good idea, I was going to suggest an enum but this is simpler. > > > > > ret = zswap_load(folio); > > if (ret != -ENOENT) { > > folio_unlock(folio); > > goto finish; > > } > > > > read from swapfile... > > > > Then in zswap_load(), move uptodate further up like this (I had > > previously suggested this): > > > > if (!zswap_decompress(entry, folio)) > > return -EIO; > > > > folio_mark_uptodate(folio); > > > > and I think it would be clear, even without or just minimal comments. > > Another possibility is moving folio_mark_uptodate() back to > swap_read_folio(), which should make things even clearer imo as the > success/failure logic is all in one place: That works. bdev, swapfile and zeromap set the flag in that file. > ret = zswap_load(folio); > if (ret != -ENOENT) { > folio_unlock(folio); > /* Comment about not marking uptodate */ > if (!ret) > folio_mark_uptodate(); > goto finish; > } Personally, I like this one ^. The comment isn't needed IMO, as now zswap really isn't doing anything special compared to the others. > or we can make it crystal clear we have 3 distinct cases: > > ret = zswap_load(folio); > if (!ret) { > folio_unlock(folio); > folio_mark_uptodate(); > goto finish; > } else if (ret != -ENOENT) { > /* Comment about not marking uptodate */ > folio_unlock(folio); > goto finish; > } This seems unnecessarily repetetive.
On Thu, Feb 27, 2025 at 01:16:16AM -0500, Johannes Weiner wrote: > On Thu, Feb 27, 2025 at 05:44:29AM +0000, Yosry Ahmed wrote: > > On Wed, Feb 26, 2025 at 11:31:41PM -0500, Johannes Weiner wrote: > > > On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > > > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > > > > return true; > > > > > > > > > > + entry = 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? > > > > > > > > > + if (!zswap_decompress(entry, folio)) > > > > > + return true; > > > > > > How about an actual -ev and have this in swap_read_folio(): > > > > Good idea, I was going to suggest an enum but this is simpler. > > > > > > > > ret = zswap_load(folio); > > > if (ret != -ENOENT) { > > > folio_unlock(folio); > > > goto finish; > > > } > > > > > > read from swapfile... > > > > > > Then in zswap_load(), move uptodate further up like this (I had > > > previously suggested this): > > > > > > if (!zswap_decompress(entry, folio)) > > > return -EIO; > > > > > > folio_mark_uptodate(folio); > > > > > > and I think it would be clear, even without or just minimal comments. > > > > Another possibility is moving folio_mark_uptodate() back to > > swap_read_folio(), which should make things even clearer imo as the > > success/failure logic is all in one place: > > That works. bdev, swapfile and zeromap set the flag in that file. > > > ret = zswap_load(folio); > > if (ret != -ENOENT) { > > folio_unlock(folio); > > /* Comment about not marking uptodate */ > > if (!ret) > > folio_mark_uptodate(); > > goto finish; > > } > > Personally, I like this one ^. The comment isn't needed IMO, as now > zswap really isn't doing anything special compared to the others. > > > or we can make it crystal clear we have 3 distinct cases: > > > > ret = zswap_load(folio); > > if (!ret) { > > folio_unlock(folio); > > folio_mark_uptodate(); > > goto finish; > > } else if (ret != -ENOENT) { > > /* Comment about not marking uptodate */ > > folio_unlock(folio); > > goto finish; > > } > > This seems unnecessarily repetetive. I know, but looking at the two, this one makes it clearer to me there are 3 distinct cases, and the redundancy is not terrible. So I personally prefer the latter, but I am fine either way.
On Thu, Feb 27, 2025 at 07:11:59AM +0000, Yosry Ahmed wrote: > On Thu, Feb 27, 2025 at 01:16:16AM -0500, Johannes Weiner wrote: > > On Thu, Feb 27, 2025 at 05:44:29AM +0000, Yosry Ahmed wrote: > > > On Wed, Feb 26, 2025 at 11:31:41PM -0500, Johannes Weiner wrote: > > > > On Thu, Feb 27, 2025 at 01:19:31AM +0000, Yosry Ahmed wrote: > > > > > On Wed, Feb 26, 2025 at 04:14:45PM -0800, Nhat Pham wrote: > > > > > > if (WARN_ON_ONCE(folio_test_large(folio))) > > > > > > return true; > > > > > > > > > > > > + entry = 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? > > > > > > > > > > > + if (!zswap_decompress(entry, folio)) > > > > > > + return true; > > > > > > > > How about an actual -ev and have this in swap_read_folio(): > > > > > > Good idea, I was going to suggest an enum but this is simpler. > > > > > > > > > > > ret = zswap_load(folio); > > > > if (ret != -ENOENT) { > > > > folio_unlock(folio); > > > > goto finish; > > > > } > > > > > > > > read from swapfile... > > > > > > > > Then in zswap_load(), move uptodate further up like this (I had > > > > previously suggested this): > > > > > > > > if (!zswap_decompress(entry, folio)) > > > > return -EIO; > > > > > > > > folio_mark_uptodate(folio); > > > > > > > > and I think it would be clear, even without or just minimal comments. > > > > > > Another possibility is moving folio_mark_uptodate() back to > > > swap_read_folio(), which should make things even clearer imo as the > > > success/failure logic is all in one place: > > > > That works. bdev, swapfile and zeromap set the flag in that file. > > > > > ret = zswap_load(folio); > > > if (ret != -ENOENT) { > > > folio_unlock(folio); > > > /* Comment about not marking uptodate */ > > > if (!ret) > > > folio_mark_uptodate(); > > > goto finish; > > > } > > > > Personally, I like this one ^. The comment isn't needed IMO, as now > > zswap really isn't doing anything special compared to the others. > > > > > or we can make it crystal clear we have 3 distinct cases: > > > > > > ret = zswap_load(folio); > > > if (!ret) { > > > folio_unlock(folio); > > > folio_mark_uptodate(); > > > goto finish; > > > } else if (ret != -ENOENT) { > > > /* Comment about not marking uptodate */ > > > folio_unlock(folio); > > > goto finish; > > > } > > > > This seems unnecessarily repetetive. > > I know, but looking at the two, this one makes it clearer to me there > are 3 distinct cases, and the redundancy is not terrible. > > So I personally prefer the latter, but I am fine either way. I just realized that swap_read_folio_zeromap() does the same trick, so we should probably also move the folio_mark_uptodate() in there to swap_read_folio(). Maybe we can do something like this: /* Returns true if the folio was in the zeromap or zswap */ bool swap_read_folio_in_memory(struct folio *folio) { int ret; ret = swap_read_folio_zeromap(folio); if (ret == -ENOENT) ret = zswap_load(folio); if (ret == 0) { folio_mark_uptodate(folio); folio_unlock(folio); return true; } else if (ret != -ENOENT) { folio_unlock(folio); return true; } else { return false; } } void swap_read_folio(struct folio *folio, struct swap_iocb **plug) { ... if (swap_read_folio_in_memory(folio)) goto finish; ... } Admittedly, swap_read_folio_in_memory() is not a good name. Maybe swap_read_folio_zeromap_or_zswap() :)
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 (rare) */ @@ -996,11 +998,13 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, return comp_ret == 0 && alloc_ret == 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 = entry->pool->zpool; struct scatterlist input, output; struct crypto_acomp_ctx *acomp_ctx; + int decomp_ret; + bool ret = true; u8 *src; acomp_ctx = 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 != PAGE_SIZE); + decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait); + if (decomp_ret || acomp_ctx->req->dlen != PAGE_SIZE) { + ret = false; + zswap_decompress_fail++; + pr_alert_ratelimited( + "decompression failed with returned value %d on zswap 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", + 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 != acomp_ctx->buffer) zpool_unmap_handle(zpool, entry->handle); acomp_ctx_put_unlock(acomp_ctx); + return ret; } /********************************* @@ -1060,6 +1077,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 */ si = get_swap_device(swpentry); @@ -1081,8 +1099,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; } /* @@ -1095,14 +1113,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 delete_unlock; + } + + if (!zswap_decompress(entry, folio)) { + ret = -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_entry *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; } /********************************* @@ -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 otherwise. + * + * Note that the zswap_load() return value doesn't indicate success or failure, + * but whether zswap owns the swapped out contents. This MUST return true 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 garbage from + * the backend. + * + * Success is signaled by marking the folio uptodate. + */ bool zswap_load(struct folio *folio) { swp_entry_t swp = folio->swap; @@ -1644,6 +1684,17 @@ bool zswap_load(struct folio *folio) if (WARN_ON_ONCE(folio_test_large(folio))) return true; + entry = xa_load(tree, offset); + if (!entry) + return false; + + 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 = xa_erase(tree, offset); - else - entry = 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_fail); debugfs_create_u64("reject_compress_poor", 0444, zswap_debugfs_root, &zswap_reject_compress_poor); + 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,
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 corrupted 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 swap in the page will check for the uptodate folio flag and SIGBUS (see do_swap_page() in mm/memory.c for more details). See [1] for a recent upstream discussion about this. [1]: https://lore.kernel.org/all/ZsiLElTykamcYZ6J@casper.infradead.org/ Suggested-by: Matthew Wilcox <willy@infradead.org> Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev> Signed-off-by: Nhat Pham <nphamcs@gmail.com> --- mm/zswap.c | 94 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 67 insertions(+), 27 deletions(-) base-commit: 598d34afeca6bb10554846cf157a3ded8729516c