Re: [PATCH v3 02/11] mm, swap: support zswap and zeroswap as vswap backends

[Nhat Pham]

On Wed, Aug 19, 2026 at 9:21 PM Kunwu Chan <kunwu.chan@xxxxxxxxx> wrote:
>
> On Thu,  6 Aug 2026 11:42:45 -0700 Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> Hi Nhat,
> [...]
> syzbot reported a NULL dereference in the v3 series:
>
>   __vtable_get()
>     vswap_to_phys()
>       swap_entry_backend_has_flag()
>
> Seems like the underlying issue is introduced by this patch's
> `virtual_table` lifetime management.
>
> This patch adds:
>
> > @@ -70,6 +70,7 @@ struct swap_cluster_info_dynamic {
> >       struct swap_cluster_info ci;
> >       unsigned int index;             /* for cluster_index() */
> >       struct rcu_head rcu;
> > +     atomic_long_t *virtual_table;   /* Backing pointers for vswap slots */
> >  };
>
> [...]
>
> while the read side does:
>
> > +static inline unsigned long __vtable_get(struct swap_cluster_info_dynamic *ci_dyn,
> > +                                      unsigned int off)
> > +{
> > +     VM_WARN_ON_ONCE(off >= SWAPFILE_CLUSTER);
> > +     return atomic_long_read(&ci_dyn->virtual_table[off]);
> > +}
> > +
>
> `atomic_long_read()` only makes the access atomic; it does not protect
> the lifetime of the allocation being accessed.
>
> [...]
>
> > +static inline void vswap_cluster_free_vtable(struct swap_cluster_info *ci)
> > +{
> > +     struct swap_cluster_info_dynamic *ci_dyn;
> > +
>
> and frees it synchronously:
>
> > +     ci_dyn = container_of(ci, struct swap_cluster_info_dynamic, ci);
> > +     kfree(ci_dyn->virtual_table);
> > +     ci_dyn->virtual_table = NULL;
> > +}
> > +
>
> The existing `ci->table` already has an RCU-aware lifetime: readers use
> the corresponding RCU access rules, and the storage is not freed until
> after the appropriate grace period. `virtual_table` introduced here does
> not have an equivalent lifetime rule.
>
> The syzbot crash shows that the current teardown/read-side synchronization
> is insufficient: `__vtable_get()` can observe a torn-down
> `virtual_table` and dereference NULL.
>
> I don't think a NULL check in `__vtable_get()` alone would be the right
> fix. The NULL dereference is a symptom of the missing lifetime guarantee.
> `virtual_table` needs to remain valid for as long as a reader can reach
> and access the corresponding dynamic cluster.

Thanks for taking a look, Kunwu.

Null-check by itself is not sufficient, correct. But I think a full
dance of: rcu read section + spin_lock + checking for
CLUSTER_FLAG_DEAD should suffice. As long as cluster is valid inside
this rcu read section, we can get the lock. Once we have the lock, no
one can free the cluster under us. and as long as the
"CLUSTER_FLAG_DEAD" flag checks out under the lock, we can exit the
rcu_read_section(), and proceed with reading the vtable. I already
fixed this in (to-be-sent) v4.

Note that this is actually only required where we don't have any
reference pin. One example is the one syzbot detected phys swap device
backend flag check, because at do_swap_time() we don't actually have
any reference pins. The PTE is not locked, so swap entry can gets
released underneath us. But if we have a pin - for e.g if we own the
swap cache folio, then we don't really need this dance.