Re: [PATCH ipsec-next 1/5] xfrm: delay initialization of offload path till its actually requested

[Leon Romanovsky <leon@kernel.org>]

On Thu, Feb 06, 2025 at 02:16:08PM +0530, Bharat Bhushan wrote:
> Hi Leon,
> 
> On Wed, Feb 5, 2025 at 11:50 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > From: Leon Romanovsky <leonro@nvidia.com>
> >
> > XFRM offload path is probed even if offload isn't needed at all. Let's
> > make sure that x->type_offload pointer stays NULL for such path to
> > reduce ambiguity.
> >
> > Fixes: 9d389d7f84bb ("xfrm: Add a xfrm type offload.")
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> >  include/net/xfrm.h     | 12 +++++++++++-
> >  net/xfrm/xfrm_device.c | 14 +++++++++-----
> >  net/xfrm/xfrm_state.c  | 22 +++++++++-------------
> >  net/xfrm/xfrm_user.c   |  2 +-
> >  4 files changed, 30 insertions(+), 20 deletions(-)

<...>

> > +       x->type_offload = xfrm_get_type_offload(x->id.proto, x->props.family);
> > +       if (!x->type_offload) {

<...>

> > +               xfrm_put_type_offload(x->type_offload);
> > +               x->type_offload = NULL;
> 
> We always set type_offload to NULL. Can we move type_offload = NULL in
> xfrm_put_type_offload() ?

We can, but it will require change to xfrm_get_type_offload() too,
otherwise we will get asymmetrical get/put.

Do you want something like that?
int xfrm_get_type_offload(struct xfrm_state *x);
void xfrm_put_type_offload(struct xfrm_state *x);

Thansk

> 
> Thanks
> -Bharat
> 
> >                 /* User explicitly requested packet offload mode and configured
> >                  * policy in addition to the XFRM state. So be civil to users,
> >                  * and return an error instead of taking fallback path.
> > diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> > index ad2202fa82f3..568fe8df7741 100644
> > --- a/net/xfrm/xfrm_state.c
> > +++ b/net/xfrm/xfrm_state.c
> > @@ -424,11 +424,12 @@ void xfrm_unregister_type_offload(const struct xfrm_type_offload *type,
> >  }
> >  EXPORT_SYMBOL(xfrm_unregister_type_offload);
> >
> > -static const struct xfrm_type_offload *
> > -xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
> > +const struct xfrm_type_offload *xfrm_get_type_offload(u8 proto,
> > +                                                     unsigned short family)
> >  {
> >         const struct xfrm_type_offload *type = NULL;
> >         struct xfrm_state_afinfo *afinfo;
> > +       bool try_load = true;
> >
> >  retry:
> >         afinfo = xfrm_state_get_afinfo(family);
> > @@ -456,11 +457,7 @@ xfrm_get_type_offload(u8 proto, unsigned short family, bool try_load)
> >
> >         return type;
> >  }
> > -
> > -static void xfrm_put_type_offload(const struct xfrm_type_offload *type)
> > -{
> > -       module_put(type->owner);
> > -}
> > +EXPORT_SYMBOL(xfrm_get_type_offload);
> >
> >  static const struct xfrm_mode xfrm4_mode_map[XFRM_MODE_MAX] = {
> >         [XFRM_MODE_BEET] = {
> > @@ -609,8 +606,6 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
> >         kfree(x->coaddr);
> >         kfree(x->replay_esn);
> >         kfree(x->preplay_esn);
> > -       if (x->type_offload)
> > -               xfrm_put_type_offload(x->type_offload);
> >         if (x->type) {
> >                 x->type->destructor(x);
> >                 xfrm_put_type(x->type);
> > @@ -784,6 +779,9 @@ void xfrm_dev_state_free(struct xfrm_state *x)
> >         struct xfrm_dev_offload *xso = &x->xso;
> >         struct net_device *dev = READ_ONCE(xso->dev);
> >
> > +       xfrm_put_type_offload(x->type_offload);
> > +       x->type_offload = NULL;
> > +
> >         if (dev && dev->xfrmdev_ops) {
> >                 spin_lock_bh(&xfrm_state_dev_gc_lock);
> >                 if (!hlist_unhashed(&x->dev_gclist))
> > @@ -3122,7 +3120,7 @@ u32 xfrm_state_mtu(struct xfrm_state *x, int mtu)
> >  }
> >  EXPORT_SYMBOL_GPL(xfrm_state_mtu);
> >
> > -int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
> > +int __xfrm_init_state(struct xfrm_state *x, bool init_replay,
> >                       struct netlink_ext_ack *extack)
> >  {
> >         const struct xfrm_mode *inner_mode;
> > @@ -3178,8 +3176,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload,
> >                 goto error;
> >         }
> >
> > -       x->type_offload = xfrm_get_type_offload(x->id.proto, family, offload);
> > -
> >         err = x->type->init_state(x, extack);
> >         if (err)
> >                 goto error;
> > @@ -3229,7 +3225,7 @@ int xfrm_init_state(struct xfrm_state *x)
> >  {
> >         int err;
> >
> > -       err = __xfrm_init_state(x, true, false, NULL);
> > +       err = __xfrm_init_state(x, true, NULL);
> >         if (!err)
> >                 x->km.state = XFRM_STATE_VALID;
> >
> > diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> > index 08c6d6f0179f..82a768500999 100644
> > --- a/net/xfrm/xfrm_user.c
> > +++ b/net/xfrm/xfrm_user.c
> > @@ -907,7 +907,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
> >                         goto error;
> >         }
> >
> > -       err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV], extack);
> > +       err = __xfrm_init_state(x, false, extack);
> >         if (err)
> >                 goto error;
> >
> > --
> > 2.48.1
> >
> >
>