Re: [PATCH ipsec-next 4/6] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration
From: Simon Horman <horms@kernel.org>
Date: 2026-01-13 14:57:20
On Fri, Jan 09, 2026 at 02:38:05PM +0100, Antony Antony 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. Signed-off-by: Antony Antony <redacted>
...
quoted hunk ↗ jump to hunk
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index ef832ce477b6..04c893e42bc1 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c@@ -1967,7 +1967,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) + struct xfrm_migrate *m,
Hi Antony, Not strictly related to this patch, but FWIIW, it seems that m could be const in this call stack. And, moreover, I think there would be some value in constifying parameters throughout xfrm.
quoted hunk ↗ jump to hunk
+ struct netlink_ext_ack *extack) { struct net *net = xs_net(orig); struct xfrm_state *x = xfrm_state_alloc(net);@@ -1979,9 +1980,13 @@ 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; x->props.saddr = orig->props.saddr; + if (orig->props.reqid != m->new_reqid) + x->props.reqid = m->new_reqid; + else + x->props.reqid = orig->props.reqid; +
Claude Code with Review Prompts [1] flags that until the next patch of this series m->new_reqid is used uninitialised in the following call stack: xfrm_do_migrate -> xfrm_migrate -> xfrm_state_migrate -> xfrm_state_clone_and_setup Also, while I could have missed something, it seems to me that it is also uninitialised in this call stack: pfkey_migrate -> xfrm_migrate -> xfrm_state_migrate -> xfrm_state_clone_and_setup [1] https://github.com/masoncl/review-prompts/
quoted hunk ↗ jump to hunk
if (orig->aalg) { x->aalg = xfrm_algo_auth_clone(orig->aalg); if (!x->aalg)@@ -2059,7 +2064,6 @@ static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig, goto error; } -
nit: this hunk doesn't seem related to the rest of the patch.
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));
...
quoted hunk ↗ jump to hunk
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
...
+static inline unsigned int xfrm_migrate_state_msgsize(bool with_encap, bool with_xuo)
Please don't use the inline keyword in .c files unless there is a demonstrable - usually performance - reason to do so. Rather, please let the compiler inline (or not) code as it sees fit.
+{
+ 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)
+{
+ 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);
+ if (err < 0) {
+ WARN_ON(1);
+ return err;skb seems to be leaked here. Also flagged by Review Prompts.
+ } + + return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE); +}
...