[PATCH ipsec-next v5 0/8] xfrm: XFRM_MSG_MIGRATE_STATE new netlink message

[PATCH ipsec-next v5 0/8] xfrm: XFRM_MSG_MIGRATE_STATE new netlink message

[Antony Antony]

The current XFRM_MSG_MIGRATE interface is tightly coupled to policy and
SA migration, and it lacks the information required to reliably migrate
individual SAs. This makes it unsuitable for IKEv2 deployments,
dual-stack setups (IPv4/IPv6), and scenarios where policies are managed
externally (e.g., by daemons other than the IKE daemon).

Mandatory SA selector list
The current API requires a non-empty SA selector list, which does not
reflect the IKEv2 use case.
A single Child SA may correspond to multiple policies,
and SA discovery already occurs via address and reqid matching. With
dual-stack Child SAs this leads to excessive churn: the current method
would have to be called up to six times (in/out/fwd × v4/v6) on SA,
while the new method only requires two calls.

Selectors lack SPI (and marks)
XFRM_MSG_MIGRATE cannot uniquely identify an SA when multiple SAs share
the same policies (per-CPU SAs, SELinux label-based SAs, etc.). Without
the SPI, the kernel may update the wrong SA instance.

Reqid cannot be changed
Some implementations allocate reqids based on traffic selectors. In
host-to-host or selector-changing scenarios, the reqid must change,
which the current API cannot express.

Because strongSwan and other implementations manage policies
independently of the kernel, an interface that updates only a specific
SA - with complete and unambiguous identification - is required.

XFRM_MSG_MIGRATE_STATE provides that interface. It supports migration
of a single SA via xfrm_usersa_id (including SPI) and we fix
encap removal in this patch set, reqid updates, address changes,
and other SA-specific parameters. It avoids the structural limitations
of XFRM_MSG_MIGRATE and provides a simpler, extensible mechanism for
precise per-SA migration without involving policies.

New migration steps: first install block policy, remove the old policy,
call XFRM_MSG_MIGRATE_STATE for each state, then re-install the
policies and remove the block policy.

If the target SA tuple (daddr, SPI, proto, family) is already
occupied, the operation returns -EEXIST. In this case the original
SA is not preserved. Userspace must handle -EEXIST by
re-establishing the SA at the IKE level and manage policies.

Antony Antony (8):
  xfrm: add missing __rcu annotation to nlsk
  xfrm: remove redundant assignments
  xfrm: allow migration from UDP encapsulated to non-encapsulated ESP
  xfrm: rename reqid in xfrm_migrate
  xfrm: split xfrm_state_migrate into create and install functions
  xfrm: add state synchronization after migration
  xfrm: add error messages to state migration
  xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

 include/net/netns/xfrm.h    |   2 +-
 include/net/xfrm.h          |  60 ++++++++++--
 include/uapi/linux/xfrm.h   |  11 +++
 net/key/af_key.c            |  10 +-
 net/xfrm/xfrm_policy.c      |   5 +-
 net/xfrm/xfrm_state.c       | 119 +++++++++++++++---------
 net/xfrm/xfrm_user.c        | 180 +++++++++++++++++++++++++++++++++++-
 security/selinux/nlmsgtab.c |   3 +-
 8 files changed, 324 insertions(+), 66 deletions(-)
---
v1->v2: dropped 6/6. That check is already there where the func is called
    - merged patch 4/6 and 5/6, to fix use uninitialized value
    - fix commit messages

v2->v3: - fix commit message formatting

v3->v4: add patch to fix pre-existing missing __rcu annotation on nlsk
   (exposed by sparse when modifying xfrm_user.c)
v4->v5: add synchronize after migrate and delete it inside a lock
    - split xfrm_state_migrate into create and install functions

-antony

[PATCH ipsec-next v5 1/8] xfrm: add missing __rcu annotation to nlsk

[Antony Antony]

The nlsk field in struct netns_xfrm is RCU-protected, as seen by
the use of rcu_assign_pointer() and RCU_INIT_POINTER() when updating
it.
However, the field lacks the __rcu annotation, and most read-side
accesses don't use rcu_dereference().

Add the missing __rcu annotation and convert all read-side accesses to
use rcu_dereference() for correctness and to silence sparse warnings.

Sparse warning reported by NIPA allmodconfig test when modifying
net/xfrm/xfrm_user.c. The warning is a pre-existing issue in
xfrm_nlmsg_multicast(). This series added a new call to this function
and NIPA testing reported a new warning was added by this series.

