Re: [devel-ipsec] Re: [PATCH ipsec-next v5 7/8] xfrm: add error messages to state migration

[Antony Antony <antony@phenome.org>]

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:
> > On Fri, Jan 30, 2026 at 01:14:39PM +0100, Sabrina Dubroca via Devel wrote:
> > > 2026-01-27, 11:43:42 +0100, Antony Antony wrote:
> > > > 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 <antony.antony@secunet.com>
> > > > ---
> > > > 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 :))
> 
> 
> > > >  		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. 

> 
> > > >  		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.
> > > 
> > > >  		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.
> 
> > > >  			if (xuo)
> > > >  				xfrm_dev_state_delete(xc);
> > > >  			xc->km.state = XFRM_STATE_DEAD;
> > > 
> 
> -- 
> Sabrina

thanks,
-antony