Re: [PATCH 02/12] xfrm: serialize state GC with device state flush

On Mon, Sep 14, 2026 at 7:23 PM Steffen Klassert
[off-list ref] wrote:

Ccing Author of the patch.

On Tue, Sep 08, 2026 at 03:48:54PM -0700, Jakub Kicinski wrote:

quoted

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: serialize state GC with device state flush

This adds a global mutex, xfrm_state_gc_mutex, taken by the state GC worker
around xfrm_state_gc_destroy() and by the deferred-device pass of
xfrm_dev_state_flush(), so the two paths no longer run xfrm_dev_state_free()
on the same state concurrently.  The KASAN slab-use-after-free reported in
xfrm_dev_state_free() from xfrm_dev_state_flush() is addressed by that
serialization.

quoted

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 36a4f6793edef..de097bba803b7 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -226,6 +226,7 @@ static struct xfrm_state_afinfo __rcu *xfrm_state_afinfo[NPROTO];

 static DEFINE_SPINLOCK(xfrm_state_gc_lock);
 static DEFINE_SPINLOCK(xfrm_state_dev_gc_lock);
+static DEFINE_MUTEX(xfrm_state_gc_mutex);

 int __xfrm_state_delete(struct xfrm_state *x);

@@ -632,8 +633,10 @@ static void xfrm_state_gc_task(struct work_struct *work)

    synchronize_rcu();

+   mutex_lock(&xfrm_state_gc_mutex);
    hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
            xfrm_state_gc_destroy(x);
+   mutex_unlock(&xfrm_state_gc_mutex);
 }

 static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)

@@ -1000,6 +1003,7 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 out:
    spin_unlock_bh(&net->xfrm.xfrm_state_lock);

+   mutex_lock(&xfrm_state_gc_mutex);
    spin_lock_bh(&xfrm_state_dev_gc_lock);
 restart_gc:
    hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {

[Severity: High]
Is the new mutex enough to cover every xfrm_dev_state_free() caller?  There
is a third caller in the acquire path in xfrm_state_find() that takes
neither the new mutex nor RTNL:

net/xfrm/xfrm_state.c:xfrm_state_find() {
      ...
#ifdef CONFIG_XFRM_OFFLOAD
              struct xfrm_dev_offload *xso = &x->xso;

              if (xso->type == XFRM_DEV_OFFLOAD_PACKET) {
                      xfrm_dev_state_delete(x);
                      xfrm_dev_state_free(x);
              }
#endif
      ...
}

That code runs inside the rcu_read_lock() taken earlier in
xfrm_state_find() (it uses xfrm_state_hold_rcu() on the found state), so it
cannot take a sleeping mutex even if one were added there.

That just pointed me to the xfrm_dev_state_free callback of the bonding driver,
it uses a mutex. But this is a different issue.

quoted

xfrm_dev_state_delete() publishes x on the global, reference-less device GC
list while xso->dev is still set:

net/xfrm/xfrm_state.c:xfrm_dev_state_delete() {
      if (dev) {
              dev->xfrmdev_ops->xdo_dev_state_delete(dev, x);
              spin_lock_bh(&xfrm_state_dev_gc_lock);
              hlist_add_head(&x->dev_gclist, &xfrm_state_dev_gc_list);
              spin_unlock_bh(&xfrm_state_dev_gc_lock);
      }
}

so the deferred pass added here, which matches only on xso->dev == dev, can
pick up the same state and call xfrm_dev_state_free() on it in parallel:

  CPU 0 (xfrm_state_find(), packet offload, km_query() failed)
  xfrm_dev_state_delete(x)            /* x now on xfrm_state_dev_gc_list */
  xfrm_dev_state_free(x)
    dev = READ_ONCE(xso->dev)         /* non-NULL */

  CPU 1 (xfrm_dev_event() -> xfrm_dev_state_flush(), same dev)
  mutex_lock(&xfrm_state_gc_mutex)
  finds x, xso->dev == dev
  spin_unlock_bh(&xfrm_state_dev_gc_lock)
  xfrm_dev_state_free(x)
    dev = READ_ONCE(xso->dev)         /* also non-NULL */

Can both CPUs then run the driver callback and the device put twice?

net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
      if (dev && dev->xfrmdev_ops) {
              spin_lock_bh(&xfrm_state_dev_gc_lock);
              if (!hlist_unhashed(&x->dev_gclist))
                      hlist_del(&x->dev_gclist);
              spin_unlock_bh(&xfrm_state_dev_gc_lock);

              if (dev->xfrmdev_ops->xdo_dev_state_free)
                      dev->xfrmdev_ops->xdo_dev_state_free(dev, x);
              WRITE_ONCE(xso->dev, NULL);
              xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
              netdev_put(dev, &xso->dev_tracker);
      }
}

The hlist_unhashed() test only guards the list removal, not
xdo_dev_state_free() or netdev_put(), and dev is sampled before the
spinlock, so a double driver teardown and a net_device refcount underflow
look possible.

Also, since hlist_del() is used rather than hlist_del_init(), does the
second caller see the poisoned pprev as still hashed and call __hlist_del()
again, writing through LIST_POISON2?

This is the same failure mode the commit message describes:

  "Both paths can invoke the driver callback and drop the device reference."

Would addressing the root cause the commit message names -- "The device GC
list does not hold an xfrm_state reference" -- be a better fit, either by
taking an xfrm_state reference for the device GC list, or by making the
release in xfrm_dev_state_free() idempotent (test-and-clear of xso->dev plus
hlist_del_init() under xfrm_state_dev_gc_lock)?  That would also cover the
xfrm_state_find() caller, which cannot participate in a mutex-based scheme.

This approach can indeed not solve the problem completely. I tend to
revert this one in favor of a fix with the above approach. This is
the destruction path, so a refcount would probably not harm too
much.

Chengfeng, can you have a look at this?

No problem at all, I will prepare a replacement patch with refcount
fix to completely fix the issue.

Best regards,
Chengfeng