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

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.

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

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.

For reference, no later commit in this series through 231bb4a13161 changes
xfrm_dev_state_free(), xfrm_dev_state_delete() or the xfrm_state_find() call
site.