[PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

Add 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.

The reqid is invariant in the old migration.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v1->v2: merged next patch here to fix use uninitialized value
        - removed unnecessary inline
        - added const when possible

v2->v3: free the skb on the error path

v3->v4: preserve reqid invariant for each state migrated

v4->v5: - set portid, seq in XFRM_MSG_MIGRATE_STATE netlink notification
	- rename error label to out for clarity
	- add locking and synchronize after cloning
	- change some if(x) to if(!x) for clarity
	- call __xfrm_state_delete() inside the lock
	- return error from xfrm_send_migrate_state() instead of always
	  returning 0
---
 include/net/xfrm.h          |   1 +
 include/uapi/linux/xfrm.h   |  11 +++
 net/xfrm/xfrm_policy.c      |   1 +
 net/xfrm/xfrm_state.c       |   8 +-
 net/xfrm/xfrm_user.c        | 176 ++++++++++++++++++++++++++++++++++++
 security/selinux/nlmsgtab.c |   3 +-
 6 files changed, 195 insertions(+), 5 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 6064ea0a6f2b..906027aec40b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -686,6 +686,7 @@ struct xfrm_migrate {
 	u8			mode;
 	u16			reserved;
 	u32			old_reqid;
+	u32			new_reqid;
 	u16			old_family;
 	u16			new_family;
 };
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index a23495c0e0a1..60b1f201b237 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -227,6 +227,9 @@ enum {
 #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT
 	XFRM_MSG_GETDEFAULT,
 #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT
+
+	XFRM_MSG_MIGRATE_STATE,
+#define XFRM_MSG_MIGRATE_STATE XFRM_MSG_MIGRATE_STATE
 	__XFRM_MSG_MAX
 };
 #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1)
@@ -507,6 +510,14 @@ struct xfrm_user_migrate {
 	__u16				new_family;
 };

+struct xfrm_user_migrate_state {
+	struct xfrm_usersa_id id;
+	xfrm_address_t new_saddr;
+	xfrm_address_t new_daddr;
+	__u16 new_family;
+	__u32 new_reqid;
+};
+
 struct xfrm_user_mapping {
 	struct xfrm_usersa_id		id;
 	__u32				reqid;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 854dfc16ed55..72678053bd69 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4672,6 +4672,7 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
 			x_cur[nx_cur] = x;
 			nx_cur++;
+			mp->new_reqid = x->props.reqid; /* reqid is invariant in XFRM_MSG_MIGRATE */
 			xc = xfrm_state_migrate(x, mp, encap, net, xuo, extack);
 			if (xc) {
 				x_new[nx_new] = xc;
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 2e03871ae872..945e0e470c0f 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1979,7 +1979,6 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 	memcpy(&x->lft, &orig->lft, sizeof(x->lft));
 	x->props.mode = orig->props.mode;
 	x->props.replay_window = orig->props.replay_window;
-	x->props.reqid = orig->props.reqid;

 	if (orig->aalg) {
 		x->aalg = xfrm_algo_auth_clone(orig->aalg);
@@ -2053,7 +2052,7 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 			goto error;
 	}

-
+	x->props.reqid = m->new_reqid;
 	x->props.family = m->new_family;
 	memcpy(&x->id.daddr, &m->new_daddr, sizeof(x->id.daddr));
 	memcpy(&x->props.saddr, &m->new_saddr, sizeof(x->props.saddr));
@@ -2159,9 +2158,10 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
 			       struct xfrm_user_offload *xuo,
 			       struct netlink_ext_ack *extack)
 {
-	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
+	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) ||
+	    x->props.reqid != xc->props.reqid) {
 		/*
-		 * Care is needed when the destination address
+		 * Care is needed when the destination address or reqid
 		 * of the state is to be updated as it is a part of triplet.
 		 */
 		xfrm_state_insert(xc);
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 26b82d94acc1..79e65e3e278a 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3052,6 +3052,20 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
 }

 #ifdef CONFIG_XFRM_MIGRATE
+static void copy_from_user_migrate_state(struct xfrm_migrate *ma,
+					 const struct xfrm_user_migrate_state *um)
+{
+	memcpy(&ma->old_daddr, &um->id.daddr, sizeof(ma->old_daddr));
+	memcpy(&ma->new_daddr, &um->new_daddr, sizeof(ma->new_daddr));
+	memcpy(&ma->new_saddr, &um->new_saddr, sizeof(ma->new_saddr));
+
+	ma->proto = um->id.proto;
+	ma->new_reqid = um->new_reqid;
+
+	ma->old_family = um->id.family;
+	ma->new_family = um->new_family;
+}
+
 static int copy_from_user_migrate(struct xfrm_migrate *ma,
 				  struct xfrm_kmaddress *k,
 				  struct nlattr **attrs, int *num,
@@ -3154,7 +3168,167 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
 	kfree(xuo);
 	return err;
 }
+
+static int build_migrate_state(struct sk_buff *skb,
+			       const struct xfrm_user_migrate_state *m,
+			       const struct xfrm_encap_tmpl *encap,
+			       const struct xfrm_user_offload *xuo,
+			       u32 portid, u32 seq)
+{
+	int err;
+	struct nlmsghdr *nlh;
+	struct xfrm_user_migrate_state *um;
+
+	nlh = nlmsg_put(skb, portid, seq, XFRM_MSG_MIGRATE_STATE,
+			sizeof(struct xfrm_user_migrate_state), 0);
+	if (!nlh)
+		return -EMSGSIZE;
+
+	um = nlmsg_data(nlh);
+	*um = *m;
+
+	if (encap) {
+		err = nla_put(skb, XFRMA_ENCAP, sizeof(*encap), encap);
+		if (err)
+			goto out_cancel;
+	}
+
+	if (xuo) {
+		err = nla_put(skb, XFRMA_OFFLOAD_DEV, sizeof(*xuo), xuo);
+		if (err)
+			goto out_cancel;
+	}
+
+	nlmsg_end(skb, nlh);
+	return 0;
+
+out_cancel:
+	nlmsg_cancel(skb, nlh);
+	return err;
+}
+
+static unsigned int xfrm_migrate_state_msgsize(bool with_encap, bool with_xuo)
+{
+	return NLMSG_ALIGN(sizeof(struct xfrm_user_migrate_state)) +
+		(with_encap ? nla_total_size(sizeof(struct xfrm_encap_tmpl)) : 0) +
+		(with_xuo ? nla_total_size(sizeof(struct xfrm_user_offload)) : 0);
+}
+
+static int xfrm_send_migrate_state(const struct xfrm_user_migrate_state *um,
+				   const struct xfrm_encap_tmpl *encap,
+				   const struct xfrm_user_offload *xuo,
+				   u32 portid, u32 seq)
+{
+	int err;
+	struct sk_buff *skb;
+	struct net *net = &init_net;
+
+	skb = nlmsg_new(xfrm_migrate_state_msgsize(!!encap, !!xuo), GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	err = build_migrate_state(skb, um, encap, xuo, portid, seq);
+	if (err < 0) {
+		kfree_skb(skb);
+		return err;
+	}
+
+	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
+}
+
+static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
+				 struct nlattr **attrs, struct netlink_ext_ack *extack)
+{
+	int err = -ESRCH;
+	struct xfrm_state *x;
+	struct xfrm_state *xc;
+	struct net *net = sock_net(skb->sk);
+	struct xfrm_encap_tmpl *encap = NULL;
+	struct xfrm_user_offload *xuo = NULL;
+	struct xfrm_migrate m = {};
+	struct xfrm_user_migrate_state *um = nlmsg_data(nlh);
+
+	if (!um->id.spi) {
+		NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
+		return -EINVAL;
+	}
+
+	copy_from_user_migrate_state(&m, um);
+
+	x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
+	if (!x) {
+		NL_SET_ERR_MSG(extack, "Can not find state");
+		return err;
+	}
+
+	if (!x->dir) {
+		NL_SET_ERR_MSG(extack, "State direction is invalid");
+		err = -EINVAL;
+		goto out;
+	}
+
+	if (attrs[XFRMA_ENCAP]) {
+		encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]), sizeof(*encap),
+				GFP_KERNEL);
+		if (!encap) {
+			err = -ENOMEM;
+			goto out;
+		}
+	}
+
+	if (attrs[XFRMA_OFFLOAD_DEV]) {
+		xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
+			      sizeof(*xuo), GFP_KERNEL);
+		if (!xuo) {
+			err = -ENOMEM;
+			goto out;
+		}
+	}
+
+	xc = xfrm_state_migrate_create(x, &m, encap, net, xuo, extack);
+	if (!xc) {
+		if (extack && !extack->_msg)
+			NL_SET_ERR_MSG(extack, "State migration clone failed");
+		err = -EINVAL;
+		goto out;
+	}
+
+	spin_lock_bh(&x->lock);
+	/* synchronize to prevent SN/IV reuse */
+	xfrm_migrate_sync(xc, x);
+	__xfrm_state_delete(x);
+	spin_unlock_bh(&x->lock);
+
+	err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
+	if (err < 0) {
+		/*
+		 * In this rare case both the old SA and the new SA
+		 * will disappear.
+		 * Alternatives risk duplicate SN/IV usage which must not occur.
+		 * Userspace must handle this error, -EEXIST.
+		 */
+		goto out;
+	}
+
+	err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
+				      nlh->nlmsg_seq);
+	if (err < 0)
+		NL_SET_ERR_MSG(extack, "Failed to send migration notification");
+out:
+	xfrm_state_put(x);
+	kfree(encap);
+	kfree(xuo);
+	return err;
+}
+
 #else
