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

[Antony Antony <antony@phenome.org>]

On Tue, Mar 10, 2026 at 12:09:27PM +0100, Sabrina Dubroca wrote:
> 2026-03-08, 15:42:58 +0100, Antony Antony wrote:
> > On Fri, Feb 27, 2026 at 03:14:21PM -0800, Yan Yan via Devel wrote:
> > > > Anything that we leave as implicit copy will have to be "forever"
> > > > implicitly copied with this new MIGRATE_STATE op -- unless we find a
> > > > way to pass a new "clear these properties" flag (probably via a list
> > > > of XFRMA_* attribute names)
> > 
> > that is a limitation we should avoid. It would be nice to extend it
> > over time. We have been there before and it is a pain point. So it is
> > worth investigating alternatives if there is momentum here, otherwise
> > I would keep it simple:)
> 
> Well, because it's a known pain point, we shouldn't just jump into an
> implementation.
> 
> (FWIW, as a kernel-only developer, ie not involved at all in the *swan
> code, I don't have much of an opinion on which property should behave
> which way, just that we know what we're getting into)

Understandable. Partly *swan world, Android, IETF RFC guidelines, NIST et 
al. The *swan world usually wakes late after the time the kernel
reaches distributions.This is too late to fix kernel without breaking ABI.
standards world has its own path. Kernel often implements that what
makes practical sense in the moment and hard depreciate/remove ABI:)

I feel these days there are more interactions such as this, which is a good 
sign!

> > > That is true. I also have the concern that if all updatable attributes
> > > follow the "omit-to-clear" pattern, we lose the extensibility. Thus
> > > ideally we should switch to an "omit-to-inherit" model for some, if
> > > not all, attributes to ensure that adding new SA properties doesn't
> > > break backward compatibility.
> 
> Implicit copy ("omit-to-inherit") gets us in the situation we have
> with the old MIGRATE code, we don't have a way to _not inherit_
> properties. Well, apparently we do, based on what Antony wrote below.
> 
> 
> 
> > Here is my proposal. I extended the code and am testing it now; I hope
> > to send out v6 soon.
> 
> I think it would have been nice to postpone v6 a little bit so that
> others had time to answer here (and avoid a respin if there's some
> disagreement on what the behavior should be).
> 
> > How would omit-to-inherit look in practice? Specify almost all XFRMA
> > attributes supported in XFRM_MSG_NEWSA, minus some immutable ones.
> > The immutable attributes that come to mind are:
> > 
> >     - XFRMA_ALG_*      : crypto must not change during the life of an SA;
> >                          also *swan userspace does not keep this in memory
> >                          after the SA is installed, which is correct
> >                          behaviour.
> >     - XFRMA_SA_DIR     : direction is fixed at SA creation.
> >     - XFRMA_SEC_CTX    : security context is immutable.
> 
> So we should be rejecting an attempt to pass those attributes in a
> MIGRATE_STATE request.

yes. I added it o v6.It reject all unused attributes.  
NL_SET_ERR_MSG_ATTR(extack, attrs[i], "Unsupported attribute in 
XFRM_MSG_MIGRATE_STATE");

> > currently supported attributes, using omit-to-inherit semantics:
> > 
> >     sentinel value to clear, omit to inherit:
> >     - XFRMA_ENCAP              : encap_type=0 to clear
> 
> That would be a new special case value for that attribute, ok.
> 
> >     - XFRMA_OFFLOAD_DEV        : ifindex=0 to clear
> 
> OFFLOAD_DEV with xuo.ifindex=0 is a valid attribute to request offload
> and let the kernel figure out which device to use. We can't use that
> to clear/disable offload of an SA.

Good catch, thanks.

Two options to fix this:

Option 1: add XFRM_OFFLOAD_CLEAR to xfrm_user_offload flags in uapi xfrm.h:

#define XFRM_OFFLOAD_CLEAR  (1 << 7)
When set in XFRMA_OFFLOAD_DEV, it means remove offload rather than configure it.

Option 2: add a __u32 flags field to xfrm_user_migrate_state in uapi xfrm.h.
There is a __u16 reserved currently used for alignment, but 16 bits feels
too small if we want to cover clearing other attributes in the future.
A __u32 at the end of the struct avoids that constraint.

I am leaning toward option 2. Any preference? 


> For the rest, it seems that passing 0 is equivalent to omitting the
> attribute in the current code. Except XFRMA_SA_PCPU, where we consider
> UINT_MAX as "disable" (default value for x->pcpu_num), but we reject
> userspace passing us that value, and XFRMA_SA_PCPU containing 0 is a
> valid value.

yes XFRMA_SA_PCPU that would be UINT_MAX.

-antony