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] [day] [month] [year] [list]

Message-ID: <Z7-r43-8ongoUDdZ@google.com>
Date: Thu, 27 Feb 2025 00:03:47 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Nhat Pham <nphamcs@...il.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.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

[..]
> > zswap_decompress()
> > {
> >         mutex_lock();
> >         ...
> >         ret = crypto_wait_req(..);
> >         ...
> >         mutex_unlock();
> >         ...
> >         if (ret || acomp_ctx->req->dlen != PAGE_SIZE) {
> 
> Hmmmm, do we need the mutex to protect acomp_ctx->req->dlen? No strong
> opinions here from my end TBH.

We can copy the length to a local variable if that's the case.

> 
> >                 /* SHIT */
> >                 return false;
> >         }
> >         return true;
> > }
> [...]
> >
> > We are doing load + erase here and in the writeback path now, so two
> > xarray walks instead of one. How does this affect performance? We had a
> > similar about the possiblity of doing a lockless xas_load() followed by
> > a conditional xas_erase() for zswap_invalidate():
> >
> > https://lore.kernel.org/lkml/20241018192525.95862-1-ryncsn@gmail.com/
> >
> > Unfortunately it seems like we can't trivially do that unless we keep
> > the tree locked, which we probably don't want to do throughout
> > decompression.
> >
> > How crazy would it be to remove the entry from the tree and re-add it if
> > compression fails? Does swapcache_prepare() provide sufficient
> > protection for us to do this without anyone else looking at this entry
> > (seems like it)?
> 
> My concern is, what happens if xa_store() in the re-add path fails
> because we do not have enough memory?

Hmm good point. xa_erase() is essentially xas_store(NULL), but I think
at some point if a node is made of entirely NULLs we'll free it, and it
would be theoritically possible that we hit this case.

Let's start by checking if the added xarray walk adds any noticable
overhead and go from there. Ideally we want to test with a large (and
sparse?) xarray to try and hit the worst case.

> 
> >
> > Anyway, this is all moot if the second walk is not noticeable from a
> > perf perspective.