* Re: [PATCH ipsec v2] xfrm: fix races in device state cleanup
2026-09-14 13:35 [PATCH ipsec v2] xfrm: fix races in device state cleanup Chengfeng Ye
@ 2026-09-16 17:38 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-16 17:38 UTC (permalink / raw)
To: nicoyip.dev
Cc: steffen.klassert, herbert, davem, edumazet, kuba, pabeni, horms,
netdev, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 2+ messages in thread