Re: [PATCH v8 12/14] mm: zswap: Simplify acomp_ctx resource allocation/deletion and mutex lock usage.

[Yosry Ahmed]

On Sat, Mar 08, 2025 at 02:47:15AM +0000, Sridhar, Kanchana P wrote:
> 
[..]
> > > > >  	u8 *buffer;
> > > > > +	u8 nr_reqs;
> > > > > +	struct crypto_wait wait;
> > > > >  	struct mutex mutex;
> > > > >  	bool is_sleepable;
> > > > > +	bool __online;
> > > >
> > > > I don't believe we need this.
> > > >
> > > > If we are not freeing resources during CPU offlining, then we do not
> > > > need a CPU offline callback and acomp_ctx->__online serves no purpose.
> > > >
> > > > The whole point of synchronizing between offlining and
> > > > compress/decompress operations is to avoid UAF. If offlining does not
> > > > free resources, then we can hold the mutex directly in the
> > > > compress/decompress path and drop the hotunplug callback completely.
> > > >
> > > > I also believe nr_reqs can be dropped from this patch, as it seems like
> > > > it's only used know when to set __online.
> > >
> > > All great points! In fact, that was the original solution I had implemented
> > > (not having an offline callback). But then, I spent some time understanding
> > > the v6.13 hotfix for synchronizing freeing of resources, and this comment
> > > in zswap_cpu_comp_prepare():
> > >
> > > 	/*
> > > 	 * 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.
> > > 	 */
> > >
> > > Hence, I figured the constraint of "recurse into zswap through reclaim" was
> > > something to comprehend in the simplification (even though I had a tough
> > > time imagining how this could happen).
> > 
> > The constraint here is about zswap_cpu_comp_prepare() holding the mutex,
> > making an allocation which internally triggers reclaim, then recursing
> > into zswap and trying to hold the same mutex again causing a deadlock.
> > 
> > If zswap_cpu_comp_prepare() does not need to hold the mutex to begin
> > with, the constraint naturally goes away.
> 
> Actually, if it is possible for the allocations in zswap_cpu_comp_prepare()
> to trigger reclaim, then I believe we need some way for reclaim to know if
> the acomp_ctx resources are available. Hence, this seems like a potential
> for deadlock regardless of the mutex.

I took a closer look and I believe my hotfix was actually unnecessary. I
sent it out in response to a syzbot report, but upon closer look it
seems like it was not an actual problem. Sorry if my patch confused you.

Looking at enum cpuhp_state in include/linux/cpuhotplug.h, it seems like
CPUHP_MM_ZSWP_POOL_PREPARE is in the PREPARE section. The comment above
says:

 * PREPARE: The callbacks are invoked on a control CPU before the
 * hotplugged CPU is started up or after the hotplugged CPU has died.

So even if we go into reclaim during zswap_cpu_comp_prepare(), it will
never be on the CPU that we are allocating resources for.

The other case where zswap_cpu_comp_prepare() could race with
compression/decompression is when a pool is being created. In this case,
reclaim from zswap_cpu_comp_prepare() can recurse into zswap on the same
CPU AFAICT. However, because the pool is still under creation, it will
not be used (i.e. zswap_pool_current_get() won't find it).

So I think we don't need to worry about zswap_cpu_comp_prepare() racing
with compression or decompression for the same pool and CPU.

> 
> I verified that all the zswap_cpu_comp_prepare() allocations are done with
> GFP_KERNEL, which implicitly allows direct reclaim. So this appears to be a
> risk for deadlock between zswap_compress() and zswap_cpu_comp_prepare()
> in general, i.e., aside from this patchset.
> 
> I can think of the following options to resolve this, and would welcome
> other suggestions:
> 
> 1) Less intrusive: acomp_ctx_get_cpu_lock() should get the mutex, check
>     if acomp_ctx->__online is true, and if so, return the mutex. If
>     acomp_ctx->__online is false, then it returns NULL. In other words, we
>     don't have the for loop.
>     - This will cause recursions into direct reclaim from zswap_cpu_comp_prepare()
>        to fail, cpuhotplug to fail. However, there is no deadlock.
>         - zswap_compress() will need to detect NULL returned by
>           acomp_ctx_get_cpu_lock(), and return an error.
>         - zswap_decompress() will need a BUG_ON(!acomp_ctx) after calling
>           acomp_ctx_get_cpu_lock().
>     - We won't be migrated to a different CPU because we hold the mutex, hence
>       zswap_cpu_comp_dead() will wait on the mutex.
> 
> 2) More intrusive: We would need to use a gfp_t that prevents direct reclaim
>     and kswapd, i.e., something similar to GFP_TRANSHUGE_LIGHT in gfp_types.h,
>     but for non-THP allocations. If we decide to adopt this approach, we would
>     need changes in include/crypto/acompress.h, crypto/api.c, and crypto/acompress.c
>     to allow crypto_create_tfm_node() to call crypto_alloc_tfmmem() with this
>     new gfp_t, in lieu of GFP_KERNEL.
> 
> > 
> > >
> > > Hence, I added the "bool __online" because zswap_cpu_comp_prepare()
> > > does not acquire the mutex lock while allocating resources. We have
> > already
> > > initialized the mutex, so in theory, it is possible for compress/decompress
> > > to acquire the mutex lock. The __online acts as a way to indicate whether
> > > compress/decompress can proceed reliably to use the resources.
> > 
> > For compress/decompress to acquire the mutex they need to run on that
> > CPU, and I don't think that's possible before onlining completes, so
> > zswap_cpu_comp_prepare() must have already completed before
> > compress/decompress can use that CPU IIUC.
> 
> If we can make this assumption, that would be great! However, I am not
> totally sure because of the GFP_KERNEL allocations in
> zswap_cpu_comp_prepare().