+static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
+				 struct nlattr **attrs, struct netlink_ext_ack *extack)
+{
+	NL_SET_ERR_MSG(extack, "XFRM_MSG_MIGRATE_STATE is not supported");
+	return -ENOPROTOOPT;
+}
+
 static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
 			   struct nlattr **attrs, struct netlink_ext_ack *extack)
 {
@@ -3307,6 +3481,7 @@ const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
 	[XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = sizeof(u32),
 	[XFRM_MSG_SETDEFAULT  - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_default),
 	[XFRM_MSG_GETDEFAULT  - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_default),
+	[XFRM_MSG_MIGRATE_STATE - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_migrate_state),
 };
 EXPORT_SYMBOL_GPL(xfrm_msg_min);

@@ -3400,6 +3575,7 @@ static const struct xfrm_link {
 	[XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = { .doit = xfrm_get_spdinfo   },
 	[XFRM_MSG_SETDEFAULT  - XFRM_MSG_BASE] = { .doit = xfrm_set_default   },
 	[XFRM_MSG_GETDEFAULT  - XFRM_MSG_BASE] = { .doit = xfrm_get_default   },
+	[XFRM_MSG_MIGRATE_STATE - XFRM_MSG_BASE] = { .doit = xfrm_do_migrate_state },
 };

 static int xfrm_reject_unused_attr(int type, struct nlattr **attrs,
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 2c0b07f9fbbd..655d2616c9d2 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -128,6 +128,7 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] = {
 	{ XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ },
 	{ XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
 	{ XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ },
+	{ XFRM_MSG_MIGRATE_STATE, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
 };

 static const struct nlmsg_perm nlmsg_audit_perms[] = {
@@ -203,7 +204,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
 		 * structures at the top of this file with the new mappings
 		 * before updating the BUILD_BUG_ON() macro!
 		 */
-		BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT);
+		BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MIGRATE_STATE);

 		if (selinux_policycap_netlink_xperm()) {
 			*perm = NETLINK_XFRM_SOCKET__NLMSG;
--
2.39.5

Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Sabrina Dubroca]

2026-01-27, 11:44:11 +0100, Antony Antony wrote:
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index a23495c0e0a1..60b1f201b237 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
[...]
> +struct xfrm_user_migrate_state {
> +	struct xfrm_usersa_id id;
> +	xfrm_address_t new_saddr;
> +	xfrm_address_t new_daddr;
> +	__u16 new_family;
> +	__u32 new_reqid;
> +};

I'm not entirely clear on why this struct has those fields (maybe, in
particular, new_saddr but no old_saddr, assuming that id.daddr is
old_daddr). My guess is:

  - usersa_id because it's roughly equivalent to a GETSA request,
    which makes the old_saddr unnecessary (id uniquely identifies the
    target SA)

  - new_{saddr,daddr,family,reqid}
    equivalent to the new_* from xfrm_user_migrate (+reqid)

Is that correct?


> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 2e03871ae872..945e0e470c0f 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
[...]
> @@ -2159,9 +2158,10 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
>  			       struct xfrm_user_offload *xuo,
>  			       struct netlink_ext_ack *extack)
>  {
> -	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> +	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) ||

[piggy-backing on this patch review, but it's an older issue, and may
also be present in a few other places]

Is it valid to call xfrm_addr_equal without checking new_family ==
old_family? My feeling is "no", addresses of different families can't
be equal at all.

What we end up doing here:
old_family = AF_INET, new_family = AF_INET6
old_daddr has only 4B containing valid data, we're comparing the whole
16B to new_daddr (but what's in the other 12B?)

old_family = AF_INET6, new_family = AF_INET
we're comparing using new_family, so we only compare the first 4B of
old_daddr to the new address


> +	    x->props.reqid != xc->props.reqid) {
>  		/*
> -		 * Care is needed when the destination address
> +		 * Care is needed when the destination address or reqid
>  		 * of the state is to be updated as it is a part of triplet.

I'm quite confused by this bit. The existing code is "unchanged daddr,
use _insert, otherwise _add" (to let _add check for collisions that
are irrelevant with an unchanged daddr?). The new code is for a change
of reqid. Why does reqid need to be handled with care? And should the
reqid condition be reversed (same reqid => use _insert)?


> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 26b82d94acc1..79e65e3e278a 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
[...]
> +static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
> +				 struct nlattr **attrs, struct netlink_ext_ack *extack)
> +{
> +	int err = -ESRCH;
> +	struct xfrm_state *x;
> +	struct xfrm_state *xc;
> +	struct net *net = sock_net(skb->sk);
> +	struct xfrm_encap_tmpl *encap = NULL;
> +	struct xfrm_user_offload *xuo = NULL;
> +	struct xfrm_migrate m = {};
> +	struct xfrm_user_migrate_state *um = nlmsg_data(nlh);

I don't know if Steffen requires it, but networking normally uses
reverse xmas tree order.

> +	if (!um->id.spi) {
> +		NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
> +		return -EINVAL;
> +	}
> +
> +	copy_from_user_migrate_state(&m, um);
> +
> +	x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
> +	if (!x) {
> +		NL_SET_ERR_MSG(extack, "Can not find state");
> +		return err;
> +	}
> +
> +	if (!x->dir) {
> +		NL_SET_ERR_MSG(extack, "State direction is invalid");

Why this restriction?
Also, should there be a match against XFRMA_SA_DIR? (I don't see one in
xfrm_user_state_lookup)


I think we should also reject attributes that we're not handling for
all new netlink message types. This would give us more freedom of
interpretation in future updates to this code.

> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (attrs[XFRMA_ENCAP]) {
> +		encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]), sizeof(*encap),

I guess you c/p'd this from the old migrate code but... do we really
need a kmemdup here? xfrm_state_clone_and_setup() will make another
copy to assign to x->encap so here encap could just point to
nla_data(attrs[XFRMA_ENCAP])?


> +				GFP_KERNEL);
> +		if (!encap) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +	}
> +
> +	if (attrs[XFRMA_OFFLOAD_DEV]) {
> +		xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> +			      sizeof(*xuo), GFP_KERNEL);

And same here, I don't think we actually need a copy of that memory.

> +		if (!xuo) {
> +			err = -ENOMEM;
> +			goto out;
> +		}
> +	}
> +
> +	xc = xfrm_state_migrate_create(x, &m, encap, net, xuo, extack);
> +	if (!xc) {
> +		if (extack && !extack->_msg)
> +			NL_SET_ERR_MSG(extack, "State migration clone failed");

NL_SET_ERR_MSG_WEAK(...)

> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	spin_lock_bh(&x->lock);
> +	/* synchronize to prevent SN/IV reuse */
> +	xfrm_migrate_sync(xc, x);
> +	__xfrm_state_delete(x);
> +	spin_unlock_bh(&x->lock);
> +
> +	err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
> +	if (err < 0) {
> +		/*
> +		 * In this rare case both the old SA and the new SA
> +		 * will disappear.
> +		 * Alternatives risk duplicate SN/IV usage which must not occur.
> +		 * Userspace must handle this error, -EEXIST.
> +		 */
> +		goto out;
> +	}
> +
> +	err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
> +				      nlh->nlmsg_seq);
> +	if (err < 0)
> +		NL_SET_ERR_MSG(extack, "Failed to send migration notification");

I feel this is a bit problematic as it will look like the operation
failed, but in reality only the notification has not been sent (but
the MIGRATE_STATE operation itself succeeded).

> +out:
> +	xfrm_state_put(x);
> +	kfree(encap);
> +	kfree(xuo);
> +	return err;
> +}
> +

