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

[Antony Antony <antony@phenome.org>]

On Thu, Feb 26, 2026 at 07:05:59PM +0100, Sabrina Dubroca via Devel wrote:
> 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)

Yes good idea. Done!


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

From recollection, at least one of the *swan log it, and over time
you start to notice the pattern. That said, out-of-memory is a tough case.
When that happens, all bets are off anyway. So it really comes down to
personal preference. I prefer to set something to notify.

My frustration when testing, typically on a low-memory VM, was watching 'ip 
xfrm monitor' and not seeing a netlink notification, left wondering what had 
happened.


> 
> [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)]

You picked up an interesting design choice I made. Since PF_KEY/AF_KEY
is on life support I omitted going through km_state_notify.  So I would
like to have extack when it is possible.

-antony