Re: [devel-ipsec] Re: [ipsec-next,v6,12/14] xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration

[Simon Horman <horms@kernel.org>]

On Wed, Mar 11, 2026 at 09:43:04PM +0100, Antony Antony wrote:
> Hi Simon,
> 
> On Wed, Mar 11, 2026 at 07:57:31PM +0000, Simon Horman via Devel wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > 
> > For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> > ---
> > xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration
> > 
> > This patch adds a new netlink method to migrate a single xfrm_state.
> > Unlike the existing migration mechanism (SA + policy), this supports
> > migrating only the SA and allows changing the reqid.
> > 
> > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > > index 4b29ab92c2a7..ae2a1cf2311a 100644
> > > --- a/include/net/xfrm.h
> > > +++ b/include/net/xfrm.h
> > > @@ -683,11 +683,17 @@ struct xfrm_migrate {
> > >  	xfrm_address_t		new_daddr;
> > >  	xfrm_address_t		new_saddr;
> > >  	struct xfrm_encap_tmpl *encap;
> > > -	struct xfrm_user_offload *xuo;
> > > +	struct xfrm_user_offload xuo;
> > 
> > [ ... ]
> > 
> > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > > index 2f23ca5b62a9..de8b038fc08c 100644
> > > --- a/net/xfrm/xfrm_policy.c
> > > +++ b/net/xfrm/xfrm_policy.c
> > > @@ -4642,6 +4642,19 @@ static int xfrm_migrate_check(const struct xfrm_migrate *m, int num_migrate,
> > >  	return 0;
> > >  }
> > >
> > > +/* Fill migrate fields that are invariant in XFRM_MSG_MIGRATE: inherited
> > > + * from the existing SA unchanged.
> > > + */
> > > +static void xfrm_migrate_copy_old(struct xfrm_migrate *mp,
> > > +				  const struct xfrm_state *x)
> > > +{
> > > +	mp->smark                  = x->props.smark;
> > > +	mp->new_reqid              = x->props.reqid;
> > > +	mp->nat_keepalive_interval = x->nat_keepalive_interval;
> > > +	mp->mapping_maxage         = x->mapping_maxage;
> > > +	mp->new_mark               = x->mark;
> > > +}
> > > +
> > >  int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
> > >  		 struct xfrm_migrate *m, int num_migrate,
> > >  		 struct xfrm_kmaddress *k, struct net *net,
> > > @@ -4680,7 +4693,10 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
> > >  			x_cur[nx_cur] = x;
> > >  			nx_cur++;
> > >  			mp->encap = encap;
> > > -			mp->xuo = xuo;
> > > +			if (xuo)
> > > +				mp->xuo = *xuo;
> >                                      ^^^^
> > >  			xfrm_migrate_copy_old(mp, x);
> > >
> > 
> > What happens to mp->xuo when xuo is NULL?
> 
> thanks. I also red this on NIPA AI reviews. I will fix it in the next 
> version.

Thanks, good to know.

> 
> > 
> > The xfrm_migrate structure changed its xuo field from a pointer
> > (struct xfrm_user_offload *xuo) to an embedded value
> > (struct xfrm_user_offload xuo).
> > 
> > In xfrm_migrate(), when the xuo parameter is NULL (PF_KEY path via
> > pfkey_migrate, or XFRM_MSG_MIGRATE without XFRMA_OFFLOAD_DEV), the
> > 'if (xuo) mp->xuo = *xuo' branch is skipped, leaving mp->xuo with
> > uninitialized stack data.
> > 
> > Both callers declare 'struct xfrm_migrate m[XFRM_MAX_DEPTH]' on the
> > stack without zeroing:
> > 
> > net/key/af_key.c:pfkey_migrate() {
> >     struct xfrm_migrate m[XFRM_MAX_DEPTH];
> >     ...
> >     err = xfrm_migrate(&sel, dir, XFRM_POLICY_TYPE_MAIN, m, i,
> >                        k.local && k.remote ? &k : NULL, net,
> >                        NULL, 0, NULL);
> > }
> > 
> > net/xfrm/xfrm_user.c:xfrm_do_migrate() {
> >     struct xfrm_migrate m[XFRM_MAX_DEPTH];
> 
> -  struct xfrm_migrate m[XFRM_MAX_DEPTH];
> +  struct xfrm_migrate m[XFRM_MAX_DEPTH] = {};
> 
> this should fix it.

Yes, I agree that should fix the problem.

> 
> >     ...
> >     err = xfrm_migrate(&sel, dir, type, m, n, k, net, encap, xuo, extack);
> > }

...