[PATCH ipsec v2] xfrm: fix races in device state cleanup

[PATCH ipsec v2] xfrm: fix races in device state cleanup

[Chengfeng Ye]

The deferred-device pass in xfrm_dev_state_flush() finds states under
xfrm_state_dev_gc_lock, but drops the lock before calling
xfrm_dev_state_free() because the driver callback may sleep. The device
GC list does not hold an xfrm_state reference, so the state GC worker can
destroy the same state concurrently.

The race can proceed as follows:

  CPU 0                               CPU 1
  find x on the device GC list
  drop xfrm_state_dev_gc_lock
                                      xfrm_state_gc_destroy(x)
                                        xfrm_dev_state_free(x)
                                        xfrm_state_free(x)
  xfrm_dev_state_free(x)

Both paths can invoke the driver callback and drop the device reference.
CPU 0 can also access the xfrm_state after CPU 1 has freed it.

KASAN reported:

  BUG: KASAN: slab-use-after-free in xfrm_dev_state_free+0x24c/0x2a0
  Read of size 8 at addr ffff88810bbaa960 by task poc/102

  Call Trace:
   xfrm_dev_state_free+0x24c/0x2a0
   xfrm_dev_state_flush+0x353/0x400
   xfrm_dev_event+0x26d/0x3a0
   notifier_call_chain+0xc0/0x280
   __dev_notify_flags+0x169/0x250
   netif_change_flags+0xe7/0x160
   dev_change_flags+0x96/0x220
   devinet_ioctl+0x7f4/0x1880

  Freed by task 57:
   kmem_cache_free+0xcb/0x3d0
   xfrm_state_gc_task+0x4a8/0x650
   process_one_work+0x63a/0x1070

A third xfrm_dev_state_free() caller in xfrm_state_find() can also race
with the deferred pass. Both callers can sample the same device before
either clears xso->dev, then invoke the callback and netdev_put() twice.
Since hlist_del() leaves the node poisoned rather than unhashed, they can
also unlink the device GC node twice.

Acquire a state reference while the deferred pass still holds
xfrm_state_dev_gc_lock. If the reference count is already zero, leave
destruction to the state GC worker. Also claim device cleanup under the
same lock by clearing xso->dev and unlinking the node with
hlist_del_init(). Only the caller that claims the device invokes the
callback and drops the device reference. The deferred pass releases its
temporary state reference after cleanup.

Fixes: 07b87f9eea0c ("xfrm: Fix unregister netdevice hang on hardware offload.")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
---

Changes in v2:
- Replace the global mutex with a temporary state reference acquired
  while walking the device GC list.
- Claim device cleanup under xfrm_state_dev_gc_lock so concurrent
  callers cannot run the callback or netdev_put() twice.
- Use hlist_del_init() to leave the device GC node unhashed.
- Cover the xfrm_state_find() cleanup path identified during review.

v1: https://lore.kernel.org/netdev/20260730103543.2077749-1-nicoyip.dev@gmail.com/

 net/xfrm/xfrm_state.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 36a4f6793ede..282d09d0c247 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -780,17 +780,20 @@ EXPORT_SYMBOL_GPL(xfrm_dev_state_delete);
 void xfrm_dev_state_free(struct xfrm_state *x)
 {
 	struct xfrm_dev_offload *xso = &x->xso;
-	struct net_device *dev = READ_ONCE(xso->dev);
+	struct net_device *dev;
 
+	spin_lock_bh(&xfrm_state_dev_gc_lock);
+	dev = READ_ONCE(xso->dev);
 	if (dev && dev->xfrmdev_ops) {
-		spin_lock_bh(&xfrm_state_dev_gc_lock);
+		WRITE_ONCE(xso->dev, NULL);
 		if (!hlist_unhashed(&x->dev_gclist))
-			hlist_del(&x->dev_gclist);
-		spin_unlock_bh(&xfrm_state_dev_gc_lock);
+			hlist_del_init(&x->dev_gclist);
+	}
+	spin_unlock_bh(&xfrm_state_dev_gc_lock);
 
+	if (dev && dev->xfrmdev_ops) {
 		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);
 	}
@@ -1005,9 +1008,10 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
 	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
 		xso = &x->xso;
 
