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

[Antony Antony <antony@phenome.org>]

Hi Simon,

On Tue, Jan 13, 2026 at 02:57:16PM +0000, Simon Horman via Devel wrote:
> 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 <antony.antony@secunet.com>
> 
> ...
> 
> > 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.

thanks. It is good advise. I sprinkled a couple of const.

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

thanks. I fxied this by squashing the next patch to this one.
 
> [1] https://github.com/masoncl/review-prompts/

thanks! that looks interesting.

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

fixed.

> 
> >  	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));
> 
> ...
> 
> > 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.

removed

> 
> > +{
> > +	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.

I don't see a skb leak. It also looks similar to the functions above.

> 
> > +	}
> > +
> > +	return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_MIGRATE);
> > +}

I will send a new v2.

regards
-antony