Re: [PATCH v4] page_io: zswap: do not crash the kernel on decompression failure

[Nhat Pham <nphamcs@gmail.com>]

On Mon, May 12, 2025 at 3:03 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Mar 06, 2025 at 12:50:10PM -0800, Nhat Pham wrote:
> > -static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> > +static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
>
> Hm, why do it this way?  I had it as:
>
> -static void zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> +static int zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> ...
> +       err = crypto_acomp_decompress(acomp_ctx->req);
> +       err = crypto_wait_req(err, &acomp_ctx->wait);
> +       if (!err && acomp_ctx->req->dlen != PAGE_SIZE)
> +               err = -EIO;
>
> which allows us to return something more meaningful than -EIO.  Or is
> doing that a bad idea and we should squash all decompression failures
> to EIO?

Not a bad idea, just no usecase yet. From the POV of
zswap_decompress()'s callers, any decompression failure is handled the
same way, no matter the cause.

Where it might be useful is the debug print statement right below
this. However, zstd and lz4 only return 0 or -EINVAL, I think - not
super useful.

https://github.com/torvalds/linux/blob/master/crypto/zstd.c#L163-L165
https://github.com/torvalds/linux/blob/master/crypto/lz4.c#L61

Not sure about other compressors.

>
> (also i really dislike the chained approach:
>
>         decomp_ret = crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait);
>
> that's much harder to understand than the two lines i have above)

I can send a style fix sometimes if people dislike this - no big deal :)

BTW, hopefully you don't mind the Suggested-by: tag. I know it
deviated a bit from your original suggestion, but the main spirit of
that suggestion remains, so I feel like I should credit you and Yosry.