Re: [PATCH v3 8/8] mm: zswap: Batched zswap_compress() for storing large folios in batches.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm: zswap: corrupted and unterminated dst scatterlist in
  zswap_compress()
- [Medium] mm: zswap: unclamped nr_batch_pages on remainder batch in
  zswap_compress()
- [Medium] mm: zswap: unsigned sg->length check mishandles negative
  errors in zswap_compress()
--

commit 212f5740068394b4c82b7b1e5452b1ac6625322f
Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>

mm: zswap: Batched zswap_compress() for storing large folios in batches.

This commit replaces per-page zswap_compress() calls with a batched
implementation that compresses up to ZSWAP_MAX_BATCH_SIZE pages using a
per-CPU scatterlist table and updates the batch entries in
zswap_update_entries().

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 5ef1ae67e67a9..45fd8dfaf916d 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c

[ ... ]

> @@ -898,6 +914,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)

[ ... ]

> +	acomp_ctx->comp.sg_table = kmalloc(sizeof(*acomp_ctx->comp.sg_table),
> +					   GFP_KERNEL);
> +	if (!acomp_ctx->comp.sg_table) {
> +		ret = -ENOMEM;
> +		goto req_fail;
> +	}
> +
> +	if (sg_alloc_table(acomp_ctx->comp.sg_table, ZSWAP_MAX_BATCH_SIZE,
> +			   GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto req_fail;
> +	}
> +
> +	/*
> +	 * Statically map the per-CPU destination buffers to the per-CPU
> +	 * SG lists; crucial for performance.
> +	 */
> +	for_each_sg(acomp_ctx->comp.sg_table->sgl, sg, ZSWAP_MAX_BATCH_SIZE, i)
> +		sg_set_buf(sg, acomp_ctx->comp.buffers[i], PAGE_SIZE);

[ ... ]

> +static bool zswap_compress(struct zswap_store_context *zc)
> +{
> +	u8 batch_start, comp_batch_iter;
> +	struct scatterlist *sg, *dst_sg;
> +	int err;
>  
>  	/*
> -	 * it maybe looks a little bit silly that we send an asynchronous request,
> -	 * then wait for its completion synchronously. This makes the process look
> -	 * synchronous in fact.
> -	 * Theoretically, acomp supports users send multiple acomp requests in one
> -	 * acomp instance, then get those requests done simultaneously. but in this
> -	 * case, zswap actually does store and load page by page, there is no
> -	 * existing method to send the second page before the first page is done
> -	 * in one thread doing zswap.
> -	 * but in different threads running on different cpu, we have different
> -	 * acomp instance, and compression and decompression use separate
> -	 * requests, so multiple threads can do (de)compression in parallel.
> +	 * @nr_batch_pages is constrained by the per-CPU comp_ctx's compressor
> +	 * batch size and the @zc->nr_pages in this store batch. @slen indicates
> +	 * the total source length bytes for @nr_batch_pages.
>  	 */
> +	zc->nr_batch_pages = min(zc->comp_ctx->compr_batch_size, zc->nr_pages);
> +	zc->slen = zc->nr_batch_pages * PAGE_SIZE;
>  
>  	/*
> +	 * Compress the @nr_pages in @folio starting at index @folio_start
> +	 * in batches of @nr_batch_pages.
>  	 */
> +	for (batch_start = 0; batch_start < zc->nr_pages;
> +	     batch_start += zc->nr_batch_pages) {

[Severity: Medium]
Can zc->nr_batch_pages exceed the remaining pages in the store batch on the
final loop iteration?

zc->nr_batch_pages and zc->slen are computed once before the loop instead
of being clamped to zc->nr_pages - batch_start on each iteration.  If a
compressor has a compr_batch_size that does not evenly divide zc->nr_pages
(for example, compr_batch_size == 3 with zc->nr_pages == 4), the final
iteration reads past the end of the folio batch and walks past the SG_END
terminator of zc->comp_ctx->sg_table->sgl in for_each_sg(), causing a NULL
pointer dereference when sg_next() returns NULL.

> +		/*
> +		 * Send @nr_batch_pages to crypto_acomp for compression:
> +		 *
> +		 * These pages are in @folio's range of indices in the interval
> +		 *     [@folio_start + @batch_start,
> +		 *      @folio_start + @batch_start + @nr_batch_pages).
> +		 *
> +		 * The source folio pages in the batch are directly submitted to
> +		 * crypto_acomp via acomp_request_set_src_folio().
> +		 */
> +		acomp_request_set_src_folio(zc->comp_ctx->areq.req, zc->folio,
> +				(zc->folio_start + batch_start) * PAGE_SIZE,
> +				zc->slen);
> +
> +		dst_sg = zc->comp_ctx->sg_table->sgl + batch_start;
> +		dst_sg->length = zc->slen;
> +
> +		/*
> +		 * The per-CPU @zc->comp_ctx->sg_table scatterlists are statically
> +		 * mapped to the per-CPU dst @buffers at pool creation time.
> +		 * There are ZSWAP_MAX_BATCH_SIZE per-CPU SG lists and dst
> +		 * buffers allocated for all compressors.
> +		 */
> +		acomp_request_set_dst_sg(zc->comp_ctx->areq.req,
> +					 dst_sg, zc->slen);

[Severity: High]
Does setting dst_sg->length = zc->slen corrupt the per-CPU scatterlist table
in zc->comp_ctx->sg_table?

In zswap_cpu_comp_prepare(), each entry in sg_table->sgl is mapped to a
separate PAGE_SIZE buffer and only sgl[ZSWAP_MAX_BATCH_SIZE - 1] has SG_END
set.  During compression, each entry's sg->length is overwritten with the
compressed size or a negative error code.

When zc->nr_batch_pages > 1, setting dst_sg->length = zc->slen tells the
compressor that dst_sg[0]'s single PAGE_SIZE buffer is nr_batch_pages *
PAGE_SIZE bytes long, while dst_sg[1 .. nr_batch_pages - 1].length are never
reset to PAGE_SIZE and retain stale lengths or negative error codes from the
previous batch.  If crypto_acomp_compress() fails while zc->wb_enabled is
true without updating every entry's sg->length, any entry still holding a
stale length in (0, PAGE_SIZE) bypasses the sg->length >= PAGE_SIZE check
and causes zswap_update_entries() to store stale data from the previous
folio.

In addition, because SG_END is only set on sgl[3], passing dst_sg = sgl +
batch_start leaves 4 - batch_start entries linked in req->dst.  Hardware
acomp drivers that check sg_nents(req->dst) or DMA-map every entry in the
scatterlist walk past the current batch into trailing entries whose
sg->length may still hold PAGE_SIZE or (unsigned int)-ENOSPC (0xffffffe4)
from an earlier failed compression.

> +
> +		err = crypto_wait_req(crypto_acomp_compress(zc->comp_ctx->areq.req),
> +				      &zc->comp_ctx->areq.wait);
> +
> +		/*
> +		 * If a page cannot be compressed into a size smaller than
> +		 * PAGE_SIZE, save the content as is without a compression, to
> +		 * keep the LRU order of writebacks.  If writeback is disabled,
> +		 * reject the page since it only adds metadata overhead.
> +		 * swap_writeout() will put the page back to the active LRU list
> +		 * in the case.
> +		 */
> +		if (err && !zc->wb_enabled)
> +			goto compress_error;
>  
> +		/*
> +		 * For each output SG list in @acomp_ctx->req->sg_table->sgl,
> +		 * the @sg->length should be set to either the page's compressed
> +		 * length (success), or it's negative value compression error.
> +		 */
> +		for_each_sg(dst_sg, sg, zc->nr_batch_pages, comp_batch_iter) {
> +
> +			if (sg->length <= 0 || sg->length >= PAGE_SIZE) {
> +				if (!zc->wb_enabled) {
> +					if (sg->length == PAGE_SIZE)
> +						sg->length = -ENOSPC;
> +					else if (WARN_ON_ONCE(!sg->length || sg->length > PAGE_SIZE))
> +						sg->length = -EINVAL;
> +					goto compress_error;
> +				}
> +
> +				sg->length = PAGE_SIZE;
> +			}
> +		}
>  	}
>  
> +	return zswap_update_entries(zc);
>  
> +compress_error:
> +	for_each_sg(dst_sg, sg, zc->nr_batch_pages, comp_batch_iter) {
> +		if ((int)sg->length < 0) {
> +			if ((int)sg->length == -ENOSPC)
> +				zswap_reject_compress_poor++;
> +			else
> +				zswap_reject_compress_fail++;
> +		}
> +	}
> +
> +	return false;
>  }

[Severity: Medium]
Does checking sg->length without casting to int mishandle negative
compression error codes when !zc->wb_enabled?

scatterlist.length is an unsigned int, so a negative error code stored in
sg->length (such as -ENOSPC) is never <= 0 and instead triggers
WARN_ON_ONCE(!sg->length || sg->length > PAGE_SIZE) and overwrites
sg->length with -EINVAL before compress_error: checks for -ENOSPC.
Conversely, if crypto_acomp_compress() returns an error before setting
negative values in sg->length, jumping directly to compress_error: finds no
entries with (int)sg->length < 0 and increments neither rejection counter.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106072412.7909-1-kanchanapsridhar2026@gmail.com?part=8