[PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

This patchset persists the zswap pool's per-CPU acomp_ctx resources to
last until the pool is destroyed. It then simplifies the per-CPU
acomp_ctx mutex locking in zswap_compress()/zswap_decompress().

Further, it consistently uses the same checks for valid
acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
acomp_ctx members.

These are independent submissions of patches 23 and 24 from [1], to
facilitate merging. The Acks are preserved from [1].

[1]: https://patchwork.kernel.org/project/linux-mm/list/?series=1046677

Kanchana P. Sridhar (2):
  mm: zswap: Tie per-CPU acomp_ctx lifetime to the pool.
  mm: zswap: Consistently use IS_ERR_OR_NULL() to check acomp_ctx
    resources.

 mm/zswap.c | 163 ++++++++++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 97 deletions(-)

-- 
2.39.5

[PATCH 1/2] mm: zswap: Tie per-CPU acomp_ctx lifetime to the pool.

[Kanchana P. Sridhar]

Currently, per-CPU acomp_ctx are allocated on pool creation and/or CPU
hotplug, and destroyed on pool destruction or CPU hotunplug. This
complicates the lifetime management to save memory while a CPU is
offlined, which is not very common.

Simplify lifetime management by allocating per-CPU acomp_ctx once on
pool creation (or CPU hotplug for CPUs onlined later), and keeping them
allocated until the pool is destroyed.

Refactor cleanup code from zswap_cpu_comp_dead() into
acomp_ctx_dealloc() to be used elsewhere.

The main benefit of using the CPU hotplug multi state instance startup
callback to allocate the acomp_ctx resources is that it prevents the
cores from being offlined until the multi state instance addition call
returns.

  From Documentation/core-api/cpu_hotplug.rst:

    "The node list add/remove operations and the callback invocations are
     serialized against CPU hotplug operations."

Furthermore, zswap_[de]compress() cannot contend with
zswap_cpu_comp_prepare() because:

  - During pool creation/deletion, the pool is not in the zswap_pools
    list.

  - During CPU hot[un]plug, the CPU is not yet online, as Yosry pointed
    out. zswap_cpu_comp_prepare() will be run on a control CPU,
    since CPUHP_MM_ZSWP_POOL_PREPARE is in the PREPARE section of "enum
    cpuhp_state".

  In both these cases, any recursions into zswap reclaim from
  zswap_cpu_comp_prepare() will be handled by the old pool.

The above two observations enable the following simplifications:

 1) zswap_cpu_comp_prepare():

    a) acomp_ctx mutex locking:

       If the process gets migrated while zswap_cpu_comp_prepare() is
       running, it will complete on the new CPU. In case of failures, we
       pass the acomp_ctx pointer obtained at the start of
       zswap_cpu_comp_prepare() to acomp_ctx_dealloc(), which again, can
       only undergo migration. There appear to be no contention
       scenarios that might cause inconsistent values of acomp_ctx's
       members. Hence, it seems there is no need for
       mutex_lock(&acomp_ctx->mutex) in zswap_cpu_comp_prepare().

    b) acomp_ctx mutex initialization:

       Since the pool is not yet on zswap_pools list, we don't need to
       initialize the per-CPU acomp_ctx mutex in
       zswap_pool_create(). This has been restored to occur in
       zswap_cpu_comp_prepare().

    c) Subsequent CPU offline-online transitions:

       zswap_cpu_comp_prepare() checks upfront if acomp_ctx->acomp is
       valid. If so, it returns success. This should handle any CPU
       hotplug online-offline transitions after pool creation is done.

 2) CPU offline vis-a-vis zswap ops:

    Let's suppose the process is migrated to another CPU before the
    current CPU is dysfunctional. If zswap_[de]compress() holds the
    acomp_ctx->mutex lock of the offlined CPU, that mutex will be
    released once it completes on the new CPU. Since there is no
    teardown callback, there is no possibility of UAF.

 3) Pool creation/deletion and process migration to another CPU:

    During pool creation/deletion, the pool is not in the zswap_pools
    list. Hence it cannot contend with zswap ops on that CPU. However,
    the process can get migrated.

    a) Pool creation --> zswap_cpu_comp_prepare()
                                --> process migrated:
                                    * Old CPU offline: no-op.
                                    * zswap_cpu_comp_prepare() continues
                                      to run on the new CPU to finish
                                      allocating acomp_ctx resources for
                                      the offlined CPU.

    b) Pool deletion --> acomp_ctx_dealloc()
                                --> process migrated:
                                    * Old CPU offline: no-op.
                                    * acomp_ctx_dealloc() continues
                                      to run on the new CPU to finish
                                      de-allocating acomp_ctx resources
                                      for the offlined CPU.

 4) Pool deletion vis-a-vis CPU onlining:

    The call to cpuhp_state_remove_instance() cannot race with
    zswap_cpu_comp_prepare() because of hotplug synchronization.

