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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOb+sWFi+8df32zdAL5AmkfCpFBMG6hU=_+S3U-X_Zd6386r6g@mail.gmail.com>
Date: Mon, 10 Mar 2025 20:20:39 +0800
From: Chiachang Wang <chiachangwang@...gle.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com, 
	stanleyjhu@...gle.com, yumike@...gle.com
Subject: Re: [PATCH ipsec-next v4 2/2] xfrm: Refactor migration setup during
 the cloning process

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.

Thank you!

Leon Romanovsky <leon@...nel.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@...gle.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;