On Thu, Feb 26, 2026 at 05:59:52PM +0100, Sabrina Dubroca via Devel wrote:

2026-02-26, 16:43:22 +0100, Antony Antony wrote:

quoted

On Fri, Jan 30, 2026 at 01:14:39PM +0100, Sabrina Dubroca via Devel wrote:

quoted

2026-01-27, 11:43:42 +0100, Antony Antony wrote:

quoted

Add descriptive(extack) error messages for all error paths
in state migration. This improves diagnostics by
providing clear feedback when migration fails.

Signed-off-by: Antony Antony <redacted>
---
v4->v5: - added this patch
---
 net/xfrm/xfrm_state.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 88a362e46972..2e03871ae872 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -2129,15 +2129,21 @@ struct xfrm_state *xfrm_state_migrate_create(struct xfrm_state *x,
 	struct xfrm_state *xc;

 	xc = xfrm_state_clone_and_setup(x, encap, m);
-	if (!xc)
+	if (!xc) {
+		NL_SET_ERR_MSG(extack, "Failed to clone and setup state");

When xfrm_state_clone_and_setup fails it's because some allocation
failed and the user won't be able to do much about this, right? I
don't feel extack in those situations is super helpful.

I felt it was usefaul to know, and to log this happened. May not a great 
idea.

I don't have a super strong opinion. IIRC that was the approach I
picked when I added extack (no extack for kernel events that the user
can't do anything about and don't result from an invalid netlink
message), but maybe that kind of stuff deserves an extack too.

Also, I thought that something that ends up returning ENOMEM to
userspace is explicit enough, without adding a string "failed to
allocate memory for $object" in extack. But I don't work on *swan, so
maybe it's more useful than I think.

*swans are slowly catching up with extack. For years we ignored it
due to two reasons: lower coverage and lack of documentation.
Both are improving over time, so I think it's worth embracing more broadly now.
I hope we add a better extack support in xfrm_init_state().

E* errors I find hard to figure out as user, may be *swans log them as 
numbers not as friendly names!

(Steffen has the final word, and you're closer to him than I am :))

quoted

quoted

quoted

 		return NULL;
+	}

-	if (xfrm_init_state(xc) < 0)
+	if (xfrm_init_state(xc) < 0) {
+		NL_SET_ERR_MSG(extack, "Failed to initialize migrated state");

xfrm_init_state itself doesn't handle extack, but it's just a wrapper
around functions that do. Maybe better to make xfrm_init_state
propagate extack?

That is a great idea. May be in a future patch set. For now, I will drop 
this patch from this series. To move forward quickly.

Ok. Or keep the patch with just the fixup right below this, I'm not
NACKing it.

thanks for clarifying.  I will keep the patch without xfrm_dev_state_add() 
case. 

quoted

quoted

quoted

 		goto error;
+	}

 	/* configure the hardware if offload is requested */
-	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack))
+	if (xuo && xfrm_dev_state_add(net, xc, xuo, extack)) {
+		NL_SET_ERR_MSG(extack, "Failed to initialize state offload");

We already set an extack in xfrm_dev_state_add, this chunk should be
dropped to avoid overwriting the more specific info we got.

quoted

 		goto error;
+	}

 	return xc;
 error:

@@ -2161,6 +2167,7 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
 		xfrm_state_insert(xc);
 	} else {
 		if (xfrm_state_add(xc) < 0) {
+			NL_SET_ERR_MSG(extack, "Failed to add migrated state");

Not a strong objection, but this case would be the EEXIST situation
from xfrm_state_add, and there's not much the user can do about this?

Fair point, but logging it still has value too, userspace can track these
over time and adapt. Let's revisit when we add extack to xfrm_init_state.

Ok.

quoted

quoted

quoted

 			if (xuo)
 				xfrm_dev_state_delete(xc);
 			xc->km.state = XFRM_STATE_DEAD;

-- 
Sabrina

thanks,
-antony