The current acomp_ctx_get_cpu_lock()/acomp_ctx_put_unlock() are
deleted. Instead, zswap_[de]compress() directly call
mutex_[un]lock(&acomp_ctx->mutex).

The per-CPU memory cost of not deleting the acomp_ctx resources upon CPU
offlining, and only deleting them when the pool is destroyed, is 8.28 KB
on x86_64. This cost is only paid when a CPU is offlined, until it is
onlined again.

Co-developed-by: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
Signed-off-by: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com>
Acked-by: Yosry Ahmed <yosry.ahmed@linux.dev>
---
 mm/zswap.c | 163 ++++++++++++++++++++++-------------------------------
 1 file changed, 66 insertions(+), 97 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index bdd24430f6ff..26de0468e33b 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -242,6 +242,20 @@ static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
 **********************************/
 static void __zswap_pool_empty(struct percpu_ref *ref);
 
+static void acomp_ctx_dealloc(struct crypto_acomp_ctx *acomp_ctx)
+{
+	if (IS_ERR_OR_NULL(acomp_ctx))
+		return;
+
+	if (!IS_ERR_OR_NULL(acomp_ctx->req))
+		acomp_request_free(acomp_ctx->req);
+
+	if (!IS_ERR_OR_NULL(acomp_ctx->acomp))
+		crypto_free_acomp(acomp_ctx->acomp);
+
+	kfree(acomp_ctx->buffer);
+}
+
 static struct zswap_pool *zswap_pool_create(char *compressor)
 {
 	struct zswap_pool *pool;
@@ -263,19 +277,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
 
 	strscpy(pool->tfm_name, compressor, sizeof(pool->tfm_name));
 
-	pool->acomp_ctx = alloc_percpu(*pool->acomp_ctx);
+	/* Many things rely on the zero-initialization. */
+	pool->acomp_ctx = alloc_percpu_gfp(*pool->acomp_ctx,
+					   GFP_KERNEL | __GFP_ZERO);
 	if (!pool->acomp_ctx) {
 		pr_err("percpu alloc failed\n");
 		goto error;
 	}
 
-	for_each_possible_cpu(cpu)
-		mutex_init(&per_cpu_ptr(pool->acomp_ctx, cpu)->mutex);
-
+	/*
+	 * This is serialized against CPU hotplug operations. Hence, cores
+	 * cannot be offlined until this finishes.
+	 */
 	ret = cpuhp_state_add_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
 				       &pool->node);
+
+	/*
+	 * cpuhp_state_add_instance() will not cleanup on failure since
+	 * we don't register a hotunplug callback.
+	 */
 	if (ret)
-		goto error;
+		goto cpuhp_add_fail;
 
 	/* being the current pool takes 1 ref; this func expects the
 	 * caller to always add the new pool as the current pool
@@ -292,6 +314,10 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
 
 ref_fail:
 	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
+
+cpuhp_add_fail:
+	for_each_possible_cpu(cpu)
+		acomp_ctx_dealloc(per_cpu_ptr(pool->acomp_ctx, cpu));
 error:
 	if (pool->acomp_ctx)
 		free_percpu(pool->acomp_ctx);
@@ -322,9 +348,15 @@ static struct zswap_pool *__zswap_pool_create_fallback(void)
 
 static void zswap_pool_destroy(struct zswap_pool *pool)
 {
+	int cpu;
+
 	zswap_pool_debug("destroying", pool);
 
 	cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
+
+	for_each_possible_cpu(cpu)
+		acomp_ctx_dealloc(per_cpu_ptr(pool->acomp_ctx, cpu));
+
 	free_percpu(pool->acomp_ctx);
 
 	zs_destroy_pool(pool->zs_pool);
@@ -738,39 +770,36 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 {
 	struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
 	struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);
-	struct crypto_acomp *acomp = NULL;
-	struct acomp_req *req = NULL;
-	u8 *buffer = NULL;
-	int ret;
+	int ret = -ENOMEM;
 
-	buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail;
+	/*
+	 * To handle cases where the CPU goes through online-offline-online
+	 * transitions, we return if the acomp_ctx has already been initialized.
+	 */
+	if (acomp_ctx->acomp) {
+		WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
+		return 0;
 	}
 
