Message-ID: <aYJnuw5mB8qn236R@krikkit>
Date: Tue, 3 Feb 2026 22:25:15 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <antony.antony@...unet.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Chiachang Wang <chiachangwang@...gle.com>,
	Yan Yan <evitayan@...gle.com>, devel@...ux-ipsec.org,
	Simon Horman <horms@...nel.org>, Paul Moore <paul@...l-moore.com>,
	Stephen Smalley <stephen.smalley.work@...il.com>,
	Ondrej Mosnacek <omosnace@...hat.com>, linux-kernel@...r.kernel.org,
	selinux@...r.kernel.org
Subject: Re: [PATCH ipsec-next v5 8/8] xfrm: add XFRM_MSG_MIGRATE_STATE for
 single SA migration

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

Powered by blists - more mailing lists