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:

quoted

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:

quoted

On Tue, Feb 03, 2026 at 10:25:15PM +0100, Sabrina Dubroca via Devel wrote:

quoted

2026-01-27, 11:44:11 +0100, Antony Antony wrote:

quoted

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

[...]

quoted

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


[...]

quoted

quoted

quoted

+	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