linux-kernel - Re: [PATCH] zswap: do not crash the kernel on decompression failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7-rG7Y3DK33MyCl@google.com>
Date: Thu, 27 Feb 2025 00:00:27 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Nhat Pham <nphamcs@...il.com>
Cc: Johannes Weiner <hannes@...xchg.org>, akpm@...ux-foundation.org,
	chengming.zhou@...ux.dev, linux-mm@...ck.org, kernel-team@...a.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] zswap: do not crash the kernel on decompression failure

On Wed, Feb 26, 2025 at 03:20:13PM -0800, Nhat Pham wrote:
> On Wed, Feb 26, 2025 at 7:33 AM Yosry Ahmed <yosry.ahmed@...ux.dev> wrote:
> >
> > On Tue, Feb 25, 2025 at 11:57:27PM -0500, Johannes Weiner wrote:
> > > On Wed, Feb 26, 2025 at 03:12:35AM +0000, Yosry Ahmed wrote:
> > > > On Tue, Feb 25, 2025 at 01:32:00PM -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.
> > > >
> > > > Some relevant observations/questions, but not really actionable for this
> > > > patch, perhaps some future work, or more likely some incoherent
> > > > illogical thoughts :
> > > >
> > > > (1) It seems like not making the folio uptodate will cause shmem faults
> > > > to mark the swap entry as hwpoisoned, but I don't see similar handling
> > > > for do_swap_page(). So it seems like even if we SIGBUS the process,
> > > > other processes mapping the same page could follow in the same
> > > > footsteps.
> > >
> > > It's analogous to what __end_swap_bio_read() does for block backends,
> > > so it's hitchhiking on the standard swap protocol for read failures.
> >
> > Right, that's also how I got the idea when I did the same for large
> > folios handling.
> 
> And your handling of the large folio (along with the comment in the
> other thread) was how I got the idea for this patch :)
> 
> >
> > >
> > > The page sticks around if there are other users. It can get reclaimed,
> > > but since it's not marked dirty, it won't get overwritten. Another
> > > access will either find it in the swapcache and die on !uptodate; if
> > > it was reclaimed, it will attempt another decompression. If all
> > > references have been killed, zswap_invalidate() will finally drop it.
> > >
> > > Swapoff actually poisons the page table as well (unuse_pte).
> >
> > Right. My question was basically why don't we also poison the page table
> > in do_swap_page() in this case. It's like that we never swapoff.
> 
> That would require a rmap walk right? To also poison the other PTEs
> that point to the faulty (z)swap entry?
> 
> Or am I misunderstanding your point :)

Oh I meant why not just mark the entry where the fault happened as
poisoned at least. Finding other PTEs that point to the swap entry is a
different story. I don't think we can even use the rmap here.