-	acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
-	if (IS_ERR(acomp)) {
+	acomp_ctx->buffer = kmalloc_node(PAGE_SIZE, GFP_KERNEL, cpu_to_node(cpu));
+	if (!acomp_ctx->buffer)
+		return ret;
+
+	acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
+	if (IS_ERR(acomp_ctx->acomp)) {
 		pr_err("could not alloc crypto acomp %s : %pe\n",
-				pool->tfm_name, acomp);
-		ret = PTR_ERR(acomp);
+				pool->tfm_name, acomp_ctx->acomp);
+		ret = PTR_ERR(acomp_ctx->acomp);
 		goto fail;
 	}
 
-	req = acomp_request_alloc(acomp);
-	if (!req) {
+	acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
+	if (!acomp_ctx->req) {
 		pr_err("could not alloc crypto acomp_request %s\n",
 		       pool->tfm_name);
-		ret = -ENOMEM;
 		goto fail;
 	}
 
-	/*
-	 * Only hold the mutex after completing allocations, otherwise we may
-	 * recurse into zswap through reclaim and attempt to hold the mutex
-	 * again resulting in a deadlock.
-	 */
-	mutex_lock(&acomp_ctx->mutex);
 	crypto_init_wait(&acomp_ctx->wait);
 
 	/*
@@ -778,80 +807,17 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 	 * crypto_wait_req(); if the backend of acomp is scomp, the callback
 	 * won't be called, crypto_wait_req() will return without blocking.
 	 */
-	acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+	acomp_request_set_callback(acomp_ctx->req, CRYPTO_TFM_REQ_MAY_BACKLOG,
 				   crypto_req_done, &acomp_ctx->wait);
 
-	acomp_ctx->buffer = buffer;
-	acomp_ctx->acomp = acomp;
-	acomp_ctx->req = req;
-	mutex_unlock(&acomp_ctx->mutex);
+	mutex_init(&acomp_ctx->mutex);
 	return 0;
 
 fail:
-	if (!IS_ERR_OR_NULL(acomp))
-		crypto_free_acomp(acomp);
-	kfree(buffer);
+	acomp_ctx_dealloc(acomp_ctx);
 	return ret;
 }
 
-static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node)
-{
-	struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node);
-	struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu);
-	struct acomp_req *req;
-	struct crypto_acomp *acomp;
-	u8 *buffer;
-
-	if (IS_ERR_OR_NULL(acomp_ctx))
-		return 0;
-
-	mutex_lock(&acomp_ctx->mutex);
-	req = acomp_ctx->req;
-	acomp = acomp_ctx->acomp;
-	buffer = acomp_ctx->buffer;
-	acomp_ctx->req = NULL;
-	acomp_ctx->acomp = NULL;
-	acomp_ctx->buffer = NULL;
-	mutex_unlock(&acomp_ctx->mutex);
-
-	/*
-	 * Do the actual freeing after releasing the mutex to avoid subtle
-	 * locking dependencies causing deadlocks.
-	 */
-	if (!IS_ERR_OR_NULL(req))
-		acomp_request_free(req);
-	if (!IS_ERR_OR_NULL(acomp))
-		crypto_free_acomp(acomp);
-	kfree(buffer);
-
-	return 0;
-}
-
-static struct crypto_acomp_ctx *acomp_ctx_get_cpu_lock(struct zswap_pool *pool)
-{
-	struct crypto_acomp_ctx *acomp_ctx;
-
-	for (;;) {
-		acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
-		mutex_lock(&acomp_ctx->mutex);
-		if (likely(acomp_ctx->req))
-			return acomp_ctx;
-		/*
-		 * It is possible that we were migrated to a different CPU after
-		 * getting the per-CPU ctx but before the mutex was acquired. If
-		 * the old CPU got offlined, zswap_cpu_comp_dead() could have
-		 * already freed ctx->req (among other things) and set it to
-		 * NULL. Just try again on the new CPU that we ended up on.
-		 */
-		mutex_unlock(&acomp_ctx->mutex);
-	}
-}
-
-static void acomp_ctx_put_unlock(struct crypto_acomp_ctx *acomp_ctx)
-{
-	mutex_unlock(&acomp_ctx->mutex);
-}
-
 static bool zswap_compress(struct page *page, struct zswap_entry *entry,
 			   struct zswap_pool *pool)
 {
@@ -864,7 +830,9 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
 	u8 *dst;
 	bool mapped = false;
 
-	acomp_ctx = acomp_ctx_get_cpu_lock(pool);
+	acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
+	mutex_lock(&acomp_ctx->mutex);
+
 	dst = acomp_ctx->buffer;
 	sg_init_table(&input, 1);
 	sg_set_page(&input, page, PAGE_SIZE, 0);
@@ -930,7 +898,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
 	else if (alloc_ret)
 		zswap_reject_alloc_fail++;
 
-	acomp_ctx_put_unlock(acomp_ctx);
+	mutex_unlock(&acomp_ctx->mutex);
 	return comp_ret == 0 && alloc_ret == 0;
 }
 