As I mentioned above, when zswap_cpu_comp_prepare() is run we are in one
of two situations:
- The pool is under creation, so we cannot race with stores/loads from
  that same pool.
- The CPU is being onlined, in which case zswap_cpu_comp_prepare() is
  called from a control CPU before tasks start running on the CPU being
  onlined.

Please correct me if I am wrong.

[..]
> > > > > @@ -285,13 +403,21 @@ static struct zswap_pool
> > > > *zswap_pool_create(char *type, char *compressor)
> > > > >  		goto error;
> > > > >  	}
> > > > >
> > > > > -	for_each_possible_cpu(cpu)
> > > > > -		mutex_init(&per_cpu_ptr(pool->acomp_ctx, cpu)->mutex);
> > > > > +	for_each_possible_cpu(cpu) {
> > > > > +		struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool-
> > > > >acomp_ctx, cpu);
> > > > > +
> > > > > +		acomp_ctx->acomp = NULL;
> > > > > +		acomp_ctx->req = NULL;
> > > > > +		acomp_ctx->buffer = NULL;
> > > > > +		acomp_ctx->__online = false;
> > > > > +		acomp_ctx->nr_reqs = 0;
> > > >
> > > > Why is this needed? Wouldn't zswap_cpu_comp_prepare() initialize them
> > > > right away?
> > >
> > > Yes, I figured this is needed for two reasons:
> > >
> > > 1) For the error handling in zswap_cpu_comp_prepare() and calls into
> > >     zswap_cpu_comp_dealloc() to be handled by the common procedure
> > >     "acomp_ctx_dealloc()" unambiguously.
> > 
> > This makes sense. When you move the refactoring to create
> > acomp_ctx_dealloc() to a separate patch, please include this change in
> > it and call it out explicitly in the commit message.
> 
> Sure.
> 
> > 
> > > 2) The second scenario I thought of that would need this, is let's say
> > >      the zswap compressor is switched immediately after setting the
> > >      compressor. Some cores have executed the onlining code and
> > >      some haven't. Because there are no pool refs held,
> > >      zswap_cpu_comp_dealloc() would be called per-CPU. Hence, I figured
> > >      it would help to initialize these acomp_ctx members before the
> > >      hand-off to "cpuhp_state_add_instance()" in zswap_pool_create().
> > 
> > I believe cpuhp_state_add_instance() calls the onlining function
> > synchronously on all present CPUs, so I don't think it's possible to end
> > up in a state where the pool is being destroyed and some CPU executed
> > zswap_cpu_comp_prepare() while others haven't.
> 
> I looked at the cpuhotplug code some more. The startup callback is
> invoked sequentially for_each_present_cpu(). If an error occurs for any
> one of them, it calls the teardown callback only on the earlier cores that
> have already finished running the startup callback. However, 
> zswap_cpu_comp_dealloc() will be called for all cores, even the ones
> for which the startup callback was not run. Hence, I believe the
> zero initialization is useful, albeit using alloc_percpu_gfp(__GFP_ZERO)
> to allocate the acomp_ctx.

Yeah this is point (1) above IIUC, and I agree about zero initialization
for that.

> 
> > 
> > That being said, this made me think of a different problem. If pool
> > destruction races with CPU onlining, there could be a race between
> > zswap_cpu_comp_prepare() allocating resources and
> > zswap_cpu_comp_dealloc() (or acomp_ctx_dealloc()) freeing them.
> > 
> > I believe we must always call cpuhp_state_remove_instance() *before*
> > freeing the resources to prevent this race from happening. This needs to
> > be documented with a comment.
> 
> Yes, this race condition is possible, thanks for catching this! The problem with
> calling cpuhp_state_remove_instance() before freeing the resources is that
> cpuhp_state_add_instance() and cpuhp_state_remove_instance() both
> acquire a "mutex_lock(&cpuhp_state_mutex);" at the beginning; and hence
> are serialized.
> 
> For the reasons motivating why acomp_ctx->__online is set to false in
> zswap_cpu_comp_dead(), I cannot call cpuhp_state_remove_instance()
> before calling acomp_ctx_dealloc() because the latter could wait until
> acomp_ctx->__online to be true before deleting the resources. I will
> think about this some more.

