[PATCH ipsec v2 0/2] xfrm: fix offload state use racing with device teardown

[PATCH ipsec v2 0/2] xfrm: fix offload state use racing with device teardown

[Cen Zhang]

NETDEV_DOWN and NETDEV_UNREGISTER flush a device's offloaded SAs with
xfrm_dev_state_flush(). It deletes each SA under x->lock and then frees
its offload binding with xfrm_dev_state_free() without that lock: the
driver frees its offload state, xso.dev is cleared and the device
reference is dropped. Netlink requests that hold only a reference to
the SA can still be using the binding after they checked xso.dev:

  1/2 The offload statistics refresh in GETSA, ALLOCSPI replies and SA
      notifications, and in the expiry timer, calls into the driver.
      Refresh the statistics only of SAs that are not dead, under
      x->lock, which orders the refresh with the teardown.

  2/2 XFRM_MSG_MIGRATE_STATE reads the ifindex through xso.dev. Use the
      ifindex cached at attachment, as state dumps already do.

Cen Zhang (2):
  xfrm: protect offload statistics updates from device teardown
  xfrm: use cached offload ifindex when migrating an SA

 net/xfrm/xfrm_state.c |  4 ++--
 net/xfrm/xfrm_user.c  | 14 +++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

---
Changes in v2:
- Address the incomplete RCU protection raised by Sashiko:
  https://sashiko.dev/#/patchset/97864
- Patch 1: refresh the statistics under x->lock and only for SAs that are
  not dead, instead of using RCU, which covered neither the driver offload
  state nor the cleared xso.dev; fix the same race in the expiry timer.
- Add patch 2 for the same race in xfrm_do_migrate_state(), also raised by
  Sashiko.
- Trim the crash reports.

Link to v1: https://lore.kernel.org/r/pm-xfrm-core-objects-candidate-0050-v3-dbfb7194ffb750193d02@gmail.com

[PATCH ipsec v2 1/2] xfrm: protect offload statistics updates from device teardown

[Cen Zhang]

copy_to_user_state() refreshes the statistics of an offloaded SA through
the driver's xdo_dev_state_update_stats() callback. GETSA and SA
notifications run it with only a reference to the xfrm_state, which does
not keep the offload binding alive, and the netlink configuration mutex
does not serialize them with the netdevice notifier.

NETDEV_DOWN and NETDEV_UNREGISTER flush the device's states with
xfrm_dev_state_flush(). It deletes each state under x->lock and then
calls xfrm_dev_state_free() without that lock, which lets the driver
free its offload state, clears xso.dev and drops the device reference.
A refresh that has already seen xso.dev set can then run the driver
callback on freed offload state or after xso.dev became NULL, and can
read the net_device after unregistration has freed it:

  GETSA                              Device teardown (RTNL)
  -----                              ----------------------
  look up and reference x
  copy_to_user_state(x)
    x->xso.dev is set
                                     xfrm_dev_state_flush()
                                       xfrm_state_delete(x)
                                       xfrm_dev_state_free(x)
                                         xdo_dev_state_free()
                                         clear x->xso.dev
                                         netdev_put(dev)
                                     unregister frees dev
    xfrm_dev_state_update_stats(x)
      reads dev->xfrmdev_ops

The expiry timer has the same problem. It refreshes the statistics
before it checks for a dead state, and it can still fire after the
flush has deleted the state.

Every path that frees the offload state of a reachable SA marks the SA
dead under x->lock first. Refresh the statistics only for SAs that are
not dead, with x->lock held, so a refresh either completes before the
teardown or sees the dead state. GETSA, ALLOCSPI replies and SA
notifications now take x->lock around the copy. Expire notifications
already run under x->lock. Dumps run under xfrm_state_lock while the SA
is still linked, and deletion needs that lock before the offload state
can be freed. The timer now checks for a dead state before the refresh.
A dead SA keeps the counters from its last refresh.

The report below was collected on an earlier kernel with a built-in test
device that supplies no-op offload add and delete callbacks. Scheduling
probes paused GETSA after it sampled the device, then ran normal
link-down and link-delete requests before resuming the read.

