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

Sabrina Dubroca
2026-02-26T18:06:05Z

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