@@ -942,7 +910,8 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
 	struct crypto_acomp_ctx *acomp_ctx;
 	int ret = 0, dlen;
 
-	acomp_ctx = acomp_ctx_get_cpu_lock(pool);
+	acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
+	mutex_lock(&acomp_ctx->mutex);
 	zs_obj_read_sg_begin(pool->zs_pool, entry->handle, input, entry->length);
 
 	/* zswap entries of length PAGE_SIZE are not compressed. */
@@ -961,7 +930,7 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
 	}
 
 	zs_obj_read_sg_end(pool->zs_pool, entry->handle);
-	acomp_ctx_put_unlock(acomp_ctx);
+	mutex_unlock(&acomp_ctx->mutex);
 
 	if (!ret && dlen == PAGE_SIZE)
 		return true;
@@ -1781,7 +1750,7 @@ static int zswap_setup(void)
 	ret = cpuhp_setup_state_multi(CPUHP_MM_ZSWP_POOL_PREPARE,
 				      "mm/zswap_pool:prepare",
 				      zswap_cpu_comp_prepare,
-				      zswap_cpu_comp_dead);
+				      NULL);
 	if (ret)
 		goto hp_fail;
 
-- 
2.39.5

Re: [PATCH 1/2] mm: zswap: Tie per-CPU acomp_ctx lifetime to the pool.

[Yosry Ahmed]

> diff --git a/mm/zswap.c b/mm/zswap.c
> index bdd24430f6ff..26de0468e33b 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -242,6 +242,20 @@ static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
>  **********************************/
>  static void __zswap_pool_empty(struct percpu_ref *ref);
>  
> +static void acomp_ctx_dealloc(struct crypto_acomp_ctx *acomp_ctx)

Nit: If you respin, please call this acomp_ctx_free(). It's more
consistent with other naming in zswap.c

Re: [PATCH 1/2] mm: zswap: Tie per-CPU acomp_ctx lifetime to the pool.

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 8:07 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index bdd24430f6ff..26de0468e33b 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -242,6 +242,20 @@ static inline struct xarray *swap_zswap_tree(swp_entry_t swp)
> >  **********************************/
> >  static void __zswap_pool_empty(struct percpu_ref *ref);
> >
> > +static void acomp_ctx_dealloc(struct crypto_acomp_ctx *acomp_ctx)
>
> Nit: If you respin, please call this acomp_ctx_free(). It's more
> consistent with other naming in zswap.c

Sure.

[PATCH 2/2] mm: zswap: Consistently use IS_ERR_OR_NULL() to check acomp_ctx resources.

[Kanchana P. Sridhar]

Use IS_ERR_OR_NULL() in zswap_cpu_comp_prepare() to check for valid
acomp/req, making it consistent with acomp_ctx_dealloc().

Co-developed-by: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
Signed-off-by: Kanchana P. Sridhar <kanchanapsridhar2026@gmail.com>
Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com>
Acked-by: Yosry Ahmed <yosry.ahmed@linux.dev>
Acked-by: Nhat Pham <nphamcs@gmail.com>
---
 mm/zswap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/zswap.c b/mm/zswap.c
index 26de0468e33b..6c6c14433556 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 		return ret;
 
 	acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
-	if (IS_ERR(acomp_ctx->acomp)) {
+	if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
 		pr_err("could not alloc crypto acomp %s : %pe\n",
 				pool->tfm_name, acomp_ctx->acomp);
 		ret = PTR_ERR(acomp_ctx->acomp);
@@ -794,7 +794,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
 	}
 
 	acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