KASAN report as below:

    ==================================================================
    BUG: KASAN: slab-use-after-free in copy_to_user_state+0x6ce/0x6e0
    Read of size 8 at addr ffff888104be0308 by task ip/446

    Call Trace:
     <TASK>
     dump_stack_lvl+0x93/0xd0
     print_report+0xce/0x630
     kasan_report+0xe0/0x110
     copy_to_user_state+0x6ce/0x6e0
     copy_to_user_state_extra+0x8a/0x1450
     dump_one_state+0x1fd/0x440
     xfrm_state_netlink+0xda/0x150
     xfrm_get_sa+0x12d/0x320
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     ____sys_sendmsg+0x88b/0x9d0
     ___sys_sendmsg+0x125/0x1d0
     __sys_sendmsg+0x13e/0x1e0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f
     </TASK>

    Allocated by task 420:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     __kasan_kmalloc+0xaa/0xb0
     __kvmalloc_node_noprof+0x353/0x930
     alloc_netdev_mqs+0xa0/0x1250
     rtnl_create_link+0xb8e/0xeb0
     rtnl_newlink+0xb7b/0x1f40
     rtnetlink_rcv_msg+0x7b9/0xce0
     netlink_rcv_skb+0x133/0x390
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     ____sys_sendmsg+0x88b/0x9d0
     ___sys_sendmsg+0x125/0x1d0
     __sys_sendmsg+0x13e/0x1e0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

    Freed by task 451:
     kasan_save_stack+0x33/0x60
     kasan_save_track+0x14/0x30
     kasan_save_free_info+0x3b/0x60
     __kasan_slab_free+0x5f/0x80
     kfree+0x307/0x580
     device_release+0xd7/0x280
     kobject_put+0x1ad/0x610
     netdev_run_todo+0x927/0x13d0
     rtnl_dellink+0x3aa/0xa30
     rtnetlink_rcv_msg+0x7b9/0xce0
     netlink_rcv_skb+0x133/0x390
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     ____sys_sendmsg+0x88b/0x9d0
     ___sys_sendmsg+0x125/0x1d0
     __sys_sendmsg+0x13e/0x1e0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f

    The buggy address belongs to the object at ffff888104be0000
     which belongs to the cache kmalloc-4k of size 4096
    The buggy address is located 776 bytes inside of
     freed 4096-byte region [ffff888104be0000, ffff888104be1000)
    ==================================================================

Fixes: 07b87f9eea0c ("xfrm: Fix unregister netdevice hang on hardware offload.")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

 net/xfrm/xfrm_state.c |  4 ++--
 net/xfrm/xfrm_user.c  | 10 +++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 0555e5d796ed..1aeb6208ea8f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -649,10 +649,10 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
 	int err = 0;
 
 	spin_lock(&x->lock);
-	xfrm_dev_state_update_stats(x);
-
 	if (x->km.state == XFRM_STATE_DEAD)
 		goto out;
