| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <SJ2PR11MB84729A04737171FCF31DB9FDC9AEA@SJ2PR11MB8472.namprd11.prod.outlook.com>
Date: Fri, 12 Dec 2025 18:17:07 +0000
From: "Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>, "hannes@...xchg.org"
<hannes@...xchg.org>, "nphamcs@...il.com" <nphamcs@...il.com>,
"chengming.zhou@...ux.dev" <chengming.zhou@...ux.dev>,
"usamaarif642@...il.com" <usamaarif642@...il.com>, "ryan.roberts@....com"
<ryan.roberts@....com>, "21cnbao@...il.com" <21cnbao@...il.com>,
"ying.huang@...ux.alibaba.com" <ying.huang@...ux.alibaba.com>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"senozhatsky@...omium.org" <senozhatsky@...omium.org>, "sj@...nel.org"
<sj@...nel.org>, "kasong@...cent.com" <kasong@...cent.com>,
"linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
"herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
"davem@...emloft.net" <davem@...emloft.net>, "clabbe@...libre.com"
<clabbe@...libre.com>, "ardb@...nel.org" <ardb@...nel.org>,
"ebiggers@...gle.com" <ebiggers@...gle.com>, "surenb@...gle.com"
<surenb@...gle.com>, "Accardi, Kristen C" <kristen.c.accardi@...el.com>,
"Gomes, Vinicius" <vinicius.gomes@...el.com>, "Feghali, Wajdi K"
<wajdi.k.feghali@...el.com>, "Gopal, Vinodh" <vinodh.gopal@...el.com>,
"Sridhar, Kanchana P" <kanchana.p.sridhar@...el.com>
Subject: RE: [PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources exist
from pool creation to deletion.
> -----Original Message-----
> From: Yosry Ahmed <yosry.ahmed@...ux.dev>
> Sent: Thursday, November 13, 2025 12:24 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@...el.com>
> Cc: linux-kernel@...r.kernel.org; linux-mm@...ck.org;
> hannes@...xchg.org; nphamcs@...il.com; chengming.zhou@...ux.dev;
> usamaarif642@...il.com; ryan.roberts@....com; 21cnbao@...il.com;
> ying.huang@...ux.alibaba.com; akpm@...ux-foundation.org;
> senozhatsky@...omium.org; sj@...nel.org; kasong@...cent.com; linux-
> crypto@...r.kernel.org; herbert@...dor.apana.org.au;
> davem@...emloft.net; clabbe@...libre.com; ardb@...nel.org;
> ebiggers@...gle.com; surenb@...gle.com; Accardi, Kristen C
> <kristen.c.accardi@...el.com>; Gomes, Vinicius <vinicius.gomes@...el.com>;
> Feghali, Wajdi K <wajdi.k.feghali@...el.com>; Gopal, Vinodh
> <vinodh.gopal@...el.com>
> Subject: Re: [PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources
> exist from pool creation to deletion.
>
> On Tue, Nov 04, 2025 at 01:12:32AM -0800, Kanchana P Sridhar wrote:
>
[...]
> > mm/zswap.c | 164 +++++++++++++++++++++--------------------------------
> > 1 file changed, 64 insertions(+), 100 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 4897ed689b9f..87d50786f61f 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,26 @@ 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.
> > + * In case of errors, we need to goto "ref_fail" instead of "error"
> > + * because there is no teardown callback registered anymore, for
> > + * cpuhp_state_add_instance() to de-allocate resources as it rolls
> back
> > + * state on cores before the CPU on which error was encountered.
> > + */
>
> Do we need to manually call acomp_ctx_dealloc() on each CPU on failure
> because cpuhp_state_add_instance() relies on the hotunplug callback for
> cleanup, and we don't have any?
That's correct.
>
> If that's the case:
>
> /*
> * cpuhp_state_add_instance() will not cleanup on failure since
> * we don't register a hotunplug callback.
> */
>
> Describing what the code does is not helpful, and things like "anymore"
> do not make sense once the code is merged.
Ok.
>
> > ret =
> cpuhp_state_add_instance(CPUHP_MM_ZSWP_POOL_PREPARE,
> > &pool->node);
> > if (ret)
> > - goto error;
> > + goto ref_fail;
>
> IIUC we shouldn't call cpuhp_state_remove_instance() on failure, we
> probably should add a new label.
In this case we should because it is part of the pool creation failure
handling flow, at the end of which, the pool will be deleted.
>
> >
> > /* being the current pool takes 1 ref; this func expects the
> > * caller to always add the new pool as the current pool
> > @@ -292,6 +313,9 @@ static struct zswap_pool *zswap_pool_create(char
> *compressor)
> >
> > ref_fail:
> > 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));
> > error:
> > if (pool->acomp_ctx)
> > free_percpu(pool->acomp_ctx);
> > @@ -322,9 +346,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);
> > @@ -736,39 +766,35 @@ 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 (!IS_ERR_OR_NULL(acomp_ctx->acomp))
> > + return 0;
>
> Is it possible for acomp_ctx->acomp to be an ERR value here? If it is,
> then zswap initialization should have failed. Maybe WARN_ON_ONCE() for
> that case?
This is checking for a valid acomp_ctx->acomp using the same criteria
being uniformly used in acomp_ctx_dealloc(). This check is necessary to
handle the case where the CPU goes through online-offline-online state
transitions.
Thanks,
Kanchana
>
> >
> > - 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 : %ld\n",
> > - pool->tfm_name, PTR_ERR(acomp));
> > - ret = PTR_ERR(acomp);
> > + pool->tfm_name, PTR_ERR(acomp_ctx-
> >acomp));
> > + ret = PTR_ERR(acomp_ctx->acomp);
> > goto fail;
> > }
> > + acomp_ctx->is_sleepable = acomp_is_async(acomp_ctx->acomp);
> >
> > - 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);
> >
> > /*
> [..]
Powered by blists - more mailing lists