Re: [PATCH ipsec-next v4 2/2] xfrm: Refactor migration setup during the cloning process

[Leon Romanovsky <leon@kernel.org>]

On Mon, Mar 10, 2025 at 08:20:39PM +0800, Chiachang Wang wrote:
> While the xfrm_state_migrate() is the only caller for this method
> currently, this check can be removed indeed.
> I add this for the feasibility of other callers without performing the
> validation. If you have a strong opinion on this. I can update to
> remove this.
> Please let me know if you prefer to do so.

Sure, please remove. We are adding code when it is actually needed.

Thanks

> 
> Thank you!
> 
> Leon Romanovsky <leon@kernel.org> 於 2025年3月10日 週一 下午7:52寫道：
> >
> > On Mon, Mar 10, 2025 at 09:16:20AM +0000, Chiachang Wang wrote:
> > > Previously, migration related setup, such as updating family,
> > > destination address, and source address, was performed after
> > > the clone was created in `xfrm_state_migrate`. This change
> > > moves this setup into the cloning function itself, improving
> > > code locality and reducing redundancy.
> > >
> > > The `xfrm_state_clone_and_setup` function now conditionally
> > > applies the migration parameters from struct xfrm_migrate
> > > if it is provided. This allows the function to be used both
> > > for simple cloning and for cloning with migration setup.
> > >
> > > Test: Tested with kernel test in the Android tree located
> > >       in https://android.googlesource.com/kernel/tests/
> > >       The xfrm_tunnel_test.py under the tests folder in
> > >       particular.
> > > Signed-off-by: Chiachang Wang <chiachangwang@google.com>
> > > ---
> > >  net/xfrm/xfrm_state.c | 18 ++++++++++--------
> > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > > index 9cd707362767..0365daedea32 100644
> > > --- a/net/xfrm/xfrm_state.c
> > > +++ b/net/xfrm/xfrm_state.c
> > > @@ -1958,8 +1958,9 @@ static inline int clone_security(struct xfrm_state *x, struct xfrm_sec_ctx *secu
> > >       return 0;
> > >  }
> > >
> > > -static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > > -                                        struct xfrm_encap_tmpl *encap)
> > > +static struct xfrm_state *xfrm_state_clone_and_setup(struct xfrm_state *orig,
> > > +                                        struct xfrm_encap_tmpl *encap,
> > > +                                        struct xfrm_migrate *m)
> > >  {
> > >       struct net *net = xs_net(orig);
> > >       struct xfrm_state *x = xfrm_state_alloc(net);
> > > @@ -2058,6 +2059,12 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
> > >                       goto error;
> > >       }
> > >
> > > +     if (m) {
> >
> > Why do you need this "if (m)"? "m" should be valid at this stage.
> >
> > Thanks
> >
> > > +             x->props.family = m->new_family;
> > > +             memcpy(&x->id.daddr, &m->new_daddr, sizeof(x->id.daddr));
> > > +             memcpy(&x->props.saddr, &m->new_saddr, sizeof(x->props.saddr));
> > > +     }
> > > +
> > >       return x;
>