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

[netdev-bot+sashiko@kernel.org]

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