To reproduce (requires sparse):
make C=2 net/xfrm/
net/xfrm/xfrm_user.c:1574:29: error: incompatible types in comparison
expression (different address spaces):
net/xfrm/xfrm_user.c:1574:29:    struct sock [noderef] __rcu *
net/xfrm/xfrm_user.c:1574:29:    struct sock *

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v3->v4: added this patch
---
 include/net/netns/xfrm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 23dd647fe024..ed8eee81bbb0 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -59,7 +59,7 @@ struct netns_xfrm {
 	struct list_head	inexact_bins;


-	struct sock		*nlsk;
+	struct sock __rcu	*nlsk;
 	struct sock		*nlsk_stash;

 	u32			sysctl_aevent_etime;
--
2.39.5

[PATCH ipsec-next v5 2/8] xfrm: remove redundant assignments

[Antony Antony]

These assignments are overwritten within the same function further down

commit e8961c50ee9cc ("xfrm: Refactor migration setup
during the cloning process")
x->props.family = m->new_family;

Which actually moved it in the
commit e03c3bba351f9 ("xfrm: Fix xfrm migrate issues
when address family changes")

And the initial
commit 80c9abaabf428 ("[XFRM]: Extension for dynamic
update of endpoint address(es)")

added x->props.saddr = orig->props.saddr; and
memcpy(&xc->props.saddr, &m->new_saddr, sizeof(xc->props.saddr));

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v1->v2: remove extra saddr copy, previous line
---
 net/xfrm/xfrm_state.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 98b362d51836..3ee92f93dbd2 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1980,8 +1980,6 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 	x->props.mode = orig->props.mode;
 	x->props.replay_window = orig->props.replay_window;
 	x->props.reqid = orig->props.reqid;
-	x->props.family = orig->props.family;
-	x->props.saddr = orig->props.saddr;

 	if (orig->aalg) {
 		x->aalg = xfrm_algo_auth_clone(orig->aalg);
--
2.39.5

[PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Antony Antony]

The current code prevents migrating an SA from UDP encapsulation to
plain ESP. This is needed when moving from a NATed path to a non-NATed
one, for example when switching from IPv4+NAT to IPv6.

Only copy the existing encapsulation during migration if the encap
attribute is explicitly provided.

Note: PF_KEY's SADB_X_MIGRATE always passes encap=NULL and never
supported encapsulation in migration. PF_KEY is deprecated and was
in feature freeze when UDP encapsulation was added to xfrm.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/xfrm/xfrm_state.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 3ee92f93dbd2..9ddd033f5d79 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2008,14 +2008,8 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 	}
 	x->props.calgo = orig->props.calgo;
 
-	if (encap || orig->encap) {
-		if (encap)
-			x->encap = kmemdup(encap, sizeof(*x->encap),
-					GFP_KERNEL);
-		else
-			x->encap = kmemdup(orig->encap, sizeof(*x->encap),
-					GFP_KERNEL);
-
+	if (encap) {
+		x->encap = kmemdup(encap, sizeof(*x->encap), GFP_KERNEL);
 		if (!x->encap)
 			goto error;
 	}
-- 
2.39.5

[PATCH ipsec-next v5 4/8] xfrm: rename reqid in xfrm_migrate

[Antony Antony]

In preparation for a later patch in this series s/reqid/old_reqid/.
No functional change.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 include/net/xfrm.h     |  2 +-
 net/key/af_key.c       | 10 +++++-----
 net/xfrm/xfrm_policy.c |  4 ++--
 net/xfrm/xfrm_state.c  |  6 +++---
 net/xfrm/xfrm_user.c   |  4 ++--
 5 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 0a14daaa5dd4..05fa0552523d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -685,7 +685,7 @@ struct xfrm_migrate {
 	u8			proto;
 	u8			mode;
 	u16			reserved;
-	u32			reqid;
+	u32			old_reqid;
 	u16			old_family;
 	u16			new_family;
 };
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 571200433aa9..a856bdd0c0d7 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2538,7 +2538,7 @@ static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
 	if ((mode = pfkey_mode_to_xfrm(rq1->sadb_x_ipsecrequest_mode)) < 0)
 		return -EINVAL;
 	m->mode = mode;
-	m->reqid = rq1->sadb_x_ipsecrequest_reqid;
+	m->old_reqid = rq1->sadb_x_ipsecrequest_reqid;
 
 	return ((int)(rq1->sadb_x_ipsecrequest_len +
 		      rq2->sadb_x_ipsecrequest_len));
@@ -3634,15 +3634,15 @@ static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 		if (mode < 0)
 			goto err;
 		if (set_ipsecrequest(skb, mp->proto, mode,
-				     (mp->reqid ?  IPSEC_LEVEL_UNIQUE : IPSEC_LEVEL_REQUIRE),
-				     mp->reqid, mp->old_family,
+				     (mp->old_reqid ? IPSEC_LEVEL_UNIQUE : IPSEC_LEVEL_REQUIRE),
+				     mp->old_reqid, mp->old_family,
 				     &mp->old_saddr, &mp->old_daddr) < 0)
 			goto err;
 
 		/* new ipsecrequest */
 		if (set_ipsecrequest(skb, mp->proto, mode,
-				     (mp->reqid ? IPSEC_LEVEL_UNIQUE : IPSEC_LEVEL_REQUIRE),
-				     mp->reqid, mp->new_family,
+				     (mp->old_reqid ? IPSEC_LEVEL_UNIQUE : IPSEC_LEVEL_REQUIRE),
+				     mp->old_reqid, mp->new_family,
 				     &mp->new_saddr, &mp->new_daddr) < 0)
 			goto err;
 	}
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 62486f866975..854dfc16ed55 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4530,7 +4530,7 @@ static int migrate_tmpl_match(const struct xfrm_migrate *m, const struct xfrm_tm
 	int match = 0;
 
 	if (t->mode == m->mode && t->id.proto == m->proto &&
-	    (m->reqid == 0 || t->reqid == m->reqid)) {
+	    (m->old_reqid == 0 || t->reqid == m->old_reqid)) {
 		switch (t->mode) {
 		case XFRM_MODE_TUNNEL:
 		case XFRM_MODE_BEET:
@@ -4624,7 +4624,7 @@ static int xfrm_migrate_check(const struct xfrm_migrate *m, int num_migrate,
 				    sizeof(m[i].old_saddr)) &&
 			    m[i].proto == m[j].proto &&
 			    m[i].mode == m[j].mode &&
-			    m[i].reqid == m[j].reqid &&
+			    m[i].old_reqid == m[j].old_reqid &&
 			    m[i].old_family == m[j].old_family) {
 				NL_SET_ERR_MSG(extack, "Entries in the MIGRATE attribute's list must be unique");
 				return -EINVAL;
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 9ddd033f5d79..bbdb0dba16b9 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2080,14 +2080,14 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 
 	spin_lock_bh(&net->xfrm.xfrm_state_lock);
 
-	if (m->reqid) {
+	if (m->old_reqid) {
 		h = xfrm_dst_hash(net, &m->old_daddr, &m->old_saddr,
-				  m->reqid, m->old_family);
+				  m->old_reqid, m->old_family);
 		hlist_for_each_entry(x, net->xfrm.state_bydst+h, bydst) {
 			if (x->props.mode != m->mode ||
 			    x->id.proto != m->proto)
 				continue;
-			if (m->reqid && x->props.reqid != m->reqid)
+			if (m->old_reqid && x->props.reqid != m->old_reqid)
 				continue;
 			if (if_id != 0 && x->if_id != if_id)
 				continue;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 403b5ecac2c5..26b82d94acc1 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3087,7 +3087,7 @@ static int copy_from_user_migrate(struct xfrm_migrate *ma,
 
 		ma->proto = um->proto;
 		ma->mode = um->mode;
-		ma->reqid = um->reqid;
+		ma->old_reqid = um->reqid;
 
 		ma->old_family = um->old_family;
 		ma->new_family = um->new_family;
@@ -3170,7 +3170,7 @@ static int copy_to_user_migrate(const struct xfrm_migrate *m, struct sk_buff *sk
 	memset(&um, 0, sizeof(um));
 	um.proto = m->proto;
 	um.mode = m->mode;
-	um.reqid = m->reqid;
+	um.reqid = m->old_reqid;
 	um.old_family = m->old_family;
 	memcpy(&um.old_daddr, &m->old_daddr, sizeof(um.old_daddr));
 	memcpy(&um.old_saddr, &m->old_saddr, sizeof(um.old_saddr));
-- 
2.39.5

[PATCH ipsec-next v5 5/8] xfrm: split xfrm_state_migrate into create and install functions

[Antony Antony]

To prepare for subsequent patches, split
xfrm_state_migrate() into two functions:
- xfrm_state_migrate_create(): creates the migrated state
- xfrm_state_migrate_install(): installs it into the state table

splitting will help to avoid SN/IV reuse when migrating AEAD SA.

And add const whenever possible.
No functional change.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v4->v5: - added this patch
---
 include/net/xfrm.h    | 11 +++++++
 net/xfrm/xfrm_state.c | 73 +++++++++++++++++++++++++++++++------------
 2 files changed, 64 insertions(+), 20 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 05fa0552523d..a920d64d8cfa 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1895,6 +1895,17 @@ int km_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
 	       const struct xfrm_encap_tmpl *encap);
 struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *net,
 						u32 if_id);
+struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
+					     const struct xfrm_migrate *m,
+					     const struct xfrm_encap_tmpl *encap,
+					     struct net *net,
+					     struct xfrm_user_offload *xuo,
+					     struct netlink_ext_ack *extack);
+int xfrm_state_migrate_install(const struct xfrm_state *x,
+			       struct xfrm_state *xc,
+			       const struct xfrm_migrate *m,
+			       struct xfrm_user_offload *xuo,
+			       struct netlink_ext_ack *extack);
 struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 				      struct xfrm_migrate *m,
 				      struct xfrm_encap_tmpl *encap,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index bbdb0dba16b9..35c82926665a 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1966,8 +1966,8 @@ static inline int clone_security(struct xfrm_state *x, struct xfrm_sec_ctx *secu
 }

 static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
-					   struct xfrm_encap_tmpl *encap,
-					   struct xfrm_migrate *m)
+					   const struct xfrm_encap_tmpl *encap,
+					   const struct xfrm_migrate *m)
 {
 	struct net *net = xs_net(orig);
 	struct xfrm_state *x = xfrm_state_alloc(net);
@@ -2124,12 +2124,12 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
 }
 EXPORT_SYMBOL(xfrm_migrate_state_find);

-struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
-				      struct xfrm_migrate *m,
-				      struct xfrm_encap_tmpl *encap,
-				      struct net *net,
-				      struct xfrm_user_offload *xuo,
-				      struct netlink_ext_ack *extack)
+struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
+					     const struct xfrm_migrate *m,
+					     const struct xfrm_encap_tmpl *encap,
+					     struct net *net,
+					     struct xfrm_user_offload *xuo,
+					     struct netlink_ext_ack *extack)
 {
 	struct xfrm_state *xc;

@@ -2144,24 +2144,57 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
 		goto error;

-	/* add state */
+	return xc;
+error:
+	xc->km.state = XFRM_STATE_DEAD;
+	xfrm_state_put(xc);
+	return NULL;
+}
+EXPORT_SYMBOL(xfrm_state_migrate_create);
+
+int xfrm_state_migrate_install(const struct xfrm_state *x,
+			       struct xfrm_state *xc,
+			       const struct xfrm_migrate *m,
+			       struct xfrm_user_offload *xuo,
+			       struct netlink_ext_ack *extack)
+{
 	if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
-		/* a care is needed when the destination address of the
-		   state is to be updated as it is a part of triplet */
+		/*
+		 * Care is needed when the destination address
+		 * of the state is to be updated as it is a part of triplet.
+		 */
 		xfrm_state_insert(xc);
 	} else {
-		if (xfrm_state_add(xc) < 0)
-			goto error_add;
+		if (xfrm_state_add(xc) < 0) {
+			if (xuo)
+				xfrm_dev_state_delete(xc);
+			xc->km.state = XFRM_STATE_DEAD;
+			xfrm_state_put(xc);
+			return -EEXIST;
+		}
 	}

+	return 0;
+}
+EXPORT_SYMBOL(xfrm_state_migrate_install);
+
+struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
+				      struct xfrm_migrate *m,
+				      struct xfrm_encap_tmpl *encap,
+				      struct net *net,
+				      struct xfrm_user_offload *xuo,
+				      struct netlink_ext_ack *extack)
+{
+	struct xfrm_state *xc;
+
+	xc = xfrm_state_migrate_create(x, m, encap, net, xuo, extack);
+	if (!xc)
+		return NULL;
+
+	if (xfrm_state_migrate_install(x, xc, m, xuo, extack) < 0)
+		return NULL;
+
 	return xc;
-error_add:
-	if (xuo)
-		xfrm_dev_state_delete(xc);
-error:
-	xc->km.state = XFRM_STATE_DEAD;
-	xfrm_state_put(xc);
-	return NULL;
 }
 EXPORT_SYMBOL(xfrm_state_migrate);
 #endif
--
2.39.5

[PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration

[Antony Antony]

Add descriptive(extack) error messages for all error paths
in state migration. This improves diagnostics by
providing clear feedback when migration fails.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v4->v5: - added this patch
---
 net/xfrm/xfrm_state.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 88a362e46972..2e03871ae872 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
 	struct xfrm_state *xc;

 	xc = xfrm_state_clone_and_setup(x, encap, m);
-	if (!xc)
+	if (!xc) {
+		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");
 		return NULL;
+	}

-	if (xfrm_init_state(xc) < 0)
+	if (xfrm_init_state(xc) < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");
 		goto error;
+	}

 	/* configure the hardware if offload is requested */
-	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
+	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
+		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");
 		goto error;
+	}

 	return xc;
 error:
@@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
 		xfrm_state_insert(xc);
 	} else {
 		if (xfrm_state_add(xc) < 0) {
+			NL_SET_ERR_MSG(extack, "Failed to add migrated state");
 			if (xuo)
 				xfrm_dev_state_delete(xc);
 			xc->km.state = XFRM_STATE_DEAD;
--
2.39.5

[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

[PATCH ipsec-next v5 6/8] xfrm: add state synchronization after migration

[Antony Antony]

Add xfrm_migrate_sync() to synchronize curlft and replay state after
state installation, this can be called under lock without memory
allocation.  In preparation for a subsequent patch in this series.

This ensures the migrated state captures the latest lifetime counters
and replay state from the original after installation completes.
Within the same lock, the original xfrm state is deleted.

No functional change.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
v4->v5: - added this patch
---
 include/net/xfrm.h    | 46 ++++++++++++++++++++++++++++++++++---------
 net/xfrm/xfrm_state.c | 11 ++++-------
 2 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index a920d64d8cfa..6064ea0a6f2b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -2024,23 +2024,51 @@ static inline unsigned int xfrm_replay_state_esn_len(struct xfrm_replay_state_es

 #ifdef CONFIG_XFRM_MIGRATE
 static inline int xfrm_replay_clone(struct xfrm_state *x,
-				     struct xfrm_state *orig)
+				    const struct xfrm_state *orig)
 {
+	/* Counters synced later in xfrm_replay_sync() */

-	x->replay_esn = kmemdup(orig->replay_esn,
+	x->replay = orig->replay;
+	x->preplay = orig->preplay;
+
+	if (orig->replay_esn) {
+		x->replay_esn = kmemdup(orig->replay_esn,
 				xfrm_replay_state_esn_len(orig->replay_esn),
 				GFP_KERNEL);
-	if (!x->replay_esn)
-		return -ENOMEM;
-	x->preplay_esn = kmemdup(orig->preplay_esn,
-				 xfrm_replay_state_esn_len(orig->preplay_esn),
-				 GFP_KERNEL);
-	if (!x->preplay_esn)
-		return -ENOMEM;
+		if (!x->replay_esn)
+			return -ENOMEM;
+		x->preplay_esn = kmemdup(orig->preplay_esn,
+				xfrm_replay_state_esn_len(orig->preplay_esn),
+				GFP_KERNEL);
+		if (!x->preplay_esn)
+			return -ENOMEM;
+	}

 	return 0;
 }

+static inline void xfrm_replay_sync(struct xfrm_state *x, const struct xfrm_state *orig)
+{
+	x->replay = orig->replay;
+	x->preplay = orig->preplay;
+
+	if (orig->replay_esn) {
+		memcpy(x->replay_esn, orig->replay_esn,
+				xfrm_replay_state_esn_len(orig->replay_esn));
+
+		memcpy(x->preplay_esn, orig->preplay_esn,
+				xfrm_replay_state_esn_len(orig->preplay_esn));
+	}
+}
+
+static inline void xfrm_migrate_sync(struct xfrm_state *x,
+					  const struct xfrm_state *orig)
+{
+	/* called under lock so no race conditions or mallocs allowed */
+	memcpy(&x->curlft, &orig->curlft, sizeof(x->curlft));
+	xfrm_replay_sync(x, orig);
+}
+
 static inline struct xfrm_algo_aead *xfrm_algo_aead_clone(struct xfrm_algo_aead *orig)
 {
 	return kmemdup(orig, aead_len(orig), GFP_KERNEL);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 35c82926665a..88a362e46972 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2025,10 +2025,8 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 			goto error;
 	}

-	if (orig->replay_esn) {
-		if (xfrm_replay_clone(x, orig))
-			goto error;
-	}
+	if (xfrm_replay_clone(x, orig))
+		goto error;

 	memcpy(&x->mark, &orig->mark, sizeof(x->mark));
 	memcpy(&x->props.smark, &orig->props.smark, sizeof(x->props.smark));
@@ -2041,11 +2039,8 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
 	x->tfcpad = orig->tfcpad;
 	x->replay_maxdiff = orig->replay_maxdiff;
 	x->replay_maxage = orig->replay_maxage;
-	memcpy(&x->curlft, &orig->curlft, sizeof(x->curlft));
 	x->km.state = orig->km.state;
 	x->km.seq = orig->km.seq;
-	x->replay = orig->replay;
-	x->preplay = orig->preplay;
 	x->mapping_maxage = orig->mapping_maxage;
 	x->lastused = orig->lastused;
 	x->new_mapping = 0;
@@ -2194,6 +2189,8 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
 	if (xfrm_state_migrate_install(x, xc, m, xuo, extack) < 0)
 		return NULL;

+	xfrm_migrate_sync(xc, x);
+
 	return xc;
 }
 EXPORT_SYMBOL(xfrm_state_migrate);
--
2.39.5

Re: [PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Sabrina Dubroca]

2026-01-27, 11:42:40 +0100, Antony Antony wrote:
> The current code prevents migrating an SA from UDP encapsulation to
> plain ESP. This is needed when moving from a NATed path to a non-NATed
> one, for example when switching from IPv4+NAT to IPv6.
> 
> Only copy the existing encapsulation during migration if the encap
> attribute is explicitly provided.

Are we sure nobody out there relies on this behavior (silently copying
the existing UDP encap without having to explicitly request it in the
MIGRATE request)? If there are, this patch would break their setup by
clearing the encap that they expect to still be present.

-- 
Sabrina

Re: [PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration

[Sabrina Dubroca]

2026-01-27, 11:43:42 +0100, Antony Antony wrote:
> Add descriptive(extack) error messages for all error paths
> in state migration. This improves diagnostics by
> providing clear feedback when migration fails.
> 
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
> v4->v5: - added this patch
> ---
>  net/xfrm/xfrm_state.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 88a362e46972..2e03871ae872 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
>  	struct xfrm_state *xc;
> 
>  	xc = xfrm_state_clone_and_setup(x, encap, m);
> -	if (!xc)
> +	if (!xc) {
> +		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");

When xfrm_state_clone_and_setup fails it's because some allocation
failed and the user won't be able to do much about this, right? I
don't feel extack in those situations is super helpful.

>  		return NULL;
> +	}
> 
> -	if (xfrm_init_state(xc) < 0)
> +	if (xfrm_init_state(xc) < 0) {
> +		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");

xfrm_init_state itself doesn't handle extack, but it's just a wrapper
around functions that do. Maybe better to make xfrm_init_state
propagate extack?

>  		goto error;
> +	}
> 
>  	/* configure the hardware if offload is requested */
> -	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
> +	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
> +		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");

We already set an extack in xfrm_dev_state_add, this chunk should be
dropped to avoid overwriting the more specific info we got.

>  		goto error;
> +	}
> 
>  	return xc;
>  error:
> @@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
>  		xfrm_state_insert(xc);
>  	} else {
>  		if (xfrm_state_add(xc) < 0) {
> +			NL_SET_ERR_MSG(extack, "Failed to add migrated state");

Not a strong objection, but this case would be the EEXIST situation
from xfrm_state_add, and there's not much the user can do about this?

>  			if (xuo)
>  				xfrm_dev_state_delete(xc);
>  			xc->km.state = XFRM_STATE_DEAD;

-- 
Sabrina

Re: [PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Antony Antony]

On Fri, Jan 30, 2026 at 12:28:19 +0100, Sabrina Dubroca wrote:
> 2026-01-27, 11:42:40 +0100, Antony Antony wrote:
> > The current code prevents migrating an SA from UDP encapsulation to
> > plain ESP. This is needed when moving from a NATed path to a non-NATed
> > one, for example when switching from IPv4+NAT to IPv6.
> > 
> > Only copy the existing encapsulation during migration if the encap
> > attribute is explicitly provided.
> 
> Are we sure nobody out there relies on this behavior (silently copying
> the existing UDP encap without having to explicitly request it in the
> MIGRATE request)? If there are, this patch would break their setup by
> clearing the encap that they expect to still be present.

Libreswan and Android are the main users of migrate method. Libreswan sets the
value in every call. I am guessing Android does that too.

Yan, would this patch cause regression in Android?

Without this fix migrating from v4 nat to v6 and no v4 nat won't  work.

Also the ENCAP migrate with UDP port was broken before, 2017,
the commit 4ab47d47af20 ("xfrm: extend MIGRATE with UDP encapsulation port") ?
So likely it was never used by older code and PF_KEY.

For the new methed strongSwan wants to support migrating from UDP encap
to no UDP encap.

regards
-antony

PS : Steffen advised not to Fixes tag.

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Antony Antony]

On Mon, Feb 02, 2026 at 10:15:24AM -0800, Nathan Harold via Devel wrote:
> Unfortunately, I believe Android relies on this behavior (at least for
> now). We never re-send the encap parameters.
> 
> https://cs.android.com/android/platform/superproject/main/+/main:system/netd/server/XfrmController.cpp;l=1183;drc=61197364367c9e404c7da6900658f1b16c42d0da

Thanks Nathan. It is good to know.

The next question is how do you feel about changing the behavior in
Android? Would you be willing re-send ports every time the SA has it?

This will allow more flexible migration. Migrating from NAT to no NAT an
IPv6 without NAT would be possible.

If that is a bad idea, I would limit this change to the new method only.

regards,
-antony

> 
> -Nathan
> 
> 
> On Mon, Feb 2, 2026 at 4:58 AM Antony Antony via Devel <
> devel@lists.linux-ipsec.org> wrote:
> 
> > On Fri, Jan 30, 2026 at 12:28:19 +0100, Sabrina Dubroca wrote:
> > > 2026-01-27, 11:42:40 +0100, Antony Antony wrote:
> > > > The current code prevents migrating an SA from UDP encapsulation to
> > > > plain ESP. This is needed when moving from a NATed path to a non-NATed
> > > > one, for example when switching from IPv4+NAT to IPv6.
> > > >
> > > > Only copy the existing encapsulation during migration if the encap
> > > > attribute is explicitly provided.
> > >
> > > Are we sure nobody out there relies on this behavior (silently copying
> > > the existing UDP encap without having to explicitly request it in the
> > > MIGRATE request)? If there are, this patch would break their setup by
> > > clearing the encap that they expect to still be present.
> >
> > Libreswan and Android are the main users of migrate method. Libreswan sets
> > the
> > value in every call. I am guessing Android does that too.
> >
> > Yan, would this patch cause regression in Android?
> >
> > Without this fix migrating from v4 nat to v6 and no v4 nat won't  work.
> >
> > Also the ENCAP migrate with UDP port was broken before, 2017,
> > the commit 4ab47d47af20 ("xfrm: extend MIGRATE with UDP encapsulation
> > port") ?
> > So likely it was never used by older code and PF_KEY.
> >
> > For the new methed strongSwan wants to support migrating from UDP encap
> > to no UDP encap.
> >
> > regards
> > -antony
> >
> > PS : Steffen advised not to Fixes tag.
> > --
> > Devel mailing list -- devel@lists.linux-ipsec.org
> > To unsubscribe send an email to devel-leave@lists.linux-ipsec.org
> >

> -- 
> Devel mailing list -- devel@lists.linux-ipsec.org
> To unsubscribe send an email to devel-leave@lists.linux-ipsec.org

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: [devel-ipsec] Re: [PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Yan Yan]

Hi Antony,

Sorry for the late reply. We’ve prototyped this and confirmed that
Android can be changed to explicitly provide the encap_tmpl in the
MIGRATE requests. Also we are excited to have kernel support for
encap-to-non-encap migration.

Thanks,
Yan and Nathan

On Mon, Feb 2, 2026 at 11:38 AM Antony Antony <antony@phenome.org> wrote:
>
> On Mon, Feb 02, 2026 at 10:15:24AM -0800, Nathan Harold via Devel wrote:
> > Unfortunately, I believe Android relies on this behavior (at least for
> > now). We never re-send the encap parameters.
> >
> > https://cs.android.com/android/platform/superproject/main/+/main:system/netd/server/XfrmController.cpp;l=1183;drc=61197364367c9e404c7da6900658f1b16c42d0da
>
> Thanks Nathan. It is good to know.
>
> The next question is how do you feel about changing the behavior in
> Android? Would you be willing re-send ports every time the SA has it?
>
> This will allow more flexible migration. Migrating from NAT to no NAT an
> IPv6 without NAT would be possible.
>
> If that is a bad idea, I would limit this change to the new method only.
>
> regards,
> -antony
>
> >
> > -Nathan
> >
> >
> > On Mon, Feb 2, 2026 at 4:58 AM Antony Antony via Devel <
> > devel@lists.linux-ipsec.org> wrote:
> >
> > > On Fri, Jan 30, 2026 at 12:28:19 +0100, Sabrina Dubroca wrote:
> > > > 2026-01-27, 11:42:40 +0100, Antony Antony wrote:
> > > > > The current code prevents migrating an SA from UDP encapsulation to
> > > > > plain ESP. This is needed when moving from a NATed path to a non-NATed
> > > > > one, for example when switching from IPv4+NAT to IPv6.
> > > > >
> > > > > Only copy the existing encapsulation during migration if the encap
> > > > > attribute is explicitly provided.
> > > >
> > > > Are we sure nobody out there relies on this behavior (silently copying
> > > > the existing UDP encap without having to explicitly request it in the
> > > > MIGRATE request)? If there are, this patch would break their setup by
> > > > clearing the encap that they expect to still be present.
> > >
> > > Libreswan and Android are the main users of migrate method. Libreswan sets
> > > the
> > > value in every call. I am guessing Android does that too.
> > >
> > > Yan, would this patch cause regression in Android?
> > >
> > > Without this fix migrating from v4 nat to v6 and no v4 nat won't  work.
> > >
> > > Also the ENCAP migrate with UDP port was broken before, 2017,
> > > the commit 4ab47d47af20 ("xfrm: extend MIGRATE with UDP encapsulation
> > > port") ?
> > > So likely it was never used by older code and PF_KEY.
> > >
> > > For the new methed strongSwan wants to support migrating from UDP encap
> > > to no UDP encap.
> > >
> > > regards
> > > -antony
> > >
> > > PS : Steffen advised not to Fixes tag.
> > > --
> > > Devel mailing list -- devel@lists.linux-ipsec.org
> > > To unsubscribe send an email to devel-leave@lists.linux-ipsec.org
> > >
>
> > --
> > Devel mailing list -- devel@lists.linux-ipsec.org
> > To unsubscribe send an email to devel-leave@lists.linux-ipsec.org
>


-- 
--
Best,
Yan

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Antony Antony]

Hi Yan,

On Mon, Feb 23, 2026 at 07:28:06PM -0800, Yan Yan wrote:
> Hi Antony,
> 
> Sorry for the late reply. We’ve prototyped this and confirmed that
> Android can be changed to explicitly provide the encap_tmpl in the
> MIGRATE requests. Also we are excited to have kernel support for
> encap-to-non-encap migration.

Thanks for the confirmation and prototype testing. I will send out v6 soon.

Would  you like to add any tags Tested or Reviewd...

-antony

Re: [devel-ipsec] Re: [PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration

[Antony Antony]

On Fri, Jan 30, 2026 at 01:14:39PM +0100, Sabrina Dubroca via Devel wrote:
> 2026-01-27, 11:43:42 +0100, Antony Antony wrote:
> > Add descriptive(extack) error messages for all error paths
> > in state migration. This improves diagnostics by
> > providing clear feedback when migration fails.
> > 
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> > v4->v5: - added this patch
> > ---
> >  net/xfrm/xfrm_state.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index 88a362e46972..2e03871ae872 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
> >  	struct xfrm_state *xc;
> > 
> >  	xc = xfrm_state_clone_and_setup(x, encap, m);
> > -	if (!xc)
> > +	if (!xc) {
> > +		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");
> 
> When xfrm_state_clone_and_setup fails it's because some allocation
> failed and the user won't be able to do much about this, right? I
> don't feel extack in those situations is super helpful.

I felt it was usefaul to know, and to log this happened. May not a great 
idea.

> 
> >  		return NULL;
> > +	}
> > 
> > -	if (xfrm_init_state(xc) < 0)
> > +	if (xfrm_init_state(xc) < 0) {
> > +		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");
> 
> xfrm_init_state itself doesn't handle extack, but it's just a wrapper
> around functions that do. Maybe better to make xfrm_init_state
> propagate extack?

That is a great idea. May be in a future patch set. For now, I will drop 
this patch from this series. To move forward quickly.

> 
> >  		goto error;
> > +	}
> > 
> >  	/* configure the hardware if offload is requested */
> > -	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
> > +	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
> > +		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");
> 
> We already set an extack in xfrm_dev_state_add, this chunk should be
> dropped to avoid overwriting the more specific info we got.
> 
> >  		goto error;
> > +	}
> > 
> >  	return xc;
> >  error:
> > @@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
> >  		xfrm_state_insert(xc);
> >  	} else {
> >  		if (xfrm_state_add(xc) < 0) {
> > +			NL_SET_ERR_MSG(extack, "Failed to add migrated state");
> 
> Not a strong objection, but this case would be the EEXIST situation
> from xfrm_state_add, and there's not much the user can do about this?

Fair point, but logging it still has value too, userspace can track these
over time and adapt. Let's revisit when we add extack to xfrm_init_state.

> 
> >  			if (xuo)
> >  				xfrm_dev_state_delete(xc);
> >  			xc->km.state = XFRM_STATE_DEAD;
> 

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: [devel-ipsec] Re: [PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration

[Sabrina Dubroca]

2026-02-26, 16:43:22 +0100, Antony Antony wrote:
> On Fri, Jan 30, 2026 at 01:14:39PM +0100, Sabrina Dubroca via Devel wrote:
> > 2026-01-27, 11:43:42 +0100, Antony Antony wrote:
> > > Add descriptive(extack) error messages for all error paths
> > > in state migration. This improves diagnostics by
> > > providing clear feedback when migration fails.
> > > 
> > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > ---
> > > v4->v5: - added this patch
> > > ---
> > >  net/xfrm/xfrm_state.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index 88a362e46972..2e03871ae872 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
> > >  	struct xfrm_state *xc;
> > > 
> > >  	xc = xfrm_state_clone_and_setup(x, encap, m);
> > > -	if (!xc)
> > > +	if (!xc) {
> > > +		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");
> > 
> > When xfrm_state_clone_and_setup fails it's because some allocation
> > failed and the user won't be able to do much about this, right? I
> > don't feel extack in those situations is super helpful.
> 
> I felt it was usefaul to know, and to log this happened. May not a great 
> idea.

I don't have a super strong opinion. IIRC that was the approach I
picked when I added extack (no extack for kernel events that the user
can't do anything about and don't result from an invalid netlink
message), but maybe that kind of stuff deserves an extack too.

Also, I thought that something that ends up returning ENOMEM to
userspace is explicit enough, without adding a string "failed to
allocate memory for $object" in extack. But I don't work on *swan, so
maybe it's more useful than I think.

(Steffen has the final word, and you're closer to him than I am :))


> > >  		return NULL;
> > > +	}
> > > 
> > > -	if (xfrm_init_state(xc) < 0)
> > > +	if (xfrm_init_state(xc) < 0) {
> > > +		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");
> > 
> > xfrm_init_state itself doesn't handle extack, but it's just a wrapper
> > around functions that do. Maybe better to make xfrm_init_state
> > propagate extack?
> 
> That is a great idea. May be in a future patch set. For now, I will drop 
> this patch from this series. To move forward quickly.

Ok. Or keep the patch with just the fixup right below this, I'm not
NACKing it.

> > >  		goto error;
> > > +	}
> > > 
> > >  	/* configure the hardware if offload is requested */
> > > -	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
> > > +	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
> > > +		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");
> > 
> > We already set an extack in xfrm_dev_state_add, this chunk should be
> > dropped to avoid overwriting the more specific info we got.
> > 
> > >  		goto error;
> > > +	}
> > > 
> > >  	return xc;
> > >  error:
> > > @@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
> > >  		xfrm_state_insert(xc);
> > >  	} else {
> > >  		if (xfrm_state_add(xc) < 0) {
> > > +			NL_SET_ERR_MSG(extack, "Failed to add migrated state");
> > 
> > Not a strong objection, but this case would be the EEXIST situation
> > from xfrm_state_add, and there's not much the user can do about this?
> 
> Fair point, but logging it still has value too, userspace can track these
> over time and adapt. Let's revisit when we add extack to xfrm_init_state.

Ok.

> > >  			if (xuo)
> > >  				xfrm_dev_state_delete(xc);
> > >  			xc->km.state = XFRM_STATE_DEAD;
> > 

-- 
Sabrina

Re: [PATCH ipsec-next v5 1/8] xfrm: add missing __rcu annotation to nlsk

[Sabrina Dubroca]

2026-01-27, 11:42:01 +0100, Antony Antony wrote:
> The nlsk field in struct netns_xfrm is RCU-protected, as seen by
> the use of rcu_assign_pointer() and RCU_INIT_POINTER() when updating
> it.
> However, the field lacks the __rcu annotation, and most read-side
> accesses don't use rcu_dereference().
> 
> Add the missing __rcu annotation and convert all read-side accesses to
> use rcu_dereference() for correctness and to silence sparse warnings.
> 
> Sparse warning reported by NIPA allmodconfig test when modifying
> net/xfrm/xfrm_user.c. The warning is a pre-existing issue in
> xfrm_nlmsg_multicast(). This series added a new call to this function
> and NIPA testing reported a new warning was added by this series.
> 
> To reproduce (requires sparse):
> make C=2 net/xfrm/
> net/xfrm/xfrm_user.c:1574:29: error: incompatible types in comparison
> expression (different address spaces):
> net/xfrm/xfrm_user.c:1574:29:    struct sock [noderef] __rcu *
> net/xfrm/xfrm_user.c:1574:29:    struct sock *

BTW, after this, sparse will complain about the other accesses to nlsk
in net/xfrm/xfrm_user.c (in the nlmsg_unicast(net->xfrm.nlsk, ...)
calls).

I have a patch adding this __rcu annotation, and fixing the warnings
that it causes. It's part of the series I'm planning to submit very
soon, which fixes a lot of rcu-related warnings in net/xfrm/*.

-- 
Sabrina

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 7/8] xfrm: add error messages to state migration

[Antony Antony]

On Thu, Feb 26, 2026 at 05:59:52PM +0100, Sabrina Dubroca via Devel wrote:
> 2026-02-26, 16:43:22 +0100, Antony Antony wrote:
> > On Fri, Jan 30, 2026 at 01:14:39PM +0100, Sabrina Dubroca via Devel wrote:
> > > 2026-01-27, 11:43:42 +0100, Antony Antony wrote:
> > > > Add descriptive(extack) error messages for all error paths
> > > > in state migration. This improves diagnostics by
> > > > providing clear feedback when migration fails.
> > > > 
> > > > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > > > ---
> > > > v4->v5: - added this patch
> > > > ---
> > > >  net/xfrm/xfrm_state.c | 13 ++++++++++---
> > > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > > index 88a362e46972..2e03871ae872 100644
> > > > --- a/net/xfrm/xfrm_state.c
> > > > +++ b/net/xfrm/xfrm_state.c
> > > > @@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
> > > >  	struct xfrm_state *xc;
> > > > 
> > > >  	xc = xfrm_state_clone_and_setup(x, encap, m);
> > > > -	if (!xc)
> > > > +	if (!xc) {
> > > > +		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");
> > > 
> > > When xfrm_state_clone_and_setup fails it's because some allocation
> > > failed and the user won't be able to do much about this, right? I
> > > don't feel extack in those situations is super helpful.
> > 
> > I felt it was usefaul to know, and to log this happened. May not a great 
> > idea.
> 
> I don't have a super strong opinion. IIRC that was the approach I
> picked when I added extack (no extack for kernel events that the user
> can't do anything about and don't result from an invalid netlink
> message), but maybe that kind of stuff deserves an extack too.
> 
> Also, I thought that something that ends up returning ENOMEM to
> userspace is explicit enough, without adding a string "failed to
> allocate memory for $object" in extack. But I don't work on *swan, so
> maybe it's more useful than I think.

*swans are slowly catching up with extack. For years we ignored it
due to two reasons: lower coverage and lack of documentation.
Both are improving over time, so I think it's worth embracing more broadly now.
I hope we add a better extack support in xfrm_init_state().

E* errors I find hard to figure out as user, may be *swans log them as 
numbers not as friendly names!

> (Steffen has the final word, and you're closer to him than I am :))
> 
> 
> > > >  		return NULL;
> > > > +	}
> > > > 
> > > > -	if (xfrm_init_state(xc) < 0)
> > > > +	if (xfrm_init_state(xc) < 0) {
> > > > +		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");
> > > 
> > > xfrm_init_state itself doesn't handle extack, but it's just a wrapper
> > > around functions that do. Maybe better to make xfrm_init_state
> > > propagate extack?
> > 
> > That is a great idea. May be in a future patch set. For now, I will drop 
> > this patch from this series. To move forward quickly.
> 
> Ok. Or keep the patch with just the fixup right below this, I'm not
> NACKing it.

thanks for clarifying.  I will keep the patch without xfrm_dev_state_add() 
case. 

> 
> > > >  		goto error;
> > > > +	}
> > > > 
> > > >  	/* configure the hardware if offload is requested */
> > > > -	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
> > > > +	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
> > > > +		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");
> > > 
> > > We already set an extack in xfrm_dev_state_add, this chunk should be
> > > dropped to avoid overwriting the more specific info we got.
> > > 
> > > >  		goto error;
> > > > +	}
> > > > 
> > > >  	return xc;
> > > >  error:
> > > > @@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
> > > >  		xfrm_state_insert(xc);
> > > >  	} else {
> > > >  		if (xfrm_state_add(xc) < 0) {
> > > > +			NL_SET_ERR_MSG(extack, "Failed to add migrated state");
> > > 
> > > Not a strong objection, but this case would be the EEXIST situation
> > > from xfrm_state_add, and there's not much the user can do about this?
> > 
> > Fair point, but logging it still has value too, userspace can track these
> > over time and adapt. Let's revisit when we add extack to xfrm_init_state.
> 
> Ok.
> 
> > > >  			if (xuo)
> > > >  				xfrm_dev_state_delete(xc);
> > > >  			xc->km.state = XFRM_STATE_DEAD;
> > > 
> 
> -- 
> Sabrina

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 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 1/8] xfrm: add missing __rcu annotation to nlsk

[Antony Antony]

On Thu, Feb 26, 2026 at 06:07:41PM +0100, Sabrina Dubroca via Devel wrote:
> 2026-01-27, 11:42:01 +0100, Antony Antony wrote:
> > The nlsk field in struct netns_xfrm is RCU-protected, as seen by
> > the use of rcu_assign_pointer() and RCU_INIT_POINTER() when updating
> > it.
> > However, the field lacks the __rcu annotation, and most read-side
> > accesses don't use rcu_dereference().
> > 
> > Add the missing __rcu annotation and convert all read-side accesses to
> > use rcu_dereference() for correctness and to silence sparse warnings.
> > 
> > Sparse warning reported by NIPA allmodconfig test when modifying
> > net/xfrm/xfrm_user.c. The warning is a pre-existing issue in
> > xfrm_nlmsg_multicast(). This series added a new call to this function
> > and NIPA testing reported a new warning was added by this series.
> > 
> > To reproduce (requires sparse):
> > make C=2 net/xfrm/
> > net/xfrm/xfrm_user.c:1574:29: error: incompatible types in comparison
> > expression (different address spaces):
> > net/xfrm/xfrm_user.c:1574:29:    struct sock [noderef] __rcu *
> > net/xfrm/xfrm_user.c:1574:29:    struct sock *
> 
> BTW, after this, sparse will complain about the other accesses to nlsk
> in net/xfrm/xfrm_user.c (in the nlmsg_unicast(net->xfrm.nlsk, ...)
> calls).

Indeed there are more sparse warning. I am glad to hear you working on a 
borader patch!

I stayed focused on this one since it was directly triggered by my patch
series, and I couldn't find much guidance on the others easily.

> 
> I have a patch adding this __rcu annotation, and fixing the warnings
> that it causes. It's part of the series I'm planning to submit very
> soon, which fixes a lot of rcu-related warnings in net/xfrm/*.

I am looking forward to this. I'll keep my patch for now as a fix. If your 
series lands first, I'm happy to drop mine.

-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 3/8] xfrm: allow migration from UDP encapsulated to non-encapsulated ESP

[Yan Yan]

Hi Antony,

I am happy to be added with a Tested-by tag. I set up a few more tests
 to ensure full coverage. For the record, I verified the following 16
cases from Android userspace and with your patch, covering all address
family combinations (v4→v4, v4→v6, v6→v4, v6→v6) for each:

- Encap ↔ Non-Encap
- Encap ↔ Encap
- Non-Encap ↔ Non-Encap
- Non-Encap ↔ Encap

Everything works as expected.

Also I am following the discussion of "omit-to-clear" vs.
"omit-to-inherit" strategy on the other thread ([PATCH ipsec-next v5
8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration). I'm
not sure if that will result in any changes to your implementation,
but I'm looking forward to the v6 patch!

On Thu, Feb 26, 2026 at 7:41 AM Antony Antony <antony@phenome.org> wrote:
>
> Hi Yan,
>
> On Mon, Feb 23, 2026 at 07:28:06PM -0800, Yan Yan wrote:
> > Hi Antony,
> >
> > Sorry for the late reply. We’ve prototyped this and confirmed that
> > Android can be changed to explicitly provide the encap_tmpl in the
> > MIGRATE requests. Also we are excited to have kernel support for
> > encap-to-non-encap migration.
>
> Thanks for the confirmation and prototype testing. I will send out v6 soon.
>
> Would  you like to add any tags Tested or Reviewd...
>
> -antony



--
--
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, 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