-- 
Sabrina

Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

Hi Sabrina,

Thanks for your extensive review. Along the way I also noticed a couple of 
more minor issues and fixed them. I will send
a v6 addressing the points from this email.

On Tue, Feb 03, 2026 at 10:25:15PM +0100, Sabrina Dubroca via Devel wrote:
> 2026-01-27, 11:44:11 +0100, Antony Antony wrote:
> > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > index a23495c0e0a1..60b1f201b237 100644
> > --- a/include/uapi/linux/xfrm.h
> > +++ b/include/uapi/linux/xfrm.h
> [...]
> > +struct xfrm_user_migrate_state {
> > +	struct xfrm_usersa_id id;
> > +	xfrm_address_t new_saddr;
> > +	xfrm_address_t new_daddr;
> > +	__u16 new_family;
> > +	__u32 new_reqid;
> > +};
> 
> I'm not entirely clear on why this struct has those fields (maybe, in
> particular, new_saddr but no old_saddr, assuming that id.daddr is
> old_daddr). My guess is:
> 
>   - usersa_id because it's roughly equivalent to a GETSA request,
>     which makes the old_saddr unnecessary (id uniquely identifies the
>     target SA)
> 
>   - new_{saddr,daddr,family,reqid}
>     equivalent to the new_* from xfrm_user_migrate (+reqid)
> 
> Is that correct?

Yes, exactly. The SA is looked up via xfrm_usersa_id, which uniquely
identifies it, so old_saddr is not needed. old_daddr is carried in
xfrm_usersa_id.daddr.

> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 2e03871ae872..945e0e470c0f 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> [...]
> > @@ -2159,9 +2158,10 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
> >  			       struct xfrm_user_offload *xuo,
> >  			       struct netlink_ext_ack *extack)
> >  {
> > -	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> > +	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) ||
> 
> [piggy-backing on this patch review, but it's an older issue, and may
> also be present in a few other places]
> 
> Is it valid to call xfrm_addr_equal without checking new_family ==
> old_family? My feeling is "no", addresses of different families can't
> be equal at all.
> 
> What we end up doing here:
> old_family = AF_INET, new_family = AF_INET6
> old_daddr has only 4B containing valid data, we're comparing the whole
> 16B to new_daddr (but what's in the other 12B?)
> 
> old_family = AF_INET6, new_family = AF_INET
> we're comparing using new_family, so we only compare the first 4B of
> old_daddr to the new address

good catch. It existed before. I will send a fix as part of this series.

> 
> 
> > +	    x->props.reqid != xc->props.reqid) {
> >  		/*
> > -		 * Care is needed when the destination address
> > +		 * Care is needed when the destination address or reqid
> >  		 * of the state is to be updated as it is a part of triplet.
> 
> I'm quite confused by this bit. The existing code is "unchanged daddr,
> use _insert, otherwise _add" (to let _add check for collisions that
> are irrelevant with an unchanged daddr?). The new code is for a change
> of reqid. Why does reqid need to be handled with care? And should the
> reqid condition be reversed (same reqid => use _insert)?

I revisited it and reqid change does not need insert. _add is good enough.  
Fixed. Thanks.

> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index 26b82d94acc1..79e65e3e278a 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> [...]
> > +static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
> > +				 struct nlattr **attrs, struct netlink_ext_ack *extack)
> > +{
> > +	int err = -ESRCH;
> > +	struct xfrm_state *x;
> > +	struct xfrm_state *xc;
> > +	struct net *net = sock_net(skb->sk);
> > +	struct xfrm_encap_tmpl *encap = NULL;
> > +	struct xfrm_user_offload *xuo = NULL;
> > +	struct xfrm_migrate m = {};
> > +	struct xfrm_user_migrate_state *um = nlmsg_data(nlh);
> 
> I don't know if Steffen requires it, but networking normally uses
> reverse xmas tree order. 

It is good to keep that style. Fixed.

> > +	if (!um->id.spi) {
> > +		NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
> > +		return -EINVAL;
> > +	}
> > +
> > +	copy_from_user_migrate_state(&m, um);
> > +
> > +	x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
> > +	if (!x) {
> > +		NL_SET_ERR_MSG(extack, "Can not find state");
> > +		return err;
> > +	}
> > +
> > +	if (!x->dir) {
> > +		NL_SET_ERR_MSG(extack, "State direction is invalid");
> 
> Why this restriction?
> Also, should there be a match against XFRMA_SA_DIR? (I don't see one in
> xfrm_user_state_lookup)

The !x->dir check is not strictly necessary. It was a leftover from an 
earlier iteration that was dropped. I removed it

 
> I think we should also reject attributes that we're not handling for
> all new netlink message types. This would give us more freedom of
> interpretation in future updates to this code.

good idea. I have added new validate attributes patch


> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	if (attrs[XFRMA_ENCAP]) {
> > +		encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]), sizeof(*encap),
> 
> I guess you c/p'd this from the old migrate code but... do we really
> need a kmemdup here? xfrm_state_clone_and_setup() will make another
> copy to assign to x->encap so here encap could just point to
> nla_data(attrs[XFRMA_ENCAP])?

why not:) It is time to change, though it is a widely used pattern in the 
same file.

> 
> 
> > +				GFP_KERNEL);
> > +		if (!encap) {
> > +			err = -ENOMEM;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	if (attrs[XFRMA_OFFLOAD_DEV]) {
> > +		xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> > +			      sizeof(*xuo), GFP_KERNEL);
> 
> And same here, I don't think we actually need a copy of that memory.

changed. thanks.

> 
> > +		if (!xuo) {
> > +			err = -ENOMEM;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	xc = xfrm_state_migrate_create(x, &m, encap, net, xuo, extack);
> > +	if (!xc) {
> > +		if (extack && !extack->_msg)
> > +			NL_SET_ERR_MSG(extack, "State migration clone failed");
> 
> NL_SET_ERR_MSG_WEAK(...)

thanks I was looking for this!

> 
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	spin_lock_bh(&x->lock);
> > +	/* synchronize to prevent SN/IV reuse */
> > +	xfrm_migrate_sync(xc, x);
> > +	__xfrm_state_delete(x);
> > +	spin_unlock_bh(&x->lock);
> > +
> > +	err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
> > +	if (err < 0) {
> > +		/*
> > +		 * In this rare case both the old SA and the new SA
> > +		 * will disappear.
> > +		 * Alternatives risk duplicate SN/IV usage which must not occur.
> > +		 * Userspace must handle this error, -EEXIST.
> > +		 */
> > +		goto out;
> > +	}
> > +
> > +	err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
> > +				      nlh->nlmsg_seq);
> > +	if (err < 0)
> > +		NL_SET_ERR_MSG(extack, "Failed to send migration notification");
> 
> I feel this is a bit problematic as it will look like the operation
> failed, but in reality only the notification has not been sent (but
> the MIGRATE_STATE operation itself succeeded).

It is not critical, however, the best choice is let the userspace decide.
How about this 

if (err < 0) {
                NL_SET_ERR_MSG(extack, "Failed to send migration notification");
                err = 0
} 

most likely cause is out of memory.

> 
> > +out:
> > +	xfrm_state_put(x);
> > +	kfree(encap);
> > +	kfree(xuo);
> > +	return err;
> > +}
> > +
> 

-antony

Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Sabrina Dubroca]

2026-02-26, 16:46:49 +0100, Antony Antony wrote:
> Hi Sabrina,
> 
> Thanks for your extensive review. Along the way I also noticed a couple of 
> more minor issues and fixed them. I will send
> a v6 addressing the points from this email.

Thanks Antony.

Just a few things related to your reply:

> On Tue, Feb 03, 2026 at 10:25:15PM +0100, Sabrina Dubroca via Devel wrote:
> > 2026-01-27, 11:44:11 +0100, Antony Antony wrote:
> > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > > index a23495c0e0a1..60b1f201b237 100644
> > > --- a/include/uapi/linux/xfrm.h
> > > +++ b/include/uapi/linux/xfrm.h
> > [...]
> > > +struct xfrm_user_migrate_state {
> > > +	struct xfrm_usersa_id id;
> > > +	xfrm_address_t new_saddr;
> > > +	xfrm_address_t new_daddr;
> > > +	__u16 new_family;
> > > +	__u32 new_reqid;
> > > +};
> > 
> > I'm not entirely clear on why this struct has those fields (maybe, in
> > particular, new_saddr but no old_saddr, assuming that id.daddr is
> > old_daddr). My guess is:
> > 
> >   - usersa_id because it's roughly equivalent to a GETSA request,
> >     which makes the old_saddr unnecessary (id uniquely identifies the
> >     target SA)
> > 
> >   - new_{saddr,daddr,family,reqid}
> >     equivalent to the new_* from xfrm_user_migrate (+reqid)
> > 
> > Is that correct?
> 
> Yes, exactly. The SA is looked up via xfrm_usersa_id, which uniquely
> identifies it, so old_saddr is not needed. old_daddr is carried in
> xfrm_usersa_id.daddr.

