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
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 <redacted>
---
v3->v4: added this patch
---
include/net/netns/xfrm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 <redacted>
---
v1->v2: remove extra saddr copy, previous line
---
net/xfrm/xfrm_state.c | 2 --
1 file changed, 2 deletions(-)
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 <redacted>
---
net/xfrm/xfrm_state.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
@@ -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;
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 <redacted>
---
v4->v5: - added this patch
---
include/net/xfrm.h | 11 +++++++
net/xfrm/xfrm_state.c | 73 +++++++++++++++++++++++++++++++------------
2 files changed, 64 insertions(+), 20 deletions(-)
@@ -2144,24 +2144,57 @@ struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,if(xuo&&xfrm_dev_state_add(net,xc,xuo,extack))gotoerror;-/* add state */+returnxc;+error:+xc->km.state=XFRM_STATE_DEAD;+xfrm_state_put(xc);+returnNULL;+}+EXPORT_SYMBOL(xfrm_state_migrate_create);++intxfrm_state_migrate_install(conststructxfrm_state*x,+structxfrm_state*xc,+conststructxfrm_migrate*m,+structxfrm_user_offload*xuo,+structnetlink_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-stateistobeupdatedasitisapartoftriplet*/+/*+*Careisneededwhenthedestinationaddress+*ofthestateistobeupdatedasitisapartoftriplet.+*/xfrm_state_insert(xc);}else{-if(xfrm_state_add(xc)<0)-gotoerror_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;+}}+return0;+}+EXPORT_SYMBOL(xfrm_state_migrate_install);++structxfrm_state*xfrm_state_migrate(structxfrm_state*x,+structxfrm_migrate*m,+structxfrm_encap_tmpl*encap,+structnet*net,+structxfrm_user_offload*xuo,+structnetlink_ext_ack*extack)+{+structxfrm_state*xc;++xc=xfrm_state_migrate_create(x,m,encap,net,xuo,extack);+if(!xc)+returnNULL;++if(xfrm_state_migrate_install(x,xc,m,xuo,extack)<0)+returnNULL;+returnxc;-error_add:-if(xuo)-xfrm_dev_state_delete(xc);-error:-xc->km.state=XFRM_STATE_DEAD;-xfrm_state_put(xc);-returnNULL;}EXPORT_SYMBOL(xfrm_state_migrate);#endif--
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,structxfrm_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");returnNULL;+}-if(xfrm_init_state(xc)<0)+if(xfrm_init_state(xc)<0){+NL_SET_ERR_MSG(extack,"Failed to initialize migrated state");gotoerror;+}/* 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");gotoerror;+}returnxc;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;--
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 <redacted>
---
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(-)
@@ -3154,7 +3168,167 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,kfree(xuo);returnerr;}++staticintbuild_migrate_state(structsk_buff*skb,+conststructxfrm_user_migrate_state*m,+conststructxfrm_encap_tmpl*encap,+conststructxfrm_user_offload*xuo,+u32portid,u32seq)+{+interr;+structnlmsghdr*nlh;+structxfrm_user_migrate_state*um;++nlh=nlmsg_put(skb,portid,seq,XFRM_MSG_MIGRATE_STATE,+sizeof(structxfrm_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)+gotoout_cancel;+}++if(xuo){+err=nla_put(skb,XFRMA_OFFLOAD_DEV,sizeof(*xuo),xuo);+if(err)+gotoout_cancel;+}++nlmsg_end(skb,nlh);+return0;++out_cancel:+nlmsg_cancel(skb,nlh);+returnerr;+}++staticunsignedintxfrm_migrate_state_msgsize(boolwith_encap,boolwith_xuo)+{+returnNLMSG_ALIGN(sizeof(structxfrm_user_migrate_state))++(with_encap?nla_total_size(sizeof(structxfrm_encap_tmpl)):0)++(with_xuo?nla_total_size(sizeof(structxfrm_user_offload)):0);+}++staticintxfrm_send_migrate_state(conststructxfrm_user_migrate_state*um,+conststructxfrm_encap_tmpl*encap,+conststructxfrm_user_offload*xuo,+u32portid,u32seq)+{+interr;+structsk_buff*skb;+structnet*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);+returnerr;+}++returnxfrm_nlmsg_multicast(net,skb,0,XFRMNLGRP_MIGRATE);+}++staticintxfrm_do_migrate_state(structsk_buff*skb,structnlmsghdr*nlh,+structnlattr**attrs,structnetlink_ext_ack*extack)+{+interr=-ESRCH;+structxfrm_state*x;+structxfrm_state*xc;+structnet*net=sock_net(skb->sk);+structxfrm_encap_tmpl*encap=NULL;+structxfrm_user_offload*xuo=NULL;+structxfrm_migratem={};+structxfrm_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");+returnerr;+}++if(!x->dir){+NL_SET_ERR_MSG(extack,"State direction is invalid");+err=-EINVAL;+gotoout;+}++if(attrs[XFRMA_ENCAP]){+encap=kmemdup(nla_data(attrs[XFRMA_ENCAP]),sizeof(*encap),+GFP_KERNEL);+if(!encap){+err=-ENOMEM;+gotoout;+}+}++if(attrs[XFRMA_OFFLOAD_DEV]){+xuo=kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),+sizeof(*xuo),GFP_KERNEL);+if(!xuo){+err=-ENOMEM;+gotoout;+}+}++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;+gotoout;+}++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){+/*+*InthisrarecaseboththeoldSAandthenewSA+*willdisappear.+*AlternativesriskduplicateSN/IVusagewhichmustnotoccur.+*Userspacemusthandlethiserror,-EEXIST.+*/+gotoout;+}++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);+returnerr;+}+#else+staticintxfrm_do_migrate_state(structsk_buff*skb,structnlmsghdr*nlh,+structnlattr**attrs,structnetlink_ext_ack*extack)+{+NL_SET_ERR_MSG(extack,"XFRM_MSG_MIGRATE_STATE is not supported");+return-ENOPROTOOPT;+}+staticintxfrm_do_migrate(structsk_buff*skb,structnlmsghdr*nlh,structnlattr**attrs,structnetlink_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);
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 <redacted>
---
v4->v5: - added this patch
---
include/net/xfrm.h | 46 ++++++++++++++++++++++++++++++++++---------
net/xfrm/xfrm_state.c | 11 ++++-------
2 files changed, 41 insertions(+), 16 deletions(-)
@@ -2024,23 +2024,51 @@ static inline unsigned int xfrm_replay_state_esn_len(struct xfrm_replay_state_es#ifdef CONFIG_XFRM_MIGRATEstaticinlineintxfrm_replay_clone(structxfrm_state*x,-structxfrm_state*orig)+conststructxfrm_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;+}return0;}+staticinlinevoidxfrm_replay_sync(structxfrm_state*x,conststructxfrm_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));+}+}++staticinlinevoidxfrm_migrate_sync(structxfrm_state*x,+conststructxfrm_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);+}+staticinlinestructxfrm_algo_aead*xfrm_algo_aead_clone(structxfrm_algo_aead*orig){returnkmemdup(orig,aead_len(orig),GFP_KERNEL);
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
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,structxfrm_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.
quoted hunk
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;
On Fri, Jan 30, 2026 at 12:28:19 +0100, Sabrina Dubroca wrote:
2026-01-27, 11:42:40 +0100, Antony Antony wrote:
quoted
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.
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?
@@ -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)?
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.
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])?
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).
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,structxfrm_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.
quoted
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.
quoted
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.
quoted
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.
quoted
if (xuo)
xfrm_dev_state_delete(xc);
xc->km.state = XFRM_STATE_DEAD;
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:
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.
@@ -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.
quoted
+ 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.
I don't know if Steffen requires it, but networking normally uses
reverse xmas tree order.
It is good to keep that style. Fixed.
quoted
+ 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
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.
And same here, I don't think we actually need a copy of that memory.
changed. thanks.
quoted
+ 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!
quoted
+ 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.
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,structxfrm_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 :))
quoted
quoted
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.
quoted
quoted
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.
quoted
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.
quoted
quoted
if (xuo)
xfrm_dev_state_delete(xc);
xc->km.state = XFRM_STATE_DEAD;
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
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:
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.
[...]
quoted
quoted
+ 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
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 [off-list ref] wrote:
quoted hunk
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 <redacted>
---
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(-)
@@ -3154,7 +3168,167 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,kfree(xuo);returnerr;}++staticintbuild_migrate_state(structsk_buff*skb,+conststructxfrm_user_migrate_state*m,+conststructxfrm_encap_tmpl*encap,+conststructxfrm_user_offload*xuo,+u32portid,u32seq)+{+interr;+structnlmsghdr*nlh;+structxfrm_user_migrate_state*um;++nlh=nlmsg_put(skb,portid,seq,XFRM_MSG_MIGRATE_STATE,+sizeof(structxfrm_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)+gotoout_cancel;+}++if(xuo){+err=nla_put(skb,XFRMA_OFFLOAD_DEV,sizeof(*xuo),xuo);+if(err)+gotoout_cancel;+}++nlmsg_end(skb,nlh);+return0;++out_cancel:+nlmsg_cancel(skb,nlh);+returnerr;+}++staticunsignedintxfrm_migrate_state_msgsize(boolwith_encap,boolwith_xuo)+{+returnNLMSG_ALIGN(sizeof(structxfrm_user_migrate_state))++(with_encap?nla_total_size(sizeof(structxfrm_encap_tmpl)):0)++(with_xuo?nla_total_size(sizeof(structxfrm_user_offload)):0);+}++staticintxfrm_send_migrate_state(conststructxfrm_user_migrate_state*um,+conststructxfrm_encap_tmpl*encap,+conststructxfrm_user_offload*xuo,+u32portid,u32seq)+{+interr;+structsk_buff*skb;+structnet*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);+returnerr;+}++returnxfrm_nlmsg_multicast(net,skb,0,XFRMNLGRP_MIGRATE);+}++staticintxfrm_do_migrate_state(structsk_buff*skb,structnlmsghdr*nlh,+structnlattr**attrs,structnetlink_ext_ack*extack)+{+interr=-ESRCH;+structxfrm_state*x;+structxfrm_state*xc;+structnet*net=sock_net(skb->sk);+structxfrm_encap_tmpl*encap=NULL;+structxfrm_user_offload*xuo=NULL;+structxfrm_migratem={};+structxfrm_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");+returnerr;+}++if(!x->dir){+NL_SET_ERR_MSG(extack,"State direction is invalid");+err=-EINVAL;+gotoout;+}++if(attrs[XFRMA_ENCAP]){+encap=kmemdup(nla_data(attrs[XFRMA_ENCAP]),sizeof(*encap),+GFP_KERNEL);+if(!encap){+err=-ENOMEM;+gotoout;+}+}++if(attrs[XFRMA_OFFLOAD_DEV]){+xuo=kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),+sizeof(*xuo),GFP_KERNEL);+if(!xuo){+err=-ENOMEM;+gotoout;+}+}++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;+gotoout;+}++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){+/*+*InthisrarecaseboththeoldSAandthenewSA+*willdisappear.+*AlternativesriskduplicateSN/IVusagewhichmustnotoccur.+*Userspacemusthandlethiserror,-EEXIST.+*/+gotoout;+}++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);+returnerr;+}+#else+staticintxfrm_do_migrate_state(structsk_buff*skb,structnlmsghdr*nlh,+structnlattr**attrs,structnetlink_ext_ack*extack)+{+NL_SET_ERR_MSG(extack,"XFRM_MSG_MIGRATE_STATE is not supported");+return-ENOPROTOOPT;+}+staticintxfrm_do_migrate(structsk_buff*skb,structnlmsghdr*nlh,structnlattr**attrs,structnetlink_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);
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
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 [off-list ref] wrote:
2026-02-26, 17:44:51 -0800, Yan Yan via Devel wrote:
quoted
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
@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,structxfrm_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 :))
quoted
quoted
quoted
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.
quoted
quoted
quoted
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.
quoted
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.
quoted
quoted
quoted
if (xuo)
xfrm_dev_state_delete(xc);
xc->km.state = XFRM_STATE_DEAD;
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:
quoted
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:
quoted
On Tue, Feb 03, 2026 at 10:25:15PM +0100, Sabrina Dubroca via Devel wrote:
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.
[...]
quoted
quoted
quoted
+ 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
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:
quoted
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
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
On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
quoted
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 [off-list ref] wrote:
quoted
2026-02-26, 17:44:51 -0800, Yan Yan via Devel wrote:
quoted
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
On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
quoted
quoted
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)
quoted
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
On Tue, Mar 10, 2026 at 12:09:27PM +0100, Sabrina Dubroca wrote:
2026-03-08, 15:42:58 +0100, Antony Antony wrote:
quoted
On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
quoted
quoted
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!
quoted
quoted
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.
quoted
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).
quoted
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");
quoted
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.
quoted
- 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 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
On Fri, Mar 13, 2026 at 05:32:15PM -0700, Yan Yan via Devel wrote:
quoted
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.
quoted
I am actually using xfrm_smark_init() which will accept both.
Great! Thanks for supporting that.
quoted
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.
quoted
- XFRMA_REPLAY_ESN_VAL / XFRMA_REPLAY_VAL : may be later replay type
should not change.
I agree we should keep the replay type immutable. Changing ESN flag on
the fly would make it hard to keep both sides synced, and I'm not
aware of any use case for this.
While testing XFRM_MSG_MIGRATE_STATE we ran into an issue with x->sel
migration in transport mode. In transport mode the selector is typically
a single-host entry matching the SA's saddr and daddr, so after
migration it only needs to be updated with the new addresses.
For this common case I added XFRM_MIGRATE_STATE_UPDATE_SEL to
xfrm_user_migrate_state.flags. When set, the kernel validates that the
existing selector is a single-host match for the SA addresses and
derives the new selector from new_daddr/new_saddr with the appropriate
mask for the new family.
I think this is the main use case. However, for corner cases out there,
the selector is not a simple single-host entry,
struct xfrm_user_migrate_state now carries a new_sel field. When
XFRM_MIGRATE_STATE_UPDATE_SEL is not set, new_sel is used as-is for
the migrated SA.
-antony