Re: [PATCH RFC v2 1/2] mm/zswap: replace the zswap_pools list with a fixed pools array

[Jianyue Wu]

On Tue, Aug 11, 2026 at 8:12 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> On Fri, Jul 31, 2026 at 08:32:47AM +0800, Jianyue Wu wrote:
> > Originally zswap holds its pools on an RCU list whose head also serves as
> > the "current pool".  Only a handful of pools are ever live at once, since
> > a new pool is only created when the compressor is (re)set and pools are
> > reused across compressor switches.
> >
> > A later change wants to store a reference to each entry's pool in every
> > zswap_entry, where a pointer would cost 8 bytes but a small pool index
> > only one.  To make that index possible, the current change holds the
> > pools in a fixed ZSWAP_MAX_POOLS-element array so each pool has a stable
> > slot number, and tracks the current pool with a separate rcu-protected
> > pointer.
> >
> > The array keeps the same RCU publish/retire discipline the list had, so
> > lookup and teardown stay equivalent.  Newly created pools are published
> > into the array before they become current, so zswap_total_pages() can
> > observe an empty pool briefly; that is harmless.
>
> I don't follow. A newly created pool is empty anyway, so not being
> iterated in zswap_total_pages() should be normal. Why do we need to call
> this out?
You are right, zswap_total_pages() can observe an empty pool briefly
is not needed, it is obvious.

> > This also caps the
> > number of live pools at ZSWAP_MAX_POOLS (16), which is plenty in
> > practice; pool creation warns and fails if the array ever fills, and the
> > limit can be raised.
> >
> > Suggested-by: Nhat Pham <nphamcs@xxxxxxxxx>
> > Suggested-by: Yosry Ahmed <yosry@xxxxxxxxxx>
> > Signed-off-by: Jianyue Wu <wujianyue000@xxxxxxxxx>
> > ---
> >  mm/zswap.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 67 insertions(+), 21 deletions(-)
> >
> > diff --git a/mm/zswap.c b/mm/zswap.c
> > index 4e76a4a87cdc..b203934d3be8 100644
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -154,12 +154,20 @@ struct zswap_pool {
> >       struct zs_pool *zs_pool;
> >       struct crypto_acomp_ctx __percpu *acomp_ctx;
> >       struct percpu_ref ref;
> > -     struct list_head list;
> >       struct work_struct release_work;
> >       struct hlist_node node;
> > +     u8 idx;
> >       char tfm_name[CRYPTO_MAX_ALG_NAME];
> >  };
> >
> > +#define ZSWAP_MAX_POOLS 16
> > +static struct zswap_pool __rcu *zswap_pools[ZSWAP_MAX_POOLS];
> > +/*
> > + * The current pool (NULL if none): an alias of one zswap_pools[] slot.  It
> > + * always holds a ref, so it is never retired from under us.
> > + */
> > +static struct zswap_pool __rcu *zswap_current_pool;
> > +
> >  /* Global LRU lists shared by all zswap pools. */
> >  static struct list_lru zswap_list_lru;
> >
> > @@ -200,9 +208,7 @@ struct zswap_entry {
> >  static struct xarray *zswap_trees[MAX_SWAPFILES];
> >  static unsigned int nr_zswap_trees[MAX_SWAPFILES];
> >
> > -/* RCU-protected iteration */
> > -static LIST_HEAD(zswap_pools);
> > -/* protects zswap_pools list modification */
> > +/* protects the zswap_pools array and zswap_current_pool */
> >  static DEFINE_SPINLOCK(zswap_pools_lock);
> >  /* pool counter to provide unique names to zsmalloc */
> >  static atomic_t zswap_pools_count = ATOMIC_INIT(0);
> > @@ -270,6 +276,25 @@ static void acomp_ctx_free(struct crypto_acomp_ctx *acomp_ctx)
> >       acomp_ctx->buffer = NULL;
> >  }
> >
> > +static int zswap_pool_reserve_slot(struct zswap_pool *pool)
> > +{
> > +     int i, ret = -ENOSPC;
> > +
> > +     spin_lock_bh(&zswap_pools_lock);
> > +     for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> > +             if (!rcu_access_pointer(zswap_pools[i])) {
> > +                     /* Set idx before publishing so readers never see it stale. */
> > +                     pool->idx = i;
> > +                     rcu_assign_pointer(zswap_pools[i], pool);
>
> Sashiko points out a seemingly real problem here because we add the pool
> to the array before actually making it the current pool.
>
> https://sashiko.dev/#/patchset/20260731-shrink_zswap_entry_v2-0-0-v2-0-e72083aa8734%40gmail.com
>
> What if we just reserve an index here but not actually assign the pool?
> We can add a marker to the array or sth (e.g. (void *)-1UL)).
That's a good idea, thanks. This is a real race the list version didn't
have. Publishing the real pool pointer at reserve time makes it visible to
zswap_pool_find_get() before the pool is committed as current. A concurrent
writer to the compressor param can then find it, take the else branch,
percpu_ref_resurrect() an already-live ref (hitting
WARN_ON_ONCE(!percpu_ref_is_dead())), and go on to kill what it believes is
the old current pool -- exactly the use-after-free you describe.

I'll define ZSWAP_SLOT_RESERVED as -1UL to claim the index at reserve time,
the real pointer is stored only once the pool is committed as current,
under zswap_pools_lock.
All array walkers skip the placeholder, so a concurrent zswap_pool_find_get()
no longer sees the not-yet-ready pool and the resurrect/kill/UAF chain cannot
start.

>
> > +                     ret = i;
> > +                     break;
> > +             }
> > +     }
> > +     spin_unlock_bh(&zswap_pools_lock);
> > +
> > +     return ret;
> > +}
> > +
> >  static struct zswap_pool *zswap_pool_create(char *compressor)
> >  {
> >       struct zswap_pool *pool;
> > @@ -313,19 +338,27 @@ static struct zswap_pool *zswap_pool_create(char *compressor)
> >       if (ret)
> >               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
> > +     /*
> > +      * After a successful create, the caller makes this the current pool.
> > +      * If the caller fails, it kills the ref to free the reserved slot.
> >        */
> >       ret = percpu_ref_init(&pool->ref, __zswap_pool_empty,
> >                             PERCPU_REF_ALLOW_REINIT, GFP_KERNEL);
> >       if (ret)
> >               goto ref_fail;
> > -     INIT_LIST_HEAD(&pool->list);
> > +
> > +     ret = zswap_pool_reserve_slot(pool);
> > +     if (ret < 0) {
> > +             pr_err("cannot create more than %d pools\n", ZSWAP_MAX_POOLS);
> > +             goto slot_fail;
> > +     }
> >
> >       zswap_pool_debug("created", pool);
> >
> >       return pool;
> >
> > +slot_fail:
> > +     percpu_ref_exit(&pool->ref);
> >  ref_fail:
> >       cpuhp_state_remove_instance(CPUHP_MM_ZSWP_POOL_PREPARE, &pool->node);
> >
> > @@ -388,7 +421,7 @@ static void __zswap_pool_release(struct work_struct *work)
> >       WARN_ON(!percpu_ref_is_zero(&pool->ref));
> >       percpu_ref_exit(&pool->ref);
> >
> > -     /* pool is now off zswap_pools list and has no references. */
> > +     /* Slot cleared in __zswap_pool_empty(); synchronize_rcu() drained readers. */
> >       zswap_pool_destroy(pool);
> >  }
> >
> > @@ -404,7 +437,11 @@ static void __zswap_pool_empty(struct percpu_ref *ref)
> >
> >       WARN_ON(pool == zswap_pool_current());
> >
> > -     list_del_rcu(&pool->list);
> > +     /*
> > +      * Clear the slot before scheduling the release so new readers cannot
> > +      * see it; __zswap_pool_release()'s synchronize_rcu() drains the rest.
> > +      */
> > +     rcu_assign_pointer(zswap_pools[pool->idx], NULL);
> >
> >       INIT_WORK(&pool->release_work, __zswap_pool_release);
> >       schedule_work(&pool->release_work);
>
> Not related to this change, but I wonder if we can use call_rcu() or
> similar here instead of the manual synchronize_rcu().
Agree, will use call_rcu() to replace workqueue + synchronize_rcu(),
so it no longer synchronously waits on an RCU grace period.

