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

[Sabrina Dubroca]

2026-02-26, 16:46:49 +0100, Antony Antony wrote:
> 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:
> > 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?
> 
> 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.


[...]
> > > +	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