| 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: <SJ2PR11MB84729257422FACB33E315958C9AFA@SJ2PR11MB8472.namprd11.prod.outlook.com>
Date: Sat, 13 Dec 2025 19:53:16 +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: Friday, December 12, 2025 2:25 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 Fri, Dec 12, 2025 at 08:53:13PM +0000, Sridhar, Kanchana P wrote:
> [..]
> > > On Fri, Dec 12, 2025 at 06:17:07PM +0000, Sridhar, Kanchana P wrote:
> > > > >
> > > > > > 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.
> > >
> > > What I mean is, when cpuhp_state_add_instance() fails we goto ref_fail
> > > which will call cpuhp_state_remove_instance(). But the current code does
> > > not call cpuhp_state_remove_instance() if cpuhp_state_add_instance()
> > > fails.
> >
> > I see what you mean. The current mainline code does not call
> > cpuhp_state_remove_instance() if cpuhp_state_add_instance() fails,
> because
> > the cpuhotplug code will call the teardown callback in this case.
> >
> > In this patch, I do need to call cpuhp_state_remove_instance() and
> > acomp_ctx_dealloc() in this case because there is no teardown callback
> > being registered.
>
> Hmm looking at cpuhp_state_add_instance(), it seems like it doesn't add
> the node to the list on failure. cpuhp_state_remove_instance() only
> removes the node from the list when there's no teardown cb, so it will
> be a nop in this case.
>
> What we need to do is manual cleanup, since there is no teardown cb,
> which is already being done by acomp_ctx_dealloc() IIUC.
>
> So I think calling cpuhp_state_remove_instance() when
> cpuhp_state_add_instance() fails is not needed, and I don't see other
> callers doing it.
You are right. I too have verified this. I will create a label for the call
to acomp_ctx_dealloc() and fix this.
>
> [..]
> > > > > > @@ -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.
> > >
> > > I think I am confused. I thought now we don't free this on CPU offline,
> > > so either it's NULL because this is the first time we initialize it on
> > > this CPU, or it is allocated.
> >
> > Yes, this is correct.
> >
> > > If it is an ERR value, then the pool
> > > creation should have failed and we wouldn't be calling this again on CPU
> > > online.
> > >
> > > In other words, what scenario do we expect to legitimately see an ERR
> > > value here?
> >
> > I am using "(!IS_ERR_OR_NULL(acomp_ctx->acomp)" as a check for the
> > acomp being allocated already. I could instead have used "if (acomp_ctx-
> >acomp)",
> > but use the former to be consistent with patch 20/22.
> >
> > I cannot think of a scenario where we can expect an ERR value here.
>
> Yeah maybe do if (acomp_ctx->acomp) and
> WARN_ON_ONCE(IS_ERR(acomp_ctx->acomp))?
Sure.
Thanks,
Kanchana
Powered by blists - more mailing lists