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

On Sat, Aug 22, 2026 at 1:09 AM Nhat Pham <[email protected]> wrote:
>
> On Wed, Aug 19, 2026 at 9:21 PM Kunwu Chan <[email protected]> wrote:
> >
> > On Thu,  6 Aug 2026 11:42:45 -0700 Nhat Pham <[email protected]> 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.
>

Hi Nhat,
Thanks for the clarification. That makes sense.

I agree that the NULL check alone would only mask the underlying lifetime
issue. I see the lifetime dependency now: virtual_table is protected by the
lifetime of the containing cluster, rather than by an independent RCU lifetime.

> 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.

I also understand the distinction between the unpinned path, such as
the syzbot-reported physical-swap backend flag check, and paths where
we already have a reference pin, such as owning the swap cache folio.

I'll take a closer look at the v4 implementation once it is posted,
particularly the lock/CLUSTER_FLAG_DEAD ordering and the teardown
path.

Thanks,
KunWu