Thanks. Maybe worth adding a small note in the commit message to
describe the behavior of that new op? (pretty much what you wrote
here)

I know the old stuff isn't documented much, I'm not asking for an
extensive new file in Documentation.


[...]
> > > +	err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
> > > +	if (err < 0) {
> > > +		/*
> > > +		 * In this rare case both the old SA and the new SA
> > > +		 * will disappear.
> > > +		 * Alternatives risk duplicate SN/IV usage which must not occur.
> > > +		 * Userspace must handle this error, -EEXIST.
> > > +		 */
> > > +		goto out;
> > > +	}
> > > +
> > > +	err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
> > > +				      nlh->nlmsg_seq);
> > > +	if (err < 0)
> > > +		NL_SET_ERR_MSG(extack, "Failed to send migration notification");
> > 
> > I feel this is a bit problematic as it will look like the operation
> > failed, but in reality only the notification has not been sent (but
> > the MIGRATE_STATE operation itself succeeded).
> 
> It is not critical, however, the best choice is let the userspace decide.
> How about this 
> 
> if (err < 0) {
>                 NL_SET_ERR_MSG(extack, "Failed to send migration notification");
>                 err = 0
> } 
> 
> most likely cause is out of memory.

Does userspace really check the extack it gets back when the operation
succeeds? But ok, that seems fine to me.

