Re: [PATCH v2 6/8] mm: zswap: Store large folios in batches.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: zswap: fix error return on buffer alloc failure in zswap_cpu_comp_prepare()
- [Medium] mm: zswap: unsynchronized write to pool->compr_batch_size on CPU hotplug
--

commit 13a7e2ffeb93e24e6a8f400496ff51f01ce0c408
Author: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>

mm: zswap: Store large folios in batches.

This commit introduces batching when storing large folios in zswap, allocating
multiple per-CPU compression destination buffers when supported by the
compressor and batch-allocating zswap entries from the slab cache. It also
caches the memcg writeback setting and folio NUMA node ID in a store context
and reorganizes struct zswap_pool for cacheline locality.

> diff --git a/mm/zswap.c b/mm/zswap.c
> index 19b7d60bc7d2a..c8890e8a9b836 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c

[ ... ]

> @@ -866,10 +922,28 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  		goto fail;
>  	}
>  
> +	/*
> +	 * Allocate up to ZSWAP_MAX_BATCH_SIZE dst buffers if the
> +	 * compressor supports batching.
> +	 */
> +	pool->compr_batch_size = min(ZSWAP_MAX_BATCH_SIZE,
> +				     crypto_acomp_batch_size(acomp_ctx->acomp));

[Severity: Medium]
Can updating pool->compr_batch_size inside the per-CPU hotplug callback race
with concurrent readers or cause a buffer count mismatch across CPUs?

In zswap_pool_create(), cpuhp_state_add_instance() runs zswap_cpu_comp_prepare()
only on currently online CPUs. If an offline CPU is brought online later while
the pool is active, zswap_cpu_comp_prepare() writes to pool->compr_batch_size
without synchronization while zswap_store() concurrently reads it:

    store_batch_size = (pool->compr_batch_size > 1) ?
                        pool->compr_batch_size : ZSWAP_MAX_BATCH_SIZE;

Additionally, because each CPU independently calls crypto_alloc_acomp_node()
when brought online, if a higher-priority acomp implementation for
pool->tfm_name with a different batch_size is registered before a CPU comes
online, pool->compr_batch_size will be overwritten with the new value. When
zswap_pool_destroy() later frees each CPU's buffers using the single
pool->compr_batch_size:

    for_each_possible_cpu(cpu)
        acomp_ctx_free(per_cpu_ptr(pool->acomp_ctx, cpu),
                       pool->compr_batch_size);

acomp_ctx_free() would iterate past the end of acomp_ctx->comp.buffers (or
leak buffers) for CPUs that were initialized with the earlier batch size.

> +
>  	ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp);
>  	if (ret)
>  		goto req_fail;
>  
> +	acomp_ctx->comp.buffers = kcalloc_node(pool->compr_batch_size, sizeof(u8 *),
> +					       GFP_KERNEL, nid);
> +	if (!acomp_ctx->comp.buffers)
> +		goto req_fail;
> +
> +	for (i = 0; i < pool->compr_batch_size; ++i) {
> +		acomp_ctx->comp.buffers[i] = kmalloc_node(PAGE_SIZE, GFP_KERNEL, nid);
> +		if (!acomp_ctx->comp.buffers[i])
> +			goto req_fail;
> +	}
> +
>  	/* Skip the per-CPU request when decompression can use the stack. */
>  	if (acomp_is_async(acomp_ctx->acomp)) {
>  		ret = zswap_acomp_req_init(&acomp_ctx->decomp, acomp_ctx->acomp);
> @@ -882,12 +956,13 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
>  req_fail:
>  	pr_err("could not alloc crypto acomp_request %s\n", pool->tfm_name);
>  fail:
> -	acomp_ctx_free(acomp_ctx);
> +	acomp_ctx_free(acomp_ctx, pool->compr_batch_size);
>  	return ret;
>  }

[Severity: High]
Does zswap_cpu_comp_prepare() return 0 (success) if kcalloc_node() or
kmalloc_node() fails when allocating acomp_ctx->comp.buffers?

Earlier in zswap_cpu_comp_prepare(), ret is initialized to -ENOMEM, but then
zswap_acomp_req_init() is called:

    ret = zswap_acomp_req_init(&acomp_ctx->comp.areq, acomp_ctx->acomp);
    if (ret)
        goto req_fail;

When zswap_acomp_req_init() succeeds, ret is 0. If the subsequent
kcalloc_node() for acomp_ctx->comp.buffers or kmalloc_node() for
acomp_ctx->comp.buffers[i] fails and jumps to req_fail, ret is still 0.

At req_fail, acomp_ctx_free() frees the acomp state and sets
acomp_ctx->comp.buffers, acomp_ctx->comp.areq.req, and acomp_ctx->acomp to
NULL, and zswap_cpu_comp_prepare() returns 0 to the CPU hotplug core.
Subsequent calls to zswap_compress() on that CPU will then dereference NULL at
dst = comp_ctx->buffers[0].

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