+
+	xfrm_dev_state_update_stats(x);
 	if (x->km.state == XFRM_STATE_EXPIRED)
 		goto expired;
 	if (x->lft.hard_add_expires_seconds) {
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a5fbe38839c4..781d4ec565aa 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1179,7 +1179,11 @@ static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
 	memcpy(&p->id, &x->id, sizeof(p->id));
 	memcpy(&p->sel, &x->sel, sizeof(p->sel));
 	memcpy(&p->lft, &x->lft, sizeof(p->lft));
-	if (x->xso.dev)
+	/* Device teardown marks the state dead under x->lock before it frees
+	 * the offload state. Callers hold x->lock, or xfrm_state_lock while
+	 * the state is still linked.
+	 */
+	if (x->xso.dev && x->km.state != XFRM_STATE_DEAD)
 		xfrm_dev_state_update_stats(x);
 	memcpy(&p->curlft, &x->curlft, sizeof(p->curlft));
 	put_unaligned(x->stats.replay_window, &p->stats.replay_window);
@@ -1633,7 +1637,9 @@ static struct sk_buff *xfrm_state_netlink(struct sk_buff *in_skb,
 	info.nlmsg_seq = seq;
 	info.nlmsg_flags = 0;
 
+	spin_lock_bh(&x->lock);
 	err = dump_one_state(x, 0, &info);
+	spin_unlock_bh(&x->lock);
 	if (err) {
 		kfree_skb(skb);
 		return ERR_PTR(err);
@@ -4167,7 +4173,9 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c)
 
 		p = nla_data(attr);
 	}
+	spin_lock_bh(&x->lock);
 	err = copy_to_user_state_extra(x, p, skb);
+	spin_unlock_bh(&x->lock);
 	if (err)
 		goto out_free_skb;
 

[PATCH ipsec v2 2/2] xfrm: use cached offload ifindex when migrating an SA

[Cen Zhang]

An xfrm_state reference keeps its offload record alive, but does not
prevent xfrm_dev_state_free() from clearing xso.dev and dropping the
device reference. xfrm_do_migrate_state() checks xso.dev before
xso_to_xuo() dereferences it to obtain the offload ifindex.

With XFRM offload and migration enabled, XFRM_MSG_MIGRATE_STATE inherits
the old SA's offload configuration if the request omits
XFRMA_OFFLOAD_DEV and does not set XFRM_MIGRATE_STATE_CLEAR_OFFLOAD.
Device unregister can flush that SA between the check and the read.
The supplied report used a scheduling probe to delay migration in this
interval while in-tree netdevsim was removed through sysfs.

    Migration (xfrm_cfg_mutex)       Device unregister (RTNL)
    xfrm_state_lookup(): hold x
    test x->xso.dev != NULL
                                    xfrm_dev_state_flush()
                                      xfrm_state_delete(x)
                                      xfrm_dev_state_free(x)
                                        xso->dev = NULL
                                        netdev_put(dev)
    xso_to_xuo(&x->xso, &xuo)
      xso->dev->ifindex: NULL deref

The netlink configuration mutex does not serialize with the notifier,
and migration takes the state lock only after this read. The result is
a NULL pointer dereference even though migration still holds the SA.

Use the ifindex cached in the offload record at attachment, as state
dumps already do. The SA reference protects this snapshot throughout
the read, so serialization no longer depends on the device lifetime.
Annotate the binding check with READ_ONCE() to match the concurrent
WRITE_ONCE() in xfrm_dev_state_free().

Oops report as below:

    Oops: general protection fault, probably for non-canonical address
    0xdffffc000000001c: 0000 [#1] SMP KASAN NOPTI
    KASAN: null-ptr-deref in range [0x00000000000000e0-0x00000000000000e7]
    RIP: 0010:xfrm_do_migrate_state+0x1f06/0x2c50
    Call Trace:
     <TASK>
     xfrm_user_rcv_msg+0x3b3/0xc20
     netlink_rcv_skb+0x133/0x390
     xfrm_netlink_rcv+0x76/0x90
     netlink_unicast+0x504/0x840
     netlink_sendmsg+0x7f7/0xcf0
     __sys_sendto+0x425/0x470
     __x64_sys_sendto+0xe5/0x1c0
     do_syscall_64+0x115/0x6a0
     entry_SYSCALL_64_after_hwframe+0x77/0x7f
     </TASK>

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
---
Assisted-by: LLM

 net/xfrm/xfrm_user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 781d4ec565aa..1cea24380797 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1240,7 +1240,7 @@ static void xso_to_xuo_ifindex(const struct xfrm_dev_offload *xso, int ifindex,
 static void xso_to_xuo(const struct xfrm_dev_offload *xso,
 		       struct xfrm_user_offload *xuo)
 {
-	xso_to_xuo_ifindex(xso, xso->dev->ifindex, xuo);
+	xso_to_xuo_ifindex(xso, READ_ONCE(xso->ifindex), xuo);
 }
 #endif
 
@@ -3462,7 +3462,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 	} else {
 		bool inherit_offload = !(um->flags & XFRM_MIGRATE_STATE_CLEAR_OFFLOAD);
 
-		if (inherit_offload && x->xso.dev) {
+		if (inherit_offload && READ_ONCE(x->xso.dev)) {
 			xso_to_xuo(&x->xso, &xuo);
 			m.xuo = &xuo;
 		}

Re: [PATCH ipsec v2 1/2] xfrm: protect offload statistics updates from device teardown

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix depends on the claim that 'Every path that frees the
  offload state of a reachable SA marks the SA dead under x->lock first'…
- [High] xfrm_add_sa() can overwrite XFRM_STATE_DEAD with VALID without
  holding a lock, which defeats the new DEAD check in the NEWSA…
- [Medium] The new `x->km.state != XFRM_STATE_DEAD` check in
  copy_to_user_state() (net/xfrm/xfrm_user.c:1186) also skips the last…
- [Low] The Assisted-by tag is not part of the commit message, so the AI-
  assistance disclosure will be lost.

--

xfrm: protect offload statistics updates from device teardown

This patch skips the driver offload statistics refresh in
copy_to_user_state() for SAs in XFRM_STATE_DEAD. It also takes x->lock
around the copy in xfrm_state_netlink() and xfrm_notify_sa(). The expiry
timer now checks for a dead state before refreshing the statistics.

> Fixes: 07b87f9eea0c ("xfrm: Fix unregister netdevice hang on hardware offload.")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Assisted-by: LLM

[Severity: Low]
This isn't a bug, but the Assisted-by: tag is below the --- separator.
git am will drop it, and the applied commit ends at Signed-off-by: with
no Assisted-by: line.

Could it move into the trailer block next to Fixes: and Signed-off-by:?
That is where Documentation/process/submitting-patches.rst and
Documentation/process/coding-assistants.rst ask for it.

[ ... ]

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a5fbe38839c49..781d4ec565aae 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1179,7 +1179,11 @@ static void copy_to_user_state(struct xfrm_state *x, struct xfrm_usersa_info *p)
>  	memcpy(&p->id, &x->id, sizeof(p->id));
>  	memcpy(&p->sel, &x->sel, sizeof(p->sel));
>  	memcpy(&p->lft, &x->lft, sizeof(p->lft));
> -	if (x->xso.dev)
> +	/* Device teardown marks the state dead under x->lock before it frees
> +	 * the offload state. Callers hold x->lock, or xfrm_state_lock while
> +	 * the state is still linked.
> +	 */

[Severity: High]
The commit message says:

  Every path that frees the offload state of a reachable SA marks the SA
  dead under x->lock first.

and:

  Dumps run under xfrm_state_lock while the SA is still linked, and
  deletion needs that lock before the offload state can be freed.

Does bonding failover follow this rule? bond_change_active_slave() calls
bond_ipsec_del_sa_all(). That function walks the linked, non-dead SAs on
bond->ipsec_list and does:

drivers/net/bonding/bond_main.c:bond_ipsec_del_sa_all() {
    ...
		spin_lock_bh(&ipsec->xs->lock);
		ipsec->xs->xso.real_dev = NULL;
		/* Don't double delete states killed by the user. */
		if (ipsec->xs->km.state != XFRM_STATE_DEAD)
			real_dev->xfrmdev_ops->xdo_dev_state_delete(real_dev,
								    ipsec->xs);
		spin_unlock_bh(&ipsec->xs->lock);

		if (real_dev->xfrmdev_ops->xdo_dev_state_free)
			real_dev->xfrmdev_ops->xdo_dev_state_free(real_dev,
								  ipsec->xs);
    ...
}

On this path the SA is never marked dead and xfrm_state_lock is never
taken. The slave's xdo_dev_state_free() runs with no lock held. For mlx5
that is mlx5e_xfrm_free_state(), which destroys the rules and counters and
frees sa_entry.

At the same time, a dump holds only xfrm_state_lock:

xfrm_dump_sa()
  xfrm_state_walk()              /* xfrm_state_lock held */
    dump_one_state()
      copy_to_user_state()       /* xso.dev is the bond, state VALID */
        bond_xfrm_update_stats()
          bond_ipsec_dev()       /* plain read of xso.real_dev, RCU only */
          mlx5e_xfrm_update_stats()

Suppose bond_ipsec_dev() reads real_dev before it is cleared. Can
mlx5e_xfrm_update_stats() then dereference sa_entry and ipsec_rule->fc
while bond_ipsec_del_sa_all() frees them?

xfrm_state_update_stats() also calls the driver refresh with only
xfrm_state_lock held. It serves /proc/net/xfrm_stat (mode 0444) and has no
dead check at all.

This window existed before the patch. However, the commit message and the
new comment say the bug class is closed for dumps. Should the dump paths
be covered too, or should the comment and commit message claim less?

> +	if (x->xso.dev && x->km.state != XFRM_STATE_DEAD)
>  		xfrm_dev_state_update_stats(x);

[Severity: Medium]
Does this also drop the last hardware counter read when an SA is deleted
normally, with no race?

xfrm_del_sa()
  xfrm_state_delete()
    __xfrm_state_delete()        /* sets XFRM_STATE_DEAD */
      xfrm_dev_state_delete()
  km_state_notify()
    xfrm_notify_sa()
      copy_to_user_state_extra()
        copy_to_user_state()     /* now skips the refresh */

For mlx5, xdo_dev_state_delete is mlx5e_xfrm_del_state(), which only
removes the sadb entry with xa_erase_bh(). The flow rules and counters
stay allocated until mlx5e_xfrm_free_state() runs from
xfrm_dev_state_free(). xfrm_del_sa() still holds its lookup reference
during the notify. So the read before this patch looks safe.

With packet offload, curlft is only updated by this driver callback.
After this change, the XFRM_MSG_DELSA notification reports
curlft.packets, curlft.bytes, stats.replay and stats.integrity_failed
from the last refresh. So do hard expire notifications for XFRM_MSG_EXPIRE
requests from userspace.

That last refresh may be old, because the timer only runs for time-based
lifetimes. The LINUX_MIB_XFRMIN* increments since then are also lost.
Nothing reads the counters again before xdo_dev_state_free() destroys
them.

The commit message says "A dead SA keeps the counters from its last
refresh." Would it work to read the counters once under x->lock in
__xfrm_state_delete()? That would be before the SA is marked dead and
before xfrm_dev_state_delete(), while the offload state is known to be
alive.

>  	memcpy(&p->curlft, &x->curlft, sizeof(p->curlft));
>  	put_unaligned(x->stats.replay_window, &p->stats.replay_window);

[ ... ]

> @@ -4167,7 +4173,9 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c)
>  
>  		p = nla_data(attr);
>  	}
> +	spin_lock_bh(&x->lock);
>  	err = copy_to_user_state_extra(x, p, skb);
> +	spin_unlock_bh(&x->lock);

[Severity: High]
Can xfrm_add_sa() overwrite a concurrent dead transition before this
notification runs?

xfrm_state_construct() uses __xfrm_init_state(). So the new SA is still
XFRM_STATE_VOID when xfrm_state_add() links it into state_bydst. If
XFRMA_OFFLOAD_DEV was given, xso.dev is already set at that point.

xfrm_add_sa() then does this without x->lock:

net/xfrm/xfrm_user.c:xfrm_add_sa() {
    ...
	if (x->km.state == XFRM_STATE_VOID)
		x->km.state = XFRM_STATE_VALID;
    ...
	km_state_notify(x, &c);
    ...
}

A possible sequence:

CPU A (XFRM_MSG_NEWSA)           CPU B (NETDEV_DOWN/UNREGISTER)
reads km.state == VOID
                                 xfrm_dev_state_flush()
                                   xfrm_state_delete(x)   /* DEAD */
                                   xfrm_dev_state_free(x)
                                     xdo_dev_state_free()
km.state = XFRM_STATE_VALID
km_state_notify()
  xfrm_notify_sa()
    spin_lock_bh(&x->lock)
    copy_to_user_state()
      xfrm_dev_state_update_stats()

xfrm_dev_state_free() calls xdo_dev_state_free() without x->lock, and
before WRITE_ONCE(xso->dev, NULL). So copy_to_user_state() can see
xso.dev set on a state that is no longer dead. For mlx5, that would run
mlx5e_xfrm_update_stats() on an sa_entry that mlx5e_xfrm_free_state() is
freeing.

The unlocked store existed before this patch. Still, the guarantee that
"a refresh either completes before the teardown or sees the dead state"
depends on it. Should xfrm_add_sa() change VOID to VALID under x->lock,
and leave the state alone if it is already dead?

>  	if (err)
>  		goto out_free_skb;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-xfrm-core-objects-candidate-0050-v4-3c6b8c5369a802e9be0e-0%40gmail.com

Re: [PATCH ipsec v2 2/2] xfrm: use cached offload ifindex when migrating an SA

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch's only AI-disclosure trailer, 'Assisted-by: LLM', sits
  below the '---' separator, after 'Signed-off-by: Cen Zhang'.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the patch now
  reads the cached xso->ifindex as a stable snapshot while the rest of…

--

xfrm: use cached offload ifindex when migrating an SA

This patch changes xso_to_xuo() to use the ifindex cached in the offload
record instead of dereferencing xso->dev. This closes a NULL dereference
when xfrm_dev_state_flush() races with XFRM_MSG_MIGRATE_STATE. It also
annotates the xso.dev check in xfrm_do_migrate_state() with READ_ONCE().

> Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
>
> ---
> Assisted-by: LLM

[Severity: Low]
This isn't a bug, but the Assisted-by tag is below the "---" separator,
so git am and b4 drop it when the patch is applied. The applied commit
ends at the Signed-off-by line and has no Assisted-by trailer.

Documentation/process/submitting-patches.rst ("Using Assisted-by:") asks
contributors to acknowledge tool use with an Assisted-by tag. Earlier
xfrm commits such as c4a5f0071cc6 ("xfrm: cache the offload ifindex for
netlink dumps") put it in the regular tag block.

Could the tag be moved above the "---" line, next to Fixes: and
Signed-off-by:?

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 781d4ec565aae..1cea24380797d 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1240,7 +1240,7 @@ static void xso_to_xuo_ifindex(const struct xfrm_dev_offload *xso, int ifindex,
>  static void xso_to_xuo(const struct xfrm_dev_offload *xso,
>  		       struct xfrm_user_offload *xuo)
>  {
> -	xso_to_xuo_ifindex(xso, xso->dev->ifindex, xuo);
> +	xso_to_xuo_ifindex(xso, READ_ONCE(xso->ifindex), xuo);
>  }
>  #endif
>  
> @@ -3462,7 +3462,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	} else {
>  		bool inherit_offload = !(um->flags & XFRM_MIGRATE_STATE_CLEAR_OFFLOAD);
>  
> -		if (inherit_offload && x->xso.dev) {
> +		if (inherit_offload && READ_ONCE(x->xso.dev)) {
>  			xso_to_xuo(&x->xso, &xuo);
>  			m.xuo = &xuo;
>  		}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
patch now treats the cached ifindex as a stable snapshot, but the rest of
the inherited xuo still comes from fields that teardown changes.

The dev check and xso_to_xuo() run without x->lock.
xso_to_xuo_ifindex() does plain reads of xso->dir and xso->type:

net/xfrm/xfrm_user.c:xso_to_xuo_ifindex() {
	xuo->ifindex = ifindex;
	if (xso->dir == XFRM_DEV_OFFLOAD_IN)
		xuo->flags = XFRM_OFFLOAD_INBOUND;
	if (xso->type == XFRM_DEV_OFFLOAD_PACKET)
		xuo->flags |= XFRM_OFFLOAD_PACKET;
}

At the same time, under RTNL, xfrm_dev_state_flush()->xfrm_dev_state_free()
does:

net/xfrm/xfrm_state.c:xfrm_dev_state_free() {
	...
		WRITE_ONCE(xso->dev, NULL);
		xso->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
		netdev_put(dev, &xso->dev_tracker);
	...
}

Migration holds xfrm_cfg_mutex and the notifier holds RTNL. Nothing
orders the plain store and the plain load of xso->type.

Would KCSAN report this as a data race?

Suppose the flush lands between the READ_ONCE(x->xso.dev) check and the
type read. The xuo then gets the valid cached ifindex but loses
XFRM_OFFLOAD_PACKET. xfrm_state_migrate_create() then tries
xfrm_dev_state_add() on the clone in crypto offload mode.

As far as I can tell, this is contained. xfrm_dev_state_flush() calls
xfrm_state_delete() before xfrm_dev_state_free(), so this later locked
check in xfrm_do_migrate_state() throws away the clone:

	spin_lock_bh(&x->lock);
	if (x->km.state != XFRM_STATE_VALID) {
		spin_unlock_bh(&x->lock);
		NL_SET_ERR_MSG(extack, "State already deleted");
		err = -ESRCH;
		goto out_xc;
	}

The dump path has the same plain reads of dir and type, through
copy_user_state_offload(). The xso->dev NULL dereference described in the
commit message does look fixed.

Should the dir and type reads be annotated too, or the snapshot taken
under x->lock, so the inherited xuo stays consistent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/pm-xfrm-core-objects-candidate-0050-v4-3c6b8c5369a802e9be0e-0%40gmail.com