Hi Simon,

On Wed, Mar 11, 2026 at 07:57:31PM +0000, Simon Horman via Devel wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

This patch adds a new netlink method to migrate a single xfrm_state.
Unlike the existing migration mechanism (SA + policy), this supports
migrating only the SA and allows changing the reqid.

quoted

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 4b29ab92c2a7..ae2a1cf2311a 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h

@@ -683,11 +683,17 @@ struct xfrm_migrate {
 	xfrm_address_t		new_daddr;
 	xfrm_address_t		new_saddr;
 	struct xfrm_encap_tmpl *encap;
-	struct xfrm_user_offload *xuo;
+	struct xfrm_user_offload xuo;

[ ... ]

quoted

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 2f23ca5b62a9..de8b038fc08c 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c

@@ -4642,6 +4642,19 @@ static int xfrm_migrate_check(const struct xfrm_migrate *m, int num_migrate,
 	return 0;
 }

+/* Fill migrate fields that are invariant in XFRM_MSG_MIGRATE: inherited
+ * from the existing SA unchanged.
+ */
+static void xfrm_migrate_copy_old(struct xfrm_migrate *mp,
+				  const struct xfrm_state *x)
+{
+	mp->smark                  = x->props.smark;
+	mp->new_reqid              = x->props.reqid;
+	mp->nat_keepalive_interval = x->nat_keepalive_interval;
+	mp->mapping_maxage         = x->mapping_maxage;
+	mp->new_mark               = x->mark;
+}
+
 int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		 struct xfrm_migrate *m, int num_migrate,
 		 struct xfrm_kmaddress *k, struct net *net,

@@ -4680,7 +4693,10 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 			x_cur[nx_cur] = x;
 			nx_cur++;
 			mp->encap = encap;
-			mp->xuo = xuo;
+			if (xuo)
+				mp->xuo = *xuo;

                                     ^^^^

quoted

 			xfrm_migrate_copy_old(mp, x);

What happens to mp->xuo when xuo is NULL?

thanks. I also red this on NIPA AI reviews. I will fix it in the next 
version.

The xfrm_migrate structure changed its xuo field from a pointer
(struct xfrm_user_offload *xuo) to an embedded value
(struct xfrm_user_offload xuo).

In xfrm_migrate(), when the xuo parameter is NULL (PF_KEY path via
pfkey_migrate, or XFRM_MSG_MIGRATE without XFRMA_OFFLOAD_DEV), the
'if (xuo) mp->xuo = *xuo' branch is skipped, leaving mp->xuo with
uninitialized stack data.

Both callers declare 'struct xfrm_migrate m[XFRM_MAX_DEPTH]' on the
stack without zeroing:

net/key/af_key.c:pfkey_migrate() {
    struct xfrm_migrate m[XFRM_MAX_DEPTH];
    ...
    err = xfrm_migrate(&sel, dir, XFRM_POLICY_TYPE_MAIN, m, i,
                       k.local && k.remote ? &k : NULL, net,
                       NULL, 0, NULL);
}

net/xfrm/xfrm_user.c:xfrm_do_migrate() {
    struct xfrm_migrate m[XFRM_MAX_DEPTH];

-  struct xfrm_migrate m[XFRM_MAX_DEPTH];
+  struct xfrm_migrate m[XFRM_MAX_DEPTH] = {};

this should fix it.

    ...
    err = xfrm_migrate(&sel, dir, type, m, n, k, net, encap, xuo, extack);
}

The uninitialized mp->xuo.ifindex is later read in
xfrm_state_migrate_create() and xfrm_state_migrate_install():

net/xfrm/xfrm_state.c:xfrm_state_migrate_create() {
    ...
    /* configure the hardware if offload is requested */
    if (m->xuo.ifindex && xfrm_dev_state_add(net, xc, &m->xuo, extack))
        goto error;
}

net/xfrm/xfrm_state.c:xfrm_state_migrate_install() {
    ...
    if (xfrm_state_add(xc) < 0) {
        NL_SET_ERR_MSG(extack, "Failed to add migrated state");
        if (m->xuo.ifindex)
            xfrm_dev_state_delete(xc);
        ...
    }
}

If the stack garbage contains a non-zero ifindex value,
xfrm_dev_state_add() will be called with random ifindex and flags,
potentially causing spurious hardware offload attempts or migration
failures.

Previously, 'mp->xuo = xuo' unconditionally set the pointer to NULL in
these paths, which was then properly checked as 'if (m->xuo)'.

Should mp->xuo be zero-initialized when xuo is NULL, either by adding
'else memset(&mp->xuo, 0, sizeof(mp->xuo))' after the if-block, or by
zero-initializing the m[] arrays in the callers?

thanks,
-antony