Re: [PATCH] mm/zswap: validate dlen and normalize comp_ret on compression success

[Suseendran Bhaskaran]

On Fri, 23 Oct 2026 10:26:07 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Thu, 22 Oct 2026 23:27:33 +0000 Suseendran Bhaskaran <suseendranb@xxxxxxxxxx> wrote:
>
> > In zswap_compress(), dlen is only meaningful when compression succeeds
> > (!comp_ret), as compressors do not consistently set req->dlen on error
> > (e.g. zstd, 842, and qat set dlen = 0; lz4, lz4hc, lzo, lzo-rle,
> > deflate, and iaa leave dlen at PAGE_SIZE; hisi_zip sets dlen to the
> > hardware-reported produced bytes).
> >
> > When !comp_ret and dlen == PAGE_SIZE with zswap writeback disabled,
> > zswap_compress() sets comp_ret to -EINVAL instead of -ENOSPC,
> > miscounting the page under reject_compress_fail instead of
> > reject_compress_poor.
> >
> > When !comp_ret, set comp_ret = -ENOSPC if dlen == PAGE_SIZE, or
> > comp_ret = -EINVAL if WARN_ON_ONCE(!dlen || dlen > PAGE_SIZE). This
> > allows simplifying the failure check to 'if (comp_ret)' and dropping
> > the ternary assignment in the writeback-disabled path.
>
> Sounds like we're complexicating core code in order to accommodate
> inconsistent compressor behavior.  Would it be better to
>
> a) design the compressor interface(!)
> b) document it
> c) ensure that all compressors comply with it?
>
> Maintainers may choose to go ahead with this as a backportable
> quickfix, but it all seems to be a bit of a mess in there.

Thank you very much for the review and feedback, Andrew.

I completely agree that clearly defining, documenting, and enforcing a
consistent compressor interface across all implementations is the right
long-term approach. I sent a separate patch that standardizes return
codes on destination buffer overflows [1], but having a well-defined
compressor interface would tighten all gaps.

[1] https://lore.kernel.org/all/20261021205626.3286270-2-suseendranb@xxxxxxxxxx/

> Wht you forgot to tell us (please never forget this) is the
> userspace-visible runtime effects of the bug.  Autobot says:
>
> : The userspace-visible effects are limited to corrected zswap
> : statistics and a new warning for invalid compressor results.
> :
> : When compression succeeds but produces exactly PAGE_SIZE bytes, and
> : zswap writeback is disabled, the page is rejected as before. However,
> : reject_compress_poor now increments instead of reject_compress_fail.
> : This makes the debugfs statistics distinguish incompressible pages
> : from actual compressor failures more accurately.
> :
> : A compressor which reports success with a zero or oversized output
> : length will now trigger WARN_ON_ONCE(). Previously, zswap handled
> : these results without a warning. On systems with panic_on_warn
> : enabled, this could cause a kernel panic.
> :
> : There is no intended change to which pages zswap accepts, rejects
> : or stores uncompressed, nor to normal application behavior.
> :
> : I think these runtime effects should be described more prominently
> : in the changelog, particularly the new warning.

Apologies for missing the userspace-visible runtime impact in the commit
description. I will make sure to always include this going forward.
Autobot is spot-on here. I would be happy to send a v2 with an updated
commit message that clearly documents these runtime effects and
behavioral changes.

> My autobot is all worried about the HiSilicon ZIP compressor.
>
> : I think the zswap-side fix is reasonable. On success, dlen should
> : contain the actual output length, and zswap should validate it. On
> : failure, dlen isn't necessarily meaningful, so we should rely on the
> : compressor's error code.
> :
> : Separately, I think we should make the compressors consistently return
> : -ENOSPC when the output buffer is too small. Some currently return
> : -EINVAL, which means reject_compress_fail versus reject_compress_poor
> : will remain inaccurate even with this patch.
> :
> : A compressor which successfully produces exactly PAGE_SIZE bytes
> : should still report success. Deciding that the result isn't worth
> : storing belongs in zswap.
> :
> : One thing worth checking is HiSilicon ZIP's negative-compression
> : status. It is treated as success, with dlen taken from the hardware.
> : Could that produce zero or more than PAGE_SIZE bytes? If so, the new
> : WARN_ON_ONCE() might trigger on ordinary incompressible data.
> :
> : I'd take this fix independently of the compressor cleanup, assuming
> : that last case is safe.

I checked drivers/crypto/hisilicon/zip/zip_crypto.c and the HiSilicon
ZIP hardware status definitions (in Linaro UADK drv/hisi_comp.c), and
this case is safe:

1. In hisi_zip_fill_buf_size(), sqe->input_data_length = a_req->slen and
   sqe->dest_avail_out = a_req->dlen (both PAGE_SIZE in zswap_compress()).

2. When an incompressible page overflows dest_avail_out (PAGE_SIZE), the
   hardware aborts at dest_avail_out and reports status 0x0e
   (ERR_DSTLEN_OUT, insufficient dest_avail_out), not 0x0d (HZIP_NC_ERR).
   In hisi_zip_acomp_cb(), 'status != 0 && status != HZIP_NC_ERR' sets
   err = -EIO (comp_ret != 0), so the !comp_ret branch is not entered.

3. HZIP_NC_ERR (0x0d, negative compression) only occurs when the output
   fits within dest_avail_out (0 < sqe->produced <= dest_avail_out) while
   exceeding input_data_length (i.e. when a caller passes dlen > slen).
   Even if HZIP_NC_ERR occurred with sqe->produced == PAGE_SIZE,
   'if (dlen == PAGE_SIZE)' handles it (comp_ret = -ENOSPC) before the
   'else if (WARN_ON_ONCE(!dlen || dlen > PAGE_SIZE))' check.

4. When ctx->fallback is enabled (hisi_zip_fallback_do_work()), it
   delegates to the software deflate/lz4 backend and only updates dlen on
   ret == 0 (0 < dlen <= PAGE_SIZE).

So hisi_zip will not return !comp_ret with dlen == 0 or dlen > PAGE_SIZE
on incompressible pages, and WARN_ON_ONCE() will not trigger on normal
incompressible data.

Please let me know if you would like me to send a v2 with the updated
commit message including the userspace-visible runtime impact.