[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAOb+sWEdZ-kY6-qnG2u0h_JzeVyrf0b_eT+L=2t-5zCGaXedHA@mail.gmail.com>
Date: Fri, 7 Mar 2025 13:42:25 +0800
From: Chiachang Wang <chiachangwang@...gle.com>
To: Leon Romanovsky <leonro@...dia.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
stanleyjhu@...gle.com, yumike@...gle.com
Subject: Re: [PATCH ipsec v3 1/1] xfrm: Migrate offload configuration
Hi Leon,
Thank you for your review and suggestions. I noticed your patches
haven't been merged into the tree yet. I'm unsure if rebasing my patch
onto yours would work correctly with the kernel upstream. It seems
your patches are suitable for merging. Since I'm not familiar with the
timeline for your patch's inclusion, could you please advise on how
long it might take for them to be in the tree? This would help me
rebase my patch properly. or there are any other alternative way
rather than waiting your patch?
Appreciate for your help!
Chiachang
Leon Romanovsky <leonro@...dia.com> 於 2025年2月24日 週一 下午8:50寫道:
>
> On Mon, Feb 24, 2025 at 06:15:54AM +0000, Chiachang Wang wrote:
> > Add hardware offload configuration to XFRM_MSG_MIGRATE
> > using an option netlink attribute XFRMA_OFFLOAD_DEV.
> >
> > In the existing xfrm_state_migrate(), the xfrm_init_state()
> > is called assuming no hardware offload by default. Even the
> > original xfrm_state is configured with offload, the setting will
> > be reset. If the device is configured with hardware offload,
> > it's reasonable to allow the device to maintain its hardware
> > offload mode. But the device will end up with offload disabled
> > after receiving a migration event when the device migrates the
> > connection from one netdev to another one.
> >
> > The devices that support migration may work with different
> > underlying networks, such as mobile devices. The hardware setting
> > should be forwarded to the different netdev based on the
> > migration configuration. This change provides the capability
> > for user space to migrate from one netdev to another.
> >
> > 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>
> > ---
> >
> > v2 -> v3:
> > - Modify af_key to fix kbuild error
> > v1 -> v2:
> > - Address review feedback to correct the logic in the
> > xfrm_state_migrate in the migration offload configuration
> > change.
> > - Revise the commit message for "xfrm: Migrate offload configuration"
> > ---
> > include/net/xfrm.h | 8 ++++++--
> > net/key/af_key.c | 2 +-
> > net/xfrm/xfrm_policy.c | 4 ++--
> > net/xfrm/xfrm_state.c | 14 +++++++++++---
> > net/xfrm/xfrm_user.c | 15 +++++++++++++--
> > 5 files changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> > index ed4b83696c77..9e916d812af7 100644
> > --- a/include/net/xfrm.h
> > +++ b/include/net/xfrm.h
> > @@ -1876,12 +1876,16 @@ struct xfrm_state *xfrm_migrate_state_find(struct xfrm_migrate *m, struct net *n
> > u32 if_id);
> > struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
> > struct xfrm_migrate *m,
> > - struct xfrm_encap_tmpl *encap);
> > + struct xfrm_encap_tmpl *encap,
> > + struct net *net,
> > + struct xfrm_user_offload *xuo,
> > + struct netlink_ext_ack *extack);
> > int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
> > struct xfrm_migrate *m, int num_bundles,
> > struct xfrm_kmaddress *k, struct net *net,
> > struct xfrm_encap_tmpl *encap, u32 if_id,
> > - struct netlink_ext_ack *extack);
> > + struct netlink_ext_ack *extack,
> > + struct xfrm_user_offload *xuo);
> > #endif
> >
> > int km_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, __be16 sport);
> > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > index c56bb4f451e6..efc2a91f4c48 100644
> > --- a/net/key/af_key.c
> > +++ b/net/key/af_key.c
> > @@ -2630,7 +2630,7 @@ static int pfkey_migrate(struct sock *sk, struct sk_buff *skb,
> > }
> >
> > return xfrm_migrate(&sel, dir, XFRM_POLICY_TYPE_MAIN, m, i,
> > - kma ? &k : NULL, net, NULL, 0, NULL);
> > + kma ? &k : NULL, net, NULL, 0, NULL, NULL);
> >
> > out:
> > return err;
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 6551e588fe52..82f755e39110 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -4630,7 +4630,7 @@ 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,
> > struct xfrm_encap_tmpl *encap, u32 if_id,
> > - struct netlink_ext_ack *extack)
> > + struct netlink_ext_ack *extack, struct xfrm_user_offload *xuo)
> > {
> > int i, err, nx_cur = 0, nx_new = 0;
> > struct xfrm_policy *pol = NULL;
> > @@ -4663,7 +4663,7 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
> > if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
> > x_cur[nx_cur] = x;
> > nx_cur++;
> > - xc = xfrm_state_migrate(x, mp, encap);
> > + xc = xfrm_state_migrate(x, mp, encap, net, xuo, extack);
> > if (xc) {
> > x_new[nx_new] = xc;
> > nx_new++;
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index ad2202fa82f3..0b5f7e90f4f3 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -2122,22 +2122,30 @@ EXPORT_SYMBOL(xfrm_migrate_state_find);
> >
> > struct xfrm_state *xfrm_state_migrate(struct xfrm_state *x,
> > struct xfrm_migrate *m,
> > - struct xfrm_encap_tmpl *encap)
> > + struct xfrm_encap_tmpl *encap,
> > + struct net *net,
> > + struct xfrm_user_offload *xuo,
> > + struct netlink_ext_ack *extack)
> > {
> > struct xfrm_state *xc;
> > -
> > + bool offload = (xuo);
>
> There is no need in extra variable, rely on validity of pointer.
>
> > xc = xfrm_state_clone(x, encap);
> > if (!xc)
> > return NULL;
> >
> > xc->props.family = m->new_family;
> >
> > - if (xfrm_init_state(xc) < 0)
> > + if (__xfrm_init_state(xc, true, offload, NULL) < 0)
> > goto error;
>
> Please rebase this patch on top of https://lore.kernel.org/netdev/cover.1739972570.git.leon@kernel.org/
> The __xfrm_init_state() was changed there. You can use xfrm_init_state()
> instead.
>
> >
> > + x->km.state = XFRM_STATE_VALID;
> > memcpy(&xc->id.daddr, &m->new_daddr, sizeof(xc->id.daddr));
> > memcpy(&xc->props.saddr, &m->new_saddr, sizeof(xc->props.saddr));
>
> It should be placed inside xfrm_state_clone() and worth to rename it.
>
> >
> > + /* configure the hardware if offload is requested */
> > + if (offload && xfrm_dev_state_add(net, xc, xuo, extack))
> > + goto error;
> > +
> > /* add state */
> > if (xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> > /* a care is needed when the destination address of the
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index 5877eabe9d95..4c2c74078e65 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -3069,6 +3069,7 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
> > int n = 0;
> > struct net *net = sock_net(skb->sk);
> > struct xfrm_encap_tmpl *encap = NULL;
> > + struct xfrm_user_offload *xuo = NULL;
> > u32 if_id = 0;
> >
> > if (!attrs[XFRMA_MIGRATE]) {
> > @@ -3099,11 +3100,21 @@ static int xfrm_do_migrate(struct sk_buff *skb, struct nlmsghdr *nlh,
> > if (attrs[XFRMA_IF_ID])
> > if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
> >
> > + if (attrs[XFRMA_OFFLOAD_DEV]) {
> > + xuo = kmemdup(nla_data(attrs[XFRMA_OFFLOAD_DEV]),
> > + sizeof(*xuo), GFP_KERNEL);
> > + if (!xuo) {
> > + err = -ENOMEM;
> > + goto error;
> > + }
> > + }
> > +
> > err = xfrm_migrate(&pi->sel, pi->dir, type, m, n, kmp, net, encap,
> > - if_id, extack);
> > + if_id, extack, xuo);
> >
> > +error:
> > kfree(encap);
> > -
> > + kfree(xuo);
> > return err;
> > }
> > #else
> > --
> > 2.48.1.601.g30ceb7b040-goog
> >
> >
Powered by blists - more mailing lists