-		if (xso->dev == dev) {
+		if (xso->dev == dev && xfrm_state_hold_rcu(x)) {
 			spin_unlock_bh(&xfrm_state_dev_gc_lock);
 			xfrm_dev_state_free(x);
+			xfrm_state_put(x);
 			spin_lock_bh(&xfrm_state_dev_gc_lock);
 			goto restart_gc;
 		}

base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
-- 
2.43.0

Re: [PATCH ipsec v2] xfrm: fix races in device state cleanup

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1

- [High] net/xfrm/xfrm_state.c, xfrm_dev_state_free(): the patch
  publishes `WRITE_ONCE(xso->dev, NULL)` (line 788) under…
- [Low] net/xfrm/xfrm_state.c, xfrm_dev_state_flush() line 1011: the new
  `&& xfrm_state_hold_rcu(x)` gate makes the deferred device-GC pass…

Pre-existing issues:
- [Medium] Split responsibility for validating xso->dev between core and
  driver: include/net/xfrm.h, xfrm_dev_state_update_stats() snapshots…

--

xfrm: fix races in device state cleanup

The patch makes the deferred device-GC pass in xfrm_dev_state_flush() take a
temporary xfrm_state reference while it still holds xfrm_state_dev_gc_lock,
and moves the xso->dev clearing and the dev_gclist unlink under that same
lock so only one caller claims the device cleanup. hlist_del() is replaced
by hlist_del_init() so the node is left unhashed.

A few questions below.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 36a4f6793edef..282d09d0c2475 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -780,17 +780,20 @@ EXPORT_SYMBOL_GPL(xfrm_dev_state_delete);
>  void xfrm_dev_state_free(struct xfrm_state *x)
>  {
>  	struct xfrm_dev_offload *xso = &x->xso;
> -	struct net_device *dev = READ_ONCE(xso->dev);
> +	struct net_device *dev;
>  
> +	spin_lock_bh(&xfrm_state_dev_gc_lock);
> +	dev = READ_ONCE(xso->dev);
>  	if (dev && dev->xfrmdev_ops) {
> -		spin_lock_bh(&xfrm_state_dev_gc_lock);
> +		WRITE_ONCE(xso->dev, NULL);

[Severity: High]
Can moving this clear ahead of the driver callback open a NULL dereference
window on the output path?

After this change xso->dev is published as NULL here, the lock is dropped,
and xso->type is only set to XFRM_DEV_OFFLOAD_UNSPECIFIED after
xdo_dev_state_free() returns.  For the whole duration of the callback a
lockless reader sees xso.type == XFRM_DEV_OFFLOAD_PACKET together with
xso.dev == NULL.

xfrm_output() picks the offload branch purely on the type:

net/xfrm/xfrm_output.c:xfrm_output() {
	...
	if (x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
		if (!xfrm_dev_offload_ok(skb, x)) { ... }
	...
		if (x->props.mode == XFRM_MODE_TUNNEL && !skb->sk)
			return xfrm_dev_direct_output(sk, x, skb);
	...
}

and xfrm_dev_offload_ok() explicitly tolerates a NULL device:

net/xfrm/xfrm_device.c:xfrm_dev_offload_ok() {
	struct net_device *dev = x->xso.dev;
	...
	if ((!dev || dev == xfrm_dst_path(dst)->dev) && !xdst->child->xfrm) {
	...
ok:
	if (!dev)
		return true;
}

so a forwarded (skb->sk == NULL) tunnel-mode packet reaches:

net/xfrm/xfrm_output.c:xfrm_dev_direct_output() {
	skb->dev = x->xso.dev;
	__skb_push(skb, skb->dev->hard_header_len);
}

Before this patch xso->dev was cleared after the callback, one store before
xso->type, so the inconsistent state lasted a couple of instructions.  Now
it spans the driver callback, which can block for a long time, for example:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c:mlx5e_xfrm_free_state() {
	if (sa_entry->work)
		cancel_work_sync(&sa_entry->work->work);

	if (sa_entry->dwork)
		cancel_delayed_work_sync(&sa_entry->dwork->dwork);

	mlx5e_accel_ipsec_fs_del_rule(sa_entry);
	mlx5_ipsec_free_sa_ctx(sa_entry);
}

The state can still be referenced by an xfrm_dst bundle while
xfrm_dev_state_flush() runs (ip link set <offload dev> down or device
unregister), so a packet can be inside xfrm_output() at that moment.  Would
it be safer to clear xso->type before, or together with, xso->dev under the
lock?

>  		if (!hlist_unhashed(&x->dev_gclist))
> -			hlist_del(&x->dev_gclist);
> -		spin_unlock_bh(&xfrm_state_dev_gc_lock);
> +			hlist_del_init(&x->dev_gclist);
> +	}
> +	spin_unlock_bh(&xfrm_state_dev_gc_lock);
>  
> +	if (dev && dev->xfrmdev_ops) {
>  		if (dev->xfrmdev_ops->xdo_dev_state_free)
>  			dev->xfrmdev_ops->xdo_dev_state_free(dev, x);

[Severity: Medium]
This isn't a bug introduced by this patch, but the reordering above changes
which contexts can race with an unchecked re-read of x->xso.dev in the mlx5
stats callback.

The core snapshots and validates the device, then passes only x:

include/net/xfrm.h:xfrm_dev_state_update_stats() {
	struct net_device *dev = READ_ONCE(xdo->dev);

	if (dev && dev->xfrmdev_ops &&
	    dev->xfrmdev_ops->xdo_dev_state_update_stats)
		dev->xfrmdev_ops->xdo_dev_state_update_stats(x);
}

and the driver re-reads the field with no NULL check:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c:mlx5e_xfrm_update_stats() {
	struct net *net = dev_net(x->xso.dev);

The clear in xfrm_dev_state_free() does not take x->lock and no stats reader
takes xfrm_state_dev_gc_lock, so a clear landing between the core check and
the driver re-read gives dev_net(NULL).

With this patch the clear now happens before xdo_dev_state_free(), which is
where mlx5e_xfrm_free_state() does cancel_delayed_work_sync() on the delayed
work that runs mlx5e_ipsec_handle_sw_limits() -> xfrm_state_check_expire()
-> xfrm_dev_state_update_stats().  Previously the clear only happened after
that work had been cancelled.

Should the callback receive the dev the core already validated, the way
xdo_dev_state_add/delete/free do, or should the driver NULL-check?

> -		WRITE_ONCE(xso->dev, NULL);
>  		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
>  		netdev_put(dev, &xso->dev_tracker);
>  	}
> @@ -1005,9 +1008,10 @@ int xfrm_dev_state_flush(struct net *net, struct net_device *dev, bool task_vali
>  	hlist_for_each_entry_safe(x, tmp, &xfrm_state_dev_gc_list, dev_gclist) {
>  		xso = &x->xso;
>  
> -		if (xso->dev == dev) {
> +		if (xso->dev == dev && xfrm_state_hold_rcu(x)) {

[Severity: Low]
Does this turn the deferred pass into a best-effort one?

When refcount_inc_not_zero() fails the entry is skipped with no cleanup, no
restart and no error, so xfrm_dev_state_flush() can return to
xfrm_dev_event() while the xso->dev_tracker netdev reference is still
outstanding.  The trailing xfrm_flush_gc() does not close that window,
because the state is only queued after the refcount already dropped to zero:

net/xfrm/xfrm_state.c:__xfrm_state_destroy() {
	spin_lock_bh(&xfrm_state_gc_lock);
	hlist_add_head(&x->gclist, &xfrm_state_gc_list);
	spin_unlock_bh(&xfrm_state_gc_lock);
	schedule_work(&xfrm_state_gc_work);
}

net/xfrm/xfrm_state.c:xfrm_flush_gc() {
	flush_work(&xfrm_state_gc_work);
}

so flush_work() can observe the work as idle just before it is queued.

The same applies to a state claimed by another xfrm_dev_state_free() caller,
since the claim unlinks it from dev_gclist under the lock while the netdev
reference is only dropped after the callback returns.

To be clear, this does not look like the unregister hang that 07b87f9eea0c
addressed: a zero refcount means the destroy path is committed and
unconditionally reaches xfrm_dev_state_free() -> netdev_put() through the GC
worker, and netdev_wait_allrefs_any() polls with msleep() and rebroadcasts
NETDEV_UNREGISTER once a second, which re-runs xfrm_dev_state_flush().  So
the observable effect appears to be a bounded extra delay in device
unregistration in a rare interleaving.

Separately, since every successful claim does goto restart_gc from the list
head while skipped entries stay on the list, the walk re-scans them each
time.  Was an O(n^2) re-walk of the device GC list intended here?

>  			spin_unlock_bh(&xfrm_state_dev_gc_lock);
>  			xfrm_dev_state_free(x);
> +			xfrm_state_put(x);
>  			spin_lock_bh(&xfrm_state_dev_gc_lock);
>  			goto restart_gc;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914133540.3666907-1-nicoyip.dev%40gmail.com