-	if (!acomp_ctx->req) {
+	if (IS_ERR_OR_NULL(acomp_ctx->req)) {
 		pr_err("could not alloc crypto acomp_request %s\n",
 		       pool->tfm_name);
 		goto fail;
-- 
2.39.5

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Andrew Morton]

On Fri, 13 Mar 2026 22:16:30 -0700 "Kanchana P. Sridhar" <kanchanapsridhar2026@gmail.com> wrote:

> 
> This patchset persists the zswap pool's per-CPU acomp_ctx resources to
> last until the pool is destroyed. It then simplifies the per-CPU
> acomp_ctx mutex locking in zswap_compress()/zswap_decompress().
> 
> Further, it consistently uses the same checks for valid
> acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
> acomp_ctx members.

Sashiko has questions:
https://sashiko.dev/#/patchset/20260314051632.17931-1-kanchanapsridhar2026%40gmail.com

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

On Sat, Mar 14, 2026 at 5:11 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Mar 2026 22:16:30 -0700 "Kanchana P. Sridhar" <kanchanapsridhar2026@gmail.com> wrote:
>
> >
> > This patchset persists the zswap pool's per-CPU acomp_ctx resources to
> > last until the pool is destroyed. It then simplifies the per-CPU
> > acomp_ctx mutex locking in zswap_compress()/zswap_decompress().
> >
> > Further, it consistently uses the same checks for valid
> > acomp_ctx->acomp/req in zswap procedures that allocate/deallocate
> > acomp_ctx members.
>
> Sashiko has questions:
> https://sashiko.dev/#/patchset/20260314051632.17931-1-kanchanapsridhar2026%40gmail.com
>

Thanks Andrew, for sharing the questions raised by Sashiko. I have
provided some responses below:

> > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > return ret;
> >
> > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > - if (IS_ERR(acomp_ctx->acomp)) {
> > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> Does crypto_alloc_acomp_node() ever return NULL?
> Looking at the error handling just below this check, if this were to
> actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> the function to incorrectly return 0 (success) instead of an error code,
> hiding the allocation failure.

This is a good catch. Just to provide context, this patch was
introduced based on Yosry's earlier comments in [1].

[1]: https://patchwork.kernel.org/comment/26282128/

crypto_alloc_acomp_node() currently does not return NULL. However, it
could, in future.
Since the rest of zswap_cpu_comp_prepare() dereferences
acomp_ctx->acomp, it depends on whether we want to future-proof the
code to handle a possible eventuality of crypto_alloc_acomp_node()
returning NULL.

If the maintainers think future-proofing is beneficial, I would need
to handle the PTR_ERR(NULL) which would send a false success status.
If we don't think we need to handle a future NULL return from
crypto_alloc_acomp_node(), then I don't think this change is needed.
We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
maintainers' inputs on how to proceed.

> > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > - if (!acomp_ctx->req) {
> > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> Is this change necessary for acomp_request_alloc()?
> This function strictly returns NULL on allocation failure, not an error
> pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> without providing a functional benefit.

As of now, acomp_request_alloc() returns a valid "req" or NULL in case
of an error. Same question as above. The only benefit would be making
the code more robust to handle changes in the acomp API in future.

> > pr_err("could not alloc crypto acomp_request %s\n",
> > pool->tfm_name);
> > goto fail;
> Can this error path lead to a double-free or use-after-free?
> This isn't a bug introduced by this patch, but while reviewing the modified
> error path, it appears that when zswap_cpu_comp_prepare() fails and jumps
> to fail, acomp_ctx_dealloc() frees acomp_ctx->acomp and acomp_ctx->buffer
> but does not set those pointers to NULL.
> Because acomp_ctx is persistent per-CPU memory, if the CPU later comes back
> online, the early initialization check:
> if (acomp_ctx->acomp) {
> WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp));
> return 0;
> }
> would see the dangling pointer, incorrectly return success, and leave the
> CPU with an uninitialized mutex and freed buffer.

Thanks again, good catch. I was thinking some more about this today,
and realized this could happen if let's say a batch of CPUs were
online at pool creation time, and the startup callback succeeds for
all of them. Now, if a CPU "x" is brought online after pool creation
succeeds, encounters an error in zswap_cpu_comp_prepare(), calls
acomp_ctx_dealloc(), then CPU "x"'s state rewinding to CPU_OFFLINE. If
the CPU "x" is again brought back online, the above use-after-free
would occur. I had missed this during my response to Yosry's v14
comment [2] yesterday, sorry about that.

I will set the acomp_ctx pointers to NULL, as Yosry suggested in [2].

[2]: https://patchwork.kernel.org/comment/26773983/

> Additionally, if the pool creation fails entirely, the fallback cleanup loop
> in zswap_pool_create() calls acomp_ctx_dealloc() again for all CPUs, which
> would cause a double-free on the CPU that initially failed.
> Should acomp_ctx_dealloc() clear these pointers after freeing them?

Yes, I agree, and will include a fix in v2 once I get the maintainers'
feedback on how to proceed on patch 2.

Thanks,
Kanchana

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed]

> > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > return ret;
> > >
> > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > Does crypto_alloc_acomp_node() ever return NULL?
> > Looking at the error handling just below this check, if this were to
> > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > the function to incorrectly return 0 (success) instead of an error code,
> > hiding the allocation failure.
> 
> This is a good catch. Just to provide context, this patch was
> introduced based on Yosry's earlier comments in [1].
> 
> [1]: https://patchwork.kernel.org/comment/26282128/
> 
> crypto_alloc_acomp_node() currently does not return NULL. However, it
> could, in future.
> Since the rest of zswap_cpu_comp_prepare() dereferences
> acomp_ctx->acomp, it depends on whether we want to future-proof the
> code to handle a possible eventuality of crypto_alloc_acomp_node()
> returning NULL.

Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
better documentation for the API, and the incossitency between this code
and acomp_ctx_dealloc() is arguably documenting that the function can
only return an ERR, but it can also be NULL-initialized by zswap.

> 
> If the maintainers think future-proofing is beneficial, I would need
> to handle the PTR_ERR(NULL) which would send a false success status.
> If we don't think we need to handle a future NULL return from
> crypto_alloc_acomp_node(), then I don't think this change is needed.
> We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> maintainers' inputs on how to proceed.
> 
> > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > - if (!acomp_ctx->req) {
> > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > Is this change necessary for acomp_request_alloc()?
> > This function strictly returns NULL on allocation failure, not an error
> > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > without providing a functional benefit.
> 
> As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> of an error. Same question as above. The only benefit would be making
> the code more robust to handle changes in the acomp API in future.

For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
to begin with? If acomp_request_alloc() only returns NULL, maybe that
should also be a NULL check?

In this case, we don't really need to make any changes here, and I think
this patch can just be dropped.

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed]

On Mon, Mar 16, 2026 at 03:06:32PM +0000, Yosry Ahmed wrote:
> > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > return ret;
> > > >
> > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > Does crypto_alloc_acomp_node() ever return NULL?
> > > Looking at the error handling just below this check, if this were to
> > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > the function to incorrectly return 0 (success) instead of an error code,
> > > hiding the allocation failure.
> > 
> > This is a good catch. Just to provide context, this patch was
> > introduced based on Yosry's earlier comments in [1].
> > 
> > [1]: https://patchwork.kernel.org/comment/26282128/
> > 
> > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > could, in future.
> > Since the rest of zswap_cpu_comp_prepare() dereferences
> > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > returning NULL.
> 
> Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> better documentation for the API, and the incossitency between this code
> and acomp_ctx_dealloc() is arguably documenting that the function can
> only return an ERR, but it can also be NULL-initialized by zswap.

Also, sorry for leading you astray in the first place. Looking at this
initially I thought the inconsistency was confusing, but looking at it
with fresh eyes I think it better documents the API and the different
callsites.

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 8:09 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 03:06:32PM +0000, Yosry Ahmed wrote:
> > > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > > return ret;
> > > > >
> > > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > > Does crypto_alloc_acomp_node() ever return NULL?
> > > > Looking at the error handling just below this check, if this were to
> > > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > > the function to incorrectly return 0 (success) instead of an error code,
> > > > hiding the allocation failure.
> > >
> > > This is a good catch. Just to provide context, this patch was
> > > introduced based on Yosry's earlier comments in [1].
> > >
> > > [1]: https://patchwork.kernel.org/comment/26282128/
> > >
> > > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > > could, in future.
> > > Since the rest of zswap_cpu_comp_prepare() dereferences
> > > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > > returning NULL.
> >
> > Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> > better documentation for the API, and the incossitency between this code
> > and acomp_ctx_dealloc() is arguably documenting that the function can
> > only return an ERR, but it can also be NULL-initialized by zswap.
>
> Also, sorry for leading you astray in the first place. Looking at this
> initially I thought the inconsistency was confusing, but looking at it
> with fresh eyes I think it better documents the API and the different
> callsites.