[Looking at the existing callers of xfrm_nlmsg_multicast, many
existing calls seem to completely ignore the return value
(km_state_notify -> xfrm_send_state_notify, km_policy_notify ->
xfrm_send_policy_notify, which are called from the main NETLINK_XFRM
ops), so at least returning 0 would be consistent with those (but
there's no extack on failing to notify for the other ops)]

-- 
Sabrina

Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Yan Yan]

Hi Antony,

May I request that we also support updating the XFRMA_SET_MARK as part
of the new XFRM_MSG_MIGRATE_STATE message?

In Android, the primary use case for migration is switching the
underlying physical network for an IPsec tunnel (e.g. VPN, Wifi
calling). Currently, due to the limits of XFRM_MSG_MIGRATE, we are
forced to use a separate UPDSA call to update the set-mark. Supporting
XFRMA_SET_MARK within the migrate message would allow us to update the
addresses and the routing mark together in one atomic call.

Regarding the logic, I believe the set-mark can follow the same
omit-to-clear pattern as XFRMA_ENCAP and XFRMA_OFFLOAD_DEV.

What do you think?

Yan and Nathan


On Tue, Jan 27, 2026 at 2:44 AM Antony Antony <antony.antony@secunet.com> wrote:
>
> Add 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.
>
> The reqid is invariant in the old migration.
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> v1->v2: merged next patch here to fix use uninitialized value
>         - removed unnecessary inline
>         - added const when possible
>
> v2->v3: free the skb on the error path
>
> v3->v4: preserve reqid invariant for each state migrated
>
> v4->v5: - set portid, seq in XFRM_MSG_MIGRATE_STATE netlink notification
>         - rename error label to out for clarity
>         - add locking and synchronize after cloning
>         - change some if(x) to if(!x) for clarity
>         - call __xfrm_state_delete() inside the lock
>         - return error from xfrm_send_migrate_state() instead of always
>           returning 0
> ---
>  include/net/xfrm.h          |   1 +
>  include/uapi/linux/xfrm.h   |  11 +++
>  net/xfrm/xfrm_policy.c      |   1 +
>  net/xfrm/xfrm_state.c       |   8 +-
>  net/xfrm/xfrm_user.c        | 176 ++++++++++++++++++++++++++++++++++++
>  security/selinux/nlmsgtab.c |   3 +-
>  6 files changed, 195 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 6064ea0a6f2b..906027aec40b 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -686,6 +686,7 @@ struct xfrm_migrate {
>         u8                      mode;
>         u16                     reserved;
>         u32                     old_reqid;
> +       u32                     new_reqid;
>         u16                     old_family;
>         u16                     new_family;
>  };
> diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> index a23495c0e0a1..60b1f201b237 100644
> --- a/include/uapi/linux/xfrm.h
> +++ b/include/uapi/linux/xfrm.h
> @@ -227,6 +227,9 @@ enum {
>  #define XFRM_MSG_SETDEFAULT XFRM_MSG_SETDEFAULT
>         XFRM_MSG_GETDEFAULT,
>  #define XFRM_MSG_GETDEFAULT XFRM_MSG_GETDEFAULT
> +
> +       XFRM_MSG_MIGRATE_STATE,
> +#define XFRM_MSG_MIGRATE_STATE XFRM_MSG_MIGRATE_STATE
>         __XFRM_MSG_MAX
>  };
>  #define XFRM_MSG_MAX (__XFRM_MSG_MAX - 1)
> @@ -507,6 +510,14 @@ struct xfrm_user_migrate {
>         __u16                           new_family;
>  };
>
> +struct xfrm_user_migrate_state {
> +       struct xfrm_usersa_id id;
> +       xfrm_address_t new_saddr;
> +       xfrm_address_t new_daddr;
> +       __u16 new_family;
> +       __u32 new_reqid;
> +};
> +
>  struct xfrm_user_mapping {
>         struct xfrm_usersa_id           id;
>         __u32                           reqid;
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 854dfc16ed55..72678053bd69 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -4672,6 +4672,7 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
>                 if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
>                         x_cur[nx_cur] = x;
>                         nx_cur++;
> +                       mp->new_reqid = x->props.reqid; /* reqid is invariant in XFRM_MSG_MIGRATE */
>                         xc = xfrm_state_migrate(x, mp, encap, net, xuo, extack);
>                         if (xc) {
>                                 x_new[nx_new] = xc;
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 2e03871ae872..945e0e470c0f 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1979,7 +1979,6 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
>         memcpy(&x->lft, &orig->lft, sizeof(x->lft));
>         x->props.mode = orig->props.mode;
>         x->props.replay_window = orig->props.replay_window;
> -       x->props.reqid = orig->props.reqid;
>
>         if (orig->aalg) {
>                 x->aalg = xfrm_algo_auth_clone(orig->aalg);
> @@ -2053,7 +2052,7 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
>                         goto error;
>         }
>
> -
> +       x->props.reqid = m->new_reqid;
>         x->props.family = m->new_family;
>         memcpy(&x->id.daddr, &m->new_daddr, sizeof(x->id.daddr));
>         memcpy(&x->props.saddr, &m->new_saddr, sizeof(x->props.saddr));
> @@ -2159,9 +2158,10 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
>                                struct xfrm_user_offload *xuo,
>                                struct netlink_ext_ack *extack)
>  {
> -       if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> +       if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) ||
> +           x->props.reqid != xc->props.reqid) {
>                 /*
> -                * Care is needed when the destination address
> +                * Care is needed when the destination address or reqid
>                  * of the state is to be updated as it is a part of triplet.
>                  */
>                 xfrm_state_insert(xc);
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 26b82d94acc1..79e65e3e278a 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -3052,6 +3052,20 @@ static int xfrm_add_acquire(struct sk_buff *skb, struct nlmsghdr *nlh,
>  }
>
>  #ifdef CONFIG_XFRM_MIGRATE
> +static void copy_from_user_migrate_state(struct xfrm_migrate *ma,
> +                                        const struct xfrm_user_migrate_state *um)
> +{
> +       memcpy(&ma->old_daddr, &um->id.daddr, sizeof(ma->old_daddr));
> +       memcpy(&ma->new_daddr, &um->new_daddr, sizeof(ma->new_daddr));
> +       memcpy(&ma->new_saddr, &um->new_saddr, sizeof(ma->new_saddr));
> +
> +       ma->proto = um->id.proto;
> +       ma->new_reqid = um->new_reqid;
> +
> +       ma->old_family = um->id.family;
> +       ma->new_family = um->new_family;
> +}
> +
>  static int copy_from_user_migrate(struct xfrm_migrate *ma,
>                                   struct xfrm_kmaddress *k,
>                                   struct nlattr **attrs, int *num,
> @@ -3154,7 +3168,167 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
>         kfree(xuo);
>         return err;
>  }
> +
> +static int build_migrate_state(struct sk_buff *skb,
> +                              const struct xfrm_user_migrate_state *m,
> +                              const struct xfrm_encap_tmpl *encap,
> +                              const struct xfrm_user_offload *xuo,
> +                              u32 portid, u32 seq)
> +{
> +       int err;
> +       struct nlmsghdr *nlh;
> +       struct xfrm_user_migrate_state *um;
> +
> +       nlh = nlmsg_put(skb, portid, seq, XFRM_MSG_MIGRATE_STATE,
> +                       sizeof(struct xfrm_user_migrate_state), 0);
> +       if (!nlh)
> +               return -EMSGSIZE;
> +
> +       um = nlmsg_data(nlh);
> +       *um = *m;
> +
> +       if (encap) {
> +               err = nla_put(skb, XFRMA_ENCAP, sizeof(*encap), encap);
> +               if (err)
> +                       goto out_cancel;
> +       }
> +
> +       if (xuo) {
> +               err = nla_put(skb, XFRMA_OFFLOAD_DEV, sizeof(*xuo), xuo);
> +               if (err)
> +                       goto out_cancel;
> +       }
> +
> +       nlmsg_end(skb, nlh);
> +       return 0;
> +
> +out_cancel:
> +       nlmsg_cancel(skb, nlh);
> +       return err;
> +}
> +
> +static unsigned int xfrm_migrate_state_msgsize(bool with_encap, bool with_xuo)
> +{
> +       return NLMSG_ALIGN(sizeof(struct xfrm_user_migrate_state)) +
> +               (with_encap ? nla_total_size(sizeof(struct xfrm_encap_tmpl)) : 0) +
> +               (with_xuo ? nla_total_size(sizeof(struct xfrm_user_offload)) : 0);
> +}
> +
> +static int xfrm_send_migrate_state(const struct xfrm_user_migrate_state *um,
> +                                  const struct xfrm_encap_tmpl *encap,
> +                                  const struct xfrm_user_offload *xuo,
> +                                  u32 portid, u32 seq)
> +{
> +       int err;
> +       struct sk_buff *skb;
> +       struct net *net = &init_net;
> +
> +       skb = nlmsg_new(xfrm_migrate_state_msgsize(!!encap, !!xuo), GFP_ATOMIC);
> +       if (!skb)
> +               return -ENOMEM;
> +
> +       err = build_migrate_state(skb, um, encap, xuo, portid, seq);
> +       if (err < 0) {
> +               kfree_skb(skb);
> +               return err;
> +       }
> +
> +       return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
> +}
> +
> +static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
> +                                struct nlattr **attrs, struct netlink_ext_ack *extack)
> +{
> +       int err = -ESRCH;
> +       struct xfrm_state *x;
> +       struct xfrm_state *xc;
> +       struct net *net = sock_net(skb->sk);
> +       struct xfrm_encap_tmpl *encap = NULL;
> +       struct xfrm_user_offload *xuo = NULL;
> +       struct xfrm_migrate m = {};
> +       struct xfrm_user_migrate_state *um = nlmsg_data(nlh);
> +
> +       if (!um->id.spi) {
> +               NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
> +               return -EINVAL;
> +       }
> +
> +       copy_from_user_migrate_state(&m, um);
> +
> +       x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
> +       if (!x) {
> +               NL_SET_ERR_MSG(extack, "Can not find state");
> +               return err;
> +       }
> +
> +       if (!x->dir) {
> +               NL_SET_ERR_MSG(extack, "State direction is invalid");
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
> +       if (attrs[XFRMA_ENCAP]) {
> +               encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]), sizeof(*encap),
> +                               GFP_KERNEL);
> +               if (!encap) {
> +                       err = -ENOMEM;
> +                       goto out;
> +               }
> +       }
> +
> +       if (attrs[XFRMA_OFFLOAD_DEV]) {
> +               xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> +                             sizeof(*xuo), GFP_KERNEL);
> +               if (!xuo) {
> +                       err = -ENOMEM;
> +                       goto out;
> +               }
> +       }
> +
> +       xc = xfrm_state_migrate_create(x, &m, encap, net, xuo, extack);
> +       if (!xc) {
> +               if (extack && !extack->_msg)
> +                       NL_SET_ERR_MSG(extack, "State migration clone failed");
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
> +       spin_lock_bh(&x->lock);
> +       /* synchronize to prevent SN/IV reuse */
> +       xfrm_migrate_sync(xc, x);
> +       __xfrm_state_delete(x);
> +       spin_unlock_bh(&x->lock);
> +
> +       err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
> +       if (err < 0) {
> +               /*
> +                * In this rare case both the old SA and the new SA
> +                * will disappear.
> +                * Alternatives risk duplicate SN/IV usage which must not occur.
> +                * Userspace must handle this error, -EEXIST.
> +                */
> +               goto out;
> +       }
> +
> +       err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
> +                                     nlh->nlmsg_seq);
> +       if (err < 0)
> +               NL_SET_ERR_MSG(extack, "Failed to send migration notification");
> +out:
> +       xfrm_state_put(x);
> +       kfree(encap);
> +       kfree(xuo);
> +       return err;
> +}
> +
>  #else
> +static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
> +                                struct nlattr **attrs, struct netlink_ext_ack *extack)
> +{
> +       NL_SET_ERR_MSG(extack, "XFRM_MSG_MIGRATE_STATE is not supported");
> +       return -ENOPROTOOPT;
> +}
> +
>  static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
>                            struct nlattr **attrs, struct netlink_ext_ack *extack)
>  {
> @@ -3307,6 +3481,7 @@ const int xfrm_msg_min[XFRM_NR_MSGTYPES] = {
>         [XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = sizeof(u32),
>         [XFRM_MSG_SETDEFAULT  - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_default),
>         [XFRM_MSG_GETDEFAULT  - XFRM_MSG_BASE] = XMSGSIZE(xfrm_userpolicy_default),
> +       [XFRM_MSG_MIGRATE_STATE - XFRM_MSG_BASE] = XMSGSIZE(xfrm_user_migrate_state),
>  };
>  EXPORT_SYMBOL_GPL(xfrm_msg_min);
>
> @@ -3400,6 +3575,7 @@ static const struct xfrm_link {
>         [XFRM_MSG_GETSPDINFO  - XFRM_MSG_BASE] = { .doit = xfrm_get_spdinfo   },
>         [XFRM_MSG_SETDEFAULT  - XFRM_MSG_BASE] = { .doit = xfrm_set_default   },
>         [XFRM_MSG_GETDEFAULT  - XFRM_MSG_BASE] = { .doit = xfrm_get_default   },
> +       [XFRM_MSG_MIGRATE_STATE - XFRM_MSG_BASE] = { .doit = xfrm_do_migrate_state },
>  };
>
>  static int xfrm_reject_unused_attr(int type, struct nlattr **attrs,
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 2c0b07f9fbbd..655d2616c9d2 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -128,6 +128,7 @@ static const struct nlmsg_perm nlmsg_xfrm_perms[] = {
>         { XFRM_MSG_MAPPING, NETLINK_XFRM_SOCKET__NLMSG_READ },
>         { XFRM_MSG_SETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
>         { XFRM_MSG_GETDEFAULT, NETLINK_XFRM_SOCKET__NLMSG_READ },
> +       { XFRM_MSG_MIGRATE_STATE, NETLINK_XFRM_SOCKET__NLMSG_WRITE },
>  };
>
>  static const struct nlmsg_perm nlmsg_audit_perms[] = {
> @@ -203,7 +204,7 @@ int selinux_nlmsg_lookup(u16 sclass, u16 nlmsg_type, u32 *perm)
>                  * structures at the top of this file with the new mappings
>                  * before updating the BUILD_BUG_ON() macro!
>                  */
> -               BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_GETDEFAULT);
> +               BUILD_BUG_ON(XFRM_MSG_MAX != XFRM_MSG_MIGRATE_STATE);
>
>                 if (selinux_policycap_netlink_xperm()) {
>                         *perm = NETLINK_XFRM_SOCKET__NLMSG;
> --
> 2.39.5
>


-- 
--
Best,
Yan

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Sabrina Dubroca]

2026-02-26, 17:44:51 -0800, Yan Yan via Devel wrote:
> Hi Antony,
> 
> May I request that we also support updating the XFRMA_SET_MARK as part
> of the new XFRM_MSG_MIGRATE_STATE message?
> 
> In Android, the primary use case for migration is switching the
> underlying physical network for an IPsec tunnel (e.g. VPN, Wifi
> calling). Currently, due to the limits of XFRM_MSG_MIGRATE, we are
> forced to use a separate UPDSA call to update the set-mark. Supporting
> XFRMA_SET_MARK within the migrate message would allow us to update the
> addresses and the routing mark together in one atomic call.
> 
> Regarding the logic, I believe the set-mark can follow the same
> omit-to-clear pattern as XFRMA_ENCAP and XFRMA_OFFLOAD_DEV.

I think this raises a wider question: clearly definining and
documenting which attributes need to be explicitly provided
("omit-to-clear" as you write), and which will be implicitly copied.

Currently it looks like we copy:
 - all the crypto stuff (aalg/aead/etc)
 - security context stuff
 - coaddr
 - replay/replay_esn
 - pcpu_num, if_id, tfcpad
 - dir
 - mark
 - extra_flags

but not
 - nat_keepalive_interval
 - offload
 - encap

[gathered from a quick read of xfrm_state_clone_and_setup + the
definition of xfrma_policy]

Anything that we leave as implicit copy will have to be "forever"
implicitly copied with this new MIGRATE_STATE op -- unless we find a
way to pass a new "clear these properties" flag (probably via a list
of XFRMA_* attribute names), but then we could also implement that
with the existing MIGRATE code.

-- 
Sabrina

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Yan Yan]

> Anything that we leave as implicit copy will have to be "forever"
implicitly copied with this new MIGRATE_STATE op -- unless we find a
way to pass a new "clear these properties" flag (probably via a list
of XFRMA_* attribute names)

That is true. I also have the concern that if all updatable attributes
follow the "omit-to-clear" pattern, we lose the extensibility. Thus
ideally we should switch to an "omit-to-inherit" model for some, if
not all, attributes to ensure that adding new SA properties doesn't
break backward compatibility.

On Fri, Feb 27, 2026 at 3:26 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> 2026-02-26, 17:44:51 -0800, Yan Yan via Devel wrote:
> > Hi Antony,
> >
> > May I request that we also support updating the XFRMA_SET_MARK as part
> > of the new XFRM_MSG_MIGRATE_STATE message?
> >
> > In Android, the primary use case for migration is switching the
> > underlying physical network for an IPsec tunnel (e.g. VPN, Wifi
> > calling). Currently, due to the limits of XFRM_MSG_MIGRATE, we are
> > forced to use a separate UPDSA call to update the set-mark. Supporting
> > XFRMA_SET_MARK within the migrate message would allow us to update the
> > addresses and the routing mark together in one atomic call.
> >
> > Regarding the logic, I believe the set-mark can follow the same
> > omit-to-clear pattern as XFRMA_ENCAP and XFRMA_OFFLOAD_DEV.
>
> I think this raises a wider question: clearly definining and
> documenting which attributes need to be explicitly provided
> ("omit-to-clear" as you write), and which will be implicitly copied.
>
> Currently it looks like we copy:
>  - all the crypto stuff (aalg/aead/etc)
>  - security context stuff
>  - coaddr
>  - replay/replay_esn
>  - pcpu_num, if_id, tfcpad
>  - dir
>  - mark
>  - extra_flags
>
> but not
>  - nat_keepalive_interval
>  - offload
>  - encap
>
> [gathered from a quick read of xfrm_state_clone_and_setup + the
> definition of xfrma_policy]
>
> Anything that we leave as implicit copy will have to be "forever"
> implicitly copied with this new MIGRATE_STATE op -- unless we find a
> way to pass a new "clear these properties" flag (probably via a list
> of XFRMA_* attribute names), but then we could also implement that
> with the existing MIGRATE code.
>
> --
> Sabrina



--
--
Best,
Yan

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

On Thu, Feb 26, 2026 at 07:05:59PM +0100, Sabrina Dubroca via Devel wrote:
> 2026-02-26, 16:46:49 +0100, Antony Antony wrote:
> > Hi Sabrina,
> > 
> > Thanks for your extensive review. Along the way I also noticed a couple of 
> > more minor issues and fixed them. I will send
> > a v6 addressing the points from this email.
> 
> Thanks Antony.
> 
> Just a few things related to your reply:
> 
> > On Tue, Feb 03, 2026 at 10:25:15PM +0100, Sabrina Dubroca via Devel wrote:
> > > 2026-01-27, 11:44:11 +0100, Antony Antony wrote:
> > > > diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
> > > > index a23495c0e0a1..60b1f201b237 100644
> > > > --- a/include/uapi/linux/xfrm.h
> > > > +++ b/include/uapi/linux/xfrm.h
> > > [...]
> > > > +struct xfrm_user_migrate_state {
> > > > +	struct xfrm_usersa_id id;
> > > > +	xfrm_address_t new_saddr;
> > > > +	xfrm_address_t new_daddr;
> > > > +	__u16 new_family;
> > > > +	__u32 new_reqid;
> > > > +};
> > > 
> > > I'm not entirely clear on why this struct has those fields (maybe, in
> > > particular, new_saddr but no old_saddr, assuming that id.daddr is
> > > old_daddr). My guess is:
> > > 
> > >   - usersa_id because it's roughly equivalent to a GETSA request,
> > >     which makes the old_saddr unnecessary (id uniquely identifies the
> > >     target SA)
> > > 
> > >   - new_{saddr,daddr,family,reqid}
> > >     equivalent to the new_* from xfrm_user_migrate (+reqid)
> > > 
> > > Is that correct?
> > 
> > Yes, exactly. The SA is looked up via xfrm_usersa_id, which uniquely
> > identifies it, so old_saddr is not needed. old_daddr is carried in
> > xfrm_usersa_id.daddr.
> 
> Thanks. Maybe worth adding a small note in the commit message to
> describe the behavior of that new op? (pretty much what you wrote
> here)

Yes good idea. Done!


> I know the old stuff isn't documented much, I'm not asking for an
> extensive new file in Documentation.
> 
> 
> [...]
> > > > +	err = xfrm_state_migrate_install(x, xc, &m, xuo, extack);
> > > > +	if (err < 0) {
> > > > +		/*
> > > > +		 * In this rare case both the old SA and the new SA
> > > > +		 * will disappear.
> > > > +		 * Alternatives risk duplicate SN/IV usage which must not occur.
> > > > +		 * Userspace must handle this error, -EEXIST.
> > > > +		 */
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	err = xfrm_send_migrate_state(um, encap, xuo, nlh->nlmsg_pid,
> > > > +				      nlh->nlmsg_seq);
> > > > +	if (err < 0)
> > > > +		NL_SET_ERR_MSG(extack, "Failed to send migration notification");
> > > 
> > > I feel this is a bit problematic as it will look like the operation
> > > failed, but in reality only the notification has not been sent (but
> > > the MIGRATE_STATE operation itself succeeded).
> > 
> > It is not critical, however, the best choice is let the userspace decide.
> > How about this 
> > 
> > if (err < 0) {
> >                 NL_SET_ERR_MSG(extack, "Failed to send migration notification");
> >                 err = 0
> > } 
> > 
> > most likely cause is out of memory.
> 
> Does userspace really check the extack it gets back when the operation
> succeeds? But ok, that seems fine to me. 

From recollection, at least one of the *swan log it, and over time
you start to notice the pattern. That said, out-of-memory is a tough case.
When that happens, all bets are off anyway. So it really comes down to
personal preference. I prefer to set something to notify.

My frustration when testing, typically on a low-memory VM, was watching 'ip 
xfrm monitor' and not seeing a netlink notification, left wondering what had 
happened.


> 
> [Looking at the existing callers of xfrm_nlmsg_multicast, many
> existing calls seem to completely ignore the return value
> (km_state_notify -> xfrm_send_state_notify, km_policy_notify ->
> xfrm_send_policy_notify, which are called from the main NETLINK_XFRM
> ops), so at least returning 0 would be consistent with those (but
> there's no extack on failing to notify for the other ops)]

You picked up an interesting design choice I made. Since PF_KEY/AF_KEY
is on life support I omitted going through km_state_notify.  So I would
like to have extack when it is possible.

-antony

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

On Thu, Feb 26, 2026 at 05:44:51PM -0800, Yan Yan via Devel wrote:
> Hi Antony,
> 
> May I request that we also support updating the XFRMA_SET_MARK as part
> of the new XFRM_MSG_MIGRATE_STATE message?

yes I can add that. I would add XFRMA_SET_MARK/XFRMA_SET_MARK_MASK together.  
If you set only the SET_MARK mask will be 0xffffffff.

I am actually using xfrm_smark_init() which will accept both. 

> In Android, the primary use case for migration is switching the
> underlying physical network for an IPsec tunnel (e.g. VPN, Wifi
> calling). Currently, due to the limits of XFRM_MSG_MIGRATE, we are
> forced to use a separate UPDSA call to update the set-mark. Supporting
> XFRMA_SET_MARK within the migrate message would allow us to update the
> addresses and the routing mark together in one atomic call.
>
> Regarding the logic, I believe the set-mark can follow the same
> omit-to-clear pattern as XFRMA_ENCAP and XFRMA_OFFLOAD_DEV.
>
>
> What do you think?

good idea. I would try to test, otherwise please review/test this sepcific 
part xfrm_smark_init() set 0/0 when there is no SET_MASK.

Thanks.
-antony

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
> > Anything that we leave as implicit copy will have to be "forever"
> > implicitly copied with this new MIGRATE_STATE op -- unless we find a
> > way to pass a new "clear these properties" flag (probably via a list
> > of XFRMA_* attribute names)

that is a limitation we should avoid. It would be nice to extend it
over time. We have been there before and it is a pain point. So it is
worth investigating alternatives if there is momentum here, otherwise
I would keep it simple:)

> That is true. I also have the concern that if all updatable attributes
> follow the "omit-to-clear" pattern, we lose the extensibility. Thus
> ideally we should switch to an "omit-to-inherit" model for some, if
> not all, attributes to ensure that adding new SA properties doesn't
> break backward compatibility.

Here is my proposal. I extended the code and am testing it now; I hope
to send out v6 soon.

How would omit-to-inherit look in practice? Specify almost all XFRMA
attributes supported in XFRM_MSG_NEWSA, minus some immutable ones.
The immutable attributes that come to mind are:

    - XFRMA_ALG_*      : crypto must not change during the life of an SA;
                         also *swan userspace does not keep this in memory
                         after the SA is installed, which is correct
                         behaviour.
    - XFRMA_SA_DIR     : direction is fixed at SA creation.
    - XFRMA_SEC_CTX    : security context is immutable.

currently supported attributes, using omit-to-inherit semantics:

    sentinel value to clear, omit to inherit:
    - XFRMA_ENCAP              : encap_type=0 to clear
    - XFRMA_OFFLOAD_DEV        : ifindex=0 to clear

omit to inherit; send attr with value 0/0 to clear:
    - XFRMA_SET_MARK / XFRMA_SET_MARK_MASK
    - XFRMA_MARK               : omit-to-inherit old_mark; note XFRMA_MARK
                                 serves dual purpose -- old_mark in the
                                 fixed header is the SA lookup key, and
                                 XFRMA_MARK attribute sets the new mark.

set to zero to move from NAT to no-NAT; inherit when absent:
    - XFRMA_NAT_KEEPALIVE_INTERVAL
    - XFRMA_MTIMER_THRESH

omit to inherit; send 0 to clear:
    - XFRMA_TFCPAD             : TFC padding
    - XFRMA_SA_EXTRA_FLAGS     : e.g. DONT_ENCAP_DSCP, OSEQ_MAY_WRAP;
                                 yes, these can change.
    - XFRMA_IF_ID              : xfrm interface ID; relevant when the SA
                                 moves to a different xfrm interface.
    - XFRMA_COADDR             : Care-of Address (Mobile IPv6).

    - XFRMA_REPLAY_ESN_VAL / XFRMA_REPLAY_VAL : may be later replay type 
      should not change.


Basic migration supported via fixed header fields (new_* fields):
    - src and dst address family
    - src and/or dst address
    - reqid

I also added old_mark to the SA lookup alongside the SPI, so the SA
can be uniquely identified when marks are in use. XFRMA_MARK can then
be used to set the new mark value independently.

regards,
-antony

> 
> On Fri, Feb 27, 2026 at 3:26 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> >
> > 2026-02-26, 17:44:51 -0800, Yan Yan via Devel wrote:
> > > Hi Antony,
> > >
> > > May I request that we also support updating the XFRMA_SET_MARK as part
> > > of the new XFRM_MSG_MIGRATE_STATE message?
> > >
> > > In Android, the primary use case for migration is switching the
> > > underlying physical network for an IPsec tunnel (e.g. VPN, Wifi
> > > calling). Currently, due to the limits of XFRM_MSG_MIGRATE, we are
> > > forced to use a separate UPDSA call to update the set-mark. Supporting
> > > XFRMA_SET_MARK within the migrate message would allow us to update the
> > > addresses and the routing mark together in one atomic call.
> > >
> > > Regarding the logic, I believe the set-mark can follow the same
> > > omit-to-clear pattern as XFRMA_ENCAP and XFRMA_OFFLOAD_DEV.
> >
> > I think this raises a wider question: clearly definining and
> > documenting which attributes need to be explicitly provided
> > ("omit-to-clear" as you write), and which will be implicitly copied.
> >
> > Currently it looks like we copy:
> >  - all the crypto stuff (aalg/aead/etc)
> >  - security context stuff
> >  - coaddr
> >  - replay/replay_esn
> >  - pcpu_num, if_id, tfcpad
> >  - dir
> >  - mark
> >  - extra_flags
> >
> > but not
> >  - nat_keepalive_interval
> >  - offload
> >  - encap
> >
> > [gathered from a quick read of xfrm_state_clone_and_setup + the
> > definition of xfrma_policy]
> >
> > Anything that we leave as implicit copy will have to be "forever"
> > implicitly copied with this new MIGRATE_STATE op -- unless we find a
> > way to pass a new "clear these properties" flag (probably via a list
> > of XFRMA_* attribute names), but then we could also implement that
> > with the existing MIGRATE code.
> >
> > --
> > Sabrina
> 
> 
> 
> --
> --
> Best,
> Yan
> -- 
> Devel mailing list -- devel@lists.linux-ipsec.org
> To unsubscribe send an email to devel-leave@lists.linux-ipsec.org

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Sabrina Dubroca]

2026-03-08, 15:42:58 +0100, Antony Antony wrote:
> On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
> > > Anything that we leave as implicit copy will have to be "forever"
> > > implicitly copied with this new MIGRATE_STATE op -- unless we find a
> > > way to pass a new "clear these properties" flag (probably via a list
> > > of XFRMA_* attribute names)
> 
> that is a limitation we should avoid. It would be nice to extend it
> over time. We have been there before and it is a pain point. So it is
> worth investigating alternatives if there is momentum here, otherwise
> I would keep it simple:)

Well, because it's a known pain point, we shouldn't just jump into an
implementation.

(FWIW, as a kernel-only developer, ie not involved at all in the *swan
code, I don't have much of an opinion on which property should behave
which way, just that we know what we're getting into)

> > That is true. I also have the concern that if all updatable attributes
> > follow the "omit-to-clear" pattern, we lose the extensibility. Thus
> > ideally we should switch to an "omit-to-inherit" model for some, if
> > not all, attributes to ensure that adding new SA properties doesn't
> > break backward compatibility.

Implicit copy ("omit-to-inherit") gets us in the situation we have
with the old MIGRATE code, we don't have a way to _not inherit_
properties. Well, apparently we do, based on what Antony wrote below.



> Here is my proposal. I extended the code and am testing it now; I hope
> to send out v6 soon.

I think it would have been nice to postpone v6 a little bit so that
others had time to answer here (and avoid a respin if there's some
disagreement on what the behavior should be).

> How would omit-to-inherit look in practice? Specify almost all XFRMA
> attributes supported in XFRM_MSG_NEWSA, minus some immutable ones.
> The immutable attributes that come to mind are:
> 
>     - XFRMA_ALG_*      : crypto must not change during the life of an SA;
>                          also *swan userspace does not keep this in memory
>                          after the SA is installed, which is correct
>                          behaviour.
>     - XFRMA_SA_DIR     : direction is fixed at SA creation.
>     - XFRMA_SEC_CTX    : security context is immutable.

So we should be rejecting an attempt to pass those attributes in a
MIGRATE_STATE request.

> currently supported attributes, using omit-to-inherit semantics:
> 
>     sentinel value to clear, omit to inherit:
>     - XFRMA_ENCAP              : encap_type=0 to clear

That would be a new special case value for that attribute, ok.

>     - XFRMA_OFFLOAD_DEV        : ifindex=0 to clear

OFFLOAD_DEV with xuo.ifindex=0 is a valid attribute to request offload
and let the kernel figure out which device to use. We can't use that
to clear/disable offload of an SA.

For the rest, it seems that passing 0 is equivalent to omitting the
attribute in the current code. Except XFRMA_SA_PCPU, where we consider
UINT_MAX as "disable" (default value for x->pcpu_num), but we reject
userspace passing us that value, and XFRMA_SA_PCPU containing 0 is a
valid value.

-- 
Sabrina

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

On Tue, Mar 10, 2026 at 12:09:27PM +0100, Sabrina Dubroca wrote:
> 2026-03-08, 15:42:58 +0100, Antony Antony wrote:
> > On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
> > > > Anything that we leave as implicit copy will have to be "forever"
> > > > implicitly copied with this new MIGRATE_STATE op -- unless we find a
> > > > way to pass a new "clear these properties" flag (probably via a list
> > > > of XFRMA_* attribute names)
> > 
> > that is a limitation we should avoid. It would be nice to extend it
> > over time. We have been there before and it is a pain point. So it is
> > worth investigating alternatives if there is momentum here, otherwise
> > I would keep it simple:)
> 
> Well, because it's a known pain point, we shouldn't just jump into an
> implementation.
> 
> (FWIW, as a kernel-only developer, ie not involved at all in the *swan
> code, I don't have much of an opinion on which property should behave
> which way, just that we know what we're getting into)

Understandable. Partly *swan world, Android, IETF RFC guidelines, NIST et 
al. The *swan world usually wakes late after the time the kernel
reaches distributions.This is too late to fix kernel without breaking ABI.
standards world has its own path. Kernel often implements that what
makes practical sense in the moment and hard depreciate/remove ABI:)

I feel these days there are more interactions such as this, which is a good 
sign!

> > > That is true. I also have the concern that if all updatable attributes
> > > follow the "omit-to-clear" pattern, we lose the extensibility. Thus
> > > ideally we should switch to an "omit-to-inherit" model for some, if
> > > not all, attributes to ensure that adding new SA properties doesn't
> > > break backward compatibility.
> 
> Implicit copy ("omit-to-inherit") gets us in the situation we have
> with the old MIGRATE code, we don't have a way to _not inherit_
> properties. Well, apparently we do, based on what Antony wrote below.
> 
> 
> 
> > Here is my proposal. I extended the code and am testing it now; I hope
> > to send out v6 soon.
> 
> I think it would have been nice to postpone v6 a little bit so that
> others had time to answer here (and avoid a respin if there's some
> disagreement on what the behavior should be).
> 
> > How would omit-to-inherit look in practice? Specify almost all XFRMA
> > attributes supported in XFRM_MSG_NEWSA, minus some immutable ones.
> > The immutable attributes that come to mind are:
> > 
> >     - XFRMA_ALG_*      : crypto must not change during the life of an SA;
> >                          also *swan userspace does not keep this in memory
> >                          after the SA is installed, which is correct
> >                          behaviour.
> >     - XFRMA_SA_DIR     : direction is fixed at SA creation.
> >     - XFRMA_SEC_CTX    : security context is immutable.
> 
> So we should be rejecting an attempt to pass those attributes in a
> MIGRATE_STATE request.

yes. I added it o v6.It reject all unused attributes.  
NL_SET_ERR_MSG_ATTR(extack, attrs[i], "Unsupported attribute in 
XFRM_MSG_MIGRATE_STATE");

> > currently supported attributes, using omit-to-inherit semantics:
> > 
> >     sentinel value to clear, omit to inherit:
> >     - XFRMA_ENCAP              : encap_type=0 to clear
> 
> That would be a new special case value for that attribute, ok.
> 
> >     - XFRMA_OFFLOAD_DEV        : ifindex=0 to clear
> 
> OFFLOAD_DEV with xuo.ifindex=0 is a valid attribute to request offload
> and let the kernel figure out which device to use. We can't use that
> to clear/disable offload of an SA.

Good catch, thanks.

Two options to fix this:

Option 1: add XFRM_OFFLOAD_CLEAR to xfrm_user_offload flags in uapi xfrm.h:

#define XFRM_OFFLOAD_CLEAR  (1 << 7)
When set in XFRMA_OFFLOAD_DEV, it means remove offload rather than configure it.

Option 2: add a __u32 flags field to xfrm_user_migrate_state in uapi xfrm.h.
There is a __u16 reserved currently used for alignment, but 16 bits feels
too small if we want to cover clearing other attributes in the future.
A __u32 at the end of the struct avoids that constraint.

I am leaning toward option 2. Any preference? 


> For the rest, it seems that passing 0 is equivalent to omitting the
> attribute in the current code. Except XFRMA_SA_PCPU, where we consider
> UINT_MAX as "disable" (default value for x->pcpu_num), but we reject
> userspace passing us that value, and XFRMA_SA_PCPU containing 0 is a
> valid value.

yes XFRMA_SA_PCPU that would be UINT_MAX.

-antony

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Yan Yan]

> yes I can add that. I would add XFRMA_SET_MARK/XFRMA_SET_MARK_MASK together.
> If you set only the SET_MARK mask will be 0xffffffff.

> I am actually using xfrm_smark_init() which will accept both.

Great! Thanks for supporting that.

> Option 1: add XFRM_OFFLOAD_CLEAR to xfrm_user_offload flags in uapi xfrm.h:
>
> #define XFRM_OFFLOAD_CLEAR  (1 << 7)
> When set in XFRMA_OFFLOAD_DEV, it means remove offload rather than configure it.
>
> Option 2: add a __u32 flags field to xfrm_user_migrate_state in uapi xfrm.h.
> There is a __u16 reserved currently used for alignment, but 16 bits feels
> too small if we want to cover clearing other attributes in the future.
> A __u32 at the end of the struct avoids that constraint.
>
> I am leaning toward option 2. Any preference?

I'm also in favor of option 2 for better extensibility.

>     - XFRMA_REPLAY_ESN_VAL / XFRMA_REPLAY_VAL : may be later replay type
>       should not change.

I agree we should keep the replay type immutable. Changing ESN flag on
the fly would make it hard to keep both sides synced, and I'm not
aware of any use case for this.

-- 
--
Best,
Yan

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Antony Antony]

On Fri, Mar 13, 2026 at 05:32:15PM -0700, Yan Yan via Devel wrote:
> > yes I can add that. I would add XFRMA_SET_MARK/XFRMA_SET_MARK_MASK together.
> > If you set only the SET_MARK mask will be 0xffffffff.
> 
> > I am actually using xfrm_smark_init() which will accept both.
> 
> Great! Thanks for supporting that.
> 
> > Option 1: add XFRM_OFFLOAD_CLEAR to xfrm_user_offload flags in uapi xfrm.h:
> >
> > #define XFRM_OFFLOAD_CLEAR  (1 << 7)
> > When set in XFRMA_OFFLOAD_DEV, it means remove offload rather than configure it.
> >
> > Option 2: add a __u32 flags field to xfrm_user_migrate_state in uapi xfrm.h.
> > There is a __u16 reserved currently used for alignment, but 16 bits feels
> > too small if we want to cover clearing other attributes in the future.
> > A __u32 at the end of the struct avoids that constraint.
> >
> > I am leaning toward option 2. Any preference?
> 
> I'm also in favor of option 2 for better extensibility.
> 
> >     - XFRMA_REPLAY_ESN_VAL / XFRMA_REPLAY_VAL : may be later replay type
> >       should not change.
> 
> I agree we should keep the replay type immutable. Changing ESN flag on
> the fly would make it hard to keep both sides synced, and I'm not
> aware of any use case for this.

While testing XFRM_MSG_MIGRATE_STATE we ran into an issue with x->sel
migration in transport mode. In transport mode the selector is typically
a single-host entry matching the SA's saddr and daddr, so after
migration it only needs to be updated with the new addresses.

For this common case I added XFRM_MIGRATE_STATE_UPDATE_SEL to
xfrm_user_migrate_state.flags. When set, the kernel validates that the
existing selector is a single-host match for the SA addresses and
derives the new selector from new_daddr/new_saddr with the appropriate
mask for the new family.

I think this is the main use case. However, for corner cases out there,
the selector is not a simple single-host entry,
struct xfrm_user_migrate_state now carries a new_sel field. When
XFRM_MIGRATE_STATE_UPDATE_SEL is not set, new_sel is used as-is for
the migrated SA.

-antony