> > @@ -435,7 +472,8 @@ static struct zswap_pool *__zswap_pool_current(void)
> >  {
> >       struct zswap_pool *pool;
> >
> > -     pool = list_first_or_null_rcu(&zswap_pools, typeof(*pool), list);
> > +     pool = rcu_dereference_check(zswap_current_pool,
> > +                                  lockdep_is_held(&zswap_pools_lock));
> >       WARN_ONCE(!pool && zswap_has_pool,
> >                 "%s: no page storage pool!\n", __func__);
> >
> > @@ -468,11 +506,14 @@ static struct zswap_pool *zswap_pool_current_get(void)
> >  static struct zswap_pool *zswap_pool_find_get(char *compressor)
> >  {
> >       struct zswap_pool *pool;
> > +     int i;
> >
> >       assert_spin_locked(&zswap_pools_lock);
> >
> > -     list_for_each_entry_rcu(pool, &zswap_pools, list) {
> > -             if (strcmp(pool->tfm_name, compressor))
> > +     for (i = 0; i < ZSWAP_MAX_POOLS; i++) {
> > +             pool = rcu_dereference_protected(zswap_pools[i],
> > +                                     lockdep_is_held(&zswap_pools_lock));
>
> We already have an assertion that we are holding the lock above.
Exactly, the lock is already asserted previously.
Will simplify to rcu_dereference_protected(zswap_pools[i], true);

Thanks for the review!

Best regards,
Jianyue