I believe this problem goes away with acomp_ctx->__online going away,
right?

> 
> Another possibility is to not rely on cpuhotplug in zswap, and instead
> manage the per-cpu acomp_ctx resource allocation entirely in
> zswap_pool_create(), and deletion entirely in zswap_pool_destroy(),
> along with the necessary error handling. Let me think about this some
> more as well.
> 
> > 
> > Let me know if I missed something.
> > 
> > >
> > > Please let me know if these are valid considerations.
> > >
> > > >
> > > > If it is in fact needed we should probably just use __GFP_ZERO.
> > >
> > > Sure. Are you suggesting I use "alloc_percpu_gfp()" instead of
> > "alloc_percpu()"
> > > for the acomp_ctx?
> > 
> > Yeah if we need to initialize all/most fields to 0 let's use
> > alloc_percpu_gfp() and pass GFP_KERNEL | __GFP_ZERO.
> 
> Sounds good.
> 
> > 
> > [..]
> > > > > @@ -902,16 +957,52 @@ static struct crypto_acomp_ctx
> > > > *acomp_ctx_get_cpu_lock(struct zswap_pool *pool)
> > > > >
> > > > >  	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.
> > > > > +		 * If the CPU onlining code successfully allocates acomp_ctx
> > > > resources,
> > > > > +		 * it sets acomp_ctx->__online to true. Until this happens, we
> > > > have
> > > > > +		 * two options:
> > > > > +		 *
> > > > > +		 * 1. Return NULL and fail all stores on this CPU.
> > > > > +		 * 2. Retry, until onlining has finished allocating resources.
> > > > > +		 *
> > > > > +		 * In theory, option 1 could be more appropriate, because it
> > > > > +		 * allows the calling procedure to decide how it wants to
> > > > handle
> > > > > +		 * reclaim racing with CPU hotplug. For instance, it might be
> > > > Ok
> > > > > +		 * for compress to return an error for the backing swap device
> > > > > +		 * to store the folio. Decompress could wait until we get a
> > > > > +		 * valid and locked mutex after onlining has completed. For
> > > > now,
> > > > > +		 * we go with option 2 because adding a do-while in
> > > > > +		 * zswap_decompress() adds latency for software
> > > > compressors.
> > > > > +		 *
> > > > > +		 * Once initialized, the resources will be de-allocated only
> > > > > +		 * when the pool is destroyed. The acomp_ctx will hold on to
> > > > the
> > > > > +		 * resources through CPU offlining/onlining at any time until
> > > > > +		 * the pool is destroyed.
> > > > > +		 *
> > > > > +		 * This prevents races/deadlocks between reclaim and CPU
> > > > acomp_ctx
> > > > > +		 * resource allocation that are a dependency for reclaim.
> > > > > +		 * It further simplifies the interaction with CPU onlining and
> > > > > +		 * offlining:
> > > > > +		 *
> > > > > +		 * - CPU onlining does not take the mutex. It only allocates
> > > > > +		 *   resources and sets __online to true.
> > > > > +		 * - CPU offlining acquires the mutex before setting
> > > > > +		 *   __online to false. If reclaim has acquired the mutex,
> > > > > +		 *   offlining will have to wait for reclaim to complete before
> > > > > +		 *   hotunplug can proceed. Further, hotplug merely sets
> > > > > +		 *   __online to false. It does not delete the acomp_ctx
> > > > > +		 *   resources.
> > > > > +		 *
> > > > > +		 * Option 1 is better than potentially not exiting the earlier
> > > > > +		 * for (;;) loop because the system is running low on memory
> > > > > +		 * and/or CPUs are getting offlined for whatever reason. At
> > > > > +		 * least failing this store will prevent data loss by failing
> > > > > +		 * zswap_store(), and saving the data in the backing swap
> > > > device.
> > > > >  		 */
> > > >
> > > > I believe we can dropped. I don't think we can have any store/load
> > > > operations on a CPU before it's fully onlined, and we should always have
> > > > a reference on the pool here, so the resources cannot go away.
> > > >
> > > > So unless I missed something we can drop this completely now and just
> > > > hold the mutex directly in the load/store paths.
> > >
> > > Based on the above explanations, please let me know if it is a good idea
> > > to keep the __online, or if you think further simplification is possible.
> > 
> > I still think it's not needed. Let me know if I missed anything.
> 
> Let me think some more about whether it is feasible to not have cpuhotplug
> manage the acomp_ctx resource allocation, and instead have this be done
> through the pool creation/deletion routines.

I don't think this is necessary, see my comments above.