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

[Steffen Klassert <steffen.klassert@secunet.com>]

On Thu, Jan 22, 2026 at 11:33:42AM +0100, Steffen Klassert wrote:
> On Mon, Jan 19, 2026 at 09:54:55AM +0100, Antony Antony wrote:
> > +
> > +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 net *net = sock_net(skb->sk);
> > +	struct xfrm_encap_tmpl *encap = NULL;
> > +	struct xfrm_user_offload *xuo = NULL;
> > +	struct xfrm_migrate m = { .old_saddr.a4 = 0,};
> > +	struct xfrm_user_migrate_state *um = nlmsg_data(nlh);
> > +
> > +	if (!um->id.spi) {
> > +		NL_SET_ERR_MSG(extack, "Invalid SPI 0x0");
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = copy_from_user_migrate_state(&m, um);
> > +	if (err)
> > +		return err;
> > +
> > +	x = xfrm_user_state_lookup(net, &um->id, attrs, &err);
> > +
> > +	if (x) {
> > +		struct xfrm_state *xc;
> > +
> > +		if (!x->dir) {
> > +			NL_SET_ERR_MSG(extack, "State direction is invalid");
> > +			err = -EINVAL;
> > +			goto error;
> > +		}
> > +
> > +		if (attrs[XFRMA_ENCAP]) {
> > +			encap = kmemdup(nla_data(attrs[XFRMA_ENCAP]),
> > +					sizeof(*encap), GFP_KERNEL);
> > +			if (!encap) {
> > +				err = -ENOMEM;
> > +				goto error;
> > +			}
> > +		}
> > +		if (attrs[XFRMA_OFFLOAD_DEV]) {
> > +			xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> > +				      sizeof(*xuo), GFP_KERNEL);
> > +			if (!xuo) {
> > +				err = -ENOMEM;
> > +				goto error;
> > +			}
> > +		}
> > +		xc = xfrm_state_migrate(x, &m, encap, net, xuo, extack);
> > +		if (xc) {
> > +			xfrm_state_delete(x);
> 
> This xfrm_state_delete() comes too late. You need to synchronize this
> with xfrm_state_clone_and_setup() by holding the state lock.
> Otherwise we might hand out the same sequence number etc. more than
> one time. Unfortunately xfrm_migrate() has the same problem, the
> error recovery mechanism in xfrm_migrate() looks broken.

The xfrm_migrate() problem is nasty, looks like we can't recover
if the state migration fails. We probably have to delete the
states and wait for the userspace to insert new ones.

Another thing, xfrm_user_state_lookup() returns a reference on x,
you should drop it somewhere.