No worries.

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 8:06 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > @@ -786,7 +786,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> > > > return ret;
> > > >
> > > > acomp_ctx->acomp = crypto_alloc_acomp_node(pool->tfm_name, 0, 0, cpu_to_node(cpu));
> > > > - if (IS_ERR(acomp_ctx->acomp)) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->acomp)) {
> > > Does crypto_alloc_acomp_node() ever return NULL?
> > > Looking at the error handling just below this check, if this were to
> > > actually return NULL, PTR_ERR(NULL) evaluates to 0. This would cause
> > > the function to incorrectly return 0 (success) instead of an error code,
> > > hiding the allocation failure.
> >
> > This is a good catch. Just to provide context, this patch was
> > introduced based on Yosry's earlier comments in [1].
> >
> > [1]: https://patchwork.kernel.org/comment/26282128/
> >
> > crypto_alloc_acomp_node() currently does not return NULL. However, it
> > could, in future.
> > Since the rest of zswap_cpu_comp_prepare() dereferences
> > acomp_ctx->acomp, it depends on whether we want to future-proof the
> > code to handle a possible eventuality of crypto_alloc_acomp_node()
> > returning NULL.
>
> Hmm upon revisiting this, I think keeping this as IS_ERR() here is a
> better documentation for the API, and the incossitency between this code
> and acomp_ctx_dealloc() is arguably documenting that the function can
> only return an ERR, but it can also be NULL-initialized by zswap.

Yes, makes sense.

>
> >
> > If the maintainers think future-proofing is beneficial, I would need
> > to handle the PTR_ERR(NULL) which would send a false success status.
> > If we don't think we need to handle a future NULL return from
> > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > maintainers' inputs on how to proceed.
> >
> > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > - if (!acomp_ctx->req) {
> > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > Is this change necessary for acomp_request_alloc()?
> > > This function strictly returns NULL on allocation failure, not an error
> > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > without providing a functional benefit.
> >
> > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > of an error. Same question as above. The only benefit would be making
> > the code more robust to handle changes in the acomp API in future.
>
> For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> to begin with? If acomp_request_alloc() only returns NULL, maybe that
> should also be a NULL check?

This one is debatable, since acomp_ctx_dealloc() is intended to
replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
replacing this with IS_NULL(req) makes sense, but would like to
confirm with you if changing existing behavior is Ok.

>
> In this case, we don't really need to make any changes here, and I think
> this patch can just be dropped.

I agree.

Thanks,
Kanchana

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed]

> > > If the maintainers think future-proofing is beneficial, I would need
> > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > If we don't think we need to handle a future NULL return from
> > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > maintainers' inputs on how to proceed.
> > >
> > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > - if (!acomp_ctx->req) {
> > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > Is this change necessary for acomp_request_alloc()?
> > > > This function strictly returns NULL on allocation failure, not an error
> > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > without providing a functional benefit.
> > >
> > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > of an error. Same question as above. The only benefit would be making
> > > the code more robust to handle changes in the acomp API in future.
> >
> > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > should also be a NULL check?
>
> This one is debatable, since acomp_ctx_dealloc() is intended to
> replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> replacing this with IS_NULL(req) makes sense, but would like to
> confirm with you if changing existing behavior is Ok.

I think it's fine as long as acomp_request_alloc() never returns an
error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
in the changelog, to avoid hiding that change within the bigger patch.

Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
check on acomp_ctx also misleading? Should that also just be a NULL
check?

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
>
> > > > If the maintainers think future-proofing is beneficial, I would need
> > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > If we don't think we need to handle a future NULL return from
> > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > maintainers' inputs on how to proceed.
> > > >
> > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > - if (!acomp_ctx->req) {
> > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > Is this change necessary for acomp_request_alloc()?
> > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > without providing a functional benefit.
> > > >
> > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > of an error. Same question as above. The only benefit would be making
> > > > the code more robust to handle changes in the acomp API in future.
> > >
> > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > should also be a NULL check?
> >
> > This one is debatable, since acomp_ctx_dealloc() is intended to
> > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > replacing this with IS_NULL(req) makes sense, but would like to
> > confirm with you if changing existing behavior is Ok.
>
> I think it's fine as long as acomp_request_alloc() never returns an
> error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> in the changelog, to avoid hiding that change within the bigger patch.

Sounds good.

>
> Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> check on acomp_ctx also misleading? Should that also just be a NULL
> check?

Even a NULL check would be redundant in this case, per my
understanding, because if the alloc_percpu() call in
zswap_pool_create() had failed, pool creation would have failed.

I think a NULL check on the acomp_ctx would still be a good idea, just
in case, since this is all part of CPU hotplug. I agree, we don't need
an IS_ERR() check on acomp_ctx.

Thanks,
Kanchana

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed]

On Mon, Mar 16, 2026 at 12:21 PM Kanchana P. Sridhar
<kanchanapsridhar2026@gmail.com> wrote:
>
> On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
> >
> > > > > If the maintainers think future-proofing is beneficial, I would need
> > > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > > If we don't think we need to handle a future NULL return from
> > > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > > maintainers' inputs on how to proceed.
> > > > >
> > > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > > - if (!acomp_ctx->req) {
> > > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > > Is this change necessary for acomp_request_alloc()?
> > > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > > without providing a functional benefit.
> > > > >
> > > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > > of an error. Same question as above. The only benefit would be making
> > > > > the code more robust to handle changes in the acomp API in future.
> > > >
> > > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > > should also be a NULL check?
> > >
> > > This one is debatable, since acomp_ctx_dealloc() is intended to
> > > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > > replacing this with IS_NULL(req) makes sense, but would like to
> > > confirm with you if changing existing behavior is Ok.
> >
> > I think it's fine as long as acomp_request_alloc() never returns an
> > error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> > to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> > in the changelog, to avoid hiding that change within the bigger patch.
>
> Sounds good.
>
> >
> > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > check on acomp_ctx also misleading? Should that also just be a NULL
> > check?
>
> Even a NULL check would be redundant in this case, per my
> understanding, because if the alloc_percpu() call in
> zswap_pool_create() had failed, pool creation would have failed.
>
> I think a NULL check on the acomp_ctx would still be a good idea, just
> in case, since this is all part of CPU hotplug. I agree, we don't need
> an IS_ERR() check on acomp_ctx.

So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
checks, and then the current patch 1, right?

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Kanchana P. Sridhar]

On Mon, Mar 16, 2026 at 12:24 PM Yosry Ahmed <yosry@kernel.org> wrote:
>
> On Mon, Mar 16, 2026 at 12:21 PM Kanchana P. Sridhar
> <kanchanapsridhar2026@gmail.com> wrote:
> >
> > On Mon, Mar 16, 2026 at 11:30 AM Yosry Ahmed <yosry@kernel.org> wrote:
> > >
> > > > > > If the maintainers think future-proofing is beneficial, I would need
> > > > > > to handle the PTR_ERR(NULL) which would send a false success status.
> > > > > > If we don't think we need to handle a future NULL return from
> > > > > > crypto_alloc_acomp_node(), then I don't think this change is needed.
> > > > > > We could leave it as IS_ERR(acomp_ctx->acomp). I would like to get the
> > > > > > maintainers' inputs on how to proceed.
> > > > > >
> > > > > > > > acomp_ctx->req = acomp_request_alloc(acomp_ctx->acomp);
> > > > > > > > - if (!acomp_ctx->req) {
> > > > > > > > + if (IS_ERR_OR_NULL(acomp_ctx->req)) {
> > > > > > > Is this change necessary for acomp_request_alloc()?
> > > > > > > This function strictly returns NULL on allocation failure, not an error
> > > > > > > pointer. Changing this to IS_ERR_OR_NULL() obscures the actual API contract
> > > > > > > without providing a functional benefit.
> > > > > >
> > > > > > As of now, acomp_request_alloc() returns a valid "req" or NULL in case
> > > > > > of an error. Same question as above. The only benefit would be making
> > > > > > the code more robust to handle changes in the acomp API in future.
> > > > >
> > > > > For this one, do we need to do IS_ERR_OR_NULL() in acomp_ctx_dealloc()
> > > > > to begin with? If acomp_request_alloc() only returns NULL, maybe that
> > > > > should also be a NULL check?
> > > >
> > > > This one is debatable, since acomp_ctx_dealloc() is intended to
> > > > replace zswap_cpu_comp_dead(), which has the IS_ERR_OR_NULL(). I think
> > > > replacing this with IS_NULL(req) makes sense, but would like to
> > > > confirm with you if changing existing behavior is Ok.
> > >
> > > I think it's fine as long as acomp_request_alloc() never returns an
> > > error. Maybe do it in a separate patch first, change IS_ERR_OR_NULL()
> > > to a NULL check in zswap_cpu_comp_dead(), with the reasoning explained
> > > in the changelog, to avoid hiding that change within the bigger patch.
> >
> > Sounds good.
> >
> > >
> > > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > > check on acomp_ctx also misleading? Should that also just be a NULL
> > > check?
> >
> > Even a NULL check would be redundant in this case, per my
> > understanding, because if the alloc_percpu() call in
> > zswap_pool_create() had failed, pool creation would have failed.
> >
> > I think a NULL check on the acomp_ctx would still be a good idea, just
> > in case, since this is all part of CPU hotplug. I agree, we don't need
> > an IS_ERR() check on acomp_ctx.
>
> So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
> checks, and then the current patch 1, right?

Yes.

Re: [PATCH 0/2] zswap pool per-CPU acomp_ctx simplifications

[Yosry Ahmed]

> > > > Actually looking at zswap_cpu_comp_dead(), is the IS_ERR_OR_NULL()
> > > > check on acomp_ctx also misleading? Should that also just be a NULL
> > > > check?
> > >
> > > Even a NULL check would be redundant in this case, per my
> > > understanding, because if the alloc_percpu() call in
> > > zswap_pool_create() had failed, pool creation would have failed.
> > >
> > > I think a NULL check on the acomp_ctx would still be a good idea, just
> > > in case, since this is all part of CPU hotplug. I agree, we don't need
> > > an IS_ERR() check on acomp_ctx.
> >
> > So I think we do one patch to convert both IS_ERR_OR_NULL() to NULL
> > checks, and then the current patch 1, right?
>
> Yes.

Sounds good, thanks for bearing with me :)