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

Antony Antony
2026-03